API for pressure-sensitive pens + XInput2/Wayland #8058
Conversation
creichen
force-pushed
the
pen-api
branch
9 times, most recently
from
ddfd008 to
422341d
Compare
|
I'd like to ask for help with the remaining build failure from someone with Visual C expertise:
Some of the possible (but sub-optimal) workarounds:
|
|
Why does testautomation_pen.c need access to the internal SDL structures? We try to make sure that tests only cover the API surface. |
That does simplify the build setup, but it also puts a limit on the amount of test coverage that you can hope for. Given how useful these tests have been for me (especially when running with valgrind), I'd prefer to keep them around in order to help maintain the code. |
|
Assuming it's decided that testautomation_pen.c will include SDL internal code, maybe something like the following? (Not even compile-tested.. Yes, it's hackity ugly..) diff --git a/test/testautomation_pen.c b/test/testautomation_pen.c
index 8b3bc78..376125b 100644
--- a/test/testautomation_pen.c
+++ b/test/testautomation_pen.c
@@ -25,3 +25,3 @@
-#define SDL_internal_h_ /* Inhibit dynamic symbol redefinitions that clash with ours */
+#define testautomation_pen_c /* Inhibit dynamic symbol redefinitions that clash with ours */
diff --git a/src/events/SDL_mouse_c.h b/src/events/SDL_mouse_c.h
index cd08f16..c973aca 100644
--- a/src/events/SDL_mouse_c.h
+++ b/src/events/SDL_mouse_c.h
@@ -20,3 +20,5 @@
*/
+#ifndef testautomation_pen_c
#include "SDL_internal.h"
+#endif
diff --git a/src/events/SDL_pen.c b/src/events/SDL_pen.c
index a87005e..4e29149 100644
--- a/src/events/SDL_pen.c
+++ b/src/events/SDL_pen.c
@@ -20,3 +20,5 @@
*/
+#ifndef testautomation_pen_c
#include "SDL_internal.h"
+#endif
diff --git a/src/events/SDL_pen_c.h b/src/events/SDL_pen_c.h
index c4791c7..3c44029 100644
--- a/src/events/SDL_pen_c.h
+++ b/src/events/SDL_pen_c.h
@@ -20,4 +20,5 @@
*/
-
+#ifndef testautomation_pen_c
#include "../SDL_internal.h"
+#endif
|
|
@sezero , thank you-- I'd prefer to avoid this, but if there is no good way to fix it in the MSVC build files, this approach would be the least messy of the ones that I am currently aware of. I can see if I can borrow a Windows machine next weekend; there seems to be a free trial version of MSVC. |
|
The following works: (similar to what you last did, but additional include paths applied only to testautomation_pen.c and backslashes instead of forward ones): diff.txt diff --git a/VisualC/tests/testautomation/testautomation.vcxproj b/VisualC/tests/testautomation/testautomation.vcxproj
index 2691b7d..057ae2e 100644
--- a/VisualC/tests/testautomation/testautomation.vcxproj
+++ b/VisualC/tests/testautomation/testautomation.vcxproj
@@ -83,18 +83,6 @@
<CodeAnalysisRules Condition="'$(Configuration)|$(Platform)'=='Release|x64'" />
<CodeAnalysisRuleAssemblies Condition="'$(Configuration)|$(Platform)'=='Release|x64'" />
</PropertyGroup>
- <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
- <IncludePath>$(ProjectDir)/../../src;$(IncludePath)</IncludePath>
- </PropertyGroup>
- <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
- <IncludePath>$(ProjectDir)/../../src;$(IncludePath)</IncludePath>
- </PropertyGroup>
- <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
- <IncludePath>$(ProjectDir)/../../src;$(IncludePath)</IncludePath>
- </PropertyGroup>
- <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
- <IncludePath>$(ProjectDir)/../../src;$(IncludePath)</IncludePath>
- </PropertyGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
<Midl>
<PreprocessorDefinitions>_DEBUG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
@@ -221,7 +209,12 @@
<ClCompile Include="..\..\..\test\testautomation_main.c" />
<ClCompile Include="..\..\..\test\testautomation_math.c" />
<ClCompile Include="..\..\..\test\testautomation_mouse.c" />
- <ClCompile Include="..\..\..\test\testautomation_pen.c" />
+ <ClCompile Include="..\..\..\test\testautomation_pen.c">
+ <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(ProjectDir)\..\..\..\src;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+ <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(ProjectDir)\..\..\..\src;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+ <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">$(ProjectDir)\..\..\..\src;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+ <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|x64'">$(ProjectDir)\..\..\..\src;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+ </ClCompile>
<ClCompile Include="..\..\..\test\testautomation_pixels.c" />
<ClCompile Include="..\..\..\test\testautomation_platform.c" />
<ClCompile Include="..\..\..\test\testautomation_rect.c" />P.S.: The MSVC project updates needs applying to |
|
@sezero Thank you! Let's see if I patched it correctly. I had a look at |
It doesn't. You only need to add the library-side new files to it, as far as I can see |
All seems well at github actions front.
Just noticed that you already patched VisualC-GDK -- sorry for the noise. |
|
I patched it in reply to your comment, so it wasn't noise at all-- you checked the latest push faster than I could reply, though. :-) |
|
OK then, everything is set. All this needs is maintainers' attention |
There was a problem hiding this comment.
Looks good at first glance, just a few minor things that I noticed in the main and Wayland code, which are pointed out below.
Two other general thoughts on the Wayland implementation:
- Should a hit test be done on a tablet button event, so that applications using client-side decorations can have their windows moved via a tablet tool?
- The tablet device linked-list code can be simplified and use the built-in Wayland intrusive-list functions, but that can come later.
include/SDL3/SDL_events.h
Outdated
| SDL_EVENT_WINDOW_PEN_ENTER, /**< Window has gained focus of the pressure-sensitive pen with ID "data1" */ | ||
| SDL_EVENT_WINDOW_PEN_LEAVE, /**< Window has lost focus of the pressure-sensitive pen with ID "data1" */ | ||
| SDL_EVENT_WINDOW_FIRST = SDL_EVENT_WINDOW_SHOWN, | ||
| SDL_EVENT_WINDOW_LAST = SDL_EVENT_WINDOW_DESTROYED, | ||
| SDL_EVENT_WINDOW_LAST = SDL_EVENT_WINDOW_PEN_LEAVE, |
There was a problem hiding this comment.
Can these events be moved above the destroyed event and leave destroyed as last? This also was requested when the occluded event was added.
There was a problem hiding this comment.
I'd ideally like them right after the MOUSE_ENTER events and will update the patch. The downside is that this will shift the meaning of later event IDs. As long as SDL3 isn't stable, that's not an issue, but if the patch doesn't make it for the first stable SDL3 release, we might need to reserve those two IDs for binary compatibility.
(Personally, I'd be delighted to have it included in the first stable SDL3 release, of course...)
src/events/SDL_mouse.c
Outdated
| SDL_bool | ||
| SDL_MousePositionInWindow(SDL_Window *window, SDL_MouseID mouseID, float x, float y) |
There was a problem hiding this comment.
Return type should be on the same line in SDL3
| SDL_bool | |
| SDL_MousePositionInWindow(SDL_Window *window, SDL_MouseID mouseID, float x, float y) | |
| SDL_bool SDL_MousePositionInWindow(SDL_Window *window, SDL_MouseID mouseID, float x, float y) |
| SDL_PenStatusInfo *status = &input->current_pen.update_status; | ||
| int button; | ||
| int button_mask; | ||
| Uint64 timestamp = Wayland_GetPointerTimestamp(input->sdlWaylandInput, time); |
There was a problem hiding this comment.
This interacts with the high-res timestamp protocol, and I can't find any documentation about how that protocol interacts with the tablet tool protocol. Let's just use
| Uint64 timestamp = Wayland_GetPointerTimestamp(input->sdlWaylandInput, time); | |
| Uint64 timestamp = Wayland_GetEventTimestamp(SDL_MS_TO_NS(time)); |
for now to be safe.
There was a problem hiding this comment.
Agreed; I get the same precision on Sway. I'll add a separate comment about this below.
| struct SDL_WaylandTool *sdltool = data; | ||
| struct SDL_WaylandTabletInput *input = sdltool->tablet; | ||
| Uint16 mask = 0; | ||
| SDL_bool pressed = state == ZWP_TABLET_PAD_V2_BUTTON_STATE_PRESSED ? SDL_TRUE : SDL_FALSE; | ||
|
|
||
| Wayland_UpdateImplicitGrabSerial(input->sdlWaylandInput, serial); |
There was a problem hiding this comment.
This should probably be moved to the frame done handler, since that's where the final state is committed. Just store the serial in the current pen struct.
There was a problem hiding this comment.
I assume that your comment is specifically about the Wayland_UpdateImplicitGrabSerial bit?
My reading of the spec is that there is no guarantee that the serial numbers of different callbacks in the same frame will be identical (e.g., proximity_in and button both have a serial parameter, and proximity_in explicitly states that a button event may follow in the same frame, but there is no discussion of serial).
My understanding is that I should thus track the serial number across other callbacks (like tablet_tool_handle_down()), too. I will include this in my next push.
There was a problem hiding this comment.
Added to tablet_tool_handle_down() and tablet_tool_Handle_proximity_in(). Let me know if you disagree.
There was a problem hiding this comment.
The tool down event serial is good, but proximity in gives us the serial of an entry event, so we don't want it there. Our interest is the serials for events that can result in a 'grab' (button presses, touches, key events, etc…), as some compositors need the serial from a corresponding input event before they will allow, for example, a focus change, a system menu to be spawned, or data to be offered via the clipboard.
|
@Kontrabant : Regarding high-precision timestamps for tablet tools, I couldn't find mention of this in the Wayland input timestamps specs either. Weston (which is supposedly the only implementation that has high-precision input timestamps) doesn't seem to do tablet tools. Regarding your other two points:
|
|
Here is a patch to xcode project, with no promises: patch.txt |
Thank you, but it doesn't seem to match (I re-force-pushed after rebasing in the hopes that it would work relative to the latest |
| struct SDL_WaylandTool *sdltool = data; | ||
| struct SDL_WaylandTabletInput *input = sdltool->tablet; | ||
| input->current_pen.update_status.axes[SDL_PEN_AXIS_XTILT] = (float)(SDL_sin(wl_fixed_to_double(xtilt) * M_PI / 180.0)); | ||
| input->current_pen.update_status.axes[SDL_PEN_AXIS_YTILT] = (float)(SDL_sin(wl_fixed_to_double(ytilt) * M_PI / 180.0)); |
There was a problem hiding this comment.
Can you add a comment here explaining what you're doing? It's not immediately obvious to me why you would be taking the sine of the X and Y tilt angles here.
There was a problem hiding this comment.
This is related to your later comment about whether to report angles or tilt vectors.
I will (for now) keep it as-is and add a comment, as you requested; if we switch to angles, this will go away.
include/SDL3/SDL_pen.h
Outdated
| SDL_PEN_AXIS_XTILT, /**< Pen horizontal tilt fraction. Bidirectional: -1.0..1.0 (left-to-right). This is NOT the angle, but the tilt vector x component. The physical max/min tilt may be smaller than -1.0 / 1.0, cf. \link SDL_PenCapabilityInfo \endlink . */ | ||
| SDL_PEN_AXIS_YTILT, /**< Pen vertical tilt fraction. Bidirectional: -1.0..1.0 (top-to-bottom). This is NOT the angle, but the tilt vector y component. */ | ||
| SDL_PEN_AXIS_DISTANCE, /**< Pen distance to drawing surface. Unidirectional: 0.0..1.0 */ | ||
| SDL_PEN_AXIS_ROTATION, /**< Pen barrel rotation. Bidirectional: -1.0..1.0 (clockwise, 0 is facing up). Multiply with "2.0 * M_PI" to get radians, or "360.0" for degrees. */ |
There was a problem hiding this comment.
This doesn't seem right. Multiplying -1.0 by 360 would give you a -360 degree rotation. Is this supposed to be 0.0..1.0?
There was a problem hiding this comment.
Thanks for catching this! Will update the documentation (for now).
I went with -1.0..1.0 here, but this is not a straightforward call. My rationale is below:
- Should we go from -x/2..x/2 or from 0..x?
- Should x be 1.0, π, or 360?
Negative-Positive or 0-Positive?
- negative-positive: Going from e.g. -1.0 to 1.0 with 0.0 at the centre means that "normal" pen usage will report 0, while barrel-rotating left will return negative values and right will return positive values. This symmetry makes it easy to e.g. set thresholds, but it means that rotating the pen by 180 degrees is ambiguous (is it -1.0 or 1.0?). Thus, the symmetry breaks at this point.
- 0..positive: Going from e.g. 0.0 to 1.0 (without reaching 1.0) avoids the problem with 180 degree rotation, but setting a threshold for e.g. "less than 45 degrees left" now becomes "rotation > 0.875".
Range
- Unit range (0..1) or -1..1: Homogenous wrt the other axis values, easy to convert to radians or degrees, but not a common format.
- radians (-π..π or 0..2π): Probably the most common format, and likely the most directly useful; I expect that most uses of the angle will pass it directly into
sin()andcos()anyway. However, IEEE-754 can't represent π and its multiples/fractions precisely, so anything that wants to look for exact right angles will be annoying. - degrees (0..359 or -180..179): This is what
SDL_RenderTextureRotated()takes as parameters, and it is what Win32 and Wayland report (though not XInput2). May help port code, probably the most intuitive way for writing thresholds in code, but requires conversion to radians for most other use cases.
I'd personally find radians most natural, but nobody else uses them in SDL (or in other existing event APIs), so let's rule them out. The choice then is between being most like what people would expect for a feature that describes rotation (0..359, apparently?), and what people would most expect the range of values in the axis array to look like (0.0..1.0 or -1.0..1.0). Since the latter is also more straightforward to convert to radians, I went for that.
creichen
force-pushed
the
pen-api
branch
7 times, most recently
from
38e723f to
eba23f8
Compare
|
Let's change tilt and rotation reporting to angles, which seems to be the consensus. Summary below: @cgutman pointed out that angles (especially horizontal and vertical tilt angles) are "the" standard format for reporting pen tilt (with iOS being the only outlier); as @whot pointer out, this also de-facto applies to Xorg (though I owe him another check for the angle reporting discrepancy that I observed between Xorg and Wayland). @slouken and @jigpu noted that angles are intuitive; @jigpu further highlighted a case where tilt vectors would be more awkward to use for most developers. My initial case for tilt vectors s is clearly weaker than I thought, and going by other APIs, horizontal/vertical tilt angles seem like the least surprising format to report overall. Thanks, everyone! As far as I know, this is the first SDL device that will report angles. For rendering, SDL uses degrees (e.g., I'd propose going with " |
creichen
force-pushed
the
pen-api
branch
3 times, most recently
from
7afd735 to
7e2ddcc
Compare
|
@creichen, can you update this for merging? I'd like to lock down the SDL3 ABI soon and this seems like a great addition. |
|
Will do; not 100% sure I'll get to it this week, though. |
This patch adds an API for querying pressure- sensitive pens, cf. SDL_pen.h: - Enumerate all pens - Get pen capabilities, names, GUIDs - Distinguishes pens and erasers - Distinguish attached and detached pens - Pressure and tilt support - Rotation, distance, throttle wheel support (throttle wheel untested) - Pen type and meta-information reporting (partially tested) Pen event reporting: - Three new event structures: PenTip, PenMotion, and PenButton - Report location with sub-pixel precision - Include axis and button status, is-eraser flag Internal pen tracker, intended to be independent of platform APIs, cf. SDL_pen_c.h: - Track known pens - Handle pen hotplugging Automatic test: - testautomation_pen.c Other features: - XInput2 implementation, incl. hotplugging - Wayland implementation, incl. hotplugging - Backward compatibility: pen events default to emulating pens with mouse ID SDL_PEN_MOUSEID - Can be toggled via SDL_HINT_PEN_NOT_MOUSE - Test/demo program (testpen) - Wacom pen feature identification by pen ID Acknowledgements: - Ping Cheng (Wacom) provided extensive feedback on Wacom pen features and detection so that hopefully untested Wacom devices have a realistic chance of working out of the box.
|
Is this ready for merging? |
|
@slouken, @creichen: May I suggest the following patch for sdl2-compat compatibility? EDIT: Created #8547 for it diff --git a/include/SDL3/SDL_events.h b/include/SDL3/SDL_events.h
index 186f9f4..a49d30c 100644
--- a/include/SDL3/SDL_events.h
+++ b/include/SDL3/SDL_events.h
@@ -114,8 +114,6 @@ typedef enum
SDL_EVENT_WINDOW_RESTORED, /**< Window has been restored to normal size and position */
SDL_EVENT_WINDOW_MOUSE_ENTER, /**< Window has gained mouse focus */
SDL_EVENT_WINDOW_MOUSE_LEAVE, /**< Window has lost mouse focus */
- SDL_EVENT_WINDOW_PEN_ENTER, /**< Window has gained focus of the pressure-sensitive pen with ID "data1" */
- SDL_EVENT_WINDOW_PEN_LEAVE, /**< Window has lost focus of the pressure-sensitive pen with ID "data1" */
SDL_EVENT_WINDOW_FOCUS_GAINED, /**< Window has gained keyboard focus */
SDL_EVENT_WINDOW_FOCUS_LOST, /**< Window has lost keyboard focus */
SDL_EVENT_WINDOW_CLOSE_REQUESTED, /**< The window manager requests that the window be closed */
@@ -129,8 +127,10 @@ typedef enum
in an event watcher, the window handle is still valid and can still be used to retrieve any userdata
associated with the window. Otherwise, the handle has already been destroyed and all resources
associated with it are invalid */
+ SDL_EVENT_WINDOW_PEN_ENTER, /**< Window has gained focus of the pressure-sensitive pen with ID "data1" */
+ SDL_EVENT_WINDOW_PEN_LEAVE, /**< Window has lost focus of the pressure-sensitive pen with ID "data1" */
SDL_EVENT_WINDOW_FIRST = SDL_EVENT_WINDOW_SHOWN,
- SDL_EVENT_WINDOW_LAST = SDL_EVENT_WINDOW_DESTROYED,
+ SDL_EVENT_WINDOW_LAST = SDL_EVENT_WINDOW_PEN_LEAVE,
/* Keyboard events */
SDL_EVENT_KEY_DOWN = 0x300, /**< Key pressed */ |
API for pressure-sensitive pens + XInput2 & Wayland implementations
This pull request adds support for pressure-sensitive pens to SDL. It is intended to maintain backward compatibility, and adds new events to provide more detailed information for pen movement.
It replaces #5481 (see heading "Changes since #5481" for a more detailed changelog).
TODO [NEW]
PEN_UPandPEN_DOWNSDL_GetPenType()to highlight that it is not reliable for classifying button eventsSDL_HINT_PEN_NOT_MOUSE == 0)Description
This pull request adds support for pressure-sensitive pens to SDL:
PenTip,PenMotion,PenButton)float- typedx,ypositionsSDL_pen.h)testautomation_pen.c)testpen)SDL_HINT_PEN_NOT_MOUSEallows either entirely suppressing mouse events for pen events, or sending pen movement/button updates without moving the mouse position.SDL_HINT_PEN_DELAY_MOUSE_BUTTON(default) reports mouse button presses when the pen touches/leaves the tablet, with the mouse button determined by which pen button is pressed when the pen first touches the surface. If disabled, pen buttons translate to mouse buttons even if the pen is just hovering, and the pen tip acts as left mouse button.Design Rationale
Abstractly speaking, pens are similar to mice, except that they provide additional information (pressure, tilt). In practice, they are particularly useful for writing / drawing.
To mirror SDL conventions elsewhere, the API reports normalised data:
0.0f..1.0f-90.0f..90.0f, reporting horizontal and vertical tilt angles-180.0f..179.0f, with0.0fas centre, representing barrel rotation0.0f..1.0f-1.0f..1.0f(0.0fas centre) or0.0f..1.0f(untested)PenMotionwith pressure greater than0has button 1 pressed, which simulates currentMouseMotionbehaviourwhichasSDL_PEN_MOUSEID(analogously toSDL_TOUCH_MOUSEID)(See #5481 for more discussion of the design rationale)
Backward Compatibility
Pens still report mouse events via
which=SDL_PEN_MOUSEID.They should thus work the same as before to clients who are unaware of pen events.
Users can tweak this behaviour via the hint
SDL_HINT_PEN_NOT_MOUSE.No plans to fix in this PR:
Missing testing
Below are scenarios that are difficult for me to test:
Existing Issue(s)
SDL_HINT_PEN_NOT_MOUSE=1could help fix mouse clicks reported for pen position issue reported thereSDL_WaitEventTimeout()(as in thetestpendemo), the pen API is subject to the same memory leak that affects mouse eventsChangelog
PenTipEventandSDL_EVENT_PEN_DOWN/SDL_EVENT_PEN_UPSDL_HINT_PEN_NOT_MOUSEis greater than00still stands for "pointing upwards".Change relative to #5481
Since this PR is now scheduled for SDL3.2, I have updated it to fit the SDL3 coding style.
The changes compared to the SDL2 version of this PR (#5481) are:
SDL_NumPens()andSDL_PenIDForIndex()have been replaced bySDL_GetPens(), mirroring the joystick API changesSDL_PenStatus()->SDL_GetPenStatus()SDL_PenIDForGUID()->SDL_GetPenFromGUID()SDL_PenGUIDForPenID()->SDL_GetPenGUID()SDL_PenAttached()->SDL_PenConnected()SDL_PenCapabilities()->SDL_GetPenCapabilities()SDL_PenType()->SDL_GetPenType()SDL_PenName()->SDL_GetPenName(); this function now guarantees that the caller can keep the string that it returnsSDL_EVENT_WINDOW_PEN_ENTERSDL_EVENT_WINDOW_PEN_LEAVESDL_EVENT_PEN_MOTIONSDL_EVENT_PEN_BUTTON_DOWNSDL_EVENT_PEN_BUTTON_UPSDL_PENID_INVALIDis nowSDL_PEN_INVALIDSDL_GetPen()->SDL_GetPenPtr()(to avoid confusion withSDL_GetPens())Uint64parameter to signal the event timestamp (or 0 if unknown, in which case the event handler fills in the time at which it was first notified of the event):SDL_SendPenMotion()SDL_SendPenButton()SDL_SendPenWindowEvent()SDL_PenModifyBegin()/SDL_PenModifyEnd()now acquire and release a mutex, respectively, which should allow backend developers to not have to worry about whether someone is accessing pen information in a parallel threadclang-formateverywhere, except where it reduced readability (the compressed Wacom table inSDL_pen.c) or needlessly re-shuffled header files in existing code (e.g.,SDL_x11xinput2.c).Acknowledgements and Thanks