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

Handle Touch Cancel events on iOS and Android #6871

Merged
merged 11 commits into from Jul 3, 2022
Merged

Conversation

obigu
Copy link
Contributor

@obigu obigu commented May 17, 2022

libGDX currently doesn't handle properly "Touch Cancelled" events generated on touch screen devices (iOS and Android backends affected).

On iOS they are treated as a touch up event which means that, if the user is touching the screen and there is an interruption (such as touching the Lock button or doing the the Home gesture), a touch up will be performed, causing unwanted clicks or unexpected behaviour on resume. On Android we have a bit of a mess and even if the touch up doesn't occur, we have other issues due to how we handle this (see #4866).

There's been a couple of attempts of improving the situation (#6783 and #6353) but they didn't deal with the origin of the issue that is our current inability to handle the CANCEL events.

The proposed solution adds a new touchCancelled method to InputProcessor that is only called by the Android and iOS backends when a CANCEL event is received.

To avoid changing Scene2D behaviour that deals with event cancellations by sending a touch up event with stage coordinates set to Integer.MIN_VALUE, the Stage implementation of touchCancelled calls Stage.cancelTouchFocus () which then generates the custom touch up events as usual.

This solution allows users to handle Touch Cancels on their own InputProcessor implementations, doesn't change Scene2D behaviour and cleans up the hacky onPause() code plus should fix the issues on the Android backend.

I've done some testing but some more is required, that's why I'll set it as draft.

@obigu
Copy link
Contributor Author

obigu commented May 18, 2022

Ran some more tests on iOS and Android and haven't found any issue and behaviour is now the expected. Changing to Ready for review.

@NathanSweet It'd be great if you could have a look.

@obigu obigu marked this pull request as ready for review May 18, 2022 11:12
@NathanSweet
Copy link
Member

The scene2d way of handling this is a bit of a hack. A lot of code looks at the distance from touch down to touch up, so it doesn't have to worry whether a touch up is a cancel or a touch up that is very far away. It is convenient not to need extra code for every touch down to handle touch cancel, which is relatively rare. The same could be done for InputProcessor, but it isn't terribly elegant. In some cases when it does matter then you get very strange behavior until you check if it is a cancel.

I'm not sure we want to document that it is limited to iOS and Android. It could make sense to generate a touch cancel on the desktop, even if we don't do it yet. I suppose we could remember to change the comments if we do.

The PR looks good to me. Having an explicit touch cancel event seems good and is probably not much of burden for implementations, which probably use InputAdapter. Transforming it into the scene2d touch cancel is good. Very minor style nitpicks: I tend to add public even for interfaces and I'm not a fan of @Override for library code. I think it's good to merge either way.

@obigu
Copy link
Contributor Author

obigu commented May 18, 2022

Thanks @NathanSweet.

Thanks for pointing out the code style inconsistencies, I've made changes to match the code style on the class. Much worse than the dislike that having public on interface methods or not or having @Override annotations or not may cause is to have some methods with and some without.

@obigu obigu requested a review from MrStahlfelge May 25, 2022 15:16
MrStahlfelge
MrStahlfelge previously approved these changes Jul 3, 2022
Copy link
Member

@MrStahlfelge MrStahlfelge left a comment

Choose a reason for hiding this comment

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

Besides the little comment I've made, this looks pretty good to me. I have also tested the behaviour regarding #4866 and it is finally solved.

@@ -173,6 +173,11 @@ public boolean touchUp (int x, int y, int pointer, int button) {
return false;
}

@Override
public boolean touchCancelled (int screenX, int screenY, int pointer, int button) {
return touchUp(screenX, screenY, pointer, button);
Copy link
Member

Choose a reason for hiding this comment

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

This class is rarely used, but I am not sure if this behaviour is the correct one here

Copy link
Contributor Author

@obigu obigu Jul 3, 2022

Choose a reason for hiding this comment

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

Haha, you've spotted it... yeah, me neither. My thought here was, before a touchUp was always expected after a touchDown so, to keep backwards compatibility considering I have no idea what's the use case of this class I pass it on touchCancel not to break anything.

Copy link
Member

Choose a reason for hiding this comment

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

It is for controlling a game from a remote device. A weird feature that probably could be removed.

@obigu obigu merged commit 9de4191 into libgdx:master Jul 3, 2022
@obigu obigu deleted the touchcancel branch July 3, 2022 16:07
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.

None yet

4 participants