Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KUEII opened this issue Jun 6, 2024 · 7 comments

Comments

@KUEII
Copy link

KUEII commented Jun 6, 2024

Tested versions

  • Reproducible in: 4.2.2.stable [15073af], 4.2.2 rc2, 4.2.2 rc3
  • Not Reproducible in: 4.2.stable [46dc277], 4.2.1.stable [b09f793], 4.2.2 rc1

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
Copy link

does that works properly on 4.2 stable?

@dalexeev
Copy link
Member

dalexeev commented Jun 6, 2024

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

@KUEII
Copy link
Author

KUEII commented Jun 6, 2024

does that works properly on 4.2 stable?

Yes, it does work properly on 4.2.stable.

@KUEII
Copy link
Author

KUEII commented Jun 6, 2024

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
Copy link
Contributor

Rindbee commented Jun 7, 2024

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

godot/scene/main/node.cpp

Lines 2731 to 2733 in e96ad5a

if (instantiated && get_child(i)->data.owner == this) {
continue; //part of instance
}

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
Copy link
Author

KUEII commented Jun 7, 2024

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
Copy link
Contributor

Rindbee commented Jun 7, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants