Implement smooth panscrolling

[Greenscreener]

Hello, with @whot's advice I created a basic implementation of smooth scrolling for panscrolling. It is very much a first draft. TODO:

• [x] fix bugs
  • [x] scrolling is "springy" it snaps back into the original place after letting go in absolute mode
  • [x] ~~some modification I made breaks relative mode entirely - the data passed to the valuators seems to be absolute, which makes the cursor snap to the bottom right corner of the screen~~ #226
  • [x] ~~xournalpp~~ gtk apps register only the beginning of a touch for some reason, only makes dots and doesn't make proper strokes, hover events are weird
  • [x] ~~qt apps don't register the scrolling at all~~ #247
• [x] make PanScrollThreshold the option to set the scroll speed and make them cover the same distance at the same value
• [x] ~~man+docs~~ since no options were modified, there's nothing to add
• [x] ~~tests~~
• [x] remove DBG(0...
• [x] history rewrite - split into three commits
  • [x] fixing nbaxes discrepancy
  • [x] moving from valuator array to valuator mask in convertAxes
  • [x] implementing smooth panscrolling

If anyone could give me some more bits of advice on how to tackle these issues, that would be great ; @whot's three paragraphs helped me immensely.

Thank you for your help and all the work you've put into this driver! GS

Fixes #220

[whot]

the data passed to the valuators seems to be absolute, which makes the cursor snap to the bottom right corner of the screen

xf86PostMotionEventP takes a is_absolute boolean, you pass in False and this should work. Setting up the valuator as relative axis (min == max == -1) is a good idea too then. The springiness my be related to this too btw.

implement a configuration option for scroll speed

shouldn't be needed if you keep the existing pan scrolling option. You scale into that range and that controls your scroll speed. Because you can't change the increment after SetScrollValuator I recommend picking some high enough constant or that value (e.g. 0xffff) and then scaling into that range based on the pan scroll option. IOW, pan scroll 1000, movement of 50 -> 50/1000 * 0xffff and that's the value you emit as motion delta.

implement a configuration option to switch between old and smooth scrolling

we don't need that one. Once the new method works, the old events will be emulated by the X server for us - the only need to switch to the old version is a bug (can be fixed) or some setup where the pan scrolling was misused for something else (e.g. pan scrolling mapped to button X which is mapped to an action other than scrolling).

[Greenscreener]

some modification I made breaks relative mode entirely - the data passed to the valuators seems to be absolute, which makes the cursor snap to the bottom right corner of the screen

This actually isn't related to any of my changes. Creating a separate issue: #226

[Greenscreener]

scrolling is "springy" it snaps back into the original place after letting go in absolute mode

I think this is caused by the fact that when the rel/abs mode switches, all the valuators get set to that mode, the scroll valuators should be always relative, but in absolute mode, they get set to absolute.

[Greenscreener]

gtk apps register only the beginning of a touch for some reason, only makes dots and doesn't make proper strokes, hover events are weird

This is apparently caused by calling SetScrollValuator? When the call is removed, the problem disappears. Weird.

[Greenscreener]

Ok! The problem appears when scroll valuators are sent in the same motionEvent as other valuators. If the x,y,pressure event doesn't contain the scroll valuators, it works correctly, if the event contains x,y,pressure,...,scrollx,scrolly,... (even if they're zero) it creates the above mentioned bug. This means that the scroll valuators have to be last, but if I understand correctly, abswheel2 also has to be last, which is clearly a requirement that cannot be satisfied.

[Greenscreener]

I found a possible solution to this, but I have no idea if this won't break something else. I naïvely think it shouldn't.

[Greenscreener]

This also fixes absolute mode springiness.

[Greenscreener]

qt apps don't register the scrolling at all

With QT_LOGGING_RULES='qt.qpa.input.*=true', this is what gets written out when the program (dolphin in this case) starts, seems correct:

qt.qpa.input.devices: input device  Wacom One by Wacom M Pen stylus ID 9
qt.qpa.input.devices:    has 8 buttons
qt.qpa.input.devices:    it's a keyboard
qt.qpa.input.devices:    has valuator "Abs X" recognized? true
qt.qpa.input.devices:    has valuator "Abs Y" recognized? true
qt.qpa.input.devices:    has valuator "Abs Pressure" recognized? true
qt.qpa.input.devices:    has valuator "Abs Tilt X" recognized? true
qt.qpa.input.devices:    has valuator "Abs Tilt Y" recognized? true
qt.qpa.input.devices:    has valuator "Rel Horiz Scroll" recognized? true
qt.qpa.input.devices:    has valuator "Rel Vert Scroll" recognized? true
qt.qpa.input.devices:    has valuator "Abs Wheel" recognized? true
qt.qpa.input.devices:    it's a tablet with pointer type "pen"
qt.qpa.input.devices:    it's a scrolling device

This gets logged while moving the cursor around:

qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183453 pos  793.4,  511.7 root pos  793.4,  564.7 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183461 pos  788.2,  511.8 root pos  788.2,  564.8 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183469 pos  783.1,  511.1 root pos  783.1,  564.1 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183475 pos  779.0,  509.2 root pos  779.0,  562.2 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183482 pos  776.3,  506.1 root pos  776.3,  559.1 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1440 detail 0 time 175183491 pos  775.2,  502.0 root pos  775.2,  555.0 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0

This gets logged while attempting to scroll:

qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 7 time 175240884 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 7 time 175240884 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240890 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240899 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240907 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 5 time 175240907 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 5 time 175240907 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240915 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240920 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 0 time 175240929 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 6 time 175240929 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0
qt.qpa.input.events: XI2 event on tablet 9 with tool Stylus type Pen seq 1444 detail 6 time 175240929 pos  853.3,  459.5 root pos  853.3,  512.5 buttons 0x0 pressure 0.00 tilt 0, 0 rotation   0.00 modifiers 0x0

Position stays the same, the only thing that updates is the detail field which seems to be manifesting the backwards-compatibility scroll button events sent by X.

For comparison this is what moving the cursor does on a touchpad:

qt.qpa.input.events: XI2 mouse motion 953,474, time 175362557, source MouseEventNotSynthesized
qt.qpa.input.events: XI2 mouse motion 951,464, time 175362620, source MouseEventNotSynthesized
qt.qpa.input.events: XI2 mouse motion 949,454, time 175362632, source MouseEventNotSynthesized

and scrolling:

qt.qpa.input.events: scroll wheel @ window pos QPoint(820,535) delta px QPoint(43,58) angle QPoint(43,58)
qt.qpa.input.events: XI2 mouse motion 820,535, time 175396448, source MouseEventNotSynthesized
qt.qpa.input.events: scroll wheel @ window pos QPoint(820,535) delta px QPoint(52,9) angle QPoint(52,9)
qt.qpa.input.events: XI2 mouse motion 820,535, time 175396473, source MouseEventNotSynthesized
qt.qpa.input.events: scroll wheel @ window pos QPoint(820,535) delta px QPoint(71,-48) angle QPoint(71,-48)
qt.qpa.input.events: XI2 mouse motion 820,535, time 175396498, source MouseEventNotSynthesized
qt.qpa.input.events: scroll wheel @ window pos QPoint(820,535) delta px QPoint(53,-57) angle QPoint(53,-57)
qt.qpa.input.events: XI2 mouse motion 820,535, time 175396523, source MouseEventNotSynthesized
qt.qpa.input.events: scroll wheel @ window pos QPoint(820,535) delta px QPoint(12,-43) angle QPoint(12,-43)

[whot]

Sorry, still ETIME to look at this (probably until after the 1.0 release) but one of the things you should really look into is updating the gwacom bits and wacom-record so you can debug this easier. It's a lot simpler to just run that tool and look at the yaml output than having to deal with qt or xinput or whatever.

Second - record a sequence with libinput record and replay it with libinput replay, see here. This way you always get the same event sequence at the kernel level which obv. makes debugging a lot easier as there's no variation you need to worry about. The hid-recorder and hid-replay tools would work too, they're a level lower and exercise the kernel driver too.

Finally, you may wan to look at #241 because that's where you could add tests to verify specific behaviours.

[Greenscreener]

Sorry for a longer delay on this, I actually found out that, weirdly, the qt scrolling issue is actually consistent even with the currently distributed version (for me it is 0.40.0). That is particularly odd, while the original version of the driver does scrolling entirely differently. I'll open a separate issue for this. I'll also look into it more tomorrow.

[Greenscreener]

Started working on pytest in https://github.com/Greenscreener/xf86-input-wacom/tree/panscroll%2Bpytest

Currently all tests fail with TypeError: could not get a reference to type class on line 228 of __init__.py. I have no clue how to remedy that yet.

[Greenscreener]

@whot could this please get merged with the tests being implemented separately after #241 is done? I feel like the longer this is open, the more merge conflicts and bugs get introduced.

[jigpu]

Made some comments on the tests. Hope you are able to find some time to return to this soon, @Greenscreener :)

[Greenscreener]

Yeah I do definitely plan on getting back to this, there are sadly some IRL things that have taken priority rn and I want to give this my full concentration when I get back to it. It might take a few weeks though, can't really say at the moment.

[Greenscreener]

Finally, I am able to spend some time on this. I cherry-picked the test code from panscroll+pytest, since the pytest stuff has now been merged to master.

I was able to fix the issue outlined in the cherry-picked commit. Turns out that sudo strips the LD_LIBRARY_PATH env variable even with -E, so it has to be passed directly to the command. I created a little script to run pytest easier in the root of the repo, just for my own usage and testing right now.

I have however, run into an issue I haven't been able to fix. It seems to me, that the options defined at the beginning of the function in opts don't actually get applied to the virtual device that gets created. As you can see, I inserted an interactive shell in my test to be able to play around with the virtual device. If I run xsetwacom -s get 19 all (19 is the id of the device in my system) the two options - Button2 and PanScrollThreshold seem to be set to their default values. (button +2 and 2600 respecitvely) Also, if I use IPython to run monitor.wacom_device.get_options().get("PanScrollThreshold") it is correctly set to 150, but all the other default options are not present - for instance monitor.wacom_device.get_options().get("Threshold") returns nothing.

This is also confirmed by the fact, that when I run xinput test-xi2 --root 19, the RawMotion events look like normal cursor movement events and not scroll events.

[whot]

created a little script to run pytest easier in the root of the repo, just for my own usage and testing right now.

fwiw, master now has a script too which you can run with sudo ./test/wacom-test-env.sh. And pytest is of course part of meson test but that isn't that convenient for single-test debugging.

don't actually get applied to the virtual device that gets created

If I read your description directly, this is a misunderstanding on how the test suite works. The test suite does create a uinput device which will be visible to the rest of userspace (and picked up by the wacom driver, something we should fix, see #269 ). But the test suite, through the gwacom helper library, creates a separate context of the core driver bits - and the options merely change how that context processes files. It's like running two separate grep against the same input file - they don't influence each other's output.

[Greenscreener]

I see. So if I understand correctly, this applies to the events that reach userspace too and they are going to be different than the ones that are available in the code and these ones are the only that are correct.

I thought the pytest code did some magic to have the device in X as a normal device, but I now realise that that would require X to restart each time the source code of the driver would change, which is what you're trying to avoid.

[whot]

yep, that install/restart cycle was what I was trying to avoid with that whole libgwacom work - makes the driver more testable and easier to reproduce. With #269 you'll no longer see the events in X itself, the test devices should be rejected by default.

[adrianbienias]

@Greenscreener thanks for working on this! I'm facing the same issue with jumpy pan movements and it's a really annoying experience to use Wacom tablet on Linux because of that. Fingers crossed for merging this PR.

[Greenscreener]

I finally removed all the comments with convertAxes and removed the function, since it is unused and its functionality has been replaced with convertAxesM. I also replaced the test for it with a similar test for convertAxesM.

Just a quick question (@whot?), is it possible to include external functions (like the ones I'm using to interact with valuators) in the C test suite? I'm currently getting errors, even though the same functions are used in the same file outside of the test. (Also, would you mind telling me in which file these functions are located (which file to include)? I found this repo so I could see which functions to use, but that's not the original source)

On another note, I'm currently investigating why the pytest doesn't work. It seems it doesn't kick into panscroll mode at all and sends events just as if the setting wasn't enabled. Is the line opts["Button"] = "2 pan" correct for setting this configuration option? The Button config is somewhat special, so I don't have an example to go by.

[whot]

is it possible to include external functions (like the ones I'm using to interact with valuators) in the C test suite? I'm currently getting errors, even though the same functions are used in the same file outside of the test. (Also, would you mind telling me in which file these functions are located (which file to include)? I found this repo so I could see which functions to use, but that's not the original source)

These functions are part of the X server's API, see dix/inpututils.c. But that makes it a bit of a problem, because including those functions is nontrivial and quite messy (look at the git history of the test directory, we used to have this as FAKE_SYMBOL iirc). But at least these are self-contained so the best option is to copy/paste those functions into our if TEST condition so they're only built for the tests.

[Greenscreener]

On another note, I'm currently investigating why the pytest doesn't work. It seems it doesn't kick into panscroll mode at all and sends events just as if the setting wasn't enabled. Is the line opts["Button"] = "2 pan" correct for setting this configuration option? The Button config is somewhat special, so I don't have an example to go by.

Damn, after many more hours of digging through the source code, I think I have the answer. There are two ways of getting configuration options to the driver. Via xorg.conf or using xsetwacom. The major difference is that the options in xorg.conf are a simple key-value store and thus cannot accommodate the complicated values that Button actions use. There's only support for the old button remapping from one number to another using ButtonN and an integer value, as you can see here: https://github.com/linuxwacom/xf86-input-wacom/blob/f2df07587e5ff89a6c5b2246bc468211c31cf43a/src/wcmValidateDevice.c#L910-L915 Now, gwacom and thus the test suite use the same pipeline as xorg.conf and so don't support remapping buttons. I believe it would require either reimplementing a large part of xsetwacom's functionality into gwacom or somehow enabling the test suite to call xsetwacom scoped to the gwacom-powered driver, which is in both cases currently out of scope for this pull request and I think it should be handled separately. For that reason, I'm dropping the pytest bit from this pull request.

Here's the source code for future reference, I don't think it's interesting enough to keep in commit history.

def test_scroll(mainloop, opts):
    """
    Check panscrolling works correctly
    """
    dev = Device.from_name("PTH660", "Pen")
    opts["Button2"] = "pan"
    opts["PanScrollThreshold"] = "150"

    prox_in = [
        Sev("ABS_X", 50),
        Sev("ABS_Y", 50),
        Sev("BTN_TOOL_PEN", 1),
        Sev("SYN_REPORT", 0),
    ]

    prox_out = [
        Sev("BTN_TOOL_PEN", 0),
        Sev("SYN_REPORT", 0),
    ]

    press_button2 = [
        Sev("BTN_STYLUS", 1),
        Sev("SYN_REPORT", 0),
    ]

    touchdown_pen = [
        Sev("BTN_TOUCH", 1),
        Sev("SYN_REPORT", 0),
    ]

    move_pen_x = [Sev("ABS_X", 75), Sev("SYN_REPORT", 0)]

    up_pen = [Sev("BTN_TOUCH", 0), Sev("SYN_REPORT", 0)]

    depress_button2 = [Sev("BTN_STYLUS", 0), Sev("SYN_REPORT", 0)]

    monitor = Monitor.new_from_device(dev, opts)

    monitor.write_events(prox_in)
    monitor.write_events(press_button2)
    monitor.write_events(touchdown_pen)  # Pen touchdown
    monitor.write_events(move_pen_x)  # Move pen 25% towards positive x
    monitor.write_events(up_pen)  # Pen up
    monitor.write_events(depress_button2)  # Depress button2
    monitor.write_events(prox_out)

    mainloop.run()
    logger.debug(f"We have some events: {monitor.events}")

[whot]

just for reference, xsetwacom is a wrapper around X device properties, just with a wacom-specific UI. you can get the same functionality with the generic xinput tool except you just have to know the various details of the properties. see the big parameters[] array in xsetwacom.c.

The button actions are really hard to support via the xorg.conf because they contain references between atoms, and those are created at runtime - by the client. Doing this from within the driver is difficult, error-prone and 10y ago or whenever we decided it's not worth the effort. That is primarily true for complex actions, it's less so for the pan scrolling where it's likely the only action assigned to the button (though technically you could map a button to type "hello" followed by toggling panscroll).

Anyway, we can hack around this in gwacom so we can test it, see #287, with that in place I can run your test case (minor modifications) and get pan scroll events emitted, see the branch here

[whot]

Apologies this took me so long, things are.... busy...

Anyway, I finally found the time to test this and the pan scrolling does work as expected, it's nice and smooth and equivalent to touchscreen/touchpad scrolling as one would expect. Nice work!

The one thing I am really worried about however is that some of the axes change position (see my comment). This will break some applications that have hardcoded the axis numbers for decades and we won't notice for quite a while, and then fixing it will become even harder.

Doubly so because the two valuators that shift are the artpen rotation and the airbrush slider - both devices that a) most people don't have (including me for example) and thus won't see testing and b) users with those devices are most likely to use software that hardcodes the valuators (e.g. proprietary software that cannot be patched easily).

Now, regarding your comment that the valuator 8 only gets initialized sometimes - we can work around this. It's always uninitialized on pens [1] so we can make it dual-use - scroll if IsPen() otherwise dualring if IsPad(). This should be a relatively relatively easy to do (related: #288) and provide the same functionality without breaking anything.

[1] And IIRC the Cintiq 24 was the only one with two rings I think that was discontinued in 2015 or so. @pinglinux or @jigpu may remember

[Greenscreener]

Hello, I am also somewhat busy, but here are the changes you suggested. I hope to get around to checking out the tests too soon.

On an unrelated note, for some reason, when I build the repository with meson, which is what I've been doing until now, X crashes immediately after trying to load the shared library. The last two lines in the log are always:

[  1455.766] (II) LoadModule: "wacom"
[  1455.767] (II) Loading /usr/lib64/xorg/modules/input/wacom_drv.so

After that X just dies with no other explanation.

Weirdly, it workes just fine with the snippet from here.

[Greenscreener]

when I build the repository with meson, which is what I've been doing until now, X crashes immediately after trying to load the shared library

This happens, somewhat understandably, with the tests and gwacom too. It spits out this singular line:

==18854==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

The same message gets written to the output when running X using startx while the tablet is connected. (again only when building using meson)

[Greenscreener]

I know I haven't been awfully fast with implementing these changes, but since it has been almost a month since the last activity here, I thought I might as well jump in with a quick and friendly ping, just so it isn't forgotten entirely.

[Pinglinux]

Hi @Greenscreener Peter is taking time off. He'll be back soon. Thank you for your contribution and patience!

[whot]

Closing this one. The patches were partially factored out (#296, #297) and the rest was squashed together to apply on top of current master in #298. That PR is effectively the same code as here.