-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Fix accessing editor theme items throughout the UI #81516
Fix accessing editor theme items throughout the UI #81516
Conversation
prev_scene = memnew(Button); | ||
prev_scene->set_flat(true); | ||
prev_scene->set_tooltip_text(TTR("Go to previously opened scene.")); | ||
prev_scene->set_disabled(true); | ||
prev_scene->connect("pressed", callable_mp(this, &EditorNode::_menu_option).bind(FILE_OPEN_PREV)); | ||
gui_base->add_child(prev_scene); | ||
prev_scene->set_position(Point2(3, 24)); | ||
prev_scene->hide(); |
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.
Some old control, but it doesn't appear to be used in any way. It's always hidden, and it's... just floating? I noticed it before, but didn't look further. But now it's removed because it's also requesting a non-existent icon.
void add_editor_plugin(EditorPlugin *p_plugin); | ||
void remove_editor_plugin(EditorPlugin *p_plugin); | ||
|
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.
These aren't implemented on EditorInterface
.
143e5b6
to
56b81f5
Compare
I noticed a couple more issues, one was caught by the new check (so, yay!).
|
56b81f5
to
0c2ca76
Compare
I went ahead and replaced all remaining (Also rebased to make sure this captures all present cases). |
0c2ca76
to
2fa1c03
Compare
2fa1c03
to
a50838f
Compare
a50838f
to
12de7eb
Compare
This also exposes `EditorInterface::get_editor_theme`.
12de7eb
to
8ecc0c4
Compare
Thanks for reviews! |
Closes #81492 (there may be other cases, but what's reported should be fixed now). Closes #81586.
TL;DR
This PR:
EditorInterface::get_base_control
/EditorNode::get_gui_base
, which no longer works in some contexts.EditorIterface::get_editor_theme
to be used by user plugins and by our own C# plugin.How we got here
#81130 changed some things about when and how it would be appropriate to access the editor theme. Previously, we would set it directly on the GUI base node (as well as on a dedicated theme base node, for some reason, and on the editor's main window). While not incorrect in principle, this setup meant that the editor theme would propagate to each and every control or window node as long as the chain of themable nodes was uninterrupted. This led to theme leaks.
As the goal of #81130 was to fix these leaks, among other things, the approach had to be changed. Now we define theme contexts for the editor UI. This means that the GUI base node no longer holds a theme. Instead, the editor theme is used through a global reference. Unfortunately, throughout our own codebase we would often rely on the
get_base_control()->get_theme_*
pattern to access icons and whatnot. And this did work before — even during the editor initialization, even before the editor UI was ready. It worked because the node would hold the theme and would be able to provide these theme properties at any given moment.We even recommended this approach in the documentation to assign editor icons as user plugin icons.
This is pretty bad, because this relies on something that is not guaranteed to be stable. It was fine for the time being, but a GUI rework made it break, as you can see. Preserving compatibility here would be detrimental to our ability to develop the editor. Adding the theme back to the GUI base control would reintroduce the leaks and would also slow down the editor startup.
It's a huge shame that we suggested this approach to users, though this was probably the only alternative at the time. We should've exposed a method to provide editor icons directly, without relying on GUI nodes and their state. Specifically so these icons could be used outside of the GUI code, such as in
How I'm addressing this
For our internal code it's solvable with some find and replace, by accessing
EditorNode::get_singleton()->get_editor_theme
. This resource is ready early on, and can be used by most of the editor code. Some parts of the codebase that rely on this access are GUI elements and should actually be rewritten as dedicated GUI components, so that they can benefit from theNOTIFICATION_THEME_CHANGED
signal and correctly update when users change the editor theme. But I left it as is for now, with some TODOs.Gizmos are a bit trickier, as these icons are used in constructors to pre-populate materials. I haven't worked with gizmos for quite some time, let alone with Godot 4 gizmos, so I don't remember if we could recreate these materials later on, or if it would be a bad idea. For now I left them as is. But it may be sometime to look into (also may want to add some helper methods so that creating materials from editor icons doesn't take over a hundred characters per line).
Then come user plugins, and our own C# editor plugin, which is limited, for the most part, by our public API. For these I'm adding a new method to
EditorInterface
, which allows fetching the editor theme. I'm not too happy about adding more methods toEditorInterface
when my goal is to split its responsibilities between multiple single-purpose classes, but that's the only place available now, so alas.For the C# editor plugin I recommend reworking it a bit, so the editor run bar icon is correctly updated with the editor theme. Currently it's only set when the plugin is enabled, and ignores theme changes. We've fixed a similar issue in the build panel before. I should've identified this when reviewing #79357. cc @raulsntos
For user plugins #81130 kind of becomes a breaking change, as far as accessing editor icons goes. This doesn't affect plugins that use custom icons, and this doesn't affect GUI code in these plugins, as long is it's written correctly (using theme propagation). Things like
_get_plugin_icon()
and inopportune accesses toget_base_control
would need to be fixed by plugin creators. The newEditorInterface::get_editor_theme
method will prove useful here, and it should also be possible to update plugins in a way that works both with 4.1 and 4.2, if that's required.I think that's an acceptable compromise.
Reporting theme misses
@Calinou suggested:
I like the idea, but I'm not sure there is a good way to implement this without adding editor-specific code to controls and windows, and I don't like that idea. The reason why this approach may be required is that most of these issues are because of trying to access icons and styles before there is the editor theme, or before you can access it in some context. So this can only be checked by the node itself.
For now I've added a new type,
EditorTheme
which provides warnings for cases when the editor theme is there, but the theme item requested is wrong. This actually caught a few issues, but none of the ones reported in #81492. I fixed them together with others.The new
EditorTheme
class is added as a separate independent commit and can be dropped or reverted later. There are some implementation details that I would like to change so it's less of a nuisance to maintain, but that can be done later. Trying to keep changes to the minimum here.