Node duplicate() doesn't copy internal variables values

[kubecz3k]

Bugsquad edit: This bug has been confirmed several times already. No need to confirm it further.

---

Duplicate method don't take into account variable values of duplication root as well as internal nodes.
I have prepared small test project that can be used to detect some issues related to duplication:
DuplicateTests.zip
it test various scenarios and report results in output console in the following form:

(...)
Translation test: SUCCESS
Root export variable test: SUCCESS
Inner export variable test: SUCCESS
Root variable value test: FAIL
Inner variable val test: FAIL
Inner node structure test: SUCCESS

Summary result: FAIL

17.08.2022 UPDATE
I converted test project from Godot1? to Godot4 . I can confirm I still get the same result:
Godot4 reproduction here: #3393 (comment)

Also to clarify: the issue is about usage of Object.duplicate() method and the fact that variables of returned object (even simple types) are different then in the original one.

[kubecz3k]

Related issue but with signals: #1888

[reduz]

I'm not sure if this should be a bug or not.. internal state is internal state..

[kubecz3k]

@reduz Well I don't know this as well... I'm coming from java world, where default 'clone' method is shallow copy (copying all the values and references), if developer want different behaviour then he is implementing his own clone() method. Don't really know how it's in python world, but as far as I remember it's basically the same (and I think gdscript should follow python convention).
For me personally as long as it's possible to provide your own 'duplicate' method implementation it's fine, but I would like to know what is the default intended behaviour (would be good idea to have this in docs).

[kubecz3k]

Also I think that shallow copy is not necessary the best solution since internal node references will point to nodes in original scene... I would solve this by checking if reference is pointing to the node that is on the lower scene level or not (lower from the point of duplicate root are duplicated with deep copy). When it comes to value variables like int or String the situation for me is clear (I think they should be copied).

[Nearoo]

I was searching for a bug in my game for quite some time, until I saw this thread and noticed that it wasn't a bug at all...

I find the way it is currently done unintuitive. The way it is right now, get_node(S).duplicate() and preload(S).instance() are the same thing. That shouldn't be the case. Every other attribute of get_node(S) points to the instance of that node, except for duplicate, which points to the un-instance class? I've never seen such a design.

And it's not about shallow vs. deep copy. A "shallow copy" B of an object A (which is an instance of the un-instanced class C) in most C-like languages means that the member variables of B point to the values of the instance A, not to a copy of the variables of C. If I wanted a new object with the values I hard-coded into C, I simply would make a new instance of C. Which is the equivalent of calling 'preload(S).instance()' in gdscript, at least that's what it seems to me.

I would suggest to change the API to the following:

N.duplicate(use_instancing=false, deep_duplicate=true)

Makes a copy of the instance of N. If deep_duplicate is true, all values get copied and get a new place in memory. If it is false, the values are shared. However, I've never seen this anywhere else in godot, so maybe this shouldn't be an option at all. GDscript exists only for developers to make it as easy as possible, and to think about pointers and instancing would make it hard for newcomers, I think.

Anyway, I would call the method that does what your duplicate method currently does differently. I don't know exactly how your systems work, but from what I know so far, this would make sense:

preload(S) # Returns an un-instanced class
load(S) # This too
File # This is an uninstanced class as well

# To instance a class, call "instance()"

var t = preload(S).instance()
var k = load(S).instance()

f = File.instance() # Instead of new(), for consistency 
f.load(xyz)

# And to do this what duplicate currently does:
t.new_instance()
# or
t.get_root_class().instance()  # Bad name, but it's about the idea

# This could be used to spawn multiple new nodes of t:

t_root_class = t.get_packed_scene()
for i in range(10):
   add_child(t_root_class.instance())

[bojidar-bg]

Note this comes from scene/main/node.cpp:2226.

Since script variables get only PROPERTY_USAGE_SCRIPT_VARIABLE, and not PROPERTY_USAGE_STORAGE, they don't get persisted.

A few solutions come to mind:

• Add some way to flag GDScript variables as storage/nostorage:

  extends Node
  store var some_string_value = ""
  var _position_cache = Vector2()

  Or:

  var some_string_vaule = ""
  unstore var _position_cache = Vector2() # Better keyword anyone?

  Or, just assume all variables not starting with _ are to be store-d
• Add another DUPLICATE_* flag, DUPLICATE_SCRIPT_VARIABLES (node.h:56)
  This time, there is not way to selectively duplicate just some of the variables. It is questionable how much of a problem this is, though.

[eon-s]

@bojidar-bg I prefer selective no-storage than having to manually set everything for storage all the time (which may be more error prone), but not sure of the keyword for that.

[reduz]

duplicate() for nodes is hackish, and not generally recommended. It's there because some people insists it can be of use, because otherwise i would have removed it long ago. Using preload().instance() is always the better choice. The reason is that some nodes create subnodes (ie, a list creates it's scrollbars), so a simple duplicate() function has to be able to tell to a certain degree what to duplicate and what not to duplicate. This is not always obvious. Maybe the best it would be to document what the function can actually does, or just add more flexibility to it, not really sure what you guys prefer.

…

On Mon, Jun 12, 2017 at 11:02 AM, eon-s ***@***.***> wrote: @bojidar-bg <https://github.com/bojidar-bg> I prefer selective no-storage than having to manually set everything for storage all the time (which may be more error prone), but not sure of the keyword for that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF-Z290qBZIy62N7XjfODdHQDbHRhC9Xks5sDUT1gaJpZM4HIicK> .

[rminderhoud]

I currently use duplicate() to get a child node of an instanced scene such that it has no parent and I can use it in a replace_by() call. From the discussion it seems duplicate() is hacky but is this a valid use case?

const new_scene = preload("res://myscene.tscn")
var bar = new_scene.instance().get_node("something").duplicate () # Has no children
$OtherThing.replace_by(bar, true)

[akien-mga]

@rminderhoud Just do this:

const new_scene = preload("res://myscene.tscn")
var new_instance = new_scene.instance()
var something = new_instance.get_node("something")
new_instance.remove_child(something)
$OtherThing.replace_by(something, true)

[rminderhoud]

Thanks @akien-mga, in that case maybe duplicate() doesn't really have any valid use cases and should be documented with a warning. There was nothing in the docs to indicate it's not recommended for use.

[programaths]

I have different tiles inheriting from a base tile.
When receiving a signal, I need to create a copy of the tile.
This is where duplicate() comes into play!

The other solution is to test which type the node is and instantiate the appropriate packaged scene!
So, replacing one line by 24.

This is a use case for duplicate.

Alternative is having one type of tile which will change its behavior depending on some internal state.

[TheSHEEEP]

I second the notion that .duplicate() absolutely has many use cases and is a much quicker and easier solution than instancing preloaded scenes.

But it would need an improvement to also duplicate children. Or, create an official clone() method which does that, leaving duplicate to make only a shallow copy as it does right now.

[Janglee123]

Or add a DuplicateFlags for cloning rather than creating a new method that might make more confusion.

[paulicka]

Couldn't you do something like...

 func my_duplicate(node : Node) -> Node:
        var packed_scene = PackedScene.new()
        packed_scene.pack(node)
        return packed_scene.instance()

...and you can always debug by writing out the PackedScene with the ResourceSaver?

[RafalLauterbach]

It is still misleading name in my opinion.

Duplicate mean "duplicate" for me. If I wanted another instance of same class I would just make that instead, even if duplication would be very heavy operation, or loop indefinitely or it would cause some flaw - it's my problem - it still should duplicate 1:1 untill it crashes or finishes this operation.

I read docs on duplicate() and DuplicateFlags multiple times, in description there is note, but it don't mention fact that internal states aren't duplicated and it is counterintuitive for me. I'm on older version, but in online docs there is also no mention about it.

Perhaps in other languages cloning or duplicates are meant to be shallow copies - but I never wrote in high level programing language and for my primitive mind meaning is clear - it duplicates object.

Edit : on the other hand - what you can do is overwrite duplicate() to copy some selected variables.

[db0]

I agree that as it is, it's counter-intuitive and I just lost a couple of hours troubleshooting an issue caused by duplicate() not duplicating internal variables. If there's a use-case where internal vars should not be copied, then there should be another DuplicateFlags to set to avoid it.

[nemerle]

Just a quick note, Node::replace_by does not fully honor p_keep_data flag.
It will only keep the original node's groups.

Current logic:

	List<_NodeReplaceByPair> replace_data;
	if (p_keep_data) {
		List<PropertyInfo> plist;
		get_property_list(&plist);

		for (List<PropertyInfo>::Element *E = plist.front(); E; E = E->next()) {
			_NodeReplaceByPair rd;
			if (!(E->get().usage & PROPERTY_USAGE_STORAGE)) {
				continue;
			}
			rd.name = E->get().name;
			rd.value = get(rd.name);
		}

Notice that _NodeReplaceByPair value rd is not added to replace_data List
so the code that follows:

	for (List<_NodeReplaceByPair>::Element *E = replace_data.front(); E; E = E->next()) {
		p_node->set(E->get().name, E->get().value);
	}

is basically a no-op.

[kubecz3k]

Cant attach Godot4 reproduction by editing first post (some unknown error), attaching the Godot4 reproduction here:
DuplicateTestGd4.zip
(The issue is still the same as it was before)

Also to clarify: the issue is about usage of Object.duplicate() method and the fact that variables of returned object (even simple types) are different then in the original one.

[kubecz3k]

Also my view of this issue has not changed, we should either document it in documentation of duplicate() method or fix it.
If we decide to not fix it we at least should ensure it's possible to overwrite duplicate() from gdscript and provide your own implementation there (and it also should be documented in such case)

[unforgiven1990]

I had the same issue. I had a card game where I wanted to buff some cards and duplicate them. It was frustrating to know that the only way to duplicate the internal states was to manually create a dictionary and copy each variable over to the new card instance. It would be much better to just do card_instance.duplicate() if it also copies the buffs on them.

BUT LUCKILY, I found a solution for me that worked. JUST add @export before the variables you want to maintain the state. Then when you do card_instance.duplicate() you actually ALSO duplicate the variable values.

[m21-cerutti]

Does it would be the same for SyleBox ressources ? (#80797)

I was trying to duplicate some StyleBoxes to change only some limited variables, to make style overrides (like override only background color), but I have an empty ressource instead (so of course draw center is at false).

[hsandt]

Also I think that shallow copy is not necessary the best solution since internal node references will point to nodes in original scene... I would solve this by checking if reference is pointing to the node that is on the lower scene level or not (lower from the point of duplicate root are duplicated with deep copy).

Exactly what bit me today, and I found the issue specifically related to duplicating nodes with references to child nodes:
#78060

It even affects the editor node duplicate:
#84146 (marked as duplicate of previous bug as same cause, although this one is focused on the editor)

[AThousandShips]

@ChknNugz Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

[jamesmcm]

I hit this issue with a Resource that I had created in code at run-time in GDScript.

I just wanted to clone the created Resource - in this case it isn't possible to use preload(...).instance() as there is nothing to preload.

For now I just set all the values to be @export too as mentioned above, but is there a better way of dealing with this for Resources created programmatically at run-time?