Expose path properties save UID internally if referencing a resource

[reduz]

Currently, if a user or some resource exposes a path in a script, if this references a resource path it will be saved as a full path.

This will make refactoring not work if the resource pointed towards changes location or is renamed.

This change makes it so an uid:// path is saved internally in case a path to a resource has been selected, effectively making it refactor proof.

Screenshots:

Property is edited as a path:

But value is saved as UID if a resource.

Fixes #97931

[caimantilla]

This would also make it so that UID paths are always edited as res:// file paths in the inspector, right? Like, if somebody already used a UID path before, it would no longer display that way using this PR

[KoBeWi]

Yes, but why would you want to see UID in the inspector?

[KoBeWi]

This can be simplified to

Suggested change

	full_path = ResourceUID::get_singleton()->get_id_path(ResourceUID::get_singleton()->text_to_id(full_path));
	return ResourceUID::get_singleton()->get_id_path(ResourceUID::get_singleton()->text_to_id(full_path));

and you can make full_path const.

[akien-mga]

I was about to amend those changes in but I'm not convinced this improves the code quality much. I quite like the pattern where we have a single return statement at the end and just do conditional changes to the return value.

[caimantilla]

Yes, but why would you want to see UID in the inspector?

I certainly don't, but it's something that should be noted.

Also, while it should probably be enabled by default, I think that this feature itself should be configurable on a per-project basis. Some people might just prefer to store the file paths instead of UIDs. There are also scenarios where UIDs aren't optimal (eg. replacing a broken scene, the user expects references to stay valid because the filename is the same but suddenly they're broken).
New hints that can easily be toggled following the existing arguments of @export_file might also be good, instead, so that the user has more control over this rather than a global setting. Any engine classes which use PROPERTY_HINT_FILE would need to be updated, but I can't really think of many (if any) that use file paths as resource references outside the project settings.

One more thing: should the inspector support resource previews this way? Like, an editor setting that (visibly) replaces the file path with the existing resource property GUI if it points to a valid resource, or something like that... Maybe that makes more sense for a different PR, but given that this PR already ties file paths and resource references more closely, it might make sense to follow it up with something like that

[caimantilla]

I've thought about this a little bit, and adding a new property hint and associated annotation (eg. @export_resource_path) that has support for type constraints rather than file extensions (though this may not be possible for script classes) is probably the best way to go about this. As things are, this will break compatibility for any code that relies on processing file paths.

Still, say that modifying, using the base directory, file name, file extension, or whatever else from those paths weren't an issue...
Since FileAccess doesn't seem to account for UIDs, this would totally break my uniform_art module, along with any other tools that rely on the user entering a file path which will be used to load a file in the editor rather than a Resource.
In my case, the module's primary purpose is to generate uniform GUI assets (eg. character icons) from external art sources, which loads the Image directly since I use the same source art as a texture (often downscaled on import) for other purposes in my game.

As stated previously, a custom property hint would cleanly allow for behavior more befitting of the feature, like the ability to use a similar GUI to the one used for Resource properties. This should also make it more feasible to track references, along with storing both the file path and UID, just as is done when referencing external resources. Finally, if done in the way that I've proposed, this should also resolve godotengine/godot-proposals#7363. A circular dependency done in this way is a coveted feature, and while direct references may not be possible, this should cover most use cases where two separate resources on-disk need to reference each other (eg. type match-ups in a Pokémon-style game).

(I am willing to take a stab at this feature if all sounds good.)

[KoBeWi]

The new behavior only applies to files in res:// and only with UID. How does that break your module?

[caimantilla]

The new behavior only applies to files in res:// and only with UID. How does that break your module?

Because the files are under res://. FileAccess doesn't take UIDs into account, but all those paths would be forcibly stored as UIDs since the image is also imported (it's just that the imported version isn't used by the module).

[KoBeWi]

But the property is still accessed as path, it's only stored as UID. And even if it was UID-only, it's easy to retrieve the path from UID.

[Repiteo]

Thanks!

[caimantilla]

But the property is still accessed as path, it's only stored as UID. And even if it was UID-only, it's easy to retrieve the path from UID.

This functionality is totally obfuscated, though. This leads to serious headaches, as most developers will have no idea that this conversion even occurs because the file path is displayed normally in the editor; furthermore, having to handle cases where converting from a UID only might be necessary is just needless hassle.

[KoBeWi]

tscn/tres support comments. If users will really get confused, we could annotate the property with the corresponding path (though currently there is no support for that). I doubt it will be necessary though, most users will at most see these properties in git diffs and won't really think about it.

[caimantilla]

tscn/tres support comments. If users will really get confused, we could annotate the property with the corresponding path (though currently there is no support for that). I doubt it will be necessary though, most users will at most see these properties in git diffs and won't really think about it.

It's more so code that deals with file paths that I was referring to. Sure, I'm familiar with this change, and know to account for it, but as you said, most users won't think about it. This includes tool developers. Unless this behavior is very clearly documented, it will cause confusion. And... if the user isn't meant to think about it, then shouting warnings about it is a bit silly.
Hiding information like this doesn't help anybody; it's to be expected if there's a GUI for this, but not direct input. If a user sets a Vector2 property, should it magically be scaled down when saving to disk, and on the object itself, and then scaled back up to a different magnitude in the editor, unbeknownst to the user?
Furthermore, there is a reason that full resource references save both the file path and UID: UIDs often break! Even if most of these issues have been ironed out, UID bugs still crop up; I was dealing with one just the other day in Godot 4.3.
So, please, reconsider merging. Not only are there numerous issues which will manifest as a result of this PR, but it's not even a satisfactory solution compared to what was proposed here, for example: #91815
A new way to serialize resource references that doesn't require actually loading them, but does allow references to be tracked and paths to be auto-updated by the editor, along with a dedicated GUI for it that supports type hints, would be far more satisfactory. In other words, even if all the other issues are disregarded, the implementation of this PR is simply half-baked from a UX perspective.
There is no reason to settle.

[Summersay415]

Regression found: #99116

[KoBeWi]

If a user sets a Vector2 property, should it magically be scaled down when saving to disk, and on the object itself, and then scaled back up to a different magnitude in the editor, unbeknownst to the user?

Well, a similar thing is happening with rotation. It's edited in degrees, but stored in radians.

I was dealing with one just the other day in Godot 4.3.

Is it reported?

[reduz]

@caimantilla The plan is the move the whole engine gradually to UIDs and deprecate paths. I know its an annoyance, but the fact that refactoring basically does not work with paths is a much bigger annoyance and makes it hard to use the engine for bigger projects.

[reduz]

@KoBeWi the problem with adding a comment is that, if you refactor, the comment will be wrong and will not be renamed.

[Summersay415]

But what about leaving @export_file as is and adding something like #91815 and encouraging using that instead? It also provides useful inspector widget

[reduz]

@Summersay415 The problem is that we've seen many cases of users doing this, assigning a lot of paths, then then being super frustrated when refactoring because they don't get updated. By the time they realize they needed a special annotation its too late, so this behavior should happen by default.

The idea is that, by philosophy, paths should not be stored anywhere in future engine versions.

[Summersay415]

This looks like topic for more wide discussion, I think.

[reduz]

@Summersay415 We discussed this for hours at the Sprint in Berlin, and the conclusion was this. Benefits outweight drawbacks.

[Summersay415]

Okay, then #91815 is Godot 5 material?

[reduz]

@Summersay415 No idea, to my awareness there isn't any formal discussion on Godot 5 at the moment.

[caimantilla]

@Summersay415 The problem is that we've seen many cases of users doing this, assigning a lot of paths, then then being super frustrated when refactoring because they don't get updated. By the time they realize they needed a special annotation its too late, so this behavior should happen by default.

The idea is that, by philosophy, paths should not be stored anywhere in future engine versions.

If this is the case, then directories should also have UIDs, which would be used by @export_dir. (This would be appreciated, personally, but IDK how people feel about having folder metadata files...)
IMO, mixing the concept of a resource and a file shouldn't be happening in the first place. If this is really what was agreed on, though, then it shouldn't have so many holes. Not only that, but the UID should be converted to a file path during deserialization, not only in the editor. Assuming that all the UID bugs are stamped out (both file path and UID should be stored to prevent things from breaking anyways) then this would solve the issue of additional complexity for the user, at least. The issue is that this then would have those references break as long as the resources are loaded... so, really, the solution is still to add a feature to the resource formats for resource paths.

[simo498]

@reduz i tried the feature, but i faced the problem that the path does not update, even when you rename or move the file it keeps showing the same path