godot icon indicating copy to clipboard operation
godot copied to clipboard

Editor: Restructure editor code

Open AThousandShips opened this issue 7 months ago • 27 comments
trafficstars

As discussed with @akien-mga the editor code is very cluttered, which makes it both difficult to assign code owners to, but also makes it hard to navigate

Opening as a draft to discuss the specifics of how to organize things, where I still have some areas open for discussion:

  • [x] The structuring of sub folders of /editor/, currently these are organized mainly by type, not area, this could be changed to be more area specific (i.e. all dialogs are in /editor/dialogs/ rather than specifically /editor/animation/ etc.)
  • [x] If some files should be moved between sub folders, for example some files that are in /editor/gui/ that might be better in /editor/dialogs/
  • [ ] If some files that are under /editor/plugins/ should be moved out of there and into the main sub folders, like the dialogs under /editor/plugins/tiles/
  • [X] How and if CODEOWNERS should be restructured, currently it uses relatively rough glob patterns to handle files, this could now be made more specific (this will be fixed in a follow up PR beyond the basic changes in this, which can be done without any noise)
  • [ ] If we should use more relative paths, as you can see several paths were made more specific (including cases where the file references was moved to the same folder, making the specification unnecessary)

Generally looking for feedback on these specifics, but also generally, in my opinion this is badly needed to ensure the editor code remains manageable as it grows more complicated, currently the majority of files in /editor/ lack owners, making review more complicated


  • Supersedes: https://github.com/godotengine/godot/pull/83161

AThousandShips avatar Mar 27 '25 15:03 AThousandShips

I was about to comment that we should merge #83161 before this PR, but then I saw you linking to it. Though, I still want to say that we should merge #83161 before this PR :)

aaronfranke avatar Mar 31 '25 00:03 aaronfranke

I think this is a step in the right direction, the only thing I dislike the idea of grouping things by "type" of GUI element. I don't think dialogs should be grouped into the same folder, and same for docks.

Instead, I think we simply should have per-topic sorting. For example the inspector dock, and all inspector-related dialogs should go in the inspector folder I believe. Similarly, all filesystem-related files can go in a filesystem folder I believe.

I think we shouldn't be afraid of adding a lot more folders though (including subfolders). I think we can add, for example, a settings folder, maybe with an input subfolder, things like that.

groud avatar Apr 23 '25 15:04 groud

Agreed, picked this approach at first because it was the most straightforward, and I'm leaning towards switching that out, will hold off slightly but will look at resturcturing next week probably if we agree on that switch

AThousandShips avatar Apr 23 '25 15:04 AThousandShips

It would be nice to have some visualization of before and after the changes. GitHub isn't very friendly for previewing the changes.

KoBeWi avatar Apr 23 '25 20:04 KoBeWi

Can do a summary

AThousandShips avatar Apr 24 '25 15:04 AThousandShips

Will push some changes soon and add some tree views of the differences

AThousandShips avatar Apr 28 '25 17:04 AThousandShips

What effect does it have on scu_build? docs If there are more subfolders, this increases the build times?


https://github.com/godotengine/godot/blob/master/scu_builders.py#L292-L303

    process_folder(["editor"], ["file_system_dock", "editor_resource_preview"], 32)
    process_folder(["editor/debugger"])
    process_folder(["editor/debugger/debug_adapter"])
    process_folder(["editor/export"])
    process_folder(["editor/gui"])
    process_folder(["editor/themes"])
    process_folder(["editor/project_manager"])
    process_folder(["editor/import"])
    process_folder(["editor/import/3d"])
    process_folder(["editor/plugins"])
    process_folder(["editor/plugins/gizmos"])
    process_folder(["editor/plugins/tiles"])

Alex2782 avatar Apr 30 '25 00:04 Alex2782

Will try and compare, good point!

AThousandShips avatar Apr 30 '25 11:04 AThousandShips

I made a before and after using this script:

@tool
extends Tree

@export_global_dir var base_folder: String

func _ready() -> void:
	add_folder(base_folder, null)

func add_folder(path: String, parent: TreeItem):
	var folder_item := create_item(parent)
	folder_item.set_text(0, path.get_file())
	folder_item.set_custom_color(0, Color.YELLOW)
	
	for dir in DirAccess.get_directories_at(path):
		add_folder(path.path_join(dir), folder_item)
	
	for file in DirAccess.get_files_at(path):
		if file.get_extension() != "h":
			continue
		
		var file_item := create_item(folder_item)
		file_item.set_text(0, file.get_basename())
	
	if folder_item.get_child_count() == 0:
		folder_item.free()

Set base_folder to editor in the source code and it will make a huge folder tree 🙃

Comparison, but it's really big.
Before After
image image

I took a look at the new structure.

  • ~~There is some inconsistency between dialogs and gui. E.g. editor_file_dialog is in gui, while create_dialog, which is also technically a reusable GUI control, is inside dialogs.~~
    • ~~I think dialogs folder might be too general. There are some dialogs that belong to docks (connections_dialog), to script editor (find_in_files), to shader editor (editor_native_shader_source_visualiser, I think) etc. They should be together with their base components. I'd put only "unique" and "top-level" dialogs into dialogs folder (editor_about, editor_feature_profile etc.)~~ Addressed, but find_in_files is in the wrong place. It's part of script editor.
  • ~~Should physics and navigation go under 2d/3d, or should they be standalone folders? I think making them standalone makes more sense, although the current structure makes it easier to disable 2D and 3D separately.~~
  • ~~Texture plugins could get their own folder.~~
  • ~~EditorHelp stuff and DocData could be put into their own folder (together I mean).~~
  • ~~project_manager could be in project_manager folder.~~
  • ~~project_upgrade_tool and 3_to_4 stuff can be in a project_upgrade folder maybe~~
  • ~~extractable_translations.gen looks like it belongs to translations~~
  • ~~file_info is part of editor_file_dialog and file_system_dock. There might be better place for it than top level~~
  • ~~Might be worth it to add some folder in editor_plugins for files that aren't specific plugins? Like editor_plugin, editor_plugin_settings, but also editor_preview_plugins or editor_resource_tooltip_plugins. They look like they can be grouped together.~~
  • ~~shader_globals_editor could be in shader folder, except it's not part of the plugin., so not sure.~~
  • ~~engine_update_label can be inside project manager, it's not used anywhere else.~~
  • ~~code_editor should be in script, although technically it's also used by shader editor. Maybe gui` then?~~
  • ~~event_listener_line_edit should be in gui.~~
  • ~~register_exporters could be in export, unless there is a reason why it's where it is.~~

I could see more potential groups in the top-level folder, but it's arguable.

KoBeWi avatar May 05 '25 20:05 KoBeWi

Note that the dialogs folder has been reverted

EditorHelp and stuff is in a new folder, event_listener_line_edit is currently under a new settings folder, but can move it to gui

Will add the other suggestions and push in the next few days, possibly tomorrow

AThousandShips avatar May 05 '25 21:05 AThousandShips

There, made a major restructure, dropped the old "dialogs" division, and moved into a few new categories

Still to do:

  • [ ] Decide on moving editor/plugins/editor_plugin.h and similar, this is used everywhere and is also used in custom modules, so it's risky, and the returns are limited, but it would improve organization
  • [ ] Decide on any missing categories, and some division between them
  • [x] ~Move some code from editor/SCsub to other parts, the script parts generating documentation etc.~ Tested this and wasn't convenient
  • [ ] Figure out some further specifics on CODEOWNERS

Otherwise this is nearly in what I'd prefer as the final state

CC @KoBeWi @akien-mga @Repiteo

AThousandShips avatar May 07 '25 16:05 AThousandShips

The new structure:

Unfold

image

  • Not sure about textures/, it sounds like a folder with textures. texture/ could be better, if it's fine.
  • color_channel_selector should be in textures/.
  • canvas_item_editor_plugin should be in plugins/2d.
  • Some .gen files weren't moved to proper subfolders and are still in editor/
  • editor_build_profile could go to settings/

Also some of my previous comments still apply, but overall this new structure looks much better.

KoBeWi avatar May 07 '25 17:05 KoBeWi

Some .gen files weren't moved to proper subfolders and are still in editor/

Have you made sure to clean before building? They have been moved

canvas_item_editor_plugin should be in plugins/2d.

It belongs in the base as it isn't 2D or GUI, same reasoning why canvas_item was moved to scene/main for 4.0

Also some of my previous comments still apply

Can you please point to which, I've resolved most or responded reasons why they weren't

AThousandShips avatar May 07 '25 17:05 AThousandShips

For the prioritizing physics vs 2D/3D I prefer keeping it as is, or possibly adding a sub-folder, I prefer having things closer to the way scene is organized, it helps navigation

AThousandShips avatar May 07 '25 17:05 AThousandShips

Have you made sure to clean before building? They have been moved

I didn't. I actually didn't even build, so this is probably why 🙃

Can you please point to which, I've resolved most or responded reasons why they weren't

I crossed out resolved stuff.

KoBeWi avatar May 07 '25 17:05 KoBeWi

There are some areas to still figure out the details of (especially CODEOWNERS), but this is ready for deeper review

AThousandShips avatar Jun 03 '25 13:06 AThousandShips

Latest structure (this time properly includes all changes):

Unfold

image

  • [x] Why is doc_data_compressed.gen in translations/?
  • [x] dependency_editor belongs to file_system/.
  • [x] editor_folding can be in settings/.
  • [x] window_wrapper could be in gui/ I think?

Aside from that it's probably fine. Maybe there could be folder for audio and assetlib stuff, but these files are in different folders, so not sure.

KoBeWi avatar Jun 10 '25 08:06 KoBeWi

Will update! doc_data_compressed.gen must have been a mistake don't remember moving it there!

Audio makes sense, two files should be enough to move, in both plugins and the root, will update and rebase soon

AThousandShips avatar Jun 10 '25 08:06 AThousandShips

Moved the suggested, as well as audio both in editor/ and editor/plugins/, as well as credits_roll into editor/gui/

~Added scu for editor/plugins/script/ which was excluded previously, for these changes my condition is three or more files to justify it,~ adding SCU for these cases as it saves a bit of binary size, will compare build times later to validate

This restructure adds about 0.45% to the size of a Windows dev build of the editor, ~comparing production builds as well~ production builds are unchanged

AThousandShips avatar Jun 10 '25 10:06 AThousandShips

That's a good point, will merge those

AThousandShips avatar Jun 10 '25 14:06 AThousandShips

The gui/code_editor, script/find_in_files, and the plugins/text_editor can all be in plugins/script, since they are all part of the script editor plugin. The code_editor is also used by the shader editor, but I think its better to have it in the script folder than having it in a general folder.

kitbdev avatar Jun 10 '25 14:06 kitbdev

I'll leave files from editor/ outside editor/plugins/, they're not part of the plugins, and I'd prefer to keep editor/plugins/text_editor where it is to minimize confusion

AThousandShips avatar Jun 10 '25 14:06 AThousandShips

The editor/plugins/text_editor is just like the editor/plugins/script/script_text_editor, and it is only used by the script editor. It also isn't a plugin by itself, so I'm not sure why it should be outside of the script plugin folder.

The editor/script/find_in_files is currently created and managed by the script editor as well, it cannot work alone, so I'd say it is part of the plugin.

kitbdev avatar Jun 10 '25 15:06 kitbdev

Can move the text editor, given its scope, but I'll leave the rest

AThousandShips avatar Jun 10 '25 15:06 AThousandShips

While discussing this the suggestion was again brought up to simply break up the editor/plugins/ folder entirely, keeping only "plugins" things (like editor_plugin and editor_plugin_settings for example) in that folder, and putting the rest in the relevant base folders, like audio, and adding new categories for the stuff still in editor/plugins/, this won't be a lot of new work but would be good to see if this is desired

My suggestion for this would be:

  • editor/plugins/shader_baker/ -> editor/shader/shader_baker/
  • editor/plugins/shader_baker_export_plugin -> editor/export/
  • editor/plugins/gdextension_export_plugin -> editor/export/
  • editor/plugins/2d -> editor/scene/2d/
  • editor/plugins/3d -> editor/scene/3d/
  • editor/plugins/gui -> editor/scene/gui/
  • editor/plugins/texture -> editor/scene/texture/
  • editor/plugins/tiles -> editor/scene/2d/tiles/
  • Debugger plugins into editor/debugger/
  • editor/plugins/game_view_plugin -> editor/run/
  • editor/plugins/packed_scene_translation_parser_plugin -> editor/translations/
  • editor/plugins/dedicated_server_export_plugin -> editor/export/
  • editor/plugins/sub_viewport_preview_editor_plugin -> editor/inspector/
  • editor/plugins/editor_preview_plugins -> editor/inspector/
  • editor/plugins/editor_resource_conversion_plugin -> editor/inspector/
  • editor/plugins/editor_context_menu_plugin -> editor/inspector/
  • editor/plugins/tool_button_editor_plugin -> editor/inspector/
  • editor/plugins/editor_resource_tooltip_plugins -> editor/inspector/
  • editor/editor_asset_installer -> editor/asset_library/
  • editor/plugins/asset_library_editor_plugin -> editor/asset_library/
  • Remaining scene related plugins into editor/scene/
  • editor/editor_vcs_interface -> editor/version_control/
  • editor/plugins/version_control_editor_plugin -> editor/version_control/
  • editor/plugins/gizmos/ -> editor/scene/3d/gizmos/
  • editor/plugins/embedded_process -> editor/
  • editor/plugins/input_event_editor_plugin -> editor/inspector/

AThousandShips avatar Jun 11 '25 13:06 AThousandShips

This description will probably no longer up-to-date after the PR merge. https://docs.godotengine.org/en/stable/contributing/development/editor/introduction_to_editor_development.html

(Last updated 2 years ago)

Alex2782 avatar Jun 12 '25 00:06 Alex2782

If there's no objections to the restructure suggested above I'll see about trying it out this weekend, it shouldn't be too hard to rework

Thank you for reminding me about that documentation, will look at updating that following this!

AThousandShips avatar Jun 12 '25 07:06 AThousandShips

I incorporated the new changes, plugins is now essentially clean, and the structure is far more organized, and things are more or less in one place, i.e. there's no separation between plugins and other folders for the same area like animation

Some things could still be restructured but this should be a pretty good start, some content in editor/gui/ could still be divided out, but I think this is a good point

In a windows dev build the size increase is now just 0.4%, due to some SCU files being smaller

Will try to make a comparison of compile time with SCU from scratch

AThousandShips avatar Jun 13 '25 12:06 AThousandShips

Latest structure for reference:

Unfold

image

KoBeWi avatar Jun 16 '25 13:06 KoBeWi

Will rebase and fix the final details! Will be great to have this done, but it's been an interesting experience and I now know the editor even better!

AThousandShips avatar Jul 04 '25 16:07 AThousandShips