Add a way to distinguish mouse from tablet pen inputs #6488

Susko3 · 2025-01-05T01:31:10Z

Public additions

Tablet pen inputs are now marked with ITabletPenInput. This is useful for ppy/osu#31182 as it can be queried by MouseEvent.CurrentState.Mouse.LastSource as ITabletPenInput. The pen inputs simulate mouse input as they did previously (can be changed in the future).

A new PenHandler for SDL3 was added. No bindable settings are currently exposed.

Internal changes

The SDL3 pen logic was completely rewritten as it didn't make much sense to me. Having a separate PenHandler was needed so a lot of the logic would have changed anyway.

I've left the Windows SDL2 pen handling logic intact to prevent unintended consequences. The SDL3 logic will be ripped out as soon as SDL3-CS is released with libsdl-org/SDL#11826.

Testing

I've tested this on Windows with vTablet. The current SDL3-CS release doesn't have windows ink support, so I've manually built SDL and copied over the new SDL3.dll.

Future changes enabled by this PR

Properly supporting pen inputs as UI events. Right now, any ITabletPenInput information is lost at the PassThroughInputManager border.

OpenTabletDriver presses and releases mouse buttons without worrying about the mouse state, so why should SDL pen handling worry about it. Partially reverts commit dd45d8a.

These can be later changed to have proper UIEvent handling.

frenzibyte · 2025-01-08T00:31:10Z

I find PassThroughInputManager support to be mandatory for this change to be in. There's no rule enforcing us to add a PassThroughInputManager at a high level in the game for one reason or another, and I don't think there's any reason for us to just have this with half implementation.

I understand that following a similar style to how touch input works would produce counter-intuitive behaviour with event handling, and agree with the notion of "pen input are mouse input".

I'm not settled on a particular direction I'm foreseeing towards this, but it can remain within the idea that pen input influences the mouse state with clear indication that the input is sourced from a pen. i.e. something similar to the diff you proposed in ppy/osu#31182 (comment) with full coverage of the class (see the sync releases method at the bottom of the class).

cc @peppy @smoogipoo

osu.Framework/Input/StateChanges/ITabletPenInput.cs

frenzibyte · 2025-01-08T00:34:29Z

osu.Framework/Input/StateChanges/TabletPenInput.cs

+
+namespace osu.Framework.Input.StateChanges
+{
+    public class TabletPenInput : MouseButtonInput, ITabletPenInput


The "Tablet" prefix has been ingrained with graphic tablets in the framework codebase, I don't think it's a good fit for these classes.

I would just drop it, I think the name "Pen" is clear and general enough to indicate any kind of stylus input, direct or non-direct.

I originally had it as just PenInput, but I changed it to TabletPen to keep it consistent with existing TabletPenButtonInput. I can change it all to just Pen{…}Input, but it'll be a breaking one.

Some windows tablets have a screen, the pen input is just as direct as with an iPad, and they are still called graphics tablets.

I prefer PenInput I think.

Susko3 · 2025-01-08T02:15:11Z

I find PassThroughInputManager support to be mandatory for this change to be in.

I want that addition to be separate, because it can be. Smaller PRs are easier to review. This PR changes the native implementation to use pen inputs, and later PRs can make use of those inputs without having to worry about native. Whatever PR adds proper pen support to PassThroughInputManager, it's going to make use of these pen events.

Would you be okay with this PR going in as is if the title was changed to "Add marker IInputs for pen input" or something like that. I can make the newly added classes and interface internal, so they can't be used until PassThroughInputManager support is implemented.

frenzibyte · 2025-01-08T02:23:20Z

This doesn't change the consequence that MouseState.LastSource may not be ITabletPenInput even though it's sourced from a pen. We can't have something like this in the framework without being properly supported in valid and common use cases.

You mention making the interface internal, but if that's done, how should this be used in the game? The expectation is to update GlobalCursorDisplay to also check against the pen input interface when deciding whether the menu cursor is hidden or not.

Susko3 · 2025-01-08T02:25:11Z

You mention making the interface internal, but if that's done, how should this be used in the game?

Change it to public once PassThroughInputManager support is in.

peppy · 2025-01-08T07:32:02Z

I'm okay with the passthrough / sync logic being implemented separately because from my understanding this PR will not change behaviour to existing applications (unless they use the new interfaces in some way). But I also think adding that logic here will not increase the complexity of the PR as it's all new code, and only across what should be two classes?

Out of curiosity, what's a device with an "indirect" pen input? I'm struggling to imagine what this involves.

peppy

As commented.

Susko3 · 2025-01-08T15:19:01Z

Out of curiosity, what's a device with an "indirect" pen input? I'm struggling to imagine what this involves.

Direct and indirect are meant to be used for showing and hiding the cursor. "Indirect" is when the pen is not over the screen, so a game would need to show the cursor so the user knows where they are pointing.

It's a bad name, and is currently not that useful (it's unknown except on iPadOS). I think it's not easy to implement on windows as it sees a graphics tablet with a display as two separate devices.

I would remove it as it's not useful and confusing. osu! can just hardcode not showing the cursor on iPadOS pen input.

`syncReleasedInputs()` is not changed as the only difference is the MouseButtonInput/TabletPenInput for left click. Also, it's impossible to tell what caused the left click release in the parent state (we could check `parentState.Mouse.LastSource`, but it could have been set from a later event).

peppy

Looks pretty good, pending renames

Not really user-friendly, but gets the job done.

Susko3 · 2025-01-09T13:36:13Z

I've tested this on a real tablet (Wacom intuos 3 PTZ-930) with latest SDL3.dll and it works quite well. Events are reported multiple times if both pen and OTD are enabled, but since the mouse priority is assigned to OTD, there are no issues.

This PR is finished, but the following still needs to be done:

update SDL3(-CS)
- remove touch→mouse logic in SDL3WindowsWindow
add PenHandler to osu! input settings, so it can be disabled in case it causes problems with OTD handling
ask users to delete input.json and test with OSU_SDL3=1

peppy · 2025-01-10T04:59:43Z

ask users to delete input.json

what does this do?

Susko3 · 2025-01-11T01:05:52Z

ask users to delete input.json

what does this do?

This will reset InputHandler-related settings to default. People who had issues with SDL3 may have non-default settings. The default configuration should work reasonably well, so I think it's important to test it.

frenzibyte · 2025-01-12T07:45:21Z

osu.Framework/Input/PassThroughInputManager.cs

+                case MouseDownEvent penDown when penInput != null:
+                    Debug.Assert(penDown.Button == MouseButton.Left);
+                    new MouseButtonInputFromPen(true) { DeviceType = penInput.DeviceType }.Apply(CurrentState, this);
+                    break;


I don't believe we use case when anywhere. And in this particular instance, I would agree that it reads really weirdly. Move this to an if conditional in the mouse down event below, and/or create private methods for handling each events to keep this general handler method organized.

frenzibyte · 2025-01-12T07:45:45Z

osu.Framework/Input/StateChanges/ISourcedFromPen.cs

+    /// </summary>
+    public interface ISourcedFromPen : IInput
+    {
+        public TabletPenDeviceType DeviceType { get; }


missing xmldoc

Suggested change

public TabletPenDeviceType DeviceType { get; }

TabletPenDeviceType DeviceType { get; }

frenzibyte · 2025-01-12T07:50:24Z

osu.Framework/Platform/SDL3/SDL3Window_Input.cs

+        public event Action<Vector2>? PenMove;
+
+        /// <summary>
+        /// invoked when a pen touches (<c>true</c>) or lifts (<c>false</c>) from the tablet surface.


Suggested change

/// invoked when a pen touches (<c>true</c>) or lifts (<c>false</c>) from the tablet surface.

/// Invoked when a pen touches (<c>true</c>) or lifts (<c>false</c>) from the tablet surface.

frenzibyte · 2025-01-12T08:03:11Z

I've tested this on a real tablet (Wacom intuos 3 PTZ-930) with latest SDL3.dll and it works quite well. Events are reported multiple times if both pen and OTD are enabled, but since the mouse priority is assigned to OTD, there are no issues.

This PR is finished, but the following still needs to be done:

update SDL3(-CS)

remove touch→mouse logic in SDL3WindowsWindow

add PenHandler to osu! input settings, so it can be disabled in case it causes problems with OTD handling

ask users to delete input.json and test with OSU_SDL3=1

Adding PenHandler as an input setting will surely cause confusion to users, especially when we already offer a section for tablet input. With the way the mouse handling logic in InputManager works, input from PenHandler should 100% be discarded unless somehow OpenTabletDriverHandler is no longer sending input. I suggest we keep it hidden from the user until an actual issue raises up.

I'm also wondering about what kind of configuration do you think will affect this. If it's mandatory, effort to add migration support in InputConfigManager should be attempted at least. But I'm not sure what should be toggled on/off so I need that answered first.

Susko3 · 2025-01-12T17:14:07Z

I suggest we keep [PenHandler settings] hidden from the user until an actual issue raises up.

Sure. We can add it when it can be used to fix an issue. I'm also expecting that users will ask to add sensitivity (i.e. scaling of apsolute input wrt centre of screen/window), so it may also be added because of that.

To explain the pen handler purpose to the user, I would call it "Pen (Windows Ink)" on windows.

I'm also wondering about what kind of configuration do you think will affect this.

The MouseHandler.UseRelativeMode setting. Toggling it was the most common fix for pen input, as reported by users. Ideally, the setting wouldn't affect pen input at all.

If it's mandatory, effort to add migration support in InputConfigManager should be attempted at least.

Even if there is a "correct" value of relative mouse mode for pen input, I don't see a clean way to implement migrations. Migrations are done at startup, but we don't have a magic ball that will tell us if the user is going to use pen input or not.

Instead, I suggest writing an osu!wiki page for pen input in lazer, list common problems and fixes. Like https://osu.ppy.sh/wiki/en/Gameplay/Input_device/Graphics_tablet for stable.

If our scream test goes well, there won't be a need for either :)

Susko3 added 3 commits January 4, 2025 22:56

Remove flawed SDL pen handling

c901353

OpenTabletDriver presses and releases mouse buttons without worrying about the mouse state, so why should SDL pen handling worry about it. Partially reverts commit dd45d8a.

Add tablet pen inputs that simulate mouse inputs

61cb743

These can be later changed to have proper UIEvent handling.

Add proper SDL3 pen handling

973d803

pull-request-size bot added the size/L label Jan 5, 2025

Susko3 added the area:input label Jan 5, 2025

frenzibyte mentioned this pull request Jan 5, 2025

Apple Pencil should not display menu cursor on hover ppy/osu#31182

Open

bdach requested a review from frenzibyte January 6, 2025 07:47

frenzibyte reviewed Jan 8, 2025

View reviewed changes

peppy self-requested a review January 8, 2025 05:06

peppy reviewed Jan 8, 2025

View reviewed changes

Susko3 added 3 commits January 8, 2025 15:49

Add failing test for pen input pass through

a1de65f

Use Debug.Assert instead of NUnit

b68681a

peppy reviewed Jan 9, 2025

View reviewed changes

Susko3 added 2 commits January 9, 2025 13:19

Add test to disable/enabled input handlers

d4a95d8

Not really user-friendly, but gets the job done.

Rename pen inputs to match mouse inputs generated from touch

0fe8fcb

peppy requested a review from frenzibyte January 10, 2025 04:58

frenzibyte reviewed Jan 12, 2025

View reviewed changes

Susko3 added 2 commits January 12, 2025 16:40

Move pen handling to separate conditional

1ee969f

Fix xmldocs

2792b5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to distinguish mouse from tablet pen inputs #6488

Add a way to distinguish mouse from tablet pen inputs #6488

Susko3 commented Jan 5, 2025

frenzibyte commented Jan 8, 2025

frenzibyte Jan 8, 2025

Susko3 Jan 8, 2025

peppy Jan 8, 2025

Susko3 commented Jan 8, 2025

frenzibyte commented Jan 8, 2025

Susko3 commented Jan 8, 2025

peppy commented Jan 8, 2025 •

edited

Loading

peppy left a comment

Susko3 commented Jan 8, 2025

peppy left a comment

Susko3 commented Jan 9, 2025

peppy commented Jan 10, 2025

Susko3 commented Jan 11, 2025

frenzibyte Jan 12, 2025

frenzibyte Jan 12, 2025

frenzibyte Jan 12, 2025

frenzibyte commented Jan 12, 2025

Susko3 commented Jan 12, 2025

	public TabletPenDeviceType DeviceType { get; }
	TabletPenDeviceType DeviceType { get; }

	/// invoked when a pen touches (<c>true</c>) or lifts (<c>false</c>) from the tablet surface.
	/// Invoked when a pen touches (<c>true</c>) or lifts (<c>false</c>) from the tablet surface.

Add a way to distinguish mouse from tablet pen inputs #6488

Are you sure you want to change the base?

Add a way to distinguish mouse from tablet pen inputs #6488

Conversation

Susko3 commented Jan 5, 2025

Public additions

Internal changes

Testing

Future changes enabled by this PR

frenzibyte commented Jan 8, 2025

frenzibyte Jan 8, 2025

Choose a reason for hiding this comment

Susko3 Jan 8, 2025

Choose a reason for hiding this comment

peppy Jan 8, 2025

Choose a reason for hiding this comment

Susko3 commented Jan 8, 2025

frenzibyte commented Jan 8, 2025

Susko3 commented Jan 8, 2025

peppy commented Jan 8, 2025 • edited Loading

peppy left a comment

Choose a reason for hiding this comment

Susko3 commented Jan 8, 2025

peppy left a comment

Choose a reason for hiding this comment

Susko3 commented Jan 9, 2025

peppy commented Jan 10, 2025

Susko3 commented Jan 11, 2025

frenzibyte Jan 12, 2025

Choose a reason for hiding this comment

frenzibyte Jan 12, 2025

Choose a reason for hiding this comment

frenzibyte Jan 12, 2025

Choose a reason for hiding this comment

frenzibyte commented Jan 12, 2025

Susko3 commented Jan 12, 2025

peppy commented Jan 8, 2025 •

edited

Loading