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 touch input on Linux #14117

Merged
merged 1 commit into from Dec 20, 2023
Merged

Fix touch input on Linux #14117

merged 1 commit into from Dec 20, 2023

Conversation

grorp
Copy link
Member

@grorp grorp commented Dec 17, 2023

Fixes #14111.

The code relied on touch IDs being consecutive. This is true on Android, but not on Linux. Therefore, touch input on Linux was broken since 53886dc.

To do

This PR is a Ready for Review.

How to test

Play Minetest on Android. Verify that touchscreen input, and especially stack splitting, still work.

Play Minetest on Linux with a touchscreen. Verify that touchscreen input, and especially stack splitting, work again.

The code relied on touch IDs being consecutive. This is true on Android, but not on Linux.
Therefore, touch input on Linux was broken since 53886dc.
@okias
Copy link
Contributor

okias commented Dec 17, 2023

Play Minetest on Linux with a touchscreen. Verify that touchscreen input, and especially stack splitting, work again.

The issue has been resolved by this patch. Thank you.

@srifqi
Copy link
Member

srifqi commented Dec 17, 2023

The issue with this PR's approach is that each touch-pointer can no longer be independently moved.

5.8.0:

Minetest-5.8.0-Android-place-one.mp4

This PR:

Minetest-14117-Android-place-one.mp4

In my opinion, your original approach in the touch-screen support for SDL (minetest/irrlicht PR 262) is good. For now, that approach can be applied for Linux/X11 (my attempt).

@okias
Copy link
Contributor

okias commented Dec 17, 2023

The issue with this PR's approach is that each touch-pointer can no longer be independently moved.

5.8.0:
Minetest-5.8.0-Android-place-one.mp4

This PR:
Minetest-14117-Android-place-one.mp4

In my opinion, your original approach in the touch-screen support for SDL (minetest/irrlicht PR 262) is good. For now, that approach can be applied for Linux/X11 (my attempt).

This works for me in Linux. (not saying SDL isn't the right way) 😉

@srifqi
Copy link
Member

srifqi commented Dec 17, 2023

This works for me in Linux.

Sorry, which this did you mean?

@okias
Copy link
Contributor

okias commented Dec 17, 2023

This works for me in Linux.

Sorry, which this did you mean?

I tried to reproduce what you did on the video and it worked with patched 5.8.0 build on Linux phone.

@@ -273,7 +273,7 @@ bool GUIModalMenu::preprocessEvent(const SEvent &event)
irr_ptr<GUIModalMenu> holder;
holder.grab(this); // keep this alive until return (it might be dropped downstream [?])

if (event.TouchInput.ID == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

ouch, this shouldn't have passed code review. easy to miss however.

It sounds like what the code should be doing is remember the ID of the first and second touch when they begin.

Copy link
Member

Choose a reason for hiding this comment

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

I did not realise that the ID assignment is different for each device. I just checked on my phone and thought that it resets to zero when there is no touch. My bad.

@grorp
Copy link
Member Author

grorp commented Dec 17, 2023

Another possibility: Add a pointer index (as opposed to pointer ID) field to Irrlicht's STouchInput, and use that here. What my fix/workaround in minetest/irrlicht#262 did was essentially that it turned pointer IDs into pointer indexes.

The question is: Which solution do we choose?

@srifqi
Copy link
Member

srifqi commented Dec 17, 2023

Which solution do we choose?

How about a global helper that "remember" the order/index of each active touch? It can be in Minetest or Irrlicht (for each device type). If done this way, the other part does not have to check for the index.

@grorp
Copy link
Member Author

grorp commented Dec 18, 2023

After thinking about this some more, I believe this PR should be merged as is.

The limitation @srifqi is describing - that moving the first pointer doesn't work if a second pointer exists - only applies in formspecs and has existed approximately forever. Nobody ever complained about it AFAIK - probably because there is only a use case for tapping the screen with a second finger, not for keeping a second finger on the screen.

This PR just restores the state before the regression. More improvements would be nice, but should not be expected from a PR that is supposed to fix a regression.

Merging this soon would also be good so as not to block further work on the touchscreen code.

@sfan5 sfan5 merged commit 3b346fd into minetest:master Dec 20, 2023
13 checks passed
@grorp grorp deleted the fix-linux-touch branch December 20, 2023 20:25
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.

[regression] touchscreen stopped working with 5.8.0 on Linux
4 participants