Getting scene unique node from subscene's root fails/succeeds if subscene has any/no unique nodes itself

[kleonc]

Godot version

3.5.stable, 4.0.beta1

System information

N/A

Issue description

Currently unique nodes are checked within the owner of the node only if the node have no unique nodes stored itself:

godot/scene/main/node.cpp

Lines 1303 to 1319 in 14e1f36

	} else if (name.is_node_unique_name()) {
	if (current->data.owned_unique_nodes.size()) {
	// Has unique nodes in ownership
	Node **unique = current->data.owned_unique_nodes.getptr(name);
	if (!unique) {
	return nullptr;
	}
	next = *unique;
	} else if (current->data.owner) {
	Node **unique = current->data.owner->data.owned_unique_nodes.getptr(name);
	if (!unique) {
	return nullptr;
	}
	next = *unique;
	} else {
	return nullptr;
	}

It results in inconsitence for instanced subscenes as obtaining a unique node within the scene from the root of a subscene can succeed/fail depending on whether the subscene has unique nodes itself. Example:

Test scene:

Subscenes:

Script attached to subscenes:

extends Node

func _ready() -> void:
	print()
	print(self)
	print(" owner = %s" % [owner])
	print(" %%UniqueOuter = %s" % [$"%UniqueOuter"])

Output:

WithUnique:[Node:1268]
 owner = Test:[Node:1267]
 %UniqueOuter = [Object:null]

WithNoUnique:[Node:1270]
 owner = Test:[Node:1267]
 %UniqueOuter = UniqueOuter:[Node:1272]

Not sure what's the originally intended/desired behavior but obtaining %UniqueOuter should fail/succeed consistently for both subscenes' roots.

Steps to reproduce

See above example / MRP below.

Minimal reproduction project

subscene_unique_name_bug.zip (3.5)

[timothyqiu]

According to #60298 (comment), this is intended and should be documented.

The second use case is intended. This happens because your scene root both belongs to the parent scene and has nodes it owns. If it finds one within the scene it will use this, otherwise it will be use the one where it was instantiated to.

The second one will probably need a bit more doc when tutorial is written.

[kleonc]

If it finds one within the scene it will use this, otherwise it will be use the one where it was instantiated to.

@timothyqiu Note that the quoted explanation is not what actually happens, it currently behaves like:

1. "If it finds one within the scene it will use this."
2. If there are other unique names within the scene (and the one searched for was not matched) it will fail.
3. ", otherwise it will be use the one where it was instantiated to."

Which suggests the current implementation doesn't match the original intent.

[dustdfg]

But docs says that you can't access unique nodes outside of the scene https://docs.godotengine.org/en/stable/tutorials/scripting/scene_unique_nodes.html#same-scene-limitation

[dalexeev]

The problem is that the scene root is the only node in the scene that is not owned by the scene root. Obvious algorithm:

• If the node is the root of the scene, then unique names looked up in it.
• Otherwise, unique names looked up in the owner (if any).

The only question is how to distinguish the scene root from the non-scene root (all scenes are inserted into a single tree). There is scene_file_path property, but is that a sufficient guarantee?

[dustdfg]

I would like to have an opportunity to get unique nodes outside of the scene. It looks beautiful. Using the group for only one element looks like a dirty hack

[dustdfg]

So why the unique nodes should be restricted to their scene? Why not to allow global access?

[timothyqiu]

I think you'll get all kinds of trouble if global access is allowed. e.g. If both Scene A and Scene B contains a unique node named Foo, and Scene B is instantiated inside Scene A, then the node is no longer unique, at least one scene's logic is broken.

[kleonc]

The problem is that the scene root is the only node in the scene that is not owned by the scene root. Obvious algorithm:

* If the node is the root of the scene, then unique names looked up in it.

* Otherwise, unique names looked up in the `owner` (if any).

@dalexeev The issue with such approach is it could be considered limiting. The subscene's root is at the same:

• root of its own scene,
• normal node within the scene it's instantiated in.

Meaning if you'd have e.g.:

then referring to %Unique from a script attached to Subscene within the Scene would fail:

So it would fail even though Subscene node is owned by the Scene. That would be at least surprising.

---

The only question is how to distinguish the scene root from the non-scene root (all scenes are inserted into a single tree). There is scene_file_path property, but is that a sufficient guarantee?

@dalexeev Because of the above I'd say this question is irrelevant (hence this answer too). Nevertheless, currently checking scene_file_path is exactly how Node::_duplicate checks whether a node is a root of instantiated scene:

godot/scene/main/node.cpp

Line 2434 in c3e512e

} else if ((p_flags & DUPLICATE_USE_INSTANTIATION) && !get_scene_file_path().is_empty()) {

so seems like that's the way to go in case such check is needed.

---

What I'm suggesting is making the behavior consistent. Basically changing the current behavior which is (#66536 (comment)):

1. "If it finds one within the scene it will use this."
2. If there are other unique names within the scene (and the one searched for was not matched) it will fail.
3. ", otherwise it will be use the one where it was instantiated to."

by removing the (2.) from the list above so it would become:

1. "If it finds one within the scene it will use this."
2. ", otherwise it will be use the one where it was instantiated to."

which #60298 (comment) originally suggested.

And of course ensuring it's explained clearly in the docs that for a subscene's root node the unique name is firstly searched in the subscene's unique names, and if not found in the the "outer" scene's unique names.

Codewise:

diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index 264a152392..bddb45f2d6 100644
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -1568,22 +1568,14 @@ Node *Node::get_node_or_null(const NodePath &p_path) const {
 			}
 
 		} else if (name.is_node_unique_name()) {
-			if (current->data.owned_unique_nodes.size()) {
-				// Has unique nodes in ownership
-				Node **unique = current->data.owned_unique_nodes.getptr(name);
-				if (!unique) {
-					return nullptr;
-				}
-				next = *unique;
-			} else if (current->data.owner) {
-				Node **unique = current->data.owner->data.owned_unique_nodes.getptr(name);
-				if (!unique) {
-					return nullptr;
-				}
-				next = *unique;
-			} else {
+			Node **unique = current->data.owned_unique_nodes.getptr(name);
+			if (!unique && current->data.owner) {
+				unique = current->data.owner->data.owned_unique_nodes.getptr(name);
+			}
+			if (!unique) {
 				return nullptr;
 			}
+			next = *unique;
 
 		} else {
 			next = nullptr;

[inovachrono]

If would be great if we can get the ability to access unique from both scene and subscene. Otherwise it would be good to have the bug fixed. Since it can get difficult for beginners to figure out what went wrong when all their previously working code starts getting null references just because they made a node unique in their scene/subscene.

[kleonc]

According to #60298 (comment), this is intended and should be documented.

Turns out it was an oversight: #89686 (review). 🙃
Hence I'm relabelling it back as a bug.

[timothyqiu]

Well. Oversight means it was the intended behavior, but from the present point of view, such behavior is unnecessary.

I personally think this is a compat breaking behavior change request rather than a bug. But nevermind 😄

[kleonc]

Well. Oversight means it was the intended behavior, but from the present point of view, such behavior is unnecessary.

Or it means that the behavior in this specific case was not even considered / taken into account. In which case I wouldn't say it was intented (for this specific case).

We could ask reduz to clarify his original intentions but I'd guess he doesn't remember it anyway. 😄

I personally think this is a compat breaking behavior change request rather than a bug. But nevermind 😄

And I haven't changed my mind for all this time and still consider it as a bug. Would be surprising to me if someone will report the change from my PR as a regression. Well, we'll see... 👀