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

Closed

Conversation

huwpascoe
Copy link
Contributor

@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
Contributor 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
@huwpascoe huwpascoe force-pushed the export_resource_uid_2 branch from 194ac38 to 997ed43 Compare May 11, 2024 18:51
@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
Contributor 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 huwpascoe force-pushed the export_resource_uid_2 branch from 997ed43 to 8444f0e Compare May 12, 2024 15:06
@huwpascoe
Copy link
Contributor 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
Contributor 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 huwpascoe force-pushed the export_resource_uid_2 branch from 8444f0e to 81b0860 Compare May 17, 2024 04:23
@huwpascoe
Copy link
Contributor 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

@huwpascoe huwpascoe marked this pull request as draft August 26, 2024 21:39
@Summersay415
Copy link
Contributor

Why draft?

@huwpascoe
Copy link
Contributor Author

I think it can be further improved. I'll fix the conflicts and update it when I'm confident I can get it merged.

@huwpascoe
Copy link
Contributor Author

huwpascoe commented Sep 1, 2024

Resource Path

icon
I made it a plugin.

Now available in asset library
https://godotengine.org/asset-library/asset/3291

New improved syntax

@export_file("PackedScene")
var some_scene: String

Future Changes

  • I'll need to submit a PR to allow hiding of EditorResourcePicker context menu items so the plugin can make use of that.
    Thanks to @Summersay415 for achieving this without engine modification
  • If there's demand I could still make this engine-native, in which case I'll reopen the PR.

@simo498
Copy link

simo498 commented Sep 23, 2024

I think this should be native in the engine. There is no built in way to do that, like Unity has with assets. This could lead to load every resource with the scene initialization and might cause reference cycles, something that cannot be handled natively. Making a plugin is fine, but it might be #difficult to discover, especially for beginners (which i think is the main target of Godot)

@KoBeWi
Copy link
Member

KoBeWi commented Nov 11, 2024

This was effectively implemented by #97912

@huwpascoe
Copy link
Contributor Author

It's partially implemented.

This is encouraging enough that I'm considering making the plugin an engine feature.

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