Register custom icons in class macro

[Paperback]

Thanks for the awesome work everyone puts into this project. This is my first rust contribution, so I'm keen to get some feedback and learn.

This pull request closes issue #1200.

It adds a new icon attribute to the #[class] macro.

#[derive(GodotClass)]
#[class(base = Sprite2D, icon = "res://addons/my_extension/player.svg")]
pub struct PlayerOne {}

// default icon behaviour
#[derive(GodotClass)]
#[class(base = Sprite2D)]
pub struct PlayerTwo {}

Because the icon attribute is a TokenStream, we can also write:

const MY_ICON: &str = "res://icon.svg"
#[derive(GodotClass)]
#[class(icon = MY_ICON)]
pub struct Player {}

Considerations

• Requires api 4.4 >= as it uses GDExtensionClassCreationInfo4::icon_path
• The .gdextension file [icon] section declarations seem to take precedence

Thanks!

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1407

[Yarwin]

First of all – thanks!!!!

---

1. ICON_STRINGS_BY_NAME which keeps GStrings is not being pruned during deinit of the library, which means memory leak during, for example, hot reload.
   Please check it and report if I'm wrong. See also cleanup
   Storing GStrings to keep them valid as long as library is loaded is fine, just don't leak them.

2. This feature is not tested at all – I think I would create new file in itest/rust/src/register_tests and annotate it with #![cfg(since_api = "4.4")] (similarly to register_docs.rs). Tests allows us to watch for memory leaks, shield us against regression etc – additionally we need to guarantee that all the used APIs are OKay and give an example how they can be used.

3. I would remove ExtensionConfig (up to discussion) – right now its only purpose is to set default fallback icon for classes, which will be useless and counter-productive after 4.6: godotengine/godot#108607. I'm also finding it a little confusing (like, docs mention tools and build scripts – how would I even use it there?)

4. #[class(icon = MY_ICON)] is not documented in #[derive(GodotClass)] docs.

5. Address various silly things, like SAFETY remarks under now-safe blocks, comments not ending with a dot etc.

[Paperback]

This is great feedback, I'll make some changes tomorrow and get back to you.

[Paperback]

I have removed the entire gdextension icon section; I can always create another PR with that in mind once we have a better idea.

Just poking around LoadedClass and ClassMetadata, maybe instead of the specific icon global guard, I should use that instead? For cleanup and integration tests, I want to be able to access that lock so I've made it pub(crate) for now, but I resent that.

At the moment the integration tests I've added are just checking is_instance_valid() but ideally I should check if icons are set in that global guard I think.

Thanks!

[Bromeon]

I have removed the entire gdextension icon section; I can always create another PR with that in mind once we have a better idea.

On that note, I believe it would be better to add a virtual method returning a string, rather than do this via proc-macro. While it's inconsistent with the #[class(icon)] at first, it has some advantages:

• It allows users to provide values that aren't available at compile time (e.g. load from a file).
• If we add builder APIs (#4), users will be able to provide runtime strings for method calls, too.
• There's IDE and type system support (minor point since we already have proc-macros everywhere).

[Yarwin]

looks pretty much OK, I'll give it a proper run and see if there are any weird issues/quirks – I think we should move icon GString to LoadedClass, but it shouldn't change too much in the implementation/behavior itself.

[Yarwin]

Just poking around LoadedClass and ClassMetadata, maybe instead of the specific icon global guard, I should use that instead? For cleanup and integration tests, I want to be able to access that lock so I've made it pub(crate) for now, but I resent that.

I think using LoadedClass would be the right call! We wouldn't have to worry about cleanup and whatnot, since it would be handled auto-magically while dropping the loaded class itself (unregister_classes does it automatically after removing Vec of loaded classes).

At the moment the integration tests I've added are just checking is_instance_valid() but ideally I should check if icons are set in that global guard I think.

IMO checking if Godot is able to register given class should be enough – ideally we should be able to check if icon has been properly set on Godot's side, but (currently) we don't have any API to access that.

[Paperback]

Ok, thank you both for the feedback, I've reworked this a little bit:

1. Added ExtensionLibrary::default_icon() -> &'static str. Can be changed to GString.
2. Changed it so icon string is on LoadedClass instead of a global lock.
3. Added init::DEFAULT_ICON global, resets to empty on deinitialization.
4. Simplified icon itest.

Should I add default_icon() to the hot-reload test? That feature is not included in tests at the moment.

Thanks!