Cleanup Android input on render thread settings

[m4gr3d]

Follow up to #93933

Clean up the set of settings used to control whether Android input should be dispatched on the render thread.

Addresses comments in #93933 (comment) and #93933 (comment)

Fixes #94132
Fixes #94202

[RandomShaper]

Looks great. I also totally agree with merging the concepts of input buffering and agile flushing since they always work in tandem and that makes things simpler.

My only remaining concern is regarding accumulated input. I think whether it's enabled or nor shouldn't contribute to deciding which thread to send events through. Accumulation works regardless agile flushing is enabled or not since it will use the input buffer regardless. In case agile flushing is enabled, well, it will have fewer chances of merging events, but between flushes it will still accumulate as events come.

[m4gr3d]

My only remaining concern is regarding accumulated input. I think whether it's enabled or nor shouldn't contribute to deciding which thread to send events through. Accumulation works regardless agile flushing is enabled or not since it will use the input buffer regardless. In case agile flushing is enabled, well, it will have fewer chances of merging events, but between flushes it will still accumulate as events come.

@RandomShaper I am not sure I understand the concern, can you clarify.
Input events are dispatched on the render thread whenever input buffering is disabled. With the current logic this happens when both accumulated input and agile flushing are disabled, hence why accumulated input is checked prior to deciding where to dispatch the input events.

[RandomShaper]

Input events are dispatched on the render thread whenever input buffering is disabled. With the current logic this happens when both accumulated input and agile flushing are disabled, hence why accumulated input is checked prior to deciding where to dispatch the input events.

My point is that sending via the render thread should happen if, and only if, agile input is disabled. Whether accumulation is enabled or not is unrelated. Accumulation just means that events are not parsed directly but added to a buffer and new events can be merged with the last one in the buffer. That works the same regardless which thread sends the events. The only difference is that if agile flushing is enabled (events sent from UI thread), there will be multiple flush points per frame, and if agile is disabled (events from render thread) there will be a single flush.

[m4gr3d]

My point is that sending via the render thread should happen if, and only if, agile input is disabled. Whether accumulation is enabled or not is unrelated. Accumulation just means that events are not parsed directly but added to a buffer and new events can be merged with the last one in the buffer. That works the same regardless which thread sends the events. The only difference is that if agile flushing is enabled (events sent from UI thread), there will be multiple flush points per frame, and if agile is disabled (events from render thread) there will be a single flush.

Make sense! I've updated the criteria for dispatching on the render thread to only depend on the state of agile input.

[m4gr3d]

Make sense! I've updated the criteria for dispatching on the render thread to only depend on the state of agile input.

Thinking about it some more, I want to make sure we're all aligned on the new proposed behavior as it'll change the default input dispatch thread..

The previous criteria for dispatching input events from the UI thread was:

is there a buffer so that the UI thread doesn't block parsing and routing the input event?

This meant that both accumulated input and the default behavior (given input buffering was always enabled on Android) would dispatch input from the UI thread (a bit of a moot point though as there was no alternatives).

The new criteria for dispatching input events from the UI thread is:

is agile input enabled?

If true, we dispatch from the UI thread, otherwise we dispatch from the render thread.

The change in behavior is that previously, the default configuration (agile input disabled, accumulated input enabled) would dispatch from the UI thread.
With the update, the default configuration now dispatches from the render thread.

The concern here being that I'm leery of changing the default behavior, especially for users that are not seeing an issue with the default configuration.
As such, I'll advocate for the previous approach of conditioning the use of the render thread for dispatch on both accumulated input and agile input being disabled, as this most closely matches the previous behavior.

cc @RandomShaper @akien-mga

[akien-mga]

Looks ok to me, but I'll wait for RandomShaper to give it a second look too.

[RandomShaper]

My notion is that before #93933 input events on Android were delivered via the UI thread uncontionally (moreover, regardless accumulation being enabled or not).

Input buffering was enabled at startup regardless any setting:

godot/platform/android/display_server_android.cpp

Line 654 in e6448ca

Input::get_singleton()->set_use_input_buffering(true); // Needed because events will come directly from the UI thread

And then, for instance, when user triggered an input event, this code path woud be followed:

1. GodotInputHandler.onTouchEvent() would be the first function under our control getting the event that would then call back to another method of itself:
   

   godot/platform/android/java/lib/src/org/godotengine/godot/input/GodotInputHandler.java

   Line 225 in e6448ca

   return handleTouchEvent(event);

2. GodotInputHandler.handleTouchEvent() would then invoke the C++ implementation of dispatchTouchEvent():
   

   godot/platform/android/java/lib/src/org/godotengine/godot/input/GodotInputHandler.java

   Line 639 in e6448ca

   GodotLib.dispatchTouchEvent(eventActionOverride, actionPointerId, pointerCount, positions, doubleTap);

3. Java_org_godotengine_godot_GodotLib_dispatchTouchEvent() would in turn pass the event to AndroidInputHandler:
   

   godot/platform/android/java_godot_lib_jni.cpp

   Line 307 in e6448ca

   JNIEXPORT void JNICALL Java_org_godotengine_godot_GodotLib_dispatchTouchEvent(JNIEnv *env, jclass clazz, jint ev, jint pointer, jint pointer_count, jfloatArray position, jboolean p_double_tap) {

4. AndroidInputHandler::process_touch_event() would then send the event to Input to be parsed there:
   

   godot/platform/android/android_input_handler.cpp

   Line 238 in e6448ca

   Input::get_singleton()->parse_input_event(ev);

All that happens within the UI thread.

[RandomShaper]

I see you've changed the condition to use the render thread already. That matches my current best judgement of the situation. I hope my analysis helps bringing us to the same page.

[m4gr3d]

All that happens within the UI thread.

Yeah I agree, my comment was that now the default will be to use the render thread. If we are okay with that then I'm good to go.

Also should we have agile input flushing enabled by default?
Is there a reason why it's disabled by default?

[RandomShaper]

That's a good question. I don't have a strong opinion. If anything, given that it's only implemented on Android, it may be better to have it as opt-in per the least astonishment principle. In the future we should have the same thread split and so agile flushing will be available everywhere. If we ever reach that point, having it as a default would make more sense. But, again, I'm not a big proponent of either way.

[m4gr3d]

Working on one additional set of changes to help limit the creation of Runnable objects when dispatching input to the render thread.

[m4gr3d]

Working on one additional set of changes to help limit the creation of Runnable objects when dispatching input to the render thread.

On second thought, I'll do the memory optimizations in a separate PR, so this PR is good to go.

[Calinou]

I can confirm this fixes #94132.

[akien-mga]

Thanks!