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

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

Open
akien-mga opened this issue Oct 1, 2021 · 8 comments
Open

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

akien-mga opened this issue Oct 1, 2021 · 8 comments

Comments

@akien-mga
Copy link
Member

akien-mga commented Oct 1, 2021

Godot version

4.0.dev (06c1b40)

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:

godot/SConstruct

Lines 724 to 737 in 06c1b40

# Build subdirs, the build order is dependent on link order.
SConscript("core/SCsub")
SConscript("servers/SCsub")
SConscript("scene/SCsub")
SConscript("editor/SCsub")
SConscript("drivers/SCsub")
SConscript("platform/SCsub")
SConscript("modules/SCsub")
if env["tests"]:
SConscript("tests/SCsub")
SConscript("main/SCsub")
SConscript("platform/" + selected_platform + "/SCsub") # Build selected platform.

  • core
  • servers
  • scene
  • drivers (no issue)
  • editor
  • modules
  • tests (no issue)
  • main
  • platform (no issue since last)

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

Steps to reproduce

n/a

Minimal reproduction project

No response

@akien-mga
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Member Author

akien-mga commented Oct 1, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants