flxzt / rnote

Sketch and take handwritten notes.
https://rnote.flxzt.net
GNU General Public License v3.0
8.35k stars 293 forks source link

Pointer events from other devices cause pen to jump #665

Open cyclane opened 1 year ago

cyclane commented 1 year ago

Describe the bug
Pointer events from other devices cause the pen to jump to the other device's pointer position, usually making an annoying mark on the page.

To Reproduce Steps to reproduce the behaviour:

  1. Open Rnote in Wayland with a pen device (e.g. Wacom drawing tablet) and a mouse.
  2. Start drawing using the drawing tablet (or other pen device) and then move the mouse around.
  3. Observe the pen jumping to the mouse position and making unwanted marks.

Expected behavior
Ideally the cursor might still visually jump, but not draw to a different pointer device's position.

Console Output
(with the log::debug macro in canvas/src/input.rs, also with pointer position (x, y) and length and index of elements vector):

...
 2023-05-10T15:16:57.614Z DEBUG rnote::canvas::input > (518.0, 172.3) handle event, state: Proximity, event_time_d: 0ns, modifier_keys: [], pen_mode: Some(Pen), length: 2 [1]
 2023-05-10T15:16:57.628Z DEBUG rnote::canvas::input > (518.1, 171.3) handle event, state: Proximity, event_time_d: 15ms, modifier_keys: [], pen_mode: Some(Pen), length: 2 [0]
 2023-05-10T15:16:57.628Z DEBUG rnote::canvas::input > (518.3, 170.4) handle event, state: Proximity, event_time_d: 0ns, modifier_keys: [], pen_mode: Some(Pen), length: 2 [1]
 2023-05-10T15:16:57.644Z DEBUG rnote::canvas::input > (518.8, 168.8) handle event, state: Proximity, event_time_d: 0ns, modifier_keys: [], pen_mode: Some(Pen), length: 1 [0]
 2023-05-10T15:16:57.644Z DEBUG rnote::canvas::input > (518.8, 168.8) handle event, state: Down, event_time_d: 0ns, modifier_keys: [], pen_mode: Some(Pen), length: 1 [0]
(rnote:12): Gdk-DEBUG: 16:16:57.644: proxy 0x5555572bc320 already has listener

(rnote:12): Gdk-DEBUG: 16:16:57.644: proxy 0x5555572bc320 already has listener

 2023-05-10T15:16:57.670Z DEBUG rnote::canvas::input > (367.8, 100.1) handle event, state: Down, event_time_d: 0ns, modifier_keys: [], pen_mode: None, length: 1 [0]
 2023-05-10T15:16:57.674Z DEBUG rnote::canvas::input > (518.8, 169.0) handle event, state: Down, event_time_d: 31ms, modifier_keys: [], pen_mode: Some(Pen), length: 2 [0]
 2023-05-10T15:16:57.674Z DEBUG rnote::canvas::input > (518.9, 169.4) handle event, state: Down, event_time_d: 0ns, modifier_keys: [], pen_mode: Some(Pen), length: 2 [1]
 2023-05-10T15:16:57.689Z DEBUG rnote::canvas::input > (519.0, 170.3) handle event, state: Down, event_time_d: 14ms, modifier_keys: [], pen_mode: Some(Pen), length: 2 [0]
 ...

(note the line with pen_mode: None and a very different pointer position).

Screenshots
image

Desktop (please complete the following information):

Additional context
Why is this a problem?

Note to maintainer From my understanding this is not an issue with Rnote, but rather with Kwin / GTK4 / something else lower-level, so you might not be interested adding a work-around.

If you can confirm this is not an issue with Rnote, I would be interested in implementing support for inputs from multiple input devices at the same time which should solve this issue without being too hacky of a work-around.

flxzt commented 1 year ago

Thanks for the detailed report!

This is definitely something that we can and should address, because most likely issue #534 is a manifestation of the same thing: multiple pointers "drawing" at the same time. It can also be a bit unexpected when the touch-drawing option is enabled, and users, out of habit, attempt to pinch-to-zoom but draw zip-zap lines instead (see issue #624)

The fact that it is actually some touch-pads that emit "invalid" pointer-move events make a lot of sense, and explains why I couldn't replicate that issue. I assume it could also be if there are any oily or other capacitive substances on the touchpad.

How would you approach it? The input used to be handled differently in the past, where each input type (stylus, mouse, touch) had its own gesture that denied the others when it handled events. But that had drawbacks, so Rnote switched to the "EventControllerLegacy" and we are now handling all events ourselves. We'd probably need to write a manual implementation of claiming/denying event sequences from the different sources ourselves, or what are your thoughts here?

cyclane commented 1 year ago

Thanks for pointing me to the other issue, didn't see it before. Definitely seems like the same thing.

The fact that it is actually some touch-pads that emit "invalid" pointer-move events make a lot of sense, and explains why I couldn't replicate that issue. I assume it could also be if there are any oily or other capacitive substances on the touchpad.

Yes that could be the case for some. Unfortunately for me it seems to more be a problem with the combination of my touchpad and Kwin, since when using GNOME (Mutter) I do not have this issue.

How would you approach it?

I haven't really dug into the source code beyond the logging in canvas/src/input.rs yet and actually don't have much experience with Gtk at all, so my current idea might not be very feasible .

My thought was to have complete multi-cursor support (as in you can draw with multiple input devices e.g. pen and mouse or pen and touch) by having separate cursors for each device (with a default cursor as a fallback if needed).

I will look into the code later to understand how Rnote currently works and will respond with a more developed suggestion then.

cyclane commented 1 year ago

I have just looked in a bit more, and I think it should be possible to modify the Brush struct to handle multiple states (one for each pen device or something like that). I am going to have a go at implementing this as a draft (or fully if all goes well) the next few days (PR #666).

cyclane commented 1 year ago

I have done some work on #666 and I believe my initial approach is doable (as partially demonstrated by the current code).

I think your suggestion of writing a manual implementation of blocking events is probably best here to fully fix all related issues, after-all multi-brush support isn't a feature most people would use. What were the drawbacks of the denying by input type approach?

flxzt commented 1 year ago

To fully solve this issue using this approach I think layers below Rnote need to be debugged, after-all it's strange that it can depend on which desktop environment you use and that this only happens in Rnote and not in things like selecting the desktop in KDE or Xournal++.

I actually experienced something like #534 in Xournal++ as well a few times, but I couldn't reproduce it reliably (and do not experience it at all in Rnote, at least on X11). I agree about it originating somewhere lower-level and should be debugged more thoroughly.

I think your suggestion of writing a manual implementation of blocking events is probably best here to fully fix all related issues, after-all multi-brush support isn't a feature most people would use. What were the drawbacks of the denying by input type approach?

The way it was implemented was using different gestures: GestureStylus for stylus input, and two GestureDrag. One for touch input configured with touch-only and one configured with exclusive for mouse input. The last also captured input in the "bubble" propagation phase, so that it didn't interfere with the other.
It worked relatively well, but the major issue was that the gestures only reported events when the pointer was clicked/pressed and moved, not the "up" move events that are needed to indicate interactivity for things like the resize nodes on the selector. Also gesturestylus's events were a bit odd, it also reported button presses as moves, so we had to carefully filter those out.

So I believe it is best to keep the current raw handling of the events, but write blocking events based on some conditions ourselves.

Is checking if gdk::Device is different robust enough? If yes, we could use that and combine this condition with PenProgress. I think that would work well, since this would require finishing all in-progress strokes/shape-building with the same input method (unless we actually want to allow switching the input method while being "in-progress")

cyclane commented 1 year ago

The way it was implemented was using different gestures: GestureStylus for stylus input, and two GestureDrag. One for touch input configured with touch-only and one configured with exclusive for mouse input. The last also captured input in the "bubble" propagation phase, so that it didn't interfere with the other. It worked relatively well, but the major issue was that the gestures only reported events when the pointer was clicked/pressed and moved, not the "up" move events that are needed to indicate interactivity for things like the resize nodes on the selector. Also gesturestylus's events were a bit odd, it also reported button presses as moves, so we had to carefully filter those out.

So I believe it is best to keep the current raw handling of the events, but write blocking events based on some conditions ourselves.

I see, I think I agree especially considering the current implementation seems cleaner to me anyway.

Does checking if gdk::Device is different robust enough? If yes, we could do that and combine this condition with PenProgress. I think that would work well, since this would require finishing all in-progress strokes/shape-building with the same input method (unless we actually want to allow switching the input method while being "in-progress").

Under the hood gdk::Device::eq just compares pointers, and I can't really tell how possible (or not) it is for a new struct to be created for the same device, but I think it's more likely that it is fine. Another way to do this check could be to compare the gdk::DeviceExt::name()s (though both ways make me slightly uncomfortable, but unfortunately there seems to be no other unique identifiers for a gdk::Device).

I have opened #672 with this implementation, and it should also fix #624 .

flxzt commented 1 year ago

I added a comment in the PR.

Just for reference, I saw this page in the wayland docs: https://wayland.freedesktop.org/libinput/doc/latest/touchpad-jumping-cursors.html

digitalsignalperson commented 1 year ago

I think this bug applies to me, but let me know if this deserves it's own issue. See below for a work-around which may hint at the root cause.

Setup:

Jumps in drawing lines happens in the following ways:

A work-around that seems to work is to wait for the pen hover indicator before trying to draw new lines. I think the issue is that if you draw so quickly that the there is little time for the "pen hover" state to be detected before you are touching the screen, then something gets messed up. When I was working on a FreeRDP patch for stylus use, I also found state transitions realted to hovering to be a headache

digitalsignalperson commented 8 months ago

A nice workaround from https://github.com/flxzt/rnote/issues/534#issuecomment-1817984521 is to move the mouse outside the window. It works for me to move it to the titlebar.

A possible automatic workaround, if on kde plasma, could be to call this under some condition:

/usr/bin/dbus-send --session --dest=org.kde.KWin --type=method_call /component/kwin org.kde.kglobalaccel.Component.invokeShortcut string:"MoveMouseToFocus"

it moves the mouse to (0,0). Hmm, could maybe be a kwin script that will do that if rnote is running, and if the mouse hasn't moved for 5 seconds or something.

mistune commented 6 months ago

@cyclane Does the PR here help solve this issue? This is the only thing that is keeping this usable in Android through Termux-X11. Here are additional details from my setup https://github.com/termux/termux-x11/issues/608

cyclane commented 6 months ago

@cyclane Does the PR here help solve this issue? This is the only thing that is keeping this usable in Android through Termux-X11. Here are additional details from my setup termux/termux-x11#608

That PR partially solves the issue, but has not been merged because it's extremely inconsistent across different desktop linux installations and devices. So I'm really not sure how it would hold up on Android either. Unfortunately, last time I looked, I couldn't find a way to solve it without having to dig into gtk itself.

cyclane commented 6 months ago

@digitalsignalperson Yep that's definitely the same issue. Unfortunately it's so inconsistent across linux installations and devices though, that it's difficult to find a proper fix for it.

mistune commented 6 months ago

@cyclane I moved from Debian 12 to Fedora 40 Xfce (+virgl) on my Termux setup and have not gotten the random jumps anymore.