-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
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
Conversation
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 Also, it is somewhat inconsistent that |
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: |
a3e5b88
to
b4f0021
Compare
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). |
@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 |
@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:
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. |
@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 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 If UIDs should be preferred in most cases, the best way is to add a very clear warning to the docs of
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 |
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. |
@export_file
and add @export_file_uid
@export_file_path
to export raw paths (no UID)
@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 |
It doesn't but your idea sounds interesting. I'll try to implement something and open a separate PR. |
There was a problem hiding this 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.
Reminder that there needs to be a mention in https://docs.godotengine.org/en/stable/tutorials/migrating/upgrading_to_godot_4.4.html |
The documentation lives in a separate repository: https://github.com/godotengine/godot-docs But yes I agree that the compat breakage needs to be made explicit in that page. |
Thanks! |
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