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

Async mouse input on windows #6235

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Apr 8, 2024

Depends on:

This PR simply handles relative mouse and keyboard events in the SDL_EventFilter i.e. HandleEventFromFilter.

This works because SDL queries raw mouse and keyboard inputs on a separate thread. Our SDL_EventFilter will intercept those events before they are pushed to the internal SDL queue, handle them, and then request SDL to drop them.

Testing

Apply the diff and toggle relative mouse mode in TestSceneInputManger.
This isn't a perfect simulation of SDL_PumpEvents() lag, but it shows the async input in action.

diff --git a/osu.Framework/Platform/SDL3Window.cs b/osu.Framework/Platform/SDL3Window.cs
index aac3baf44..737547203 100644
--- a/osu.Framework/Platform/SDL3Window.cs
+++ b/osu.Framework/Platform/SDL3Window.cs
@@ -460,6 +460,7 @@ private void setSDLIcon(Image<Rgba32> image)
         private void pollSDLEvents()
         {
             SDL3.SDL_PumpEvents();
+            Thread.Sleep(100);
 
             int eventsRead;

Susko3 added 2 commits April 8, 2024 17:41
This works because SDL queries for raw mouse and keyboard input on a separate thread
and we sync all inputs to a `ConcurrentQueue`.
@Susko3 Susko3 force-pushed the windows-async-input branch from 29b83bf to 7fac7bf Compare April 8, 2024 15:41
@peppy peppy self-requested a review April 15, 2024 03:41
@peppy
Copy link
Member

peppy commented Apr 15, 2024

This seems to break input on my VM setup. Yes I have a weird setup but it did work, so something has potentially changed or broken. I haven't yet investigated why, but can if required (if the issue isn't obvious).

master:

2024-04-15.15.35.29.mp4

this PR:

2024-04-15.15.34.02.mp4

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

As commented

@Susko3
Copy link
Member Author

Susko3 commented Apr 15, 2024

Hmm, it's weird that the relative mouse movements align with the cursor in one scenario but not the other. But I wouldn't call it broken, the framework cursor is still moving proportionally to your mouse movements.

The weirder thing is that the mouse cursor isn't hidden when in relative mode.

I have no idea what could cause any of this. But if you'll be looking at this, don't forget to enable event logging with

SDL3.SDL_SetHint(SDL3.SDL_HINT_EVENT_LOGGING, "2"u8);

@peppy
Copy link
Member

peppy commented Apr 16, 2024

the framework cursor is still moving proportionally to your mouse movements

I tried to show this, but it's actually not proportionally correct either. I'll see what I can find.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jan 7, 2025
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 7, 2025
@peppy
Copy link
Member

peppy commented Jan 7, 2025

@Susko3 can you please check back on this? a lot of upstream changes meant that I basically had to reapply your changes here.

I'm also curious when we should not be using the filter method for handling things. I guess only when we want thread synchronisation on the input thread?

@Susko3 Susko3 changed the title Async mouse and keyboard input on windows Async mouse and joystick input on windows Jan 7, 2025
@Susko3
Copy link
Member Author

Susko3 commented Jan 7, 2025

I'm also curious when we should not be using the filter method for handling things. I guess only when we want thread synchronisation on the input thread?

I'm not sure. Maybe it's safe to handle all input events in the watch. Especially if we're not relying on SDL state and are just consuming fields from the SDL_Event structs. https://wiki.libsdl.org/SDL3/SDL_SetEventFilter mentions that the filter will "process all events before they change internal state", but this is not 100% true. Some internal state is updated before the event is pushed and the filter is called.

See the following review for some thread unsafety, and methods that query SDL state:

@@ -306,7 +307,39 @@ protected virtual void HandleEventFromFilter(SDL_Event evt)
case SDL_EventType.SDL_EVENT_LOW_MEMORY:
LowOnMemory?.Invoke();
break;

case SDL_EventType.SDL_EVENT_MOUSE_MOTION:
handleMouseMotionEvent(e.motion);
Copy link
Member Author

Choose a reason for hiding this comment

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

This calls SDL_GetWindowRelativeMouseMode() which could be considered as reading SDL state in a event filter, but it's currently just reading a window flag.

Comment on lines 324 to 339
case SDL_EventType.SDL_EVENT_JOYSTICK_AXIS_MOTION:
handleJoyAxisEvent(e.jaxis);
return false;

case SDL_EventType.SDL_EVENT_JOYSTICK_BALL_MOTION:
handleJoyBallEvent(e.jball);
return false;

case SDL_EventType.SDL_EVENT_JOYSTICK_HAT_MOTION:
handleJoyHatEvent(e.jhat);
return false;

case SDL_EventType.SDL_EVENT_JOYSTICK_BUTTON_DOWN:
case SDL_EventType.SDL_EVENT_JOYSTICK_BUTTON_UP:
handleJoyButtonEvent(e.jbutton);
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

These don't directly depend on SDL state, but they do depend on SDL_EVENT_GAMEPAD_ADDED, SDL_EVENT_GAMEPAD_REMOVED, etc. for correct handling. By handling some events in the filter, we may reverse the order these events are understood by o!f. controllers is also just a regular dictionary, so it's not thread safe.

@Susko3
Copy link
Member Author

Susko3 commented Jan 7, 2025

I'm fine with moving input handling to the filter, but we need to ensure thread safety (in our code, and minimising SDL function calls). Some testing is needed.

I think it's best to get the high-priority changes in (mouse & keyboard) and leave the rest for later.

Mouse events seem fine to handle in the filter, mousePressedButtons is be modified in a thread-unsafe way, but that's easy to fix. Calling SDL_GetWindowRelativeMouseMode() seems fine.

Hmm, I'm not too sure about keyboard input. It may mess up our textbox handling because the order of events might be changed.

@peppy
Copy link
Member

peppy commented Jan 10, 2025

@Susko3 sounds good. i think i'd like to make moving keyboard input there a goal (because otherwise it will run into the same issues as mouse). did you want to make any follow up changes?

@Susko3 Susko3 changed the title Async mouse and joystick input on windows Async mouse input on windows Jan 11, 2025
@Susko3
Copy link
Member Author

Susko3 commented Jan 11, 2025

@Susko3 sounds good. i think i'd like to make moving keyboard input there a goal (because otherwise it will run into the same issues as mouse).

Yep, we can keep the original issues open. Having only mouse input async will make the issue less obvious, as the mouse is smooth, but the keyboard is delayed. Keyboard input needs additional consideration because SDL_HINT_WINDOWS_RAW_KEYBOARD is off by default.

did you want to make any follow up changes?

I wanted to make handleMouseButtonEvent() / mousePressedButtons thread-safe, but it will conflict with #6488. Changes in that PR also make it easier to implement thread safety, so it'll be easier if that one goes in first.

I will try to use System.Threading.Interlocked as it's a hot path and it's just setting integer flags.

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.

Input thread blocking causes mouse handling stutters on windows
2 participants