Godot: duplicate() don't copy internal variables values

[kubecz3k]

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

[kubecz3k]

Related issue but with signals: https://github.com/godotengine/godot/issues/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:
  gdscript extends Node store var some_string_value = "" var _position_cache = Vector2()
  Or:
  gdscript 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 [e-mail] 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
https://github.com/godotengine/godot/issues/3393#issuecomment-307798657,
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?