-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix lost KEY_UP events with multiple keyboards using shared scancode state #14446
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
Conversation
src/events/SDL_keyboard.c
Outdated
| } else { | ||
| if (keyboard->keyrefcount[scancode] == 0) { | ||
| keyboard->keystate[scancode] = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else { | |
| if (keyboard->keyrefcount[scancode] == 0) { | |
| keyboard->keystate[scancode] = false; | |
| } | |
| } else if (last_release) { | |
| keyboard->keystate[scancode] = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was resolved without accepting the suggestion. Was it incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionaly they're the same, but yours is tighter.
|
I simplified this a little bit, does that work and make sense? |
|
Yeah, that’s cleaner, makes sense, and it still fixes the repro. This is the test program I’m using to verify two keyboards both get KEY_UP: #include <iostream>
#include <SDL3/SDL.h>
#include <SDL3/SDL.h>
#include <iostream>
int main() {
SDL_SetHint(SDL_HINT_WINDOWS_RAW_KEYBOARD, "1");
SDL_SetHint(SDL_HINT_WINDOWS_GAMEINPUT, "1");
if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_EVENTS) < 0) {
std::cout << "SDL_Init failed: " << SDL_GetError() << "\n";
return 1;
}
SDL_Window* window = SDL_CreateWindow("SDL Input", 800, 600, SDL_WINDOW_RESIZABLE);
if (!window) {
std::cout << "SDL_CreateWindow failed: " << SDL_GetError() << "\n";
return 1;
}
SDL_RaiseWindow(window);
bool running = true;
while (running) {
SDL_Event e;
while (SDL_PollEvent(&e)) {
switch (e.type) {
case SDL_EVENT_KEY_DOWN: {
SDL_KeyboardID id = e.key.which;
std::cout << "KEY DOWN: " << e.key.key << " device id: " << id << "\n";
if (e.key.key == SDLK_ESCAPE) {
running = false;
}
break;
}
case SDL_EVENT_KEY_UP: {
SDL_KeyboardID id = e.key.which;
std::cout << "KEY UP: " << e.key.key << " device id: " << id << "\n";
break;
}
case SDL_EVENT_QUIT: {
running = false;
break;
}
default:
break;
}
}
SDL_Delay(1);
}
SDL_DestroyWindow(window);
SDL_Quit();
return 0;
} |
|
Apologies for the extensive nit-picking. This is core SDL code though, and needs to be as easy to read as possible. |
|
It's no problem, I'm just glad the fix was this simple, it's been causing problems in a game of mine. |
|
would it be an issue with the following code: since you increment the ref count on every key down event but decrement only once when a key is released, which results in imbalance between increments and decrements. You set keystate to false only when ref is 0. |
That's a good point, any repeated key will be reported as held down indefinitely. We also can't just match against the keyboard ID because it may change as we enter and leave raw keyboard mode. I'll go ahead and revert this until we can find a better solution. |
|
Couldn't the states be reset when we enter / leave raw keyboard mode? |
Maybe, it's not something that toggles frequently like relative mouse mode. |
Description
SDL was keeping a single boolean keystate per scancode. When two physical keyboards pressed the same key and then released it at roughly the same time, the first KEY_UP cleared the global state and the second KEY_UP was dropped.
This change adds a per-scancode reference count (
keyrefcount[]) to track how many devices currently hold that key, updatesSDL_SendKeyboardKeyInternalto decrement the count on key up, and only clearskeystateand modifier state when the last device releases the key.This matches the behavior expected in #12920 and works regardless of whether the events came from Windows raw input or other backends, because the drop was happening in the shared keyboard layer.
Existing Issue(s)
Fixes #12920