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

SDL3: Handle keyboard input asynchronously when text input is disabled #6506

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Jan 23, 2025

This PR updates keyboard events to be handled asynchronously in the SDL event filter iff text input is disabled.

For now, keyboard input is handled synchronously when text input is enabled to prevent timing problems. If keyboard events are handled asynchronously, a keyboard event might come before a text input event. This is opposite to the current order and may break TextBox consuming key events that produce text. TextBox expects and is tested to first receive text, and then key input:

[Test]
public void TestTextInputConsumesKeyDown()
{
AddStep("press key to insert text", () =>
{
textInput.Text("W");
InputManager.Key(Key.W);
});
AddAssert("KeyDown event consumed", () => textBox.KeyDownQueue.Dequeue() && textBox.KeyDownQueue.Count == 0);
AddAssert("user text consumed event", () => textBox.UserConsumedTextQueue.Dequeue() == "W" && textBox.UserConsumedTextQueue.Count == 0);
}

Threading concerns

There is a possibility for a key to get stuck in a pressed state if it's quickly tapped around the time text input is disabled.

Assume a key is quickly pressed and released, generating SDL_EVENT_KEY_DOWN and a SDL_EVENT_KEY_UP.

Raw keyboard thread Main thread Update thread
Text input enabled (initial state)
Receive SDL_EVENT_KEY_DOWN
Ignored as text input is enabled
Request text input disable
Run new frame
commandScheduler.Update()
SDL_StopTextInput()
Receive SDL_EVENT_KEY_UP
Handled, added to input queue
Receive the same SDL_EVENT_KEY_DOWN
Handled, added to input queue
Check input queue:
Handle key up event (no state change)
Handle key down event

The key is now stuck. But the user can just press and release it to fix this.

The opposite is also possible: a key is pressed, but the game thinks it's released. Can happen if a held key is quickly released and pressed again.

This is only a concern if keyboard input is handled on a different thread, as things currently stand with SDL on Windows (SDL_HINT_WINDOWS_RAW_KEYBOARD is disabled by default), keyboard events are added to the SDL event queue and handled by our filter on the main thread.

@peppy
Copy link
Member

peppy commented Jan 24, 2025

Just to confirm before I make a start on testing this, the "stuck key" issue is only an issue until #6507 is merged, correct?

@Susko3
Copy link
Member Author

Susko3 commented Jan 24, 2025

Just to confirm before I make a start on testing this, the "stuck key" issue is only an issue until #6507 is merged, correct?

On windows, stuck key is not an issue until #6507 is merged.

On android, stuck key is an issue with this PR as android already uses different threads. The Android-managed UI thread generates input events, while the main thread is started separately, by SDL. No idea about other platforms.

@hwsmm
Copy link
Contributor

hwsmm commented Jan 25, 2025

I am a bit worried that if this is combined with #6507, osu!framework key events are going to get handled in the raw keyboard hook thread, right?

If that's the case, osu! may become the cause of the issue we are trying to solve. I think it's fine to enable raw keyboard on Windows, but I'd rather keep the key event handling as-is for now.

EDIT: I read SDL code, and it seems that rawinput is implemented differently to what I did with my program before. The code should be then probably fine except for the issue in OP, but I still don't want o!f key handling to run on an SDL thread.

@Susko3
Copy link
Member Author

Susko3 commented Jan 25, 2025

I still don't want o!f key handling to run on an SDL thread.

What's your main concern with that? Keep in mind that osu!framework UI events are still handled on the update thread. This PR just changes the way SDL events are collected – directly when SDL generates them. The idea is to avoid the SDL event queue, as we already have a thread-safe InputHandler.PendingInputs queue.

@hwsmm
Copy link
Contributor

hwsmm commented Jan 25, 2025

What I meant is that this solution sounds a bit roundabout than fixing SDL_PumpEvent to me, but I am not against of it as a workaround until an actual solution is found.

Edited because my comment was literally repeating yours. 😅 I really should have written it better.

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.

3 participants