node.duplicate() does not resolve exported node properties

[guessy42]

Godot version

v4.0.3.stable.mono.official [5222a99]

System information

Windows 10, Godot 4.0.3.stable, Vulken (Forward+)

Issue description

If a script has an exported node property, eg @export var node : Node pointing to a child node, when duplicated, the duplicate of said script will still point to the original node.

The ideal or expected behaviour would be:
If the exported node points to a child of the object, its duplicate should point the equivalent child of the duplicated object (the underlying NodePath does not change)
If the exported node does not point to a child of the object, its duplicate should target the same node as the original (the underlying NodePath will be different)

Currently, the behaviour is always the latter. This causes particular problems if you're assembling game elements procedurally; to give an example, a procedurally generated mob character might be assembled once and then duplicated as needed, an approach that will currently fall apart in very unpredictable ways if any part of said mob uses exported Node properties.

Steps to reproduce

Have a script with an exported node property
Call duplicate() on an instance of said script
Check the node path

Minimal reproduction project

DuplicateExportedNodeBug.zip

Open in the editor and run the scene; check the paths printed in the Output window.

[AThousandShips]

• See also: Node.Duplicate() Should Duplicate Data, Not just Node Branch godot-proposals#6512

Note that exported Nodes are not updated or modified outside the editor upon updating, and are expected to not change by default, if you use NodePath instead the idea that it's relative becomes clear, but I feel that making this behaviour different from what it is now is gonna create confusion as well, as currently duplication doesn't modify exports in this way, and you might not want it to

This could be added as a new feature, but I'd say it'd be best to have it controlled by a flag to duplicate and off by default

[Skyvastern]

This is a pretty serious issue that still persist in v4.1.1. It happens not just at runtime but in editor too. If you simply duplicate a node (Ctrl+D) that uses exported variables, then the value of exported variable will refer to the originaI node's value we duplicated, causing all sorts of unpredictable behaviours. I faced the same issue in my project, and took me a good enough time to figure out the cause.

I believe fixing this issue should be prioritized. Otherwise, making duplicates by Ctrl+D will be a useless feature if a node uses exported variables.

[wilfredjonathanjames]

This makes inventory systems that move items in and out of the scene really messy to build.

[Mickeon]

As a workaround -- and I am noticing this feels like a necessary feature to have -- there's an alternative to duplication.
You may pack the node you want to duplicate in a PackedScene, then instantiate it. The references to the newly created, exported nodes should then be relative to their new scene, and be as you may expect.

var packed_scene = PackedScene.new()
packed_scene.pack(self)
var duplicated_node = packed_scene.instantiate()
add_sibling(duplicated_node)

It's totally possible this doesn't always work (and I may have written it wrong), but it may suffice in most cases.

There's also... the DUPLICATE_USE_INSTANTIATION flag of duplicate which essentially does this, but it does absolutely nothing unless a scene_file_path is defined, and still has the same issue with Node references.

[Mickeon]

I made a test PR for the aforementioned suggestion of having it as a separate flag. #81866

[MikeFP]

As a workaround -- and I am noticing this feels like a necessary feature to have -- there's an alternative to duplication. You may pack the node you want to duplicate in a PackedScene, then instantiate it. The references to the newly created, exported nodes should then be relative to their new scene, and be as you may expect.

It's worth nothing this workaround only works if the whole subtree being packed only contains references to nodes in that same subtree. If a Node has a reference to a Node elsewhere outside of the subtree, PackedScene.pack() will fail with a common_parent is null error. At least I think that's the issue, since I couldn't debug it further.

I guess it makes sense that isn't allowed, because normally, regular scenes only get to reference external nodes via exported variables. Therefore, trying to pack Nodes with external references wouldn't work, unless all references were in the root node, and/or the engine made some magic.

[TruelyMostWanted]

As we are reaching the 4.2 version soon and this problem is still existent

2 ideas:
1) Applying the path-related logic of PackedScene.Instantiate() in Node.Duplicate()
This issue is based on 2 files of the C++ source code:
Node.Duplicate() in node.cpp
PackedScene.Instantiate() in packed_scene.cpp

and just for addition the NodePath
node_path.cpp

As the source code works in GDScript but not it C#, its either due to the script bridge breaking there or something internal.
Because the logic works as intended.

But just out of the box - what if one would test the following idea:
Isnt it possible to inspect the logic of Instantiate() and see what it does with the NodePaths,
Then extract that logic alone with matching parameters into a seperate method called "update_node_paths()".
And if a node gets duplicated that is a packed-scene, it could also call this method?

---

2) As Workaround: How about a feature called "Mass Instantiate"?

As commented above, the PackedScene.Instantiate() is the current workaround for that problem.
But drag & drop for X times is time consuming and just hitting Instantiate over and over again creates a nested hierachy like arrow-code..

What about this:
Basically expanding the existing "Instantiate" method

PackedScene.Instantiate() // -> returning a Node
PackedScene.Instantiate<T>() // -> returning a <T>

with new "MassInstantiate" methods

PackedScene.MassInstantiate(int count, Node parent) // -> returning a Node[] or Array<Node>
PackedScene.MassInstantiate<T>(int count, Node parent) // -> returning a T[] or Array<T> 

basically performing the following logic (written in C#)

public static Node[] MassInstantiate(PackedScene targetPackedScene, int count, Node parent)
{
    if(targetPackedScene == null || count < 0 || parent == null)
        throw new IllegalArgumentException("One of the parameters is invald!");

    Node[] instances = new Node[count];
    for(int i = 0; i < count; i++)
    {
        instances[i] = targetPackedScene.Instantiate();
        parent.AddChild(instances[i]);
    }
    return instances;
}

which will also get a new menu entry only available on PackedScenes (edited image) with eventually a hotkey/shortcut.

Hit that entry and you'll get a popup where you can pick a number between 1 and 100 and it will create this many node instances of the packed scene below the selected node in your scene. If you didnt select one, then it would do that below the root node of the scene?

---

[ricardobusta]

Thanks for reporting! Had same issue when moving from 4.1.1 to 4.2 beta 😅
My gdscripts were using @onready , then when moving to c# I changed it to [Export] due to the lack of [OnReady] attribute (wanted to avoid multiple lines per field).
Took me a while to figure out what was going on. 😏

[doseonfairsutdios]

This problem still occurs in godot 4.2.1.

After creating a Node created as a Scene in the editor as an object in another Scene, the new object created by duplicating the object refers to the Export of the existing object.

I'm having a hard time identifying the problem because I can't tell which node is referenced in the Export field.

[hsandt]

Got exactly that issue today, I was duplicating a UI widget (a segment of my health bar as many times as character had HP) for convenience (quick testing, "I can make it a scene to instantiate later"), and then I noticed that the last widgets were referring to and affecting the original segment, causing an invalid appearance.

I think we should at least, at an emergency measure, document this behavior (along with the fact that duplicate doesn't duplicate internal value as said in #3393).

Then we can work on adding an option to deep duplicate references to child nodes (if we want to keep retrocompatibility - if we add some deep_copy flag, personally I'd make it true by default to avoid bad surprises, but we'd have to wait for a new Godot version to change behavior).

By the way, #84146 which is about Editor duplicate was closed in favor of this (and also @Skyvastern stated it clearly), so could we change the title to include "duplicating node in editor scene tree"?

[jdies]

In the case of PackedScenes and child nodes this behavior is so confusing it should be treated as bug. Why would a PackedScene in the editor be able to point to a child node in another PackedScene that I only see as a single node.

I've run into this a bunch of times and probably lost a few hours all in all. I imagine it to be super frustrating for newbies.