Skip to content

Add @export_file_path to export raw paths (no UID) #105414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2025

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 15, 2025

Adds a new PROPERTY_HINT_FILE_PATH and equivalent @export_file_path annotation, which is denotes a raw path that doesn't turn into UID. I also added extra info in the annotation descriptions.

Closes #104379
Closes #104818

@KoBeWi KoBeWi added enhancement topic:gdscript cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Apr 15, 2025
@KoBeWi KoBeWi added this to the 4.5 milestone Apr 15, 2025
@KoBeWi KoBeWi requested review from a team as code owners April 15, 2025 11:23
@dalexeev
Copy link
Member

I did not add a new hint, because we have PROPERTY_HINT_FILE and PROPERTY_HINT_SAVE_FILE, so I'd have to add 2 hints. I just opted to add a small hack for the property hint. You can add /disable_uid at the end and it will disable UIDs. It does not break compatibility (unless it's valid to use slashes in the filters?).

I think this still breaks compatibility because the default behavior is changed and the user needs to do something to restore the old behavior. You fixed it for the @export_file annotation with a hack, but for users who used PROPERTY_HINT_FILE with @export_custom, _get_property_list() or _validate_property() the regression is not fixed because the default behavior does not match what it was in 4.3.

Also, it is somewhat inconsistent that @export_file exports the path, while PROPERTY_HINT_FILE without an additional suffix (e.g. with @export_custom) exports the UID. I think we should add a new export hint PROPERTY_HINT_FILE_UID, just like we have @export_file and @export_file_uid.

@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 16, 2025

I wanted to avoid adding new hint, because it would require updating every property to use it (we want UIDs to be used in the editor by default).

EDIT:
Pushed.

@KoBeWi KoBeWi force-pushed the disable_uid_here branch from a3e5b88 to b4f0021 Compare April 16, 2025 11:23
@KoBeWi KoBeWi requested review from a team as code owners April 16, 2025 11:23
@reduz
Copy link
Member

reduz commented Apr 22, 2025

I think all should use UIDs by default, so this would definitely not be desirable. We have come to a point in Godot that when we fix something for some people, we break something else for others but ultimately what should be done is what is expected to be the right behavior.

I think if this is really wanted, it should be the opposite (add an @export_file_path instead).

@TokisanGames
Copy link
Contributor

@reduz Juan, please read this comment. The inspector doesn't show a UID. It gets translated into a file name string. Similarly, if I request a file name as type String in a script, I do not want a UID. I can't think of any reason why I'd ever want the UID in a script unless I'm making an audit tool. I also can't think of any reason I'd want a UID in the inspector. The same logic applies. UIDs for systems, strings for people.

If you make @export_file_path to provide the old behavior of @export_file, I expect 99.99% of people will use the former, no one will ever want the UID in script, and the multiple PRs will have broken and extended the API for no gain.

@nanoquack
Copy link

@reduz I'm not sure what the general policy in Godot for breaking changes is? So far I was under the impression, that bigger breaking changes are not introduced between minor versions?

The docs (https://docs.godotengine.org/en/stable/about/release_policy.html#doc-release-policy) at least state:

The minor version is incremented for feature releases that do not break compatibility in a major way. Minor compatibility breakage in very specific areas may happen in minor versions, but the vast majority of projects should not be affected or require significant porting work.

I am an incredibly avid fan of Godot, but changes made in a way like this one make me sometimes worry what else can unexpectedly break compatibility when upgrading between minor version. Is it really a good idea, to make people wary to upgrade from one minor version to the next one?

What makes this change also very problematic in my eyes is, that during editing, the inspector shows the file path, but during debugging it shows the uid.

@geekley
Copy link

geekley commented May 16, 2025

I think all should use UIDs by default, so this would definitely not be desirable. We have come to a point in Godot that when we fix something for some people, we break something else for others but ultimately what should be done is what is expected to be the right behavior.

@reduz Understandable, and you guys' work is amazing, but I have to disagree that fixing something has to break something else if solutions can be engineered a bit more carefully. I'm worried less about this particular issue (as I tend to triple-check everything so in my case I was able to catch it before it became major for me) and mostly about the way this sort of thing wasn't originally understood as, acknowledged and documented as a breaking change.

It changes the behavior of @export_file, changing what it "returns" while not updating the docs accordingly. It's akin to silently changing a method's return type to a subclass or even an entirely different sibling class.
Regardless of whether it's better or worse, UIDs are not a superset of res paths, res paths have a different structure, purpose, use case, compatibility. So even if it technically doesn't break for many people for the way their workflow "should" be, IMHO that's still a mistake to just treat it as a non-breaking change.

Like I said in #104379, there's valid use cases for not wanting refactoring to apply, on purpose. And even if you disagree, still, in that case the proper route would have been to introduce a new annotation and if necessary deprecate @export_file with a warning. Btw, I'm not suggesting to deprecate it (there's valid uses!) just showing a way this sort of change could have been handled a bit better for your viewpoint that user code should move to UIDs.

If UIDs should be preferred in most cases, the best way is to add a very clear warning to the docs of @export_file that @export_uid or whatever is the preferred alternative to this, and explain why.
It should not be changed and I'm against even deprecating it, because, like I said before:

Side note: one of the very reasons I moved away from Unity into Godot was the way asset identity is tightly tied to its path, not based on an AssetDatabase like Unity. Just like in code classes/namespaces, it forces you to think in a more organized way, and I see that as an absolutely good thing, as long as refactoring works at least on most cases.

Res paths have its problems, but it's also one of the strongest points of Godot, along with the fact it's git-friendly and I can understand every asset I'm creating textually with a glance (and check for mistakes in git diffs). One of the reasons I respect you guys' work a lot is the fact it's so open to being analyzed and understood like everything in the Linux world.

PS: Sorry for the ping and long comment; this isn't a rant, I'm just trying to explain my point of view

@KoBeWi KoBeWi force-pushed the disable_uid_here branch from b4f0021 to e9fa3bd Compare May 21, 2025 20:42
@KoBeWi KoBeWi requested review from a team as code owners May 21, 2025 20:42
@KoBeWi
Copy link
Member Author

KoBeWi commented May 21, 2025

This was discussed in core meeting and it was decided that we should stick to UIDs, so I did what was suggested by reduz. As mentioned in the OP, the PR also improves the documentation, and I will also do a follow-up that exposes some ResourceUID helpers to make it easier to work with UIDs.

The discussion eventually derailed into replacing paths with a dedicated type (similar to godotengine/godot-proposals#5029). It would be a definitive solution for path/UID problems, for now it's being considered as a potential option.

@KoBeWi KoBeWi changed the title Restore old behavior of @export_file and add @export_file_uid Add @export_file_path to export raw paths (no UID) May 21, 2025
@geekley
Copy link

geekley commented May 21, 2025

@KoBeWi Does this PR change the inspector in any way?

IMO the inspector needs to have some visual distinction between both annotations. If the UID annotation will show the field as uid://... (preferably with its translated res://... path on hover) that's fine, but if not, then there needs to be a visual indication that the field is actually an UID although it shows a path. For example the file select icon could be a different one e.g. a "resource select" icon or a slight variation like adding a "link" symbol to differentiate it indicating whether or not refactoring would apply.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 21, 2025

Does this PR change the inspector in any way?

It doesn't but your idea sounds interesting. I'll try to implement something and open a separate PR.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I agree that adding a new annotation makes the most sense.

I understand that some users experienced a compat breakage going from 4.3 to 4.4, but if we change it back we break compat for users going from 4.4 to 4.5. And defaulting to UIDs is what we believe is the most useful behavior to allow refactoring.

Adding a new annotation gives users still on 4.3 due to this compat breakage the possibility to search and replace @export_file with @export_file_path in their scripts before upgrading from 4.3 to 4.5.

@geekley
Copy link

geekley commented Jun 12, 2025

Reminder that there needs to be a mention in https://docs.godotengine.org/en/stable/tutorials/migrating/upgrading_to_godot_4.4.html
Or would that have to be in a separate PR @akien-mga? (sorry I'm not to familiar with how this sort of change works here)

@akien-mga
Copy link
Member

akien-mga commented Jun 12, 2025

Reminder that there needs to be a mention in https://docs.godotengine.org/en/stable/tutorials/migrating/upgrading_to_godot_4.4.html Or would that have to be in a separate PR @akien-mga? (sorry I'm not to familiar with how this sort of change works here)

The documentation lives in a separate repository: https://github.com/godotengine/godot-docs
So it can't be modified in this PR.

But yes I agree that the compat breakage needs to be made explicit in that page.

@akien-mga akien-mga merged commit 2270224 into godotengine:master Jun 12, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release enhancement topic:core topic:gdscript
Projects
None yet
7 participants