Unique Node IDs (take 2) #106837

reduz · 2025-05-26T18:53:22Z

The main goal of this PR is to safeguard when a base or instantiated scene changes (nodes renamed, moved or readded), that the hierarchy is still maintained and the node can be found.

Supersedes #86960.

What it does:

Implements unique node IDs.
These IDs act as a fallback to names when saving.
The IDs are USED AS A FALLBACK, so they are just an addition. It should not break any current existing scene.
If a scene renames or moves a node, inherited or instantiated scenes will no longer lose reference to it.
This should also eventually allow for 3D asset importing that supports unique IDs to keep references in case of scene inheritance used.

Unlike the previous approach, this one is intended to be a fallback, only used if the node is not found. This makes it safer to implement and ensure that, at worst case, we fail to find the node, but nothing breaks.

Mickeon · 2025-05-26T20:39:23Z

Much better approach in my opinion. It's good that this is not exposed to the public API.

aaronfranke · 2025-06-25T15:31:55Z

Interesting that this PR uses int32_t for unique node IDs, but we already use Strings for resource UIDs.

Since this is intended to be useful for the 3D asset pipeline, is the intention to pitch to Khronos that unique node IDs use 32-bit integers in glTF? What about non-node unique IDs, like meshes having unique IDs? If we get a glTF unique ID extension, then that should apply to all items within a glTF file including nodes and resources. I would expect that Strings would be a more agnostic approach that would match resources and work the same across engines.

KoBeWi · 2025-06-26T09:19:22Z

If a scene renames or moves a node, inherited or instantiated scenes will no longer lose reference to it.

What does that mean exactly? In what situation can a node disappear and get recovered? I don't see the IDs used anywhere other than Nodes' headers in the scene.

Mickeon · 2025-06-26T10:22:35Z

In what situation can a node disappear and get recovered?

When nodes in a inherited scene are renamed or moved, inheriting scenes are not aware of the change. As such, changes applied to inheriting scenes may be lost. This is a notorious issue as it discourages scene inheritance.

You can also argue this is, in part, a consequence of no refactoring tools or automation on the editor side.
It seems though that, based on the existing UID implementation, reduz would rather enforce hard references than letting the editor figure it out (eh)

KoBeWi · 2025-06-26T11:54:56Z

When nodes in a inherited scene are renamed or moved, inheriting scenes are not aware of the change. As such, changes applied to inheriting scenes may be lost. This is a notorious issue as it discourages scene inheritance.

Ah yeah, makes sense.

I only tested whether nodes under editable children would disappear and it's still a problem, and it's btw what #73911 tries to solve. Not sure if it's planned? Another thing that IDs could fix, and they don't right now, are NodePath properties that become invalid when a node is moved externally. It's especially relevant for animations.

akien-mga · 2025-07-01T07:47:49Z

Here's a MRP which can be used to reproduce the issue of losing local child nodes parented to an inherited scene's node (via Editable Children). This PR properly solves this issue (provided that the scenes have been saved at least once with this PR merged to save the unique node IDs).

BugReproSceneComposition.zip

This scenario is described in a handful of issues I've seen in the past. I don't have time now to do a deep search, but it would be good if someone did to list which issues would be solved by this PR (or which ones could be but aren't).

I found an edge case though, seen in this modified version of the MRP:

BugReproSceneComposition2.zip

With this setup:

In ItemScene, I rename Player to PlayerOld, then save.
If the MainScene is then opened, the NodeAddedFromMainScene properly stays parented to the PlayerOld. Don't save MainScene.
In ItemScene, I rename the Node2D Other to Player, then save.
If the MainScene is still open (but not saved), it shows the expected parenting still. But if it's closed/reopened, then the parent="Player" attribute takes precedence and the NodeAddedFromMainScene node gets parented to the new (Node2D) Player.

It's a bit of a corner case but I could see it happen occasionally due to the fact that our nodes have a default name, so it's easy enough to get name conflicts/reuse old names after rename.
E.g. you may add a Sprite2D named Sprite2D, parent something to it where it's inherited, then later rename that sprite to e.g. HandSprite, while adding a new sibling sprite (e.g. for an overlay) and keep it with its default name Sprite2D. Doing this kind of edit is pretty common and if done at the same time, external nodes might get wrongly reparented.

This seems to be due to this part of the design:

These IDs act as a fallback to names when saving.

The IDs are USED AS A FALLBACK, so they are just an addition. It should not break any current existing scene.

Why can't we use them as the first source of truth if they exist, instead of using them as fallback only? If they don't exist, we should fall back silently to using parent like usual, so it should also not break existing scenes.

KoBeWi · 2025-07-03T15:19:37Z

When nodes in a inherited scene are renamed or moved, inheriting scenes are not aware of the change. As such, changes applied to inheriting scenes may be lost. This is a notorious issue as it discourages scene inheritance.
Ah yeah, makes sense.

Well I tested and it doesn't work even for that 🤷‍♂️

godot.windows.editor.dev.x86_64_cA0ieFdKBQ.mp4

Am I doing something wrong? Same with inherited scenes.

Why can't we use them as the first source of truth if they exist, instead of using them as fallback only?

IIRC UIDs were also "fallback" in the beginning.

Ivorforce

General question: Is it really safe to fall back to unique IDs when the parent instantiated scene changes? What if the structural changes imply a change to how the nodes should be stored?
Perhaps trying to restore is better than not trying, but it doesn't feel foolproof.

Ivorforce · 2025-05-27T11:11:45Z

scene/main/node.h

@@ -128,6 +128,10 @@ class Node : public Object {
 		bool operator()(const Node *p_a, const Node *p_b) const { return p_b->is_greater_than(p_a); }
 	};

+	enum {
+		UNIQUE_SCENE_ID_UNASSIGNED = 0
+	};


I think static constexpr uint32_t could be used instead of an anonymous enum.

Its my coding style and still pretty much everywhere around the project (including multiple places in this file). Not against new developers using something different though.

Yea I guess it's mostly a style choice, but supplying an explicit type would be nice at least.
Also sorry for nitpicking so early, feel free to address this only when the PR has progressed some more!

scene/main/node.h

Ivorforce · 2025-07-10T12:05:04Z

scene/resources/packed_scene.cpp

-	} else {                                             \
-		ERR_FAIL_INDEX_V(p_id & FLAG_MASK, nc, nullptr); \
-		p_name = ret_nodes[p_id & FLAG_MASK];            \
+#define NODE_FROM_ID(p_name, p_id)                                             \


I think this could be a function instead of a macro.

Maybe, but it was like this before and this code is quite critical (any bug here can corrupt a project), so I prefer not to change it to not risk introducing potential bugs. I already had to be extremely careful and work little by little when working on this PR to ensure everything is kept as is.

reduz · 2025-07-16T09:37:34Z

Some answers to comments above:

@akien-mga

Why can't we use them as the first source of truth if they exist, instead of using them as fallback only? If they don't exist, we should fall back silently to using parent like usual, so it should also not break existing scenes.

@Ivorforce

General question: Is it really safe to fall back to unique IDs when the parent instantiated scene changes? What if the structural changes imply a change to how the nodes should be stored? Perhaps trying to restore is better than not trying, but it doesn't feel foolproof.

Its just actually just a lot easier to mess up if IDs were the primary source of truth. This is what I tried in my previous PR (#86960). If you have an inherited scene, user could have copied and have duplicated IDs (used in instances or whathever in the base scene). That would be near impossible to figure out or fix for the user if broken. Text format would become so cryptic that it would be unusable to fix manually.

This is why, this approach relies on strings first, and if not found it will rely on IDs. So, if IDs are messed up, nothing really happens. It's not 100% foolproof, but I don't think its really possible to do so (or worth the effort). To put it in other words:

In the far most common cases, this PR just fixes the most common problems and does not create new ones.
By using IDs first, you have the potential of creating new problems that did not exist before.

The main goal of this PR is to safeguard when a base or instantiated scene changes (nodes renamed, moved or readded), that the hierarchy is still maintained and the node can be found. What it does: * Implements unique node IDs. * These IDs act as a fallback to names when saving. * The IDs are **USED AS A FALLBACK**, so they are just an addition. It should not break any current existing scene. * If a scene renames or moves a node, inherited or instantiated scenes will no longer lose reference to it. Unlike the previous approach, this one is intended to be a fallback, only used if the node is not found. This makes it safer to implement and ensure that, at worst case, we fail to find the node, but nothing breaks.

KoBeWi · 2025-07-22T11:06:11Z

#106837 (comment) is still not addressed.

reduz requested a review from a team as a code owner May 26, 2025 18:53

akien-mga added bug enhancement topic:core labels May 26, 2025

akien-mga added this to the 4.x milestone May 26, 2025

reduz force-pushed the unique-node-ids2 branch 4 times, most recently from 5b243bd to 8926a0b Compare May 26, 2025 20:11

clayjohn requested a review from KoBeWi June 25, 2025 15:32

yahkr mentioned this pull request Jul 8, 2025

Add the ability to expose nodes for direct access in instantiated scenes #84018

Open

Ivorforce reviewed Jul 10, 2025

View reviewed changes

reduz force-pushed the unique-node-ids2 branch from 8926a0b to 9edcc79 Compare July 16, 2025 09:51

Uh oh!

Unique Node IDs (take 2) #106837

Are you sure you want to change the base?

Unique Node IDs (take 2) #106837

Conversation

reduz commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mickeon commented May 26, 2025

Uh oh!

aaronfranke commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Jun 26, 2025

Uh oh!

Mickeon commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Jun 26, 2025

Uh oh!

akien-mga commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Jul 3, 2025

Uh oh!

Ivorforce left a comment

Choose a reason for hiding this comment

Uh oh!

Ivorforce May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reduz Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivorforce Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ivorforce Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

reduz Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

reduz commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Jul 22, 2025

Uh oh!

Uh oh!

reduz commented May 26, 2025 •

edited

Loading

aaronfranke commented Jun 25, 2025 •

edited

Loading

Mickeon commented Jun 26, 2025 •

edited

Loading

akien-mga commented Jul 1, 2025 •

edited

Loading

Ivorforce May 27, 2025 •

edited

Loading

reduz Jul 16, 2025 •

edited

Loading

reduz commented Jul 16, 2025 •

edited

Loading