Skip to content
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 includes of EditorNode (and everything else) #75765

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Apr 6, 2023

I'm starting to look into refactoring parts of EditorNode and friends (see also #75694).

So this PR does a couple of things. I removed some unnecessary includes from editor_node.h, which triggered issues in a bunch of other classes which were implicitly relying on them. So I fixed includes in those, removing unnecessary includes in process as well.

The second thing that this PR does is establishing the editor/gui folder for various GUI editor-only widgets and components. The root editor folder is growing bigger and bigger, and I'm pretty sure that parts of EditorNode will be extracted into individual classes. So it makes sense to me to start organizing a place for them. I picked some obvious ones to be the first to move, but there are definitely more that can be shifted to the new folder.


I also moved editor_file_server.h/cpp from their own folder to the debugger folder, so they don't feel so alone on their own. Debugger is the only class that uses EditorFileServer, so seems reasonable. And I renamed EditorPath to EditorObjectSelector, because that name was very awkward and perhaps even misleading.

With this and #75694 resolved, it should be possible to start reorganizing the GUI parts of EditorNode at least.

@YuriSizov YuriSizov added this to the 4.1 milestone Apr 6, 2023
@YuriSizov YuriSizov requested review from a team as code owners April 6, 2023 19:23
@YuriSizov YuriSizov force-pushed the editor-node-optimize-includes branch 2 times, most recently from 2010ae9 to 857602c Compare April 6, 2023 19:32
@KoBeWi
Copy link
Member

KoBeWi commented Apr 6, 2023

I'd also move editor spin slider, editor scale, editor dir dialog, editor fonts, editor themes, editor zoom widget and scene tree editor to gui folder. They are all reusable and/or related to GUI.

Other potential new folders I see are dialogs, docks and inspector.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 6, 2023

@YuriSizov @KoBeWi I'm not sure a gui folder would be an improvement, pretty much the entirety of the editor code is related to GUI. But the other folder ideas would likely be helpful (such as dialogs, docks, inspector, debugger).

@KoBeWi
Copy link
Member

KoBeWi commented Apr 6, 2023

The GUI elements I listed don't fall into any of these categories. They are also quite generic and reusable (or directly related to GUI, in case of editor themes and scale). Things like EditorSpinSlider, SceneTreeEditor or EditorZoomWidget could be even used in custom plugins.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 6, 2023

Still though, we could call it "misc" or "widgets" or "elements" or "primitives" or similar. Calling it "gui" seems wrong if there are things outside of it that are also GUI, similar to having a folder called "code" for code that doesn't fit in the other folders. Anyway, that's all I will say, this is not a hill to die on :P

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 7, 2023

So the way I see it, editor/gui/ can echo scene/gui/ and contain either reusable components or ones directly related to the editor GUI as a whole (like the title bar, I guess). Things like EditorSpinSlider and EditorZoomWidget would fit it perfectly, while more complex tools like the animation editor or docks would not.

I agree that docks can have their own folder, but I'd keep such a change for a separate PR to reduce possible points of friction for this to get merged. I would put SceneTreeEditor in that hypothetical editor/docks/ btw, since this is what it is closely related to in the editor codebase. Just like inspector is, though it can also form its own scope and have its own folder. But again, this should be done with a future PR.

If the purpose of editor/gui/ is not very clear, we can always rename, it's not too important.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 7, 2023

SceneTreeEditor is just an extended Tree. It's used in multiple docks and dialogs.

@YuriSizov YuriSizov force-pushed the editor-node-optimize-includes branch from 857602c to 92f5955 Compare April 7, 2023 10:26
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Apr 7, 2023

Okay, I also moved editor spin slider, editor dir dialog, editor zoom widget, and scene tree editor files to the editor/gui/ folder, and improved a bunch of includes in affected files.

Theming classes may want to have their own folder, but are also okay at the top level, IMO. But again, we can change everything later, this stuff is purely internal. I'd prefer to approach the overall organization issue here in steps.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! The reorganization seems reasonable (and can be adjusted later if necessary). Just needs a rebase.

@YuriSizov YuriSizov force-pushed the editor-node-optimize-includes branch from 92f5955 to ac3f228 Compare April 7, 2023 16:58
Also start organizing editor-specific GUI components
into a dedicated folder, `editor/gui`.
Also move `editor_file_server` next to the rest of debugger classes.
@YuriSizov YuriSizov force-pushed the editor-node-optimize-includes branch from ac3f228 to 4154039 Compare April 7, 2023 17:00
@akien-mga akien-mga merged commit c5d9470 into godotengine:master Apr 11, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the editor-node-optimize-includes branch April 11, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants