-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Improve editor support for icons of custom, scripted, and GDExtension classes #75472
Conversation
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 really good!
@@ -50,6 +50,11 @@ GDExtensionManager::LoadStatus GDExtensionManager::load_extension(const String & | |||
extension->initialize_library(GDExtension::InitializationLevel(i)); | |||
} | |||
} | |||
|
|||
for (const KeyValue<String, String> &kv : extension->class_icon_paths) { | |||
gdextension_class_icon_paths[kv.key] = kv.value; |
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.
Probably should have some check to avoid multiple extensions setting the same icon (and deleting it).
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.
Could possibly check in gdextension.cpp
when we fill its internal map that keys match registered types? Two extensions shouldn't be able to register a class with the same name, so this would be sufficient, wdyt?
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.
It should not be able to register class with the same name, but can have icons for anything in the .gdextension
, so I maybe it should only allow icons for the registered classes (but this is probably not trivial, since classes can be registered on different init levels).
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.
Yeah, I would just not support arbitrary icons, at least for now. I can see that checking against registered classes is indeed not trivial, though. Will need to look more into it, but I guess we can address it in a follow-up?
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.
For the time being I think it's ok if we just let whichever gets registered first or last take precedence. Naming conflicts should be rare and we can handle this later on.
Just to clarify: By config file you mean the |
@paddy-exe I mean the |
Also fixes godotengine/godot-cpp#863 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Ah, I guess we still need some form of width limiter to button icons. I'll add one via the theme, similar to #71872, but only for width and working exactly like in Tree and PopupMenu. |
f72ed14
to
b9f028a
Compare
b9f028a
to
6545c0a
Compare
Node that is loaded through GDExtension can have a custom icon. Icon's path can be added to gdextension file. (See godotengine/godot#75472). Commit makes a mention of this in C++ tutorial.
Node that is loaded through GDExtension can have a custom icon. Icon's path can be added via gdextension file. (See godotengine/godot#75472). Commit makes a mention of this in the C++ tutorial.
A node that is loaded through GDExtension can have a custom icon. Icon's path can be added via `gdextension` file. (See godotengine/godot#75472). Commit makes a mention of this in the C++ tutorial.
A node that is loaded through GDExtension can have a custom icon. Icon's path can be added via the `gdextension` file. (See godotengine/godot#75472). Commit makes a mention of this in the C++ tutorial.
A node that is loaded through GDExtension can have a custom icon. Icon's path can be added via the `gdextension` file. (See godotengine/godot#75472). Commit makes a mention of this in the C++ tutorial.
A node that is loaded through GDExtension can have a custom icon. Icon's path can be added via the `gdextension` file. (See godotengine/godot#75472). Commit makes a mention of this in the C++ tutorial.
This is three PRs in a trench coat! But it all makes sense, let me explain.
Streamline class icon resolution in the editor
The first commit reworks some disjointed operations related to class icon lookup. It tries to remove a bunch of duplicated logic from
EditorNode
and various other editor classes.From my search, most of the editor uses
EditorNode::get_object_icon
andEditorNode::get_class_icon
, but some important places weren't. Worse yet, those methods, while introduced at the same time as a part of #21717, worked slightly differently. They followed mostly the same base logic, but had discrepancies for no good reason. So I unified their internal logic (and moved it alongside a cache map toEditorData
for good measure), leaving them only as interfaces for two types of request: by object reference, and by name.I've also made sure that these methods are used in such places as Inspector (for categories) and Editor Help (for the header and inheritance chains). I also added the necessary call to
EditorQuickOpen
, because existing logic was faulty. It tried to fetch an icon from the editor theme based on a script name, but that would never work (implemented in #62417). I'm worried a bit that this may become much slower than before, but there are a bunch of caches in place, so hopefully it scales well for bigger projects.I removed the meta hack for icons, because there doesn't seem to be anything that uses it in the editor anymore. It was introduced sometime around #20560, #21717 refactored it a bunch, and #30697 removed it even more. So currently it's only read in our codebase, but never set. Something may still rely on this existing, as a hack, but I don't think it's worth to support such undocumented "feature".
Make icons of scripted and custom classes fit the editor UI
The second commit builds on top of the first one, and tackles the long-standing issue with custom types having icons overflow various parts of the editor.
First, I made the issue worse. I noticed that icons loaded as a part of the logic modified in commit 1 would never respect editor-specific import options, and thus would never be auto-scaled and auto-converted to match editor colors. So I switched
ImageLoader
forResourceLoader
, but that made every one of these icons to blow out in the same way custom type icons blow out. So it was a great opportunity to work on that as well.I've incorporated (and superseded) #71817 and #71818, improving upon them. There were more issues, however, in the scene tree, in the create dialog, in documentation. Several of these cases could be patched with the existing API that we have for the corresponding controls. There were a couple of exceptions exceptions:
PopupMenu
,TabBar
, andButton
.For the
PopupMenu
andTabBar
the solution was simple, as they are pretty close visually toTree
and have similar sizing considerations. So I borrowed the approach fromTree
andTreeItem
here. Now items inPopupMenu
and tabs inTabBar
can set the upper limit to the icon's width withPopupMenu::set_item_icon_max_width
andTabBar::set_tab_icon_max_width
, respectively. The height is scaled according to the icon's own size ratio.For
Button
there was already a handyexpand_icon
property, but it resulted in issues when the button was way bigger than the icon. So on top of that I had to introduce a limiter for the width as well. I used a theme constant here,icon_max_width
, so that the configuration can be shared and applied globally. This is similar to #71872, but only for the width, so it works exactly likeTree
,PopupMenu
, andTabBar
.In fact, I also added
icon_max_width
theme constant toTree
,PopupMenu
, andTabBar
, so these controls can also have shared configuration within a project. This limit is applied before the max width set on individual items, if any. To help us organize this further I added a new editor theme constant for shared use,class_icon_size
. This was a suggestion from @akien-mga in one of the linked PRs, and I agreed with the idea.Add support for icons in GDExtension classes
Finally, the third commit enhances the existing icons feature to add support for GDExtension.
This is based on @akien-mga's work (see akien-mga@242ce09), I didn't change much from that. Only adjusted the code to match my rework, and added a clean-up step to
GDExtensionManager::unload_extension
missing from the original. That's it.Icons can be added to GDExtension classes in a way proposed by @reduz on RC. Just add the following to your
*.gdextension
file:Paths are in the project scope, same as paths for binaries. On the left side is the name of a class, on the right the path to its icon. Icons aren't bundled in DLLs, as this would complicate the build setup, as far as I understand. If GDExtenstion API is not so great here, we can rework it later. The important part is the editor integration, the behind-the-scenes can be reworked as needed, if another consensus is reached.
So here we are, a bunch of fixes and improvements, that go well together. If we're okay with the new features, these commits could be cherry-picked in 4.0.x, I think. At least the first two.
Fixes #57286.
Fixes #68962.
Closes #69212.
Closes godotengine/godot-cpp#863.