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 double tap & drag on Android #76791

Merged
merged 1 commit into from
May 9, 2023

Conversation

OmarShehata
Copy link
Contributor

This fixes #76587. This is very easy to reproduce, just compile on Android device a project which prints all input events:

extends Node2D
func _input(event):
	print(event)

In master, if you double tap & drag, no events are emitted for the drag. In this PR, you correctly see the drag events. This makes Android consistent with iOS and Windows (I haven't tested other platforms behavior).

The fix involves listening for motion events in the onDoubleTapEvent listener, because the native Android GestureDetector differentiates between motion events after a double tap and regular motion events.

@OmarShehata OmarShehata requested a review from a team as a code owner May 6, 2023 20:40
}
return true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why this was always returning true even for cases when it wasn't actually handling the event. I've changed it to only return true when it handles the event, consistent with the other event listeners here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look correct, this callback should receive both down and up events and with this update the logic now returns false for down events, which would have the effect of cancelling the next up event.
Can you log the events passing through this method and check we are still receiving the expected flow.

@@ -197,8 +197,13 @@ internal class GodotGestureHandler : SimpleOnGestureListener(), OnScaleGestureLi
if (event.actionMasked == MotionEvent.ACTION_UP) {
nextDownIsDoubleTap = false
GodotInputHandler.handleMotionEvent(event)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, is this correct, that a ACTION_UP should trigger a motion event?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct, this correspond to the release of the double tap event.

}
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look correct, this callback should receive both down and up events and with this update the logic now returns false for down events, which would have the effect of cancelling the next up event.
Can you log the events passing through this method and check we are still receiving the expected flow.

@OmarShehata
Copy link
Contributor Author

Thanks for the feedback @m4gr3d, I've force pushed an update (my understanding this is the preferred workflow to keep the commit history squashed). It now maintains the original behavior, but just handles the motion event.

I was able to confirm with additional logging the incorrect behavior you expected in my original attempt. We get an ACTION_DOWN event triggered here that we need to return true for but that should NOT trigger a handleMotionEvent (otherwise it triggers more events than expected).

@OmarShehata
Copy link
Contributor Author

Also just noting down here, this could be implemented in the following way and I tested that it has the same behavior (my test was double tap, drag, & release, and comparing the events emitted on the GDScript side):

override fun onDoubleTapEvent(event: MotionEvent): Boolean {
	if (event.actionMasked == MotionEvent.ACTION_UP) {
		nextDownIsDoubleTap = false
		GodotInputHandler.handleMotionEvent(event)
		return true
	}
	if (event.actionMasked == MotionEvent.ACTION_MOVE) {
		GodotInputHandler.handleMotionEvent(event)
		return true
	}
	if (event.actionMasked == MotionEvent.ACTION_DOWN) {
		// No need to handle this, just need to return true
		// otherwise you get 2 extra events (pressed=true & pressed=false) when double tapping
		return true
	}
	return false
}

The version I pushed is more concise/closer to the original code.

@@ -197,7 +197,10 @@ internal class GodotGestureHandler : SimpleOnGestureListener(), OnScaleGestureLi
if (event.actionMasked == MotionEvent.ACTION_UP) {
nextDownIsDoubleTap = false
GodotInputHandler.handleMotionEvent(event)
} else if (event.actionMasked == MotionEvent.ACTION_MOVE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update, this looks like it should handle the issue!

There's one more scenario to consider and incorporate in the logic; when panning and scaling is enabled (panningAndScalingEnabled == true), the ScaleGestureDetector logic treats double_tap followed by swipe as a magnify event:

This capability is still private (not exposed to GDScript) and only being used by the Android editor to support scaling in the spatial editor.
To complement your fix and not break that functionality, you'll need to check that panningAndScalingEnabled == false before forwarding the motion event.

@OmarShehata
Copy link
Contributor Author

@m4gr3d just updated to prevent handling the event in the case of panningAndScalingEnabled == false. I've never used the Android version of the editor before, but I tested this by enabling panning & scaling as follows, and checking that when it is enabled, we go back to the old behavior (no drag events emitted after double tap)

this.godotGestureHandler.setPanningAndScalingEnabled(true);

Let me know if this looks right!

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.

Looks great, thanks for the fix!

@m4gr3d m4gr3d added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label May 9, 2023
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label May 9, 2023
@akien-mga akien-mga merged commit d550fdd into godotengine:master May 9, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@OmarShehata
Copy link
Contributor Author

OmarShehata commented May 9, 2023

Thank you so much @akien-mga !! Thank you & all the contributors who spent the time & effort creating this amazing engine & documenting everything so well that made me feel confident enough to give this a shot 🙏

@akien-mga
Copy link
Member

Cherry-picked for 4.0.3.

@akien-mga
Copy link
Member

Cherry-picked for 3.6.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 28, 2023
@akien-mga
Copy link
Member

This fix can't be cherry-picked for 3.5.x as is, the code is significantly different and I can't find easily where to do a similar change.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 28, 2023
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.

double_tap & drag on Android fails to detect drag
4 participants