Skip to content
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

Add export_resource_uid annotation #91815

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

huwpascoe
Copy link

@huwpascoe huwpascoe commented May 10, 2024

Resource UIDs can now be selected from the inspector in the same way a Resource would be selected.

EditorResourcePicker has new path_only property to facilitate the annotation that hides menu items unrelated to UID selection.

Example usage:

@export_resource_uid("ResourceTypeName") var item: String

func load_item() -> ResourceTypeName:
	return load(item)

Closes godotengine/godot-proposals#9216
Closes godotengine/godot-proposals#9552

@tokengamedev
Copy link

in my point of view @export_uid makes more sense than @export_resource_uid. This is up to the reviewer to decide, as I am not fully aware of the context.

@huwpascoe
Copy link
Author

in my point of view @export_uid makes more sense

I had considered that name, went with the longer one because it gives a "use me sparingly" feeling and I don't know if other uid might be used in future.

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Note that you could use @export_custom(PROPERTY_HINT_RESOURCE_UID, "Type") instead of @export_resource_uid("Type"). You could also add a test, see export_variable.gd.

modules/gdscript/doc_classes/@GDScript.xml Outdated Show resolved Hide resolved
@Summersay415
Copy link
Contributor

Can this annotation also be used with String type? I think UIDs as String are more common, and it will be easier to use in code (no need to ResourceUID.get_id_path)

@huwpascoe
Copy link
Author

Can this annotation also be used with String type?

My dumb c++ brain shrieks "wasteful!" at an extra string allocation/conversion, but what do the maintainers think? @Calinou @AThousandShips

@AThousandShips
Copy link
Member

AThousandShips commented May 12, 2024

The allocation isn't necessarily an issue, but if we use a String it'd be very much like the existing path property hints, but simply using an UID instead, but still lacking any UID pairing and updating features like the existing resource system has, which feels half baked, see for example #87418 (comment)

So I'm unsure either way here, having just a convenient way to fetch the path as an UID instead of a file path is useful, but I'm not sure it warrants a whole new property hint

Also we could have this hint support both int and String and just mean "fetch the path, which must be in res:// as an UID"

@huwpascoe
Copy link
Author

The allocation isn't necessarily an issue, but if we use a String it'd be very much like the existing path property hints, but simply using an UID instead, but still lacking any UID pairing and updating features like the existing resource system has, which feels half baked, see for example 87418

After reading that issue, it seems UID in general is half-baked? In that thread people are saying "use both" which entirely defeats the purpose of it right? I wanted this feature because @export_file is just a string, that breaks the moment anything is moved. But if UID can't be trusted either then what other options are there...

@AThousandShips
Copy link
Member

After reading that issue, it seems UID in general is half-baked? In that thread people are saying "use both" which entirely defeats the purpose of it right?

Not really IMO, UIDs are safe generally, but there are contexts where they might be reset, they shouldn't but they might be, and being able to fall back on the other path is very useful and powerful, it can be trusted generally, but both together is the best

@huwpascoe
Copy link
Author

That's good at least.

About making this a "fully baked" solution, it's not possible. Not without rewriting large sections of core/io.

@simo498
Copy link

simo498 commented May 16, 2024

Note that you could use @export_custom(PROPERTY_HINT_RESOURCE_UID, "Type") instead of @export_resource_uid("Type"). You could also add a test, see export_variable.gd.

As i saw the default custom export with the PROPERTY_HINT_RESOURCE_UID does not support creating arrays of UIDs, and it could still be convenient to have a "discoverable" way of doing that, since some hints are a bit hidden by the code suggestion menu

@huwpascoe huwpascoe closed this May 16, 2024
@huwpascoe huwpascoe reopened this May 16, 2024
Resource UIDs can now be selected from the inspector
in the same way a Resource would be selected.

EditorResourcePicker has new path_only property
that hides menu items unrelated to UID selection.
@huwpascoe
Copy link
Author

Everything is "uid://" String now to keep things simple.
Array types are supported. (turns out this happens automatically)

image

This PR is now complete as possible. Would be nice if it can be merged for 4.3.

@AThousandShips
Copy link
Member

We're in feature freeze so this might have to wait for 4.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants