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

Fix flickering issues with low processor mode on Android #59607

Merged
merged 1 commit into from Mar 29, 2022

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Mar 28, 2022

As pointed out by @Relintai in #55604 (comment), the cause of the flickers when enabling low processor mode is that the framebuffers are swapped automatically even when nothing is rendered.

To resolve the issue, I forked GLSurfaceView so that the Godot Android logic can specify when the framebuffers should be swapped.

Fixes #19304
Fixes #58932

Bugsquad edit: master version of #59606.

@m4gr3d m4gr3d added this to the 4.0 milestone Mar 28, 2022
@m4gr3d m4gr3d requested a review from a team as a code owner March 28, 2022 01:11
@m4gr3d m4gr3d force-pushed the fix_low_processor_mode_main branch 2 times, most recently from 7117a46 to 4a6adf0 Compare March 28, 2022 17:20
@m4gr3d m4gr3d requested a review from a team as a code owner March 28, 2022 17:20
@m4gr3d m4gr3d force-pushed the fix_low_processor_mode_main branch 2 times, most recently from f937f5f to f299b9b Compare March 28, 2022 22:19
@@ -243,9 +243,12 @@ JNIEXPORT void JNICALL Java_org_godotengine_godot_GodotLib_step(JNIEnv *env, jcl
DisplayServerAndroid::get_singleton()->process_magnetometer(magnetometer);
DisplayServerAndroid::get_singleton()->process_gyroscope(gyroscope);

if (os_android->main_loop_iterate()) {
bool should_swap_buffers;
Copy link
Member

Choose a reason for hiding this comment

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

I think we've favoring the defensive initialization style now, so it'd be good to initialize this one.

@@ -17,4 +17,4 @@ target_include_directories(${PROJECT_NAME}
SYSTEM PUBLIC
${GODOT_ROOT_DIR})

add_definitions(-DUNIX_ENABLED -DVULKAN_ENABLED -DTOOLS_ENABLED)
add_definitions(-DUNIX_ENABLED -DVULKAN_ENABLED -DANDROID_ENABLED)
Copy link
Member

Choose a reason for hiding this comment

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

This is a nitpick: I fell like changes of this kind should be in their own commit since they are not really part of the main task. For me, it'd be fine the alternative of just adding a line in the commit message telling that there are some additional little fixes unrelated to the main goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RandomShaper This change is mostly cosmetic and doesn't affect the engine logic.
That CMake file is only used by Android Studio to properly show the native engine code, and so has no effects on anything else.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

I've added a few comments about not very important things.

Besides them, my only real concern is the maintainability of the forked stuff. I'd suggest to follow the closest that is possible to the scheme followed for other 3rd-party code. Being Java files I'm not sure if they can be put under ´thirdparty/´, but at least it would be good to check out the changes as patch files for easier upgrading.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Mar 29, 2022

I've added a few comments about not very important things.

Besides them, my only real concern is the maintainability of the forked stuff. I'd suggest to follow the closest that is possible to the scheme followed for other 3rd-party code. Being Java files I'm not sure if they can be put under ´thirdparty/´, but at least it would be good to check out the changes as patch files for easier upgrading.

In the past for java code, we've departed from the thirdparty approach (also because I don't think that would work) and included forked files in the lib directory (see https://github.com/godotengine/godot/tree/master/platform/android/java/lib/src/com/google/android/vending).

I'm also not very concerned about maintainability given the forked files have been stable for quite a while (GLSurfaceView has been part of the Android apis since the very beginning), and are unlikely to change anytime soon (especially as Google has adopted the pattern of providing enhanced functionality in external libraries rather than updating the core apis).

@m4gr3d m4gr3d force-pushed the fix_low_processor_mode_main branch from f299b9b to b176b31 Compare March 29, 2022 19:17
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Fair enough.

@akien-mga akien-mga merged commit 04c17eb into godotengine:master Mar 29, 2022
@m4gr3d m4gr3d deleted the fix_low_processor_mode_main branch March 29, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants