[rmkit][input] copy old mt slots from previous event #77

raisjn · 2021-02-03T05:18:35Z

remove wonky backwards event coalescing, instead start with prev_ev for all events
remove MouseEvent, its not applicable since we started using resim
set lifted on TouchEvent when a finger is lifted (TRACKING_ID set to -1)
add count_fingers() to TouchEvent and use TouchEvent.fingers in gestures.cpy for figuring out how many fingers pressed

mrichards42

Seems like this should work -- I haven't tried it out myself, but I'll give it a shot in a little bit on puzzles and let you know if something is obviously wrong.

mrichards42 · 2021-02-03T23:32:30Z

src/rmkit/input/input.cpy

-    def marshal(T ev):
+    // marshal can update the event
+    def marshal(T &ev):


I'm not seeing where marshal updates an event. Is this left over from a previous version, or am I missing something (likely since I don't know this code very well)?

mrichards42 · 2021-02-04T00:03:06Z

ok, I did a little testing of my own, just in a mouse.down event.

fingers was always 0, even when the last slot was > 0
so is_multitouch was always false, since that's calculated from fingers
if I ran through slots myself and checked for slots with left > -1, I usually got the right number of fingers
... but not always. sometimes I ended up with not as many fingers registered as were actually on the screen. I'd guess it just depends on how many fingers were in the event when the first EV_SYN was received, even if the next EV_SYN picked up more fingers, so I'm not sure what to do about that.
the worst part of all this is that if I took the stylus and put my hand on the screen as if to write, my hand only registered as 1 slot, so I'm not sure how to reject touch in that case :(

code I tested with:

    target->mouse.down += [=](auto &ev) {
        auto touch_ev = dynamic_cast<input::TouchEvent*>(ev.original.get());
        if (touch_ev) {
            int f = 0;
            for (auto s : touch_ev->slots)
                if (s.left > -1)
                    f++;
            std::cerr << "touch event with "
                << touch_ev->fingers << " fingers"
                << "; " << touch_ev->slot << " slots"
                << "; " << f << " slots with left > -1"
                << std::endl;
            if (touch_ev->is_multitouch) {
                std::cerr << "skipping multitouch" << std::endl;
                return;
            }
        }
    };

mrichards42 · 2021-02-04T00:23:21Z

Ah, ok I found https://www.kernel.org/doc/html/v4.18/input/multi-touch-protocol.html (you might already be aware of similar documentation, this is all new to me) which describes what all those ABS_MT_* fields mean. I see we aren't capturing ABS_MT_TOUCH_MAJOR or ABS_MT_TOUCH_MINOR, but those describe the size of the touch on the screen. For me a finger shows up as around major=17;minor=8. The edge of my hand has a lot of different sizes, but it seems like it's always at least major >= 26 OR minor >= 17, so maybe that's a reasonable threshold.

raisjn · 2021-02-04T00:40:46Z

i did know about those docs and have read about the axis minor/major, but didn't think about how to use them for palm detection, nice!

it seems like this diff is not behaving like i thought it would - but at least you have ideas for how to fix palm touch :-D

i think that holding prev_ev and using it as the base makes a lot of sense, so i'd like to achieve that

raisjn · 2021-02-04T01:46:13Z

ok, i think this works for coalescing the previous event (tested remux, harmony, mines, simple, genie) with touch and stylus. the major piece is in d96ebde.

genie gestures work and now use count_fingers() for filtering instead of the slot.

in addition to those docs, i also used https://elixir.bootlin.com/linux/latest/source/include/linux/input.h.

raisjn · 2021-02-04T02:18:24Z

~~have to do some more testing and debugging~~

was running into strangeness with remux due to remux touch flood, now fixed

i'll likely keep testing this and merge it on the weekend, but preliminary tests seem to show its working at least as well as previously

mrichards42

This works great on my end! The only thing that still seems off is the initial finger count (calling count_fingers() does always end up with the right number).

mrichards42 · 2021-02-04T02:32:08Z

src/rmkit/input/input.cpy

+          prev_ev = event
+          event = prev_ev


I think the event = prev_ev part is redundant? Also should this have a call to event.finalize() somewhere?

event.finalize() is called on EV_SYN

oh you're totally right, it's like 5 lines above this 🤦

mrichards42 · 2021-02-04T02:34:44Z

src/rmkit/input/events.cpy

      self.x = t.x
      self.y = t.y
      self.left = t.left
+      self.lifted = t.lifted


I'm still seeing fingers=0 in a SynMotionEvent handler when going through dynamic_cast<TouchEvent*>(ev.original().get())->fingers. I think it's b/c that isn't copied here. Does this need an explicit copy constructor, or would the default work?

i removed explicit constructor, hopefully things improve, but it might be that count_fingers() is always useful to call before checking fingers (and we put that in to main_loop?).

mrichards42 · 2021-02-04T02:43:01Z

src/rmkit/input/events.cpy

-          break
+            if self.left == 0:
+              self.lifted = true


When we see tracking_id -1 would we want to reset the slot to its initial -1, -1, -1 values? I assume tracking_id=-1 should mean that any existing x,y coords are invalid.

if the tracking ID is set to -1, next time the slot is used it will get an X and Y coordinate, i believe.

one potential problem with setting to -1 at this point is that this event is still used (i believe) during mainloop's event dispatch.

* remove copy constructor

raisjn · 2021-02-04T15:34:44Z

thanks for the help with this - it definitely is better code now. i just glanced at docs again and grepped "palm" and saw MT_TOOL_PALM. i'm curious if we will get this tool type or not. (accidentally put comment in wrong PR a moment ago)

UPDATE: nope, i think evtest says MT_TOOL_PALM is not supported (max of ABS_MT_TOOL_TYPE is 1, should be >= 2 when PALM is supported)

[rmkit] copy old mt slots from previous event

93df17e

raisjn changed the title ~~[rmkit] copy old mt slots from previous event~~ [rmkit][input] copy old mt slots from previous event Feb 3, 2021

raisjn mentioned this pull request Feb 3, 2021

[rmkit] misc upgrades & asks #75

Open

12 tasks

move merge() to a separate step from marshal

b2eb74e

mrichards42 approved these changes Feb 3, 2021

View reviewed changes

raisjn marked this pull request as draft February 4, 2021 00:41

okay added 3 commits February 3, 2021 17:28

[input] use prev_ev more properly

d96ebde

[input] remove old MouseEvent, we use Resim now

7b94d0d

[input] remove merge() methods

a01cc1b

raisjn marked this pull request as ready for review February 4, 2021 01:46

[ui] small comment cleanup

ca9cb49

raisjn marked this pull request as draft February 4, 2021 02:18

[remux] when sending touch flood, reset current fingers to lifted

d2cbe43

raisjn marked this pull request as ready for review February 4, 2021 02:38

mrichards42 approved these changes Feb 4, 2021

View reviewed changes

more review feedback

685d506

* remove copy constructor

[input] don't keep fingers member in TouchEvent

cfba7e2

raisjn merged commit bcbf75a into master Feb 6, 2021

raisjn mentioned this pull request Feb 6, 2021

[rmkit] Slots not persisted between TouchEvents #74

Closed

raisjn deleted the fix_mt_slots branch February 7, 2021 13:45

[rmkit][input] copy old mt slots from previous event #77

[rmkit][input] copy old mt slots from previous event #77

Uh oh!

Conversation

raisjn commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrichards42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrichards42 commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrichards42 commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raisjn commented Feb 4, 2021

Uh oh!

raisjn commented Feb 4, 2021

Uh oh!

raisjn commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrichards42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raisjn commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

raisjn commented Feb 3, 2021 •

edited

Loading

mrichards42 commented Feb 4, 2021 •

edited

Loading

mrichards42 commented Feb 4, 2021 •

edited

Loading

raisjn commented Feb 4, 2021 •

edited

Loading

raisjn commented Feb 4, 2021 •

edited

Loading