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

[gestures] Tap gesture not properly recognized on Raspberry Pi #3335

Closed
AndrewCapon opened this issue Sep 21, 2023 · 28 comments
Closed

[gestures] Tap gesture not properly recognized on Raspberry Pi #3335

AndrewCapon opened this issue Sep 21, 2023 · 28 comments
Labels
platform: DRM/RPI DRM/Raspberry Pi platform

Comments

@AndrewCapon
Copy link

AndrewCapon commented Sep 21, 2023

Issue description

Using 'core_input_gestures' I see an issue where only HOLD, DRAG, PINCH IN and PINCH OUT are recognised.

I have had a look at the code, starting with why TAP is not recognised as I'm guessing if this started working the rest would!

So with the help of some debug logging:

[0xf78cf040] is the main thread, [0xf0155440] is the event handling thread :

'UpdateGestures()' is called via 'EndDrawing()' -> 'PollInputEvents()' every frame.

I am logging the current gesture at the start of the main loop.

DEBUG: [0xf78cf040] Start Main Loop Gesture = 0
DEBUG: [0xf78cf040] UpdateGestures()

DEBUG: [0xf78cf040] Start Main Loop Gesture = 0
DEBUG: [0xf78cf040] UpdateGestures()

DEBUG: [0xf78cf040] Start Main Loop Gesture = 0
DEBUG: [0xf78cf040] UpdateGestures()

DEBUG: [0xf78cf040] Start Main Loop Gesture = 0
DEBUG: [0xf0155440] processGestureEvent() current = 1
DEBUG: [0xf0155440] processGestureEvent() current = 1
DEBUG: [0xf0155440] processGestureEvent() current = 1
DEBUG: [0xf0155440] processGestureEvent() current = 1
DEBUG: [0xf78cf040] UpdateGestures()

DEBUG: [0xf78cf040] Start Main Loop Gesture = 4
DEBUG: [0xf78cf040] UpdateGestures()

If we look at the code for 'UpdateGestures()' we can see the first thing it does is change the TAP (1) to HOLD (4)

void UpdateGestures(void)
{
    // NOTE: Gestures are processed through system callbacks on touch events
    TraceLog(LOG_DEBUG, "[%p] UpdateGestures()", pthread_self());
    // Detect GESTURE_HOLD
    if (((GESTURES.current == GESTURE_TAP) || (GESTURES.current == GESTURE_DOUBLETAP)) && (GESTURES.Touch.pointCount < 2))
    {
        GESTURES.current = GESTURE_HOLD;
        GESTURES.Hold.timeDuration = rgGetCurrentTime();
    }

    // Detect GESTURE_NONE
    if ((GESTURES.current == GESTURE_SWIPE_RIGHT) || (GESTURES.current == GESTURE_SWIPE_UP) || (GESTURES.current == GESTURE_SWIPE_LEFT) || (GESTURES.current == GESTURE_SWIPE_DOWN))
    {
        GESTURES.current = GESTURE_NONE;
    }
}

So I'm guessing this probably once worked and something may have changed?

There is no synchronisation between the threads so I guess its fairly random what might happen but this seems to be the issue I am seeing all the time.

Anyone any ideas?

Cheers

Andy

@raysan5
Copy link
Owner

raysan5 commented Sep 21, 2023

@ubkp were you working on gestures lately? or was somebody else?

@ghost
Copy link

ghost commented Sep 21, 2023

@raysan5 I was back in july, but was mostly fixing gestures for PLATFORM_WEB and PLATFORM_DESKTOP:

I don't think I reviewed it for PLATFORM_RPI or PLATFORM_DRM. I did fix mouse wheel for PLATFORM_RPI and PLATFORM_DRM (#3193) tho, but that's unrelated.

If you don't mind keeping this open for a few days, I can review it after the split.

Edit: This will also be a lot easier/faster to fix after the split. And less prone to break web and desktop too.

@raysan5
Copy link
Owner

raysan5 commented Sep 21, 2023

This will also be a lot easier/faster to fix after the split. And less prone to break web and desktop too.

@ubkp Agree. OK, no problem, no hurries.

@AndrewCapon
Copy link
Author

Thanks Guys.

Has there been any thought put into multi-gestures, not sure that's the right word?

Basically multiple at the same time, so multiple taps for instance.

@ghost
Copy link

ghost commented Sep 21, 2023

@AndrewCapon https://github.com/raysan5/raylib/blob/master/src/rgestures.h#L405-L408

Edit: Sorry, I think I've read your message too quickly:

  1. For generic multitouch (e.g.: 3+ points) I believe only the handling logic is missing (as per the link above).
  2. For more complex multitouch (e.g.: multiple people interacting with a touch table or large screen keeping their points grouped and independent from each other), you'd probably need to build that handling logic from scratch. But that's such a niche use case that should probably be handled separately. I think it would add too much complexity for regular/common multitouch scenarios (e.g: desktop/web/mobile).

@AndrewCapon
Copy link
Author

Hi @ubkp

Thanks for the info, I was thinking of the second case but really the idea of multiple controls (say sliders or lists for example) on the screen each being able to have their own gesture handling.

I have had a look at RayGui but that seems to be based on the Mouse code so is not multi touch either.

I'll have a look what I can knock up...

Cheers

Andy

@ghost
Copy link

ghost commented Sep 22, 2023

@AndrewCapon The following are my personal opinions:

  1. Raylib as a library should endeavor to provide the primitive tools through which the user can build his solutions; as opposed to an engine that manages solutions the user can use.

  2. Functionality (like the one described), IMHO, should be handled inside the program/game.

  3. I'm always concerned of feature creep and the effects it has. For example: the camera module has a set of cameras hardcoded in it (L432-L511), IMHO, that shouldn't be there at all. Not only any program/game (aside from a quick example/demo) will have to edit rcamera.h to do basic stuff like changing the keybinds, but also, if it ever changes, if could break what you've been using. IMHO, the only things the camera module should provide is L127-L143 (and maybe L514), and examples should teach/suggest to the user how a camera could be implemented with them.

  4. I'm also always concerned about performance. The more the library tries to manage itself, the more the program/game has to carry. That's really not a problem on desktop, but try running the examples on web over mobile. You need to be really mindful of what you really need and what you can trim off. And stuff the library tries to manage become really hard to trim off.

  5. I'm in no way trying to dissuade you, that functionality looks really cool, but I just wouldn't like the library having to manage it. If the primitive tools (L1165-L1172) necessary to implement such functionality are missing, we should totally look into that tho.

Edit: edits.

@AndrewCapon
Copy link
Author

Hi @ubkp

Yep I fully understand :)

@raysan5
Copy link
Owner

raysan5 commented Sep 23, 2023

@ubkp 100% agree with all the mentioned points, specially with feature creep concerns that it's something I'll fight against for years for raylib (and still it kept growing). In that regards, I agree with rcamera.h concerns, considered removing it many times and even raylib-extras repo contains better alternatives. It was just kept for convenience for small examples and demos.

One of the biggest concerns with raylib at the moment is maintenance, it's already very difficult to maintain the library, not only the source code with its issues/PRs but also all the ecosystem around it (social networks, community, webpage, examples, sample games...), so, adding more features could just make it more difficult.

@ghost
Copy link

ghost commented Sep 23, 2023

@raysan5 Yeah, that's way too much. Some things that could help:

  1. Merge windows. For example, PRs could be evaluated just during the first week of the month. That gives lots of room for the submitter to think over it, revise it, discuss it. And if it's not ready on that week, just continue/try again on the next window. The Linux Kernel uses something akin to that.

  2. Demos instead of examples. Currently there are a lot of examples and they are usually focused on a single thing. Demos could instead focus on all related functions. For example, a single demo for all these (L1028-L1039) would be a lot easier to maintain and reference.

  3. Schedule social media posts. Social media could be updated once a month with a digest. The Linux Mint Blog is an excellent example of that. It gets updated just once a month with the gist of what's going on or when there's a new release.

  4. Focus on main/core. Raylib grew a lot and I'm not sure if it's feasible to maintain in its full extension. At some point I think it will be necessary to choose what is really essential (main/core) and what is not. Linux distros use a similar logic with core vs community/contrib/universe. The parts considered essential should be actively maintained. Everything else could lag behind and/or get updated when there's interest (or even moved out to it's own file/etc).

@raysan5 raysan5 changed the title Issue with Gestures - Raspberry Pi [gestures] Tap gesture not properly recognized on Raspberry Pi Sep 24, 2023
@raysan5 raysan5 added the platform: DRM/RPI DRM/Raspberry Pi platform label Sep 24, 2023
@ghost
Copy link

ghost commented Sep 24, 2023

@AndrewCapon @raysan5 Temporarily working from the current master branch (c8a6093) while the split is in progress. Did a similar test:

INFO: Initializing raylib 4.6-dev
[...]
UPDATE       0 NONE
[...]
UPDATE       0 NONE
PROCESS MOVE 0 NONE
PROCESS MOVE 0 NONE
PROCESS MOVE 0 NONE
UPDATE       0 NONE
[...]
UPDATE       0 NONE
PROCESS MOVE 0 NONE
PROCESS DOWN 0 NONE
PROCESS DOWN 1 TAP
UPDATE       1 TAP
UPDATE       4 HOLD
UPDATE       4 HOLD
UPDATE       4 HOLD
PROCESS DOWN 4 HOLD
PROCESS UP   1 UP
PROCESS UP   0 NONE
UPDATE       0 NONE
[...]
UPDATE       0 NONE
[...]
INFO: Window closed successfully

It appears that the gestures are correctly detected and processed.

When a tap (GESTURE_TAP) begins (TOUCH_ACTION_DOWN), it gets processed (ProcessGestureEvent) correctly. It's also correctly being turned to hold (GESTURE_HOLD) during the update (UpdateGestures). And when the tap ends (TOUCH_ACTION_UP) it also gets processed correctly and cleared (GESTURE_NONE).

Problem

The problem with PLATFORM_DRM appears to be that by the time the program/game does a GetGestureDetected() or IsGestureDetected() it won't catch the expected "PROCESS DOWN 1 TAP", likely because (unlike the other platforms) PLATFORM_DRM uses evdev which runs on its own thread and lacks a proper callback system (unlike GLFW, emscripten, NDK).

Nevertheless, I've reviewed all commits I did to the gesture module and none of them should have had any effect into that. So I rolled back to master branch before all those (fdc28fc) and it wasn't working for PLATFORM_DRM even back then. If it ever worked, it probably was way before.

Solution

Regarding how to solve this, the ideal would be PLATFORM_DRM having access to some callback system. But that's probably not an option.

Give me a few days to think some workaround for it. I just don't want to break (or have to heavily patch) the gesture module to handle just this at the cost of all other platforms.

Environment

Tested PLATFORM_DRM on Raspberry Pi 3 Model B 1.2 with Raspberry Pi OS (11 Bullseye) 32-bit armhf.

Edit 1, 2: added the test environment.

@raysan5
Copy link
Owner

raysan5 commented Sep 24, 2023

@ubkp Thank you very much for investigating this issue. It expectable that PLATFORM_DRM works different than other platforms considering the raw low-level input access. It would probably be easier to track this kind of platform-specific nuances after the platform-split.

@ghost
Copy link

ghost commented Sep 24, 2023

@raysan5 Agreed. 👍

Leaving this note here for when it's time to pick up this issue again:
We'll probably need to store the last "meaningful" gesture on some var (similar to #3193) for PLATFORM_DRM.

@AndrewCapon
Copy link
Author

Thanks guys, there is no hurry at all from my point.

@raysan5 raysan5 added the hacktoberfest Hacktoberfest recommended issues label Oct 3, 2023
@raysan5 raysan5 removed the hacktoberfest Hacktoberfest recommended issues label Oct 27, 2023
@raysan5
Copy link
Owner

raysan5 commented Nov 1, 2023

@ubkp @michaelfiber do you think this issue could be somewhat addressed now that the platform-split has been completed?

@ghost
Copy link

ghost commented Nov 1, 2023

@raysan5 I'm working on it right now. Trying to get it solved today, but it's a tricky issue. I'll do my best.

@raysan5
Copy link
Owner

raysan5 commented Nov 1, 2023

@ubkp No hurries and no pressure, take it easy! I'm just reviewing the last open issues in case something can be easely addressed but I think raylib 5.0 can be already published without them.

@ghost
Copy link

ghost commented Nov 1, 2023

@raysan5 I just noticed I missed adding the gesture handling to PLATFORM_DESKTOP_SDL. I'm so sorry, I really don't know how I missed that.

I'll try to fix both together since, without callbacks, both SDL and DRM will need a similar handling. I'm giving this total priority.

@raysan5
Copy link
Owner

raysan5 commented Nov 1, 2023

@ubkp Oh, I just checked the gestures system yesterday, do you think the gestures system could be moved to rcore.c after platform PollInputEvents() is called? I think it just processes the already registered touch events (or mouse) to translate them into gestures, not sure how platform-dependant it is...

@ghost
Copy link

ghost commented Nov 1, 2023

@raysan5 As of right now, I think just part of it could.

The UpdateGestures(), that is called every frame, could probably be moved.

But the ProcessGestureEvent(), that has to be called where the event is detected (e.g.: callbacks, EventThread, etc), couldn't, since that differs for each platform.

But I'd advise against moving it right now. Since DRM (and now SDL) are not using callbacks systems, the position/timing/location of the UpdateGestures() call may have to change on those plataforms. I'm still working on a solution for it that doesn't require having to change rgestures.h to accommodate it.

@ghost
Copy link

ghost commented Nov 1, 2023

@raysan5 Fixed the gesture system for PLATFORM_DESKTOP_SDL. I'll send a PR for it shortly.

@raysan5
Copy link
Owner

raysan5 commented Nov 1, 2023

@ubkp thanks for the review! Agree that gestures system could be quite callback-dependant, better review it after new release.

@ghost
Copy link

ghost commented Nov 5, 2023

@raysan5 I think I found why the GESTURE_DRAG and GESTURE_SWIPE_* were not working reliably on PLATFORM_DRM. For some reason the GESTURES.Touch.downPositionA and GESTURES.Touch.upPosition are sometimes zero'd on this platform. When they are not zero, it works perfectly. I'm still investigating why/where that happens.

Edits: 1. typo; 2. correction.

@raysan5
Copy link
Owner

raysan5 commented Nov 5, 2023

@ubkp thanks for investigating this issue! Considering the touch positions are updated on a secondary thread, maybe it could be related to some race-condition...

@ghost
Copy link

ghost commented Nov 5, 2023

@raysan5 Yeah, I spent the afternoon trying to confirm it, but there are so many events overlapping (EV_REL, EV_ABS, ABS_PRESSURE, EV_KEY) it's hard to isolate. I've started commenting it all and trying to debug it part by part. It's a slow process tho, but I'll get there. 👍

@ghost
Copy link

ghost commented Nov 6, 2023

@raysan5 After trying basically everything I gave up fighting the threads and just moved everything related to mouse/touch/gesture's evdev to inside PollInputEvents(), like it's done on SDL. It works great and reduces complexity considerably. @michaelfiber is absolutely correct, moving the input to the main thread is the way to go.

I'll append it all to the very end of PollInputEvents(), so i t doesn't interefere with the DRM input rework and can be easily removed when necessary. Edit: I'll clean this up and send a PR in a few hours. And GESTURE_DOUBLETAP just broke. I'm sorry, I'll need more time over this.

@raysan5
Copy link
Owner

raysan5 commented Nov 6, 2023

@ubkp thanks for the review! No worries about the threads, I know they can be problematic, moving to single-thread is fine for me, actually, afaik, only mouse should be in a second thread. Take your time!

@ghost
Copy link

ghost commented Nov 7, 2023

@raysan5 Found a solution. I'm crafting the PR, will send it shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: DRM/RPI DRM/Raspberry Pi platform
Projects
None yet
Development

No branches or pull requests

2 participants