Register custom icons in class macro #1407
Conversation
Paperback
force-pushed
the
class_icons
branch
from
71f754c to
5fbf8fc
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1407 |
Yarwin
added
feature
There was a problem hiding this comment.
First of all – thanks!!!!
-
ICON_STRINGS_BY_NAMEwhich keepsGStrings 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 alsocleanup
Storing GStrings to keep them valid as long as library is loaded is fine, just don't leak them. -
This feature is not tested at all – I think I would create new file in
itest/rust/src/register_testsand annotate it with#. 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. -
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?) -
#[class(icon = MY_ICON)]is not documented in#[derive(GodotClass)]docs. -
Address various silly things, like
SAFETYremarks under now-safe blocks, comments not ending with a dot etc.
|
This is great feedback, I'll make some changes tomorrow and get back to you. |
Paperback
force-pushed
the
class_icons
branch
from
5fbf8fc to
8c511a5
Compare
|
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 At the moment the integration tests I've added are just checking Thanks! |
Paperback
changed the title
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
|
I think using
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
force-pushed
the
class_icons
branch
2 times, most recently
from
b7bc46a to
08ccba9
Compare
|
Ok, thank you both for the feedback, I've reworked this a little bit:
Should I add Thanks! |
Paperback
force-pushed
the
class_icons
branch
from
08ccba9 to
3623307
Compare
|
I'm getting instant segfault while trying to run itest via BacktraceSee memcheck run: https://github.com/godot-rust/gdext/actions/runs/19445857918/job/55672752869?pr=1407. |
| pub struct LoadedClass { | ||
| name: ClassId, | ||
|
|
||
| // Class icon needs to be retained for registered for class lifetime, this is not accessed directly. |
There was a problem hiding this comment.
Grammar? Not clear what you mean.
| #[derive(GodotClass)] | ||
| #[class(init, base=RefCounted, icon = ICON)] | ||
| struct ClassWithIconRefCounted { | ||
| base: Base<RefCounted>, | ||
| } | ||
|
|
||
| #[derive(GodotClass)] | ||
| #[class(init, base=Node, tool, icon = ICON)] | ||
| struct ClassWithIconNode { | ||
| base: Base<Node>, | ||
| } |
There was a problem hiding this comment.
Is there any notable difference in the icon logic for Node and RefCounted?
If not, it should be enough to just test with RefCounted (to ensure it works also for non-nodes).
| /// Class icon resource path. Will use this icon if available. | ||
| /// | ||
| /// You can also set an icon via `#[class(icon = "path/to/icon.svg")]`. | ||
| fn icon() -> &'static str { | ||
| "" | ||
| } |
There was a problem hiding this comment.
Not 100% sure if this should be in the GodotClass trait... there are quite a few registration properties that aren't currently exposed like this (e.g. #[class(internal)]). For builder APIs in the future, it could otherwise be a separate step:
builder.class::<MyClass>()
.icon("path/to/icon.svg")
.internal()
.build()But if we decide GodotClass is the right place -- which needs further discussion -- then:
-
This should document how the path has to look, i.e.
- relative to where
- whether it should start with
res://or not - maybe an example
-
Could be named
editor_icon. Since trait methods can be called unqualified, and user-defined classes having their owniconisn't that unlikely.
| // Write the extension default icon path in a `Global`. | ||
| // Will be provided to Godot during class registration (if not empty) and no class icon is provided. | ||
| // Empty by default. Resets on deinitialization, see `gdext_on_level_deinit()`. | ||
| #[cfg(since_api = "4.4")] | ||
| { | ||
| let mut icon = DEFAULT_ICON.lock(); | ||
| *icon = E::default_icon(); | ||
| } |
There was a problem hiding this comment.
Might be simpler to exclude the whole fallback logic from this PR and not try to achieve too many things at once. Icon fallbacks can easily be retrofitted later.
| let chosen = if !icon.is_empty() { | ||
| icon | ||
| } else { | ||
| // Default icon from `ExtensionLibrary::default_class_icon()`. | ||
| *crate::init::DEFAULT_ICON.lock() | ||
| }; |
There was a problem hiding this comment.
When you have if/else with the condition being a ! negation, it's usually easier to understand if you swap the branches and invert the condition.
| c.icon_path = crate::builtin::GString::from(chosen); | ||
| c.godot_params.icon_path = c.icon_path.string_sys(); |
There was a problem hiding this comment.
string_sys() creates a raw pointer to that field. If you move c, you will have use-after-free UB.
It's admittedly hard to see if you are not familiar with this; maybe we should rethink this API (just requiring unsafe doesn't solve the problem).
4 participants
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
iconattribute to the#[class]macro.Because the icon attribute is a
TokenStream, we can also write:Considerations
GDExtensionClassCreationInfo4::icon_path.gdextensionfile[icon]section declarations seem to take precedenceThanks!