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

Add missing #ifndef _3D_DISABLED to main file #97442

Merged

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Sep 25, 2024

XR is disabled when 3D is disbled so there is no sense in setting xr specific settings and adding --xr-mode option

@dustdfg dustdfg requested a review from a team as a code owner September 25, 2024 11:16
main/main.cpp Outdated
@@ -2579,6 +2582,7 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
GLOBAL_DEF_BASIC("xr/openxr/extensions/hand_tracking_controller_data_source", false); // XR_HAND_TRACKING_DATA_SOURCE_CONTROLLER_EXT
GLOBAL_DEF_RST_BASIC("xr/openxr/extensions/hand_interaction_profile", false);
GLOBAL_DEF_BASIC("xr/openxr/extensions/eye_gaze_interaction", false);
#endif // _3D_DISABLED
Copy link
Member

Choose a reason for hiding this comment

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

This should enclose the few commented out lines below which are also about XR.

Copy link
Contributor Author

@dustdfg dustdfg Sep 25, 2024

Choose a reason for hiding this comment

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

I've just done force push and I've just remembered why I didn't do it before. TOOLS_ENABLED means you compile editor. Doesn't editor must to have all the features or it can be compiled without xr/3d?

Copy link
Member

Choose a reason for hiding this comment

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

It's true that the editor can't be compiled with disable_3d, so it should be equivalent either way. But semantically I think it's clearer to exclude all XR related code in this case.

Copy link
Contributor Author

@dustdfg dustdfg Sep 25, 2024

Choose a reason for hiding this comment

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

There also exist the following part of the code which also have xr option inside TOOLS_ENABLED:

godot/main/main.cpp

Lines 1045 to 1059 in 0a9d8f0

#ifdef TOOLS_ENABLED
if (arg == "--debug" ||
arg == "--verbose" ||
arg == "--disable-crash-handler") {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(arg);
forwardable_cli_arguments[CLI_SCOPE_PROJECT].push_back(arg);
}
if (arg == "--single-window") {
forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(arg);
}
if (arg == "--audio-driver" ||
arg == "--display-driver" ||
arg == "--rendering-method" ||
arg == "--rendering-driver" ||
arg == "--xr-mode") {

But I though that editor in general must contain everything and is not supposed to be built without 3d? Didn't see previous answer. So what should I do with this part (last line)?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, it could be:

diff --git a/main/main.cpp b/main/main.cpp
index 75797e31de..81bf5c735d 100644
--- a/main/main.cpp
+++ b/main/main.cpp
@@ -1055,8 +1055,11 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
                if (arg == "--audio-driver" ||
                                arg == "--display-driver" ||
                                arg == "--rendering-method" ||
-                               arg == "--rendering-driver" ||
-                               arg == "--xr-mode") {
+                               arg == "--rendering-driver"
+#ifndef _3D_DISABLED
+                               || arg == "--xr-mode"
+#endif
+               ) {
                        if (N) {
                                forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(arg);
                                forwardable_cli_arguments[CLI_SCOPE_TOOL].push_back(N->get());

but that's honestly a bit ugly, for something that's indeed not necessary as you pointed out.

It doesn't matter much either way, but I'd say in this case let's just live with the minor inconsistency and leave this one as is.

XR is disabled when 3D is disbled so there is no sense in
setting xr specific settings and adding `--xr-mode` option

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
@dustdfg dustdfg force-pushed the disable_xr/missing_disable_3d_ifndefs branch from b743641 to 4156077 Compare September 25, 2024 11:26
@akien-mga akien-mga merged commit dbf1efb into godotengine:master Sep 26, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@dustdfg dustdfg deleted the disable_xr/missing_disable_3d_ifndefs branch September 26, 2024 11:07
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.

3 participants