Expose scene unique id functionality in Resource #88111
Conversation
|
Please open a proposal to track the support and details of this feature :) |
|
Is there an example I can use to check this is working? |
Create a new scene with 3 world environments to test this with. @tool
extends Node
@onready var world_environment: WorldEnvironment = $WorldEnvironment
@onready var world_environment_2: WorldEnvironment = $WorldEnvironment2
@onready var world_environment_3: WorldEnvironment = $WorldEnvironment3
@export var run: bool:
set(v):
if !is_inside_tree(): return
run_impl()
func run_impl() -> void:
var id := Resource.generate_scene_unique_id();
# create some different resource instances, with the same id.
var env1 := make_new_resource_with(id)
var env2 := make_new_resource_with(id)
var env3 := make_new_resource_with(id)
print("1: " + env1.resource_scene_unique_id)
print("2: " + env2.resource_scene_unique_id)
print("3: " + env3.resource_scene_unique_id)
# after saving, you will notice that the world enviroment higher in the scene (in my case WorldEnviroment3)
# will have retained the id we generated, while the other two,
# will have a random id assigned to them at save time
# (which is a good thing) because no scene corruption
# use the inspector to check
world_environment.environment = env1
world_environment_2.environment = env2
world_environment_3.environment = env3
func make_new_resource_with(id: String) -> Environment:
var env := Environment.new();
env.resource_scene_unique_id = id
return env |
|
While I'm reviewing and gather reviewers can you apply the documentation changes? |
|
Makes sense, but please add the documentation in doc/classes/Resource.xml |
|
While adding documentation, please also squash the commits. See PR workflow for instructions. |
There was a problem hiding this comment.
Skeptical myself on exposing this full in the Inspector. I have many thoughts but to make it brief:
It would be a property not many would modify among others that are arguably more useful. This adds to clutter, bloat, noise, however one may want to call it. It may sound like an exaggeration but... yeah.
But if it has to be exposed it needs better documentation.
Yeah, I'm not too familiar with how Godot does things on a code level. But exposing it to the editor inspector (and to be saved as a property in the resource, as I just found out) was definitely not my intention. I just wanted to be able to use it as a property in code. So ill make a quick change in a moment to fix that. |
|
If this were a property defined in a script it would usually be assigned PROPERTY_USAGE_DEFAULT, so that it is not exposed to the Inspector and saved at all, yet still documented. For built-in properties I'm struggling to find any that actually do this, and this may be entirely by design. In fact, for these kinds of properties what other Objects do is only keeping the setter and getter. |
turns out changing it from ADD_PROPERTY(PropertyInfo(Variant::STRING, "resource_scene_unique_id"), "set_scene_unique_id", "get_scene_unique_id");to ADD_PROPERTY(PropertyInfo(Variant::STRING, "resource_scene_unique_id", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NONE), "set_scene_unique_id", "get_scene_unique_id");does the trick |
|
By the way, the most I've used git is to back up my projects, so all this advanced git stuff is new to me. If you asked me yesterday what git squash is, I'd be thinking about butternut squash soup. |
|
You need to run |
This is our fault actually. We should have linked you to the Pull request workflow page sooner. |
|
hang on i have to regenerate the docs then ill do the git squashing things |
|
You can squash at the same time or just update the original commit :) |
|
Do I delete all the other commits except for the one that I just put now? |
|
You use |
|
You need to push with |
|
There we go :) In the future remember to create your changes on a separate branch to avoid a lot of these problems, and to use rebase to update your branch and not merges |
|
Ok, thank you for your help and understanding. |
|
No worries! |
| @@ -124,6 +124,9 @@ String Resource::generate_scene_unique_id() { | |||
| return id; | |||
| } | |||
|
|
|||
| // Warning: there is no validation here. | |||
| // Use is_ascii_identifier_char() to validate before passing it in. | |||
| // Example: if you pass in a quote, it will corrupt the scene when saving. | |||
| void Resource::set_scene_unique_id(const String &p_id) { | |||
| scene_unique_id = p_id; | |||
There was a problem hiding this comment.
Is there an escape we can do that will let us use arbitrary strings but avoid breaking the scene?
There was a problem hiding this comment.
This is what I tested.
@tool
extends Node
@onready var world_environment: WorldEnvironment = $WorldEnvironment
@export var run: bool:
set(v):
if !is_inside_tree(): return
world_environment.environment.resource_scene_unique_id = "123\""Which resulted in this, and broke the scene (notice the duplicate quotes).
[gd_scene load_steps=3 format=3 uid="uid://ifg8x7gf74rm"]
[ext_resource type="Script" path="res://mydscript2.gd" id="1_pk36j"]
[sub_resource type="Environment" id="123""]
[node name="Myscene" type="Node"]
script = ExtResource("1_pk36j")
[node name="WorldEnvironment" type="WorldEnvironment" parent="."]
environment = SubResource("123"")If you make it use a \n then it will look like this (this one wont break it from loading, but it will still look weird)
[gd_scene load_steps=3 format=3 uid="uid://ifg8x7gf74rm"]
[ext_resource type="Script" path="res://mydscript2.gd" id="1_pk36j"]
[sub_resource type="Environment" id="123
"]
[node name="Myscene" type="Node"]
script = ExtResource("1_pk36j")
[node name="WorldEnvironment" type="WorldEnvironment" parent="."]
environment = SubResource("123
")I can't really answer your original question though, because the way the scene is saved is out of my domain.
There was a problem hiding this comment.
I do reiterate my original concern in #88111 (comment). That is, the method exposed to outside developers should not be able to brick scenes this way (see comment for more).
|
Okay, I added some documentation and a check before being able to set it, also if the check fails it will default to a randomly generated one. |
There was a problem hiding this comment.
Thank you for your patience. I still think that there should be a distinction between internal and exposed method, because the additional check internally is extremely redundant. But this can be done later in a separate PR, this one should be fine after the above issue is fixed and (and the commits are squashed again).
|
Please remove the body of the commit message, it's just a lot of notes on what was changed and just clutters :) |
Force pushed an amend to do this, and rebased on latest |
|
Thanks! And congrats for your |
There is no current way to set the embedded resource ID before or when saving a resource. Also, the only way to get the ID is after saving the scene (id generated during save and is based on time!) and splitting the resource path to get the ID.
In my situation, I am making a Multiset plugin for collaborative scene editing, and this is a massive road blocker when creating and keeping track of resources over the network. When the host creates a new embedded resource in a scene, I cannot keep track of it at all across clients, because the scene unique id is created at save time, so for each client the resource will actually have a different scene unique id, or not one at all. Which makes it practically impossible to keep all the changes synced, and will mess up git history as everyone's will be slightly different.
I tested this, and it works well, if there are duplicate ids in a scene (but on different resources) at save time, only the first resource will get the id, and the other ones will have regenerated ids.