Simplify `EditorNode` by removing `gui_base` and `theme_base`

[AaronRecord]

Essentially turns EditorNode into gui_base. theme_base is redundant either way.

From a quick inspection everything seems to working just fine as before ¯\(ツ)/¯ I could be missing something, but I tried changing the theme from the default theme to light theme and everything looks as expected

[trollodel]

While this seems an improvement, I still don't understand why editor theme items are fetched in that way. I think most of them can be fetched in the following 2 ways:

• Use the Control::get_theme_* directly instead of using a higher level node. This is what is done for theme items of builtin nodes, if I'm not mistaken.
• Use the editor's Theme resource directly, removing the overhead of the Control machinery when getting theme items.

Just an opinion on that.

[AaronRecord]

While this seems an improvement, I still don't understand why editor theme items are fetched in that way. I think most of them can be fetched in the following 2 ways:

• Use the Control::get_theme_* directly instead of using a higher level node. This is what is done for theme items of builtin nodes, if I'm not mistaken.
• Use the editor's Theme resource directly, removing the overhead of the Control machinery when getting theme items.

Just an opinion on that.

Agreed, but that should probably be handled in a future PR

[AaronRecord]

~~I just ran a benchmark (using CI builds) and it turns out this speeds up editor startup time by 17%... wasn't expecting that 😀~~

I thought it was too good to be true, turns out it's actually a performance regression from after this PR was made :/ This PR has no measurable effect on performance.

[YuriSizov]

I think, to be safe, I'd keep the EditorNode itself as Node and keep one GUI root node with the theme applied to it as its child. And then I'd still re-evaluate every call to EditorNode::get_singleton()->get_gui_base() and refactor it to not do that and use theme propagation instead.

PS. Did a quick edit on the current master to only remove one node and didn't witness any dramatic improvements, trying to open a non-empty project. Around 13 seconds before and after, with debug builds. Would be interesting to test this PR as is, if it was made compatible (rebased).

[AaronRecord]

I think, to be safe, I'd keep the EditorNode itself as Node and keep one GUI root node with the theme applied to it as its child. And then I'd still re-evaluate every call to EditorNode::get_singleton()->get_gui_base() and refactor it to not do that and use theme propagation instead.

I guess I'm okay with this, but what exactly is the risk with turning EditorNode into a Panel? I feel like it just makes the code simpler by removing a bunch of get_gui_base()s.

[YuriSizov]

I guess I'm okay with this, but what exactly is the risk with turning EditorNode into a Panel?

EditorNode is the root of the entire editor tree, and people who use it (or the code that uses it) may not expect it to be a GUI element that works by GUI element's rules. I'd even go further and refactor it to separate it from the GUI part completely, and keep it for the high-level initializations only (like settings and plugins?). I'd expect EditorInterface to represent the GUI base, I guess.

That said, I don't have a concrete example to point to and say "ah-ha, it breaks!" 🙃

[YuriSizov]

Okay, given that you've said on RC that the observed performance boost is more like a performance regression in the current master, I guess that closes that angle. But I love the idea of simplification here overall. So I'd definitely like to have this PR shaped into something regardless of the performance improvement.

Expanding on the idea of EditorInterface representing the GUI part of the editor, I think we can go with that. And that way we still have that reduced number of calls, because EditorInterface::get_singleton() is a thing. But EditorInterface is a part of editor_plugin and is also a Node thus far... Maybe a bigger clean up is in order.

---

So for now, my suggestion would be to remove theme_base and the related method (which incidentally didn't even use theme_base). Make gui_base the root of all editor GUI and make sure that everything in the editor accesses that.

If you can, clean up the elements that use the EditorNode singleton instead of accessing the theme. I can see that a lot of places won't be an easy fix, because they call this code from a non-GUI object. I'd be fine with a separate PR to refactor those, but I can see that some can be cleaned up here already.

[YuriSizov]

@AaronRecord Do you still want to make a simplified version of this PR? Or should we postpone any changes for now until we're ready for a more substantial refactor of EditorNode? (reduz wants it to happen for 4.1)

[AaronRecord]

@AaronRecord Do you still want to make a simplified version of this PR? Or should we postpone any changes for now until we're ready for a more substantial refactor of EditorNode? (reduz wants it to happen for 4.1)

Yeah that shouldn't take long, I'll see if I can do that this week.

[fire]

@YuriSizov were you still in favor of this or was is removing gui_base and theme_base a 4.1 thing now?

[YuriSizov]

@fire Well, Aaron hasn't found time to work on this yet, and as we've discussed, there are no immediate performance benefits. So there is no reason to rush it now and it can wait for 4.1 when reduz wants us to refactor EditorNode.

I am in favor of any and all improvements we can make to this monolithic class to make it simpler. Note that I am not available to review this for the foreseeable future.

[YuriSizov]

I think at this point this is going to be done as a part of some bigger reorganization of the codebase. So I'm going to close this PR, but thanks for your contribution nonetheless!