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

Implement OS.get_screen_orientation() for Android #43022

Merged
merged 1 commit into from
Nov 15, 2020

Conversation

Klowner
Copy link
Contributor

@Klowner Klowner commented Oct 22, 2020

I'm fairly certain this fixes #17109, I'm also happy to do this as a change against master, but I can't get that to run on my android device at the moment 😃

tested on my Google Pixel 3 👍

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

This is already fixed in master by e167af3. You can follow a similar approach.

@Klowner
Copy link
Contributor Author

Klowner commented Nov 13, 2020

Maybe there's some confusion over this proposed change. The implementation of getScreenOrientation() in GodotIO.java (in master) is just a getter for whatever value the user previously set via setScreenOrientation().

This change introduces the logic for getting the device's actual orientation, is that not the more desirable behavior? If so, this change should also be forward-ported to master.

@m4gr3d
Copy link
Contributor

m4gr3d commented Nov 13, 2020

Maybe there's some confusion over this proposed change. The implementation of getScreenOrientation() in GodotIO.java (in master) is just a getter for whatever value the user previously set via setScreenOrientation().

This change introduces the logic for getting the device's actual orientation, is that not the more desirable behavior? If so, this change should also be forward-ported to master.

@Klowner The master fix is actually the proper implementation. See the javadocs for Activity#getRequestedOrientation().

@Klowner
Copy link
Contributor Author

Klowner commented Nov 13, 2020

@m4gr3d okay, so the distinction between requested orientation and current orientation is the issue.

As long as calling setScreenOrientation(SCREEN_ORIENTATION_SENSOR) causing all subsequent calls to getScreenOrientation() to return SCREEN_ORIENTATION_SENSOR regardless of the device's current orientation is the desired behavior, then I'll close this PR.

Thanks!

@m4gr3d
Copy link
Contributor

m4gr3d commented Nov 13, 2020

@m4gr3d okay, so the distinction between requested orientation and current orientation is the issue.

As long as calling setScreenOrientation(SCREEN_ORIENTATION_SENSOR) causing all subsequent calls to getScreenOrientation() to return SCREEN_ORIENTATION_SENSOR regardless of the device's current orientation is the desired behavior, then I'll close this PR.

Thanks!

@Klowner Not quite. The screen orientation for the current app is driven by two factor:

  • At first it's set by the value in the AndroidManifest. If nothing else happen, then calling Activity#getRequestedOrientation() will return that value.
  • However, you can modify the orientation at runtime by using Activity#setRequestedOrientation(). In that case, calling Activity#getRequestedOrientation() will return the last orientation that was set.

Godot uses both approach (in theory, there's another related bug we're fixing, see #39266). So at start time, the app uses the orientation value specified by the AndroidManifest, then when Godot's main loop is initialized, it sets the requested orientation as specified in the project settings (same behavior across all platform).

So requested orientation is the same as current orientation :)

By the way, this PR is still needed since the fix for master was part of a larger rewrite (in preparation for Godot 4.0) which makes it impractical to backport to the 3.2 branch.
Your changes so far looks good, it's just the implementation in getScreenOrientation() that needs to be updated to what master is doing, i.e: return activity.getRequestedOrientation().

JNIEnv *env = ThreadAndroid::get_env();
return env->CallIntMethod(godot_io_instance, _get_screen_orientation);
} else {
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be safer to return 0 here. While incorrect, it would ensure that the value matches the OS::ScreenOrientation enum.

@Klowner
Copy link
Contributor Author

Klowner commented Nov 13, 2020

@m4gr3d ah, thank you for directing me to that ticket and I apologize for misunderstanding. I didn't realize y'all had addressed the issue via other means. 👍

I'll rebase and swap the body of getScreenOrientation() with the one from master

@Klowner Klowner force-pushed the 3.2-android-display-orientation branch from 066083f to 92ff6c5 Compare November 13, 2020 15:14
@m4gr3d m4gr3d requested a review from a team November 15, 2020 08:36
@akien-mga akien-mga merged commit c36a755 into godotengine:3.2 Nov 15, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021
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.

4 participants