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

Duplicating a scene also duplicates child nodes' UIDs. #83575

Closed
Ratslayer opened this issue Oct 18, 2023 · 14 comments
Closed

Duplicating a scene also duplicates child nodes' UIDs. #83575

Ratslayer opened this issue Oct 18, 2023 · 14 comments

Comments

@Ratslayer
Copy link

Godot version

4.1.2.stable.mono

System information

Windows 11

Issue description

Duplicating a scene duplicates UIDs of all sub-nodes, such as MeshInstance3D.

Steps to reproduce

There is a single script, PrintChildUID, that exposes a MeshInstance3D node and prints its UID in the _Ready method.
The scene print_node.tscn is a simple node3D scene with the PrintChildUID script and a single MeshInstance3D child
Repro steps:
Open main_scene.tscn
Duplicate any node in the scene
Launch the game
Observe that UIDs of duplicated nodes are all the same in the output, while they should be different.

Minimal reproduction project

uid-bug-repro.zip

@Calinou
Copy link
Member

Calinou commented Oct 18, 2023

@Rindbee
Copy link
Contributor

Rindbee commented Oct 20, 2023

When copying a node, the exported variable _mesh on the new node will reference the value of the source node. This doesn't seem to be a bug.

GD.Print($"_mesh UID: {_mesh.GetInstanceId()} ", $"child UID: {GetNode("MeshInstance3D").GetInstanceId()}");

Although the _mesh is exported as node type MeshInstance3D, it is recorded as NodePath in tscn.

[node name="Node3" parent="." instance=ExtResource("1_vwf8e")]

[node name="Node4" parent="." instance=ExtResource("1_vwf8e")]

[node name="Node5" parent="." node_paths=PackedStringArray("_mesh") instance=ExtResource("1_vwf8e")]
_mesh = NodePath("../Node4/MeshInstance3D")

@Ratslayer
Copy link
Author

When copying a node, the exported variable _mesh on the new node will reference the value of the source node. This doesn't seem to be a bug.

If you instantiate this scene through code, everything works fine, the MeshInstance3D UID is unique.
This absolutely is a bug, as it makes duplicate functionality unusable.

@Rindbee
Copy link
Contributor

Rindbee commented Oct 23, 2023

The problem is that you use _mesh to cache child node. Then you seem to be weird that the _mesh of the copied node has the same uid (refers to the same node) .

If you specify _mesh as a NodePath, it may serve your purpose better.

@export_node_path("MeshInstance3D") var _mesh

@Ratslayer
Copy link
Author

When I duplicate a PackedScene, its child nodes are also duplicated. I expect all the links to the original children to be replaced with links to the new children.
Is this not expected behaviour? If I do it through code, by calling PackedScene::Instantiate, then all the links are replaced and everything works fine.

@Rindbee
Copy link
Contributor

Rindbee commented Oct 24, 2023

There are two ways to think about it:

  1. Node instance: Saving the same instance is expected;
  2. NodePath: Keeping the value (usually a relative path) is also expected;

Consider a case that what the exported variable cache is a node in the main scene. If you copy the node, and then paste it in a different node tree depth, keep the same instance may be better.

In my opinion: Node instance is suitable for the above case; and NodePath is used for cases where the relative path does not change, such as your case.

@Ratslayer
Copy link
Author

Are you saying that I am using ExportAttribute for a wrong use case?
I just want to expose my references to the editor, so I can link them manually.
How am I supposed to expose them?

@Rindbee
Copy link
Contributor

Rindbee commented Oct 25, 2023

Please see C# exports Nodes and Node.GetNode*. Use NodePath type instead of Node type.

@Ratslayer
Copy link
Author

"Since Godot 4.0, nodes can be directly exported without having to use NodePaths.
Exporting NodePaths like in Godot 3.x is still possible, in case you need it"

This reads like exporting Nodes is supposed to replace NodePaths and should work the same way.
Yeah, I can use nodepaths, but now I lose editor filtering (I can put any node in the nodepath even if I want a MeshInstance3D) and I need to fetch every node manually at some point, like at _Ready, which is alot of boilerplate.

Duplicating scenes is the most basic feature of any engine.
There is no use case where you want your duplicated scene to reference a child node of the original.

ExportAttribute with nodes works fine if you instantiate the scene through code, or if you drag the node from the file system window to the or if you use Instantiate Child Scene (Ctrl+Shift+A).
It's only duplicate (Ctrl+D) which does not work the same way as everything else.
I don't know how you can say that this is intended behaviour.

@Rindbee
Copy link
Contributor

Rindbee commented Oct 25, 2023

It's only duplicate (Ctrl+D) which does not work the same way as everything else.
I don't know how you can say that this is intended behaviour.

Overwriting the property values of the copied node with the property values of the source node is the default behavior. So it's not unexpected behavior for them to reference the same node instance.

"Since Godot 4.0, nodes can be directly exported without having to use NodePaths.
This reads like exporting Nodes is supposed to replace NodePaths and should work the same way.

If you are just using it in a simple scene (no sub-scene, or passing node references from the main scene into the sub-scenes), yes, they work the same. Because they reference the same node instance and have the same relative node path, Ctrl + D creates sibling nodes.

Exporting NodePaths like in Godot 3.x is still possible, in case you need it"

This is exactly the case where NodePath is needed. Because what you want is that they each reference nodes in the corresponding sub-scene, not the same node in the main scene.

@Ratslayer
Copy link
Author

Why would you ever want the copied scene to reference child nodes from the main scene? There is no use case for that.

Imagine having a character scene with a script that references a hand node.
You create character A by dragging its scene into the main scene.
You create character B the same way.
Then you duplicate character A and get character C.
You make them equip weapons into their hands.
Character A and B equip weapons just fine, but character C equips its weapon into character A's hand.
If you look at the main scene, all characters look identical and should behave identically, but one of them is inexplicably linked to another, just because it was copied, instead of instantiated.

This makes using ExportAttribute with nodes completely unpredictable, therefor useless. Why even have it at all?
And I ask again. What is the use case for not replacing child references when copying?
Every other engine does it, because that's what you want as a developer. You want all your copies to behave the same way, regardless of how they were created.

@Leshy-YA
Copy link

Leshy-YA commented Nov 2, 2023

I also encountered this bug. When duplicating, scenes, all duplicated scenes reference the internals of the source scene. Which is incorrect. They should retain their internal structure. The same issue occurs if the node is copy/pasted.

Overwriting the property values of the copied node with the property values of the source node is the default behavior. So it's not unexpected behavior for them to reference the same node instance.

It is unexpected, because it doesn't make any sense from a usability perspective and there is no use case for which this would be useful. Furthermore for the most default use case this behaviour is simply incorrect.

@Leshy-YA
Copy link

Leshy-YA commented Nov 2, 2023

It would seem the workaround for this is to make sure the exported properties are not in the top level node, but in a node further down the hierarchy. Then everything works as expected.

But again, this is a serious usability issue, making duplicating nodes very error prone.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 11, 2023

Can't reproduce on master, I think #83343 fixed it.
Also you are confusing UIDs with Object IDs. They are 2 different things.

@KoBeWi KoBeWi closed this as completed Dec 11, 2023
@KoBeWi KoBeWi added this to the 4.3 milestone Dec 11, 2023
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