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.

Sign up for GitHub

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

Style: Harmonize header includes in platform ports #75932

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Style: Harmonize header includes in platform ports #75932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions drivers/coreaudio/audio_driver_coreaudio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/**************************************************************************/

#ifdef COREAUDIO_ENABLED

#include "audio_driver_coreaudio.h"

#ifdef COREAUDIO_ENABLED

#include "core/config/project_settings.h"
#include "core/os/os.h"

Expand Down
4 changes: 2 additions & 2 deletions drivers/coremidi/midi_driver_coremidi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/**************************************************************************/

#ifdef COREMIDI_ENABLED

#include "midi_driver_coremidi.h"

#ifdef COREMIDI_ENABLED

#include "core/string/print_string.h"

#import <CoreAudio/HostTime.h>
Expand Down
2 changes: 1 addition & 1 deletion platform/android/SCsub
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ android_files = [
"android_keys_utils.cpp",
"display_server_android.cpp",
"plugin/godot_plugin_jni.cpp",
"vulkan/vulkan_context_android.cpp",
"vulkan_context_android.cpp",
]

env_android = env.Clone()
Expand Down
3 changes: 2 additions & 1 deletion platform/android/android_keys_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@
#ifndef ANDROID_KEYS_UTILS_H
#define ANDROID_KEYS_UTILS_H

#include "core/os/keyboard.h"

#include <android/input.h>
#include <core/os/keyboard.h>

#define AKEYCODE_MAX 0xFFFF

Expand Down
3 changes: 2 additions & 1 deletion platform/android/api/api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@

#include "api.h"

#include "core/config/engine.h"
#include "java_class_wrapper.h"
#include "jni_singleton.h"
akien-mga marked this conversation as resolved.
Show resolved Hide resolved

#include "core/config/engine.h"

#if !defined(ANDROID_ENABLED)
static JavaClassWrapper *java_class_wrapper = nullptr;
#endif
Expand Down
3 changes: 2 additions & 1 deletion platform/android/api/jni_singleton.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@

#include "core/config/engine.h"
#include "core/variant/variant.h"

#ifdef ANDROID_ENABLED
#include "platform/android/jni_utils.h"
#include "jni_utils.h"
#endif

class JNISingleton : public Object {
Expand Down
3 changes: 2 additions & 1 deletion platform/android/dir_access_jandroid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@

#include "dir_access_jandroid.h"

#include "core/string/print_string.h"
#include "string_android.h"
#include "thread_jandroid.h"

#include "core/string/print_string.h"

jobject DirAccessJAndroid::dir_access_handler = nullptr;
jclass DirAccessJAndroid::cls = nullptr;
jmethodID DirAccessJAndroid::_dir_open = nullptr;
Expand Down
4 changes: 3 additions & 1 deletion platform/android/dir_access_jandroid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@
#ifndef DIR_ACCESS_JANDROID_H
#define DIR_ACCESS_JANDROID_H

#include "java_godot_lib_jni.h"

#include "core/io/dir_access.h"
#include "drivers/unix/dir_access_unix.h"
#include "java_godot_lib_jni.h"

#include <stdio.h>

/// Android implementation of the DirAccess interface used to provide access to
Expand Down
8 changes: 6 additions & 2 deletions platform/android/display_server_android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,23 @@

#include "display_server_android.h"

#include "core/config/project_settings.h"
#include "java_godot_io_wrapper.h"
#include "java_godot_wrapper.h"
#include "os_android.h"
#include "tts_android.h"

#include "core/config/project_settings.h"

#if defined(VULKAN_ENABLED)
#include "vulkan_context_android.h"

#include "drivers/vulkan/rendering_device_vulkan.h"
#include "platform/android/vulkan/vulkan_context_android.h"
#include "servers/rendering/renderer_rd/renderer_compositor_rd.h"
#endif

#ifdef GLES3_ENABLED
#include "drivers/gles3/rasterizer_gles3.h"

#include <EGL/egl.h>
#endif

Expand Down
3 changes: 2 additions & 1 deletion platform/android/export/export.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@

#include "export.h"

#include "export_plugin.h"

#include "core/os/os.h"
#include "editor/editor_settings.h"
#include "editor/export/editor_export.h"
#include "export_plugin.h"

void register_android_exporter_types() {
GDREGISTER_VIRTUAL_CLASS(EditorExportPlatformAndroid);
Expand Down
4 changes: 2 additions & 2 deletions platform/android/export/export_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@

#include "export_plugin.h"

#include "../logo_svg.gen.h"
#include "../run_icon_svg.gen.h"
Comment on lines +33 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like paths in other places have to be relative. E.g. platform/android/api/jni_singleton.h including jni_utils.h.

Is there some conflict that prevents resolution?

Copy link
Member Author

@akien-mga akien-mga May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Technically both #platform/android and the current folder are in the include path, so both work indeed.

I've opted for using relative paths for the "local" includes, though I seem to have missed some. Maybe we could remove #platform/<name>/ from the include path and force using paths relative to the current file?

We can alternatively standardize on keeping all paths relative to #platform/android as "root" and not use file-related paths at all. I guess that's what my "rule" formulates:

Includes from the platform port or module should be included with relative paths (relative to the root folder of the modular component, e.g. platform/linuxbsd/), in their own section before Godot's "core" includes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to admit, that while in spirit the rule is neat, looking at the includes in a random file leaves me puzzled regarding where it's coming from. IDE will resolve it for me anyway, but from the code alone it's currently unclear. And it seems hard to enforce, so going forward you can still get into situations where the included file is not where you expect it to be.

Relative paths are clear, but we tend to avoid them in other places, using project-based paths instead, except for the header definition. Plus, they can quickly become ugly if you have more than a couple levels of hierarchy.

Can we somehow enforce a rule where only #platform/<name>/ is in the include path, but not other subfolders of a platform (or a module), except for the header file of the same name, if it's cpp?

Copy link
Member Author

Choose a reason for hiding this comment

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

So yeah going by my initial suggestion, we should probably make all platform/module specific includes relative to the root of their component, and not relative (so all #platform/<name>/export/ includes would be #include "export/export_plugin.h" etc.).

Copy link
Member Author

Choose a reason for hiding this comment

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

I rebased the PR and did the changes we discussed, so the include for e.g. platform/android/export/export_plugin.h in platform/android/export/export.h is done with export/export_plugin.h.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this didn't work in the end, because the api and export folders are special and compiled on all platforms for editor builds. We don't add all platform folders to the include path, so for the api/*.cpp and export/*.cpp to compile properly, they still need to use relative include paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, shame. Well, at least most of the codebase is consistent after these changes. Platforms can be an exception

#include "gradle_export_util.h"

#include "core/config/project_settings.h"
Expand All @@ -46,8 +48,6 @@
#include "editor/editor_scale.h"
#include "editor/editor_settings.h"
#include "main/splash.gen.h"
#include "platform/android/logo_svg.gen.h"
#include "platform/android/run_icon_svg.gen.h"

#include "modules/modules_enabled.gen.h" // For svg.
#ifdef MODULE_SVG_ENABLED
Expand Down
1 change: 1 addition & 0 deletions platform/android/export/godot_plugin_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
/**************************************************************************/

#include "godot_plugin_config.h"

/*
* Set of prebuilt plugins.
* Currently unused, this is just for future reference:
Expand Down
1 change: 1 addition & 0 deletions platform/android/file_access_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#define FILE_ACCESS_ANDROID_H

#include "core/io/file_access.h"

#include <android/asset_manager.h>
#include <android/log.h>
#include <stdio.h>
Expand Down
3 changes: 2 additions & 1 deletion platform/android/file_access_filesystem_jandroid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@

#include "file_access_filesystem_jandroid.h"

#include "thread_jandroid.h"

#include "core/os/os.h"
#include "core/templates/local_vector.h"
#include "thread_jandroid.h"

#include <unistd.h>

Expand Down
3 changes: 2 additions & 1 deletion platform/android/file_access_filesystem_jandroid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@
#ifndef FILE_ACCESS_FILESYSTEM_JANDROID_H
#define FILE_ACCESS_FILESYSTEM_JANDROID_H

#include "core/io/file_access.h"
#include "java_godot_lib_jni.h"

#include "core/io/file_access.h"

class FileAccessFilesystemJAndroid : public FileAccess {
static jobject file_access_handler;
static jclass cls;
Expand Down
7 changes: 4 additions & 3 deletions platform/android/java_godot_io_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@
#ifndef JAVA_GODOT_IO_WRAPPER_H
#define JAVA_GODOT_IO_WRAPPER_H

#include <android/log.h>
#include <jni.h>
#include "string_android.h"

#include "core/math/rect2i.h"
#include "core/variant/typed_array.h"
#include "string_android.h"

#include <android/log.h>
#include <jni.h>

// Class that makes functions in java/src/org/godotengine/godot/GodotIO.java callable from C++
class GodotIOJavaWrapper {
Expand Down
23 changes: 11 additions & 12 deletions platform/android/java_godot_lib_jni.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,37 +30,36 @@

#include "java_godot_lib_jni.h"

#include "java_godot_io_wrapper.h"
#include "java_godot_wrapper.h"

#include "android/asset_manager_jni.h"
#include "android_input_handler.h"
#include "api/java_class_wrapper.h"
#include "api/jni_singleton.h"
#include "core/config/engine.h"
#include "core/config/project_settings.h"
#include "core/input/input.h"
#include "dir_access_jandroid.h"
#include "display_server_android.h"
#include "file_access_android.h"
#include "file_access_filesystem_jandroid.h"
#include "java_godot_io_wrapper.h"
#include "java_godot_wrapper.h"
#include "jni_utils.h"
#include "main/main.h"
#include "net_socket_android.h"
#include "os_android.h"
#include "string_android.h"
#include "thread_jandroid.h"
#include "tts_android.h"

#include <android/input.h>
#include <unistd.h>

#include <android/native_window_jni.h>
#include "core/config/engine.h"
#include "core/config/project_settings.h"
#include "core/input/input.h"
#include "main/main.h"

#ifdef TOOLS_ENABLED
#include "editor/editor_settings.h"
#endif

#include <android/asset_manager_jni.h>
#include <android/input.h>
#include <android/native_window_jni.h>
#include <unistd.h>

static JavaClassWrapper *java_class_wrapper = nullptr;
static OS_Android *os_android = nullptr;
static AndroidInputHandler *input_handler = nullptr;
Expand Down
5 changes: 3 additions & 2 deletions platform/android/java_godot_view_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@
#ifndef JAVA_GODOT_VIEW_WRAPPER_H
#define JAVA_GODOT_VIEW_WRAPPER_H

#include "string_android.h"

#include "core/math/vector2.h"

#include <android/log.h>
#include <jni.h>

#include "string_android.h"

// Class that makes functions in java/src/org/godotengine/godot/GodotView.java callable from C++
class GodotJavaViewWrapper {
private:
Expand Down
9 changes: 5 additions & 4 deletions platform/android/java_godot_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,14 @@
#ifndef JAVA_GODOT_WRAPPER_H
#define JAVA_GODOT_WRAPPER_H

#include <android/log.h>
#include <jni.h>

#include "core/templates/list.h"
#include "java_godot_view_wrapper.h"
#include "string_android.h"

#include "core/templates/list.h"

#include <android/log.h>
#include <jni.h>

// Class that makes functions in java/src/org/godotengine/godot/Godot.java callable from C++
class GodotJavaWrapper {
private:
Expand Down
6 changes: 4 additions & 2 deletions platform/android/jni_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@
#define JNI_UTILS_H

#include "string_android.h"
#include <core/config/engine.h>
#include <core/variant/variant.h>

#include "core/config/engine.h"
#include "core/variant/variant.h"

#include <jni.h>

struct jvalret {
Expand Down
17 changes: 8 additions & 9 deletions platform/android/os_android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,24 @@

#include "os_android.h"

#include "dir_access_jandroid.h"
#include "display_server_android.h"
#include "file_access_android.h"
#include "file_access_filesystem_jandroid.h"
#include "java_godot_io_wrapper.h"
#include "java_godot_wrapper.h"
#include "net_socket_android.h"

#include "core/config/project_settings.h"
#include "drivers/unix/dir_access_unix.h"
#include "drivers/unix/file_access_unix.h"
#include "main/main.h"
#include "platform/android/display_server_android.h"
#include "scene/main/scene_tree.h"
#include "servers/rendering_server.h"

#include "dir_access_jandroid.h"
#include "file_access_android.h"
#include "file_access_filesystem_jandroid.h"
#include "net_socket_android.h"

#include <dlfcn.h>
#include <sys/system_properties.h>

#include "java_godot_io_wrapper.h"
#include "java_godot_wrapper.h"

const char *OS_Android::ANDROID_EXEC_PATH = "apk";

String _remove_symlink(const String &dir) {
Expand Down
1 change: 1 addition & 0 deletions platform/android/os_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#define OS_ANDROID_H

#include "audio_driver_opensl.h"

#include "core/os/main_loop.h"
#include "drivers/unix/os_unix.h"
#include "servers/audio_server.h"
Expand Down
13 changes: 7 additions & 6 deletions platform/android/plugin/godot_plugin_jni.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@

#include "godot_plugin_jni.h"

#include <core/config/engine.h>
#include <core/config/project_settings.h>
#include <core/error/error_macros.h>
#include <platform/android/api/jni_singleton.h>
#include <platform/android/jni_utils.h>
#include <platform/android/string_android.h>
#include "api/jni_singleton.h"
#include "jni_utils.h"
#include "string_android.h"

#include "core/config/engine.h"
#include "core/config/project_settings.h"
#include "core/error/error_macros.h"

static HashMap<String, JNISingleton *> jni_singletons;

Expand Down
4 changes: 3 additions & 1 deletion platform/android/string_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@
#ifndef STRING_ANDROID_H
#define STRING_ANDROID_H

#include "core/string/ustring.h"
#include "thread_jandroid.h"

#include "core/string/ustring.h"

#include <jni.h>

/**
Expand Down
4 changes: 2 additions & 2 deletions platform/android/thread_jandroid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@

#include "thread_jandroid.h"

#include <android/log.h>

#include "core/os/thread.h"

#include <android/log.h>

static JavaVM *java_vm = nullptr;
static thread_local JNIEnv *env = nullptr;

Expand Down
Loading