search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
88.98k stars 20.18k forks source link

Invalid cross includes that do not respect area encapsulation rules #53295

Open akien-mga opened 2 years ago

akien-mga commented 2 years ago

Godot version

4.0.dev (06c1b40b)

System information

Any

Issue description

The engine codebase is compartmentalized in areas/folders which have a defined dependency order to prevent both cyclic dependencies and general bad design patterns.

To ensure this encapsulation, we need to make sure that we don't include headers from high level areas in low level areas. For example core is the lowest level area, so it shouldn't include headers from any other folder (aside from thirdparty, which is independent from Godot code).

We should add CI checks to prevent adding such invalid includes, but first we need to fix the problems we already have in the codebase (#29730 is related, this issue is the same but for the whole codebase/dependency path).

The dependency order should follow the build order as defined here: https://github.com/godotengine/godot/blob/06c1b40b84645b4460dd49d95694a7779d636f92/SConstruct#L724-L737

I'll list invalid includes in comments for each folder/area.

Steps to reproduce

n/a

Minimal reproduction project

No response

akien-mga commented 2 years ago

Core

Updated 2022-03-18. Original report below.

$ rg --sort path '#include "scene/'
debugger/local_debugger.cpp
34:#include "scene/main/scene_tree.h"

io/resource.cpp
39:#include "scene/main/node.h" //only so casting works
Original list of issues ``` $ rg --sort path '#include "servers/' debugger/debugger_marshalls.h 35:#include "servers/rendering_server.h" debugger/remote_debugger.cpp 41:#include "servers/display_server.h" os/os.cpp 39:#include "servers/audio_server.h" ``` ``` $ rg --sort path '#include "scene/' debugger/local_debugger.cpp 35:#include "scene/main/scene_tree.h" debugger/remote_debugger.cpp 40:#include "scene/main/node.h" io/resource.cpp 39:#include "scene/main/node.h" //only so casting works math/a_star.cpp 35:#include "scene/scene_string_names.h" multiplayer/multiplayer_api.cpp 37:#include "scene/main/node.h" multiplayer/multiplayer_replicator.cpp 34:#include "scene/main/node.h" 35:#include "scene/resources/packed_scene.h" multiplayer/rpc_manager.cpp 36:#include "scene/main/node.h" variant/variant.cpp 41:#include "scene/gui/control.h" 42:#include "scene/main/node.h" ``` ``` $ rg --sort path '#include "editor/' input/input.cpp 39:#include "editor/editor_settings.h" string/translation.cpp 38:#include "editor/editor_settings.h" ``` ``` $ rg --sort path '#include "main/' string/translation.cpp 39:#include "main/main.h" ```
akien-mga commented 2 years ago

Servers

Updated 2022-03-28. Original report below.

$ rg --sort path '#include "scene/'
audio/effects/audio_effect_record.h
38:#include "scene/resources/audio_stream_sample.h"

audio_server.cpp
42:#include "scene/resources/audio_stream_sample.h"

display_server.cpp
34:#include "scene/resources/texture.h"

navigation_server_2d.h
36:#include "scene/2d/navigation_region_2d.h"

navigation_server_3d.h
36:#include "scene/3d/navigation_region_3d.h"

rendering/rasterizer_dummy.h
36:#include "scene/resources/mesh.h"

xr/xr_positional_tracker.h
35:#include "scene/resources/mesh.h"
$ rg --sort path '#include "editor/'
audio/effects/audio_effect_record.cpp
35:#include "editor/import/resource_importer_wav.h"
Original list of issues ``` $ rg --sort path '#include "scene/' audio/effects/audio_effect_record.h 39:#include "scene/resources/audio_stream_sample.h" audio_server.cpp 42:#include "scene/resources/audio_stream_sample.h" display_server.cpp 34:#include "scene/resources/texture.h" navigation_server_2d.h 40:#include "scene/2d/navigation_region_2d.h" navigation_server_3d.h 40:#include "scene/3d/navigation_region_3d.h" physics_3d/soft_body_3d_sw.h 43:#include "scene/resources/mesh.h" rendering/rasterizer_dummy.h 37:#include "scene/resources/mesh.h" text_server.cpp 32:#include "scene/main/canvas_item.h" text_server.h 38:#include "scene/resources/texture.h" xr/xr_positional_tracker.h 35:#include "scene/resources/mesh.h" ``` ``` $ rg --sort path '#include "editor/' audio/effects/audio_effect_record.h 38:#include "editor/import/resource_importer_wav.h" ```
akien-mga commented 2 years ago

Scene

Updated 2022-03-18. Original report below.

$ rg --sort path '#include "editor/'
2d/path_2d.cpp
36:#include "editor/editor_scale.h"

2d/skeleton_2d.cpp
34:#include "editor/editor_data.h"
35:#include "editor/editor_settings.h"
36:#include "editor/plugins/canvas_item_editor_plugin.h"

3d/occluder_instance_3d.cpp
43:#include "editor/editor_node.h"

3d/skeleton_3d.cpp
35:#include "editor/plugins/skeleton_3d_editor_plugin.h"

animation/animation_player.cpp
39:#include "editor/editor_node.h"

gui/color_picker.cpp
39:#include "editor/editor_settings.h"

gui/control.cpp
50:#include "editor/plugins/control_editor_plugin.h"

gui/line_edit.cpp
43:#include "editor/editor_settings.h"

property_utils.cpp
38:#include "editor/editor_node.h"

resources/packed_scene.cpp
37:#include "editor/editor_inspector.h"

resources/skeleton_modification_2d.cpp
39:#include "editor/editor_settings.h"

resources/skeleton_modification_2d_ccdik.cpp
35:#include "editor/editor_settings.h"

resources/skeleton_modification_2d_fabrik.cpp
35:#include "editor/editor_settings.h"

resources/skeleton_modification_2d_lookat.cpp
35:#include "editor/editor_settings.h"

resources/skeleton_modification_2d_twoboneik.cpp
35:#include "editor/editor_settings.h"
$ rg --sort path '#include "modules/'
gui/rich_text_label.cpp
40:#include "modules/modules_enabled.gen.h" // For regex.
42:#include "modules/regex/regex.h"

resources/default_theme/default_theme.cpp
40:#include "modules/modules_enabled.gen.h" // For svg.
42:#include "modules/svg/image_loader_svg.h"

resources/default_theme/default_theme_icons_builders.py
39:    s.write('#include "modules/modules_enabled.gen.h"\n\n')
Original list of issues ``` $ rg --sort path '#include "editor/' 2d/path_2d.cpp 36:#include "editor/editor_scale.h" 2d/skeleton_2d.cpp 34:#include "editor/editor_settings.h" 35:#include "editor/plugins/canvas_item_editor_plugin.h" 3d/physics_body_3d.cpp 37:#include "editor/plugins/node_3d_editor_plugin.h" animation/animation_player.cpp 39:#include "editor/editor_node.h" 40:#include "editor/editor_settings.h" gui/color_picker.cpp 38:#include "editor/editor_scale.h" 39:#include "editor/editor_settings.h" gui/control.cpp 51:#include "editor/editor_settings.h" 52:#include "editor/plugins/canvas_item_editor_plugin.h" gui/dialogs.cpp 39:#include "editor/editor_node.h" 40:#include "editor/editor_scale.h" gui/gradient_edit.cpp 36:#include "editor/editor_scale.h" gui/graph_edit.cpp 40:#include "editor/editor_scale.h" gui/line_edit.cpp 43:#include "editor/editor_scale.h" 44:#include "editor/editor_settings.h" gui/rich_text_label.cpp 45:#include "editor/editor_scale.h" gui/text_edit.cpp 46:#include "editor/editor_scale.h" gui/tree.cpp 45:#include "editor/editor_scale.h" main/node.cpp 45:#include "editor/editor_settings.h" resources/material.cpp 37:#include "editor/editor_settings.h" resources/skeleton_modification_2d.cpp 39:#include "editor/editor_settings.h" resources/skeleton_modification_2d_ccdik.cpp 35:#include "editor/editor_settings.h" resources/skeleton_modification_2d_fabrik.cpp 35:#include "editor/editor_settings.h" resources/skeleton_modification_2d_lookat.cpp 35:#include "editor/editor_settings.h" resources/skeleton_modification_2d_twoboneik.cpp 35:#include "editor/editor_settings.h" ``` (Replaces #29730.) ``` $ rg --sort path '#include "modules/' gui/rich_text_label.cpp 39:#include "modules/modules_enabled.gen.h" 41:#include "modules/regex/regex.h" ```
akien-mga commented 2 years ago

Editor

Updated 2022-03-18.

$ rg --sort path '#include "modules/'
doc_tools.cpp
45:#include "modules/modules_enabled.gen.h" // For mono.

editor_themes.cpp
41:#include "modules/modules_enabled.gen.h" // For svg.
43:#include "modules/svg/image_loader_svg.h"

import/resource_importer_dynamic_font.cpp
39:#include "modules/modules_enabled.gen.h" // For freetype.

plugins/script_editor_plugin.cpp
51:#include "modules/visual_script/editor/visual_script_editor.h"

rename_dialog.cpp
33:#include "modules/modules_enabled.gen.h" // For regex.
41:#include "modules/regex/regex.h"

rename_dialog.h
34:#include "modules/modules_enabled.gen.h" // For regex.

scene_tree_dock.cpp
56:#include "modules/modules_enabled.gen.h" // For regex.

scene_tree_dock.h
49:#include "modules/modules_enabled.gen.h" // For regex.
debugger/editor_performance_profiler.cpp
35:#include "main/performance.h"

debugger/editor_performance_profiler.h
36:#include "main/performance.h"

debugger/script_editor_debugger.cpp
53:#include "main/performance.h"

editor_node.cpp
50:#include "main/main.h"

editor_paths.cpp
37:#include "main/main.h"

editor_plugin.cpp
45:#include "main/main.h"

plugins/mesh_library_editor_plugin.cpp
36:#include "main/main.h"

progress_dialog.cpp
36:#include "main/main.h"
akien-mga commented 2 years ago

Modules

Updated 2022-03-18.

Includes tests/ stuff for module tests, but there's no real way around it. On the other hand maybe tests could come later than modules in the OP dependency order (but in turn it depends on modules/modules_tests.gen.h).

$ rg '#include "main/'
mono/editor/bindings_generator.cpp
42:#include "main/main.h"

mono/editor/editor_internal_calls.cpp
44:#include "main/main.h"

openxr/register_types.cpp
32:#include "main/main.h"
$ rg '#include "platform/'
mono/mono_gd/support/android_support.cpp
48:#include "platform/android/java_godot_wrapper.h"
49:#include "platform/android/os_android.h"
50:#include "platform/android/thread_jandroid.h"
akien-mga commented 2 years ago

Main

Updated 2022-03-18.

$ rg '#include "platform/'
main.cpp
62:#include "platform/register_platform_apis.h"

That's expected, that's why this file specifically is built before modules, tests, and main.

matorin57 commented 1 year ago

I can take a look to see if this can implemented in scons so we can generate a build error for a bad include

matorin57 commented 11 months ago

There doesn't seem to be a natural way to do this functionality with scons APIs directly, however it should be fairly simple to code using just pure python.