Ensure object metadata is unique

[bojidar-bg]

Closes #32415.
Bugsquad edit: Fixes #20648, fixes #29189.

Not sure if this is the best fix. An alternative one would be to have a boolean which is true if the metadata might be shared, and having set_meta duplicate the dictionary when needed.

[Zylann]

I'd like to clarify how the issue happens and how this fixes it...
So far I was assuming metadata was like a special dictionary.

In the issue, the user sets an item within this dictionary on an instance of a scene.
In this PR, it looks like you are duplicating metadata in case the following is called: set("meta", value), however in the issue the user wrote set_meta("id", value).

So I realize the issue is that during instancing of the scene, meta was treated like a basic property, causing the dictionary to be set by reference and no CoW happens here either (like what used to happen if you have GDScript variables containing a ref type by default). That's what your PR is fixing. Am I right?
Another alternative is to do this duplicate in the instancing code instead of always doing it in the metadata setter.
Also, hopefully nobody relies on nested dictionary inside this, because it may require a deep copy.

[bojidar-bg]

Am I right?

Yes.

Another alternative is to do this duplicate in the instancing code instead of always doing it in the metadata setter.

Arrays have always been set uncopied, and when dictionaries came around, they have been set uncopied to mimic this behavior. Duplicating all dictionaries and arrays would increase memory usage and slightly decrease instancing performance, even in cases where it is not needed. Duplicating only metadata sounds like a workaround put in the wrong place.

Also, hopefully nobody relies on nested dictionary inside this, because it may require a deep copy.

Things inside metadata have always been copied by reference, without deep-copying them.

[akien-mga]

Some comments from IRC:

 14:28 <Akien> So gist of it is that since dictionaries are shared by default, different Object instances share the same metadata
14:29 <Akien> Which is confusing when you want to use metadata to differentiate instances
14:31 <lawnjelly> I thought each instance had unique metadata (maybe not!)
14:31 <reduz> Akien: I think that one is actually good
14:31 <reduz> its not very efficient though
14:33 <Akien> Maybe we should had a property hint for that?
14:34 <Akien> PROPERTY_HINT_INSTANCE_UNIQUE or something
14:34 <reduz> i think there may be one already
14:34 <Akien> To add to the `__meta__` declaration
14:34 <Akien> *property usage
14:34 <reduz> oh, metadata is a dictionary
14:34 <reduz> did it not use to be a Map<StringName,Variant> at some point?
14:35 <reduz> I am in doubt now
14:35 <Akien> Not that I remember
14:35 <Akien> cc bojidar_bg btw
14:35 <reduz> maybe it shou,d since you can only use stringmames as keys
14:36 <bojidar_bg> I was mentioned?
14:36 <Akien> Some users want direct access to the metadata dict from the inspector though, not sure if that would be possible if it's a Map? #30765
14:36 <IssueBot> #30765: Expose "meta" to the Inspector. For real. | https://git.io/fjDvB
14:36 <Akien> bojidar_bg: yeah we're discussing #32416
14:36 <IssueBot> #32416: Ensure object metadata is unique | https://git.io/JeZKQ
14:37 <Akien> Regarding adding a usage flag, we have `PROPERTY_USAGE_DO_NOT_SHARE_ON_DUPLICATE` but I guess this doesn't apply here.
14:37 <bojidar_bg> well, meta could be a map, as long as the bulk getter and setter create a dictionary
14:38 <bojidar_bg> then my PR won't be necessary though
14:39 <Akien> Wouldn't it be wasteful to have to create a dictionary each time metadata is queried? Or do I misundersdtand
14:40 <bojidar_bg> nah, that would be when doing a get("__meta__") call
14:40 <bojidar_bg> (if this displays as bold for you, blame markdown support in riot...)
14:41 <bojidar_bg> (ah, so you can disable it) * get("__meta__") call
14:42 <bojidar_bg> when doing set("__meta__", dictionary), the dictionary would have to be iterated over in order to convert it to a Map, so the duplicate won't be necessary
14:42 <bojidar_bg> and if those two work, meta would be exposable, though it won't work very efficiently
14:43 <Akien> reduz: WDYT is the best option? Simply duplicate, or switch to a Map?
14:44 <Akien> I guess duplicate would be good enough for now, and we can revisit for 4.0?
14:45 <reduz> Akien: I am not exactly fine adding a hack that we may forget about later, so how can we ensure we will not forget?
14:45 <reduz> Akien: would it not be better to just kick it to 4.0 and fix it properly?
14:45 <Akien> We open an issue tracking the 4.0 milestone
14:46 <reduz> ah alright then
14:46 <Akien> Well it's still a proper fix IMO, just maybe not the best version that we could have,
14:47 <Akien> There are other ways in which we could improve the whole metadata experience IMO if we decide to refactor it for 4.0.
14:47 <reduz> yeah you are right, i think no one will notice the impact, but we should probably not only properly fix meta but decide what to do about it, including maybe exposing it to inspector
14:47 <reduz> we kicked that back for a while

So good to go for now, but we really need to rethink the way we handle metadata for 4.0.

[akien-mga]

Thanks!