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

Fix for Android double tap not triggering correctly #46100 and #46101 #54225

Closed
wants to merge 3 commits into from
Closed

Fix for Android double tap not triggering correctly #46100 and #46101 #54225

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
726ff9f
Fix for Android double tap not triggering correctly #46100 and #46101.
Ongnissim Oct 25, 2021
dcf9bd2
Merge branch 'godotengine:master' into master
Ongnissim Nov 6, 2021
398490b
Merge remote-tracking branch 'upstream/master'
Ongnissim Jun 27, 2022
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
2 changes: 1 addition & 1 deletion platform/android/java/lib/src/org/godotengine/godot/input/GodotGestureHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public boolean onDoubleTap(MotionEvent event) {
//Log.i("GodotGesture", "onDoubleTap");
final int x = Math.round(event.getX());
final int y = Math.round(event.getY());
final int buttonMask = event.getButtonState();
final int buttonMask = event.getButtonState() + (event.getToolType(event.getActionIndex()) != MotionEvent.TOOL_TYPE_MOUSE ? 1 : 0) * event.getPointerCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ongnissim Is double tap triggered with multiple fingers? I tested double tapping with 2 and 3 fingers and the event didn't trigger.
If it's not supported, then we can simplify this logic.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it is supported, but in the event that it would be supported, this returns a button mask of 0,1,2,3... for the number of fingers. I did this so that touch input would have parity with mouse input. I never got a response so I stopped working on it.

Are you saying that onDoubleTap doesn't trigger at all on double tapping with multiple fingers? I also couldn't get this to trigger how I expected, and would like to know how to make this work.

This is my first PR ever, so I won't be offended if this isn't the correct way to implement this, and just want the bug fixed :).

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to clone this again and test to see if getPointerIndex() does what I anticipated.
The idea is that you'd be able to have one finger down, double-tap a second finger, and this would register as a right-mouse double-click. Three for MMB.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is that you'd be able to have one finger down, double-tap a second finger, and this would register as a right-mouse double-click. Three for MMB.

@Ongnissim I tested that, and yes onDoubleTap doesn't trigger in this scenario.

This is my first PR ever, so I won't be offended if this isn't the correct way to implement this, and just want the bug fixed :).

The approach is pretty close and can be improved by incorporating some of the logic in #59760.

I think the proper approach here would be to pass the tooltype value alongside the button mask.
In the native code in AndroidInputHandler::process_double_tap, when the tooltype is MotionEvent.TOOL_TYPE_FINGER, then disregard the button mask and use the logic in #59760.
Otherwise, use the current logic that takes into account the button mask.

@madmiraal @thebestnom Thoughts?

Copy link
Contributor

@madmiraal madmiraal Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, to properly fix #46100 and #46101 (beyond #59760) would require separating out the mouse double-clicks from the double-taps. So that the start of the second tap is not also detected as the start of a second single click.
Note: The main challenge is ensuring the release of the second click releases the double tap too.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that the start of the second tap is not also detected as the start of a second single click.

@madmiraal That's part of a larger issue that also affects other gestures besides double-taps. Our current input logic sends both the raw input events (down, move, up) and the processed input events (detected gestures) to the native layer, so in scenarios where a gesture is detected, we end double-counting.

We need to update the logic so that we only sends raw input events when a gesture is not detected; it's part of a larger fix I'll be looking at unless one of you is up to the task :)

For this issue, we can scope the fix to re-enabling finger double-taps using the proposal I highlighted above since at the moment they're disabled because of the button mask value.
On top of fixing finger double-taps, this will allow to leverage the current logic which provide information regarding the source of the event when it's not a finger.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem isn't just with the Android platform. It was thinking through this issue that led me to create godotengine/godot-proposals#4340

GodotLib.doubleTap(buttonMask, x, y);
return true;
}
Expand Down