Implement Unique Node IDs #86960
Implement Unique Node IDs #86960
Conversation
* Scene instantiation and inheritance now use unique IDs for referencing. * Ensures that if a node is moved or renamed, the scene that inherits keeps functioning. * Eventually will be extended to Asset IO **WARNING**: THIS NEEDS A LOT OF TESTING AND REVIEW
| // There is inheritance at play, try with parent. | ||
| Ref<PackedScene> parent = base_state->variants[base_state->base_scene_idx]; | ||
| if (parent.is_valid()) { | ||
| base_state = parent->get_state().ptr(); |
There was a problem hiding this comment.
This part here is pretty odd. It means if it did not find the node in the local scene, it will try with the scene that it inherits from. I am not sure if this is 100% correct, but It's the only way I can think of that works with inheritance.
|
I think what I may need to change is that if you instantiate a scene (not inherit it), the ID of the instance should probably be different than that of the scene when opened separatedly. I already change it if instantiated more than once, but may need to make an exception here by forcing the node ID to unassigned when called with GEN_EDIT_STATE_INSTANCE. |
|
|
||
| uint32_t ResourceUID::generate_random_u32() const { | ||
| uint32_t num; | ||
| Error err = ((CryptoCore::RandomGenerator *)crypto)->get_random_bytes((uint8_t *)&num, sizeof(uint32_t)); |
There was a problem hiding this comment.
Will this ever change to an int64? I've seen other systems like https://en.wikipedia.org/wiki/Snowflake_ID use 64 bits.
There was a problem hiding this comment.
I have a feeling 4,294,967,296 nodes in a single PackedScene is not an amount that will be reached any time soon.
There was a problem hiding this comment.
I didn't know we're incrementing. We are using random bytes here.
There was a problem hiding this comment.
Oh, then, it may actually be an issue, but unsure.
There was a problem hiding this comment.
It looks like where it is called, it checks for duplicates and calls again until a unique is found, so collisions shouldn't be a problem (aside from the time spent checking for duplicates).
There was a problem hiding this comment.
Probably no, this is only for a single scene so no need for so much.
|
I opened a scene (tscn), it added a bunch of IDs to nodes, but only that. I'd expect nodes to use these IDs instead of parent path, but that's not the case. I checked some simple inheritance case (added node in inherited scene and moved its parent in the base scene) and it still breaks. btw so far modified text scenes can still be opened in an earlier Godot version. |
|
@KoBeWi it won't use paths within the local scene because it does not make much sense (and path is more readable). IDs are used mostly when you do scene inheritance/instantiation with editable children and place nodes as children of nodes of other scenes. They are also used for signal connections. |
Well that's what I tested and it didn't work correctly. Though I'll need to check again if the IDs are used correctly. |
|
Ok I tested again and indeed, parent is always a path, even if the node is under inherited node or editable child. Both in tscn and scn. The only case that works correctly is binary inherited scene. |
|
@KoBeWi Maybe I broke it, thanks still for giving it a spin, will see if I can fix it today! |
WARNING: THIS NEEDS A LOT OF TESTING AND REVIEW - DO NOT USE IN YOUR PROJECT, IT WILL BREAK IT.
Note: This bumps the binary format version, files saved with this will no longer work in earlier versions of Godot.
Note: ResourceFormatText::save_as_binary is broken with this, but this function is no longer in use, should it be removed as part of this PR?
Note: I am allowing negative numbers in there for IDs (only 0 is unassigned), I may as well not, let me know if you want me to change this.