godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
87.26k stars 19.61k forks source link

Dynamically added subchild in an instanced scene is not duplicated using Node.duplicate() #92829

Open KUEII opened 1 month ago

KUEII commented 1 month ago

Tested versions

System information

Godot v4.2.2.stable - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA GeForce RTX 2060 (NVIDIA; 31.0.15.3699) - Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz (12 Threads)

Issue description

As the title states, nodes[C] added to any child[B] of an instanced scene[A] dynamically are not duplicated when calling Node.duplicate() on [A].

Meanwhile, it works if "[A] is not an instanced scene" or "calling Node.duplicate(7) which does not use PackedScene.instantiate() for duplicating".

For clearify the hierarchy relation, see the attachment below: image which [C] is a dynamically added node

Not sure if this is the desired behaviour with some internal changes inside Node.duplicate() after 4.2.2 rc2.

Steps to reproduce

  1. Create a scene.
  2. Add a child to the scene.
  3. Instance the scene into main scene in the editor.
  4. Add a node to the child of the instanced scene with a script.
  5. Duplicate the instanced scene in script.

Minimal reproduction project (MRP)

Test.zip

surendra019 commented 1 month ago

does that works properly on 4.2 stable?

dalexeev commented 1 month ago

Does this work if you assign owner? This may be expected behavior, but then we should document it.

KUEII commented 1 month ago

does that works properly on 4.2 stable?

Yes, it does work properly on 4.2.stable.

KUEII commented 1 month ago

Does this work if you assign owner? This may be expected behavior, but then we should document it.

I tried assigning the instanced scene as the owner of the dynamically added label in MRP by using the code below:

func _ready():
    var vbox = $VBoxContainer
    var label = Label.new()
    label.text = "added label"
    vbox.get_node("Label").add_child(label)
    label.global_position += Vector2(0, 30)
    label.owner = vbox

    var dup = vbox.duplicate()
    dup.global_position += Vector2(200.0, 0.0)
    add_child(dup)

and still could not get it working.

Moreover, Godot throws new errors in this case at line

    var dup = vbox.duplicate()

image image

Rindbee commented 1 month ago

This behavior is not so much a bug as it is a deprecated practice.

https://github.com/godotengine/godot/blob/e96ad5af98547df71b50c4c4695ac348638113e0/scene/main/node.cpp#L2731-L2733

The behavior of editing an instanced sub-scene in the main scene breaks the encapsulation of PackScene. It's difficult to predict.

Once you modify the node tree structure of a sub-scene through a script, you may also need to modify the node tree structure through a script when copying.

You are free to modify the node tree by scripts, but you may need to remember what you did.

Some solutions:

  1. You can add the new node as a direct child of the copied node with no owner, which will be automatically copied;
  2. Or manually add the new node‘s copy again to the path you want.

From the engine's perspective, we are sure that the node is managed by the user, but we cannot be sure whether it is well managed and which script manages it. If the engine handles it on its own, it may cause multiple duplicate nodes (The engine and script may be doing duplicate work).

KUEII commented 1 month ago

Thanks for the answer, so this seems like an intended behaviour changed for 4.2.2.

Just wondering, what is the exact difference between Node.duplicate(15) and Node.duplicate(7). According to the document, the difference is only at the method of duplicating using PackedScene.instantiate() or not.

Is it that duplicating with PackedScene.instantiate() now considers only the children in the scene(it seems like in version earlier than 4.2.1 Godot considers the entire node tree as well), whereas duplicating without it considers the entire runtime node tree? If it is so, would using Node.duplicate(7) also be a solution in this scenerio(the duplicating is not involved with script duplicating) apart from either manually maintaining the structure of the copied node or using the solution mentioned above?

Some solutions:

  1. You can add the new node as a direct child of the copied node with no owner, which will be automatically copied;
  2. Or manually add the new node‘s copy again to the path you want.

(by using Node.duplicate(7) everything works properly in my case whether its in version 4.2 or 4.2.2)

Rindbee commented 1 month ago

It is risky to manage the node structure in the child scene directly from the parent scene. Why not manage the sub-scene node tree in the sub-scene's scripts?

Using DUPLICATE_USE_INSTANTIATION and following some rules may be better in scenes with many deep nested scene. This flag allows you to not have to take into account dynamic nodes managed by sub-scenes.

The following situations may be common without using the DUPLICATE_USE_INSTANTIATION flag.

When a script in a sub-scene dynamically adds a node during _ready() or _enter_tree(), if the owner is not set, the dynamically added node will be copied by the engine when its ancestor node is duplicated (not use DUPLICATE_USE_INSTANTIATION), and another copy may be automatically added (by the script in the sub-scene).