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

[3.x, macOS, Windows, X11] Add stylus inverted/eraser support to InputEventMouseMotion event #62723

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

hansemro
Copy link
Contributor

@hansemro hansemro commented Jul 4, 2022

@mbrlabs
Copy link
Contributor

mbrlabs commented Jul 6, 2022

I tested it again on Windows 11 and Manjaro (Gnome Desktop) with a Wacom Intuos Pro M. Works great! @akien-mga any chance of this getting into 3.5?

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Since it's only adding a new pen_inverted property to the motion events, it's probably safe for 3.5.

@akien-mga
Copy link
Member

Since it's only adding a new pen_inverted property to the motion events, it's probably safe for 3.5.

There are a couple changes to Xi code for X11 that make me a bit wary of merging so close to stable. Otherwise yeah it looks fairly low risk.

@hansemro
Copy link
Contributor Author

hansemro commented Jul 14, 2022

There are a couple changes to Xi code for X11 that make me a bit wary of merging so close to stable. Otherwise yeah it looks fairly low risk.

@bruvzg @akien-mga Thanks for the review. I will try to describe the XI changes to help clarify your concerns.

1. Changing XIMasterPointer to XISlavePointer in OS_X11::refresh_device_info():

This change allows us to query device id and name of every pointer device under the master pointer. For reference, let's look at this output of xinput command:

xinput
⎡ Virtual core pointer                          id=2    [master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer                id=4    [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro M Pen stylus             id=10   [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro M Pad pad                id=11   [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro M Finger touch           id=12   [slave  pointer  (2)]
⎜   ↳ Synaptics TM3157-007                      id=13   [slave  pointer  (2)]
⎜   ↳ TPPS/2 IBM TrackPoint                     id=14   [slave  pointer  (2)]
⎜   ↳ Wacom Intuos Pro M Pen eraser             id=17   [slave  pointer  (2)]
...

Querying XISlavePointer gives access to the raw device names and device ids under the master pointer, whereas querying XIMasterPointer device information means that we would only obtain the name Virtual core pointer with device id 2. This is especially important for this PR since we are trying to determine the device's pen inverted property by checking its name for the word eraser, and to map the device to its own pen inverted property. Since we are individually checking devices that make up the master pointer, I don't think this will break anything.

2. Changing deviceid to sourceid in OS_X11::process_xevents:

This change fetches slave pointer's device id instead of master pointer's device id. This makes it possible to correctly retrieve the pen_inverted property from the pen_inverted_devices map.

Below, I compare debug outputs with a simple print patch to show why using sourceid is important.

Debug print patch:
diff --git a/platform/x11/os_x11.cpp b/platform/x11/os_x11.cpp
index 9a27989b77..ff9db907f1 100644
--- a/platform/x11/os_x11.cpp
+++ b/platform/x11/os_x11.cpp
@@ -673,6 +673,8 @@ bool OS_X11::refresh_device_info() {
                        continue;
                }
 
+               printf("dev %d with name %s\n", dev->deviceid, dev->name);
+
                bool direct_touch = false;
                bool absolute_mode = false;
                int resolution_x = 0;
@@ -2515,6 +2517,8 @@ void OS_X11::process_xevents() {
                                                XIRawEvent *raw_event = (XIRawEvent *)event_data;
                                                int device_id = raw_event->deviceid;
 
+                                               printf("raw_event: deviceid: %d, sourceid: %d\n", raw_event->deviceid, raw_event->sourceid);
+
                                                // Determine the axis used (called valuators in XInput for some forsaken reason)
                                                //  Mask is a bitmask indicating which axes are involved.
                                                //  We are interested in the values of axes 0 and 1.
-- 
2.37.0
Debug output sample with XIMasterPointer:
dev 2 with name Virtual core pointer <-- Trying to use Wacom Intuos Pro M Pen stylus
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
...
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
dev 2 with name Virtual core pointer <-- Trying to use Wacom Intuos Pro M Pen eraser
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
...
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
Debug output sample with XISlavePointer:
dev 4 with name Virtual core XTEST pointer  <-- Trying to use Wacom Intuos Pro M Pen stylus
dev 10 with name Wacom Intuos Pro M Pen stylus
dev 11 with name Wacom Intuos Pro M Pad pad
dev 12 with name Wacom Intuos Pro M Finger touch
dev 13 with name Synaptics TM3157-007
dev 14 with name TPPS/2 IBM TrackPoint
dev 17 with name Wacom Intuos Pro M Pen eraser
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
...
raw_event: deviceid: 2, sourceid: 10
raw_event: deviceid: 2, sourceid: 10
dev 4 with name Virtual core XTEST pointer  <-- Trying to use Wacom Intuos Pro M Pen eraser triggers device change event
dev 10 with name Wacom Intuos Pro M Pen stylus
dev 11 with name Wacom Intuos Pro M Pad pad
dev 12 with name Wacom Intuos Pro M Finger touch
dev 13 with name Synaptics TM3157-007
dev 14 with name TPPS/2 IBM TrackPoint
dev 17 with name Wacom Intuos Pro M Pen eraser
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17
raw_event: deviceid: 2, sourceid: 17

Raw events all have deviceid 2, which does not contextually give us the right information to determine which device is actually being used. By using sourceid to get device id, we can correctly get the right property information from pen_inverted_devices/pen_pressure_range/pen_tilt_*_range maps. (What would and should happen to the property maps if we plug in two tablets with differing pressure ranges in the current code base? I can't test since I don't own another tablet.)

Anyhow, if you believe this should be its own PR/commit, then let me know.

@akien-mga
Copy link
Member

Thanks for the detailed explanation!

Let's give it a spin and see how it fares in 3.5 RC7.

@akien-mga akien-mga merged commit 101cbe5 into godotengine:3.x Jul 14, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.x, 3.5 Jul 14, 2022
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.

5 participants