SDL input is buggy in version 2.16 and later

[necessarily-equal]

How many people are affected, and in what cases

At least 3 people complained about it. They seem to be running Arch most of the time.

Description of the bug

The symptoms reported by @Viech included erratic movement, the weapon switching to blaster at the wrong time, and other "ghosts" keypress: the game was doing like if key that weren't pressed were pressed.

I didn't manage to reproduce the ghosts keypress, but I did notice the following NOTICE with SDL 2.0.20:

Antoine [bsuit] entered the game 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow 
Com_QueueEvent: overflow [further messages like this will be suppressed] 

(exemple of) Steps to reproduce

On NixOS (with the experimental nix command): nix run github:necessarily-equal/nixpkgs/recent-sdl#unvanquished.

On other linux distros, install SDL 2.0.20 and recompile daemon against it.

I don't know if this affects other versions of SDL. 2.0.14 is known good, the versions between are still to be determined. @Viech would apparently also be using the .20.

I don't know if this affects non-linux platforms, or if those just don't have a recent-enough SDL.

[EDIT:] this affects last-release daemon, and I would assume current master too.

On Linux, X11 vs Wayland doesn't seem to make any difference.

[necessarily-equal]

for the Com_QueueEvent: overflow bug, I think this is a SDL problem. When in-game, when SDL_GetRelativeMouseMode()==SDL_TRUE, SDL's SDL_PollEvent(&e) gives, SDL_MOUSEMOTION events with both e.motion.xrel==0 and e.motion.yrel==0. SDL seems to have a cleanup problem or something, and sends LOTS of events each frame. Possibly even more than 1024 which would explain the warning.

[necessarily-equal]

I've tested it to be reproducible on master, before and after #478.

I've tried making a minimal reproducer, but it didn't work. Here it is: x.c.txt. (I've added a .txt at the end because github is annoying). It can be compiled and run with gcc x.c $(pkg-config --cflags --libs sdl2) && ./a.out.

I don't intend on looking more at it this issue.

[illwieckz]

What if this is an SDL bug? There had been SDL bugs previously with older versions of SDL with effects similar to “ghost inputs”. For example the released SDL build of Xonotic is affected (but it's a multiple-years old build, so old SDL version) and if I'm right some of us suffered that older SDL bug as well with Unvanquished (but it was years ago).

[necessarily-equal]

This seems to be the first bad commit in SDL. I think there were some later commits that improved the situation, though.

commit 82793ac279d19b5bde8fc2bd62877b05ba5a76e0
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Thu Oct 14 14:26:21 2021 -0700

    Fixed mouse warping while in relative mode
    
    We should get a mouse event with an absolute position and no relative motion and shouldn't change the OS cursor position at all

diff --git a/src/events/SDL_mouse.c b/src/events/SDL_mouse.c
index 6bb8307b9..a441965e1 100644
--- a/src/events/SDL_mouse.c
+++ b/src/events/SDL_mouse.c
@@ -384,6 +384,8 @@ SDL_PrivateSendMouseMotion(SDL_Window * window, SDL_MouseID mouseID, int relativ
     if (!mouse->has_position) {
         xrel = 0;
         yrel = 0;
+        mouse->x = x;
+        mouse->y = y;
         mouse->has_position = SDL_TRUE;
     } else if (!xrel && !yrel) {  /* Drop events that don't change state */
 #ifdef DEBUG_MOUSE
@@ -765,10 +767,14 @@ SDL_WarpMouseInWindow(SDL_Window * window, int x, int y)
         return;
     }
 
-    if (mouse->WarpMouse) {
+    /* Ignore the previous position when we warp */
+    mouse->has_position = SDL_FALSE;
+
+    if (mouse->WarpMouse &&
+        (!mouse->relative_mode || mouse->relative_mode_warp)) {
         mouse->WarpMouse(window, x, y);
     } else {
-        SDL_SendMouseMotion(window, mouse->mouseID, 0, x, y);
+        SDL_PrivateSendMouseMotion(window, mouse->mouseID, 0, x, y);
     }
 }

[necessarily-equal]

The commit that improves it, but with which we have this issue:

commit 31f8c3ef4409a93fafa894b78c2ce176bd0c3cf3
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Thu Jan 6 11:27:44 2022 -0800

    Fixed event pump starvation if the application frequently pushes its own events

diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c
index d1d931eb3..60737f834 100644
--- a/src/events/SDL_events.c
+++ b/src/events/SDL_events.c
@@ -59,6 +59,7 @@ static SDL_EventWatcher *SDL_event_watchers = NULL;
 static int SDL_event_watchers_count = 0;
 static SDL_bool SDL_event_watchers_dispatching = SDL_FALSE;
 static SDL_bool SDL_event_watchers_removed = SDL_FALSE;
+static SDL_atomic_t SDL_sentinel_pending;
 
 typedef struct {
     Uint32 bits[8];
@@ -479,6 +480,7 @@ SDL_StopEventLoop(void)
     SDL_EventQ.free = NULL;
     SDL_EventQ.wmmsg_used = NULL;
     SDL_EventQ.wmmsg_free = NULL;
+    SDL_AtomicSet(&SDL_sentinel_pending, 0);
 
     /* Clear disabled event state */
     for (i = 0; i < SDL_arraysize(SDL_disabled_events); ++i) {
@@ -574,7 +576,9 @@ SDL_AddEvent(SDL_Event * event)
     }
 
     entry->event = *event;
-    if (event->type == SDL_SYSWMEVENT) {
+    if (event->type == SDL_POLLSENTINEL) {
+        SDL_AtomicAdd(&SDL_sentinel_pending, 1);
+    } else if (event->type == SDL_SYSWMEVENT) {
         entry->msg = *event->syswm.msg;
         entry->event.syswm.msg = &entry->msg;
     }
@@ -620,6 +624,10 @@ SDL_CutEvent(SDL_EventEntry *entry)
         SDL_EventQ.tail = entry->prev;
     }
 
+    if (entry->event.type == SDL_POLLSENTINEL) {
+        SDL_AtomicAdd(&SDL_sentinel_pending, -1);
+    }
+
     entry->next = SDL_EventQ.free;
     SDL_EventQ.free = entry;
     SDL_assert(SDL_AtomicGet(&SDL_EventQ.count) > 0);
@@ -648,9 +656,9 @@ SDL_SendWakeupEvent()
 }
 
 /* Lock the event queue, take a peep at it, and unlock it */
-int
-SDL_PeepEvents(SDL_Event * events, int numevents, SDL_eventaction action,
-               Uint32 minType, Uint32 maxType)
+static int
+SDL_PeepEventsInternal(SDL_Event * events, int numevents, SDL_eventaction action,
+               Uint32 minType, Uint32 maxType, SDL_bool include_sentinel)
 {
     int i, used;
 
@@ -713,7 +721,9 @@ SDL_PeepEvents(SDL_Event * events, int numevents, SDL_eventaction action,
                             SDL_CutEvent(entry);
                         }
                     }
-                    ++used;
+                    if (type != SDL_POLLSENTINEL || include_sentinel) {
+                        ++used;
+                    }
                 }
             }
         }
@@ -730,6 +740,12 @@ SDL_PeepEvents(SDL_Event * events, int numevents, SDL_eventaction action,
 
     return (used);
 }
+int
+SDL_PeepEvents(SDL_Event * events, int numevents, SDL_eventaction action,
+               Uint32 minType, Uint32 maxType)
+{
+    return SDL_PeepEventsInternal(events, numevents, action, minType, maxType, SDL_FALSE);
+}
 
 SDL_bool
 SDL_HasEvent(Uint32 type)
@@ -815,6 +831,14 @@ SDL_PumpEvents(void)
 #endif
 
     SDL_SendPendingSignalEvents();  /* in case we had a signal handler fire, etc. */
+
+    if (SDL_GetEventState(SDL_POLLSENTINEL) == SDL_ENABLE) {
+        SDL_Event sentinel;
+
+        SDL_zero(sentinel);
+        sentinel.type = SDL_POLLSENTINEL;
+        SDL_PushEvent(&sentinel);
+    }
 }
 
 /* Public functions */
@@ -952,17 +976,27 @@ SDL_WaitEventTimeout(SDL_Event * event, int timeout)
     SDL_Window *wakeup_window;
     Uint32 start = 0;
     Uint32 expiration = 0;
+    SDL_bool include_sentinel = (timeout == 0) ? SDL_TRUE : SDL_FALSE;
+
+    /* If there isn't a poll sentinel event pending, pump events and add one */
+    if (SDL_AtomicGet(&SDL_sentinel_pending) == 0) {
+        SDL_PumpEvents();
+    }
 
     /* First check for existing events */
-    switch (SDL_PeepEvents(event, 1, SDL_GETEVENT, SDL_FIRSTEVENT, SDL_LASTEVENT)) {
+    switch (SDL_PeepEventsInternal(event, 1, SDL_GETEVENT, SDL_FIRSTEVENT, SDL_LASTEVENT, include_sentinel)) {
     case -1:
         return 0;
     case 0:
+        if (timeout == 0) {
+            /* No events available, and no timeout */
+            return 0;
+        }
         break;
     default:
-        /* Check whether we have reached the end of the poll cycle, and no more events are left */
-        if (timeout == 0 && event && event->type == SDL_POLLSENTINEL) {
-            return (SDL_PeepEvents(event, 1, SDL_GETEVENT, SDL_FIRSTEVENT, SDL_LASTEVENT) == 1);
+        if (event && event->type == SDL_POLLSENTINEL) {
+            /* Reached the end of a poll cycle, and no timeout */
+            return 0;
         }
         /* Has existing events */
         return 1;
@@ -973,7 +1007,7 @@ SDL_WaitEventTimeout(SDL_Event * event, int timeout)
         expiration = start + timeout;
     }
 
-    if (timeout != 0 && _this && _this->WaitEventTimeout && _this->SendWakeupEvent && !SDL_events_need_polling()) {
+    if (_this && _this->WaitEventTimeout && _this->SendWakeupEvent && !SDL_events_need_polling()) {
         /* Look if a shown window is available to send the wakeup event. */
         wakeup_window = SDL_find_active_window(_this);
         if (wakeup_window) {
@@ -993,10 +1027,6 @@ SDL_WaitEventTimeout(SDL_Event * event, int timeout)
         case -1:
             return 0;
         case 0:
-            if (timeout == 0) {
-                /* Polling and no events, just return */
-                return 0;
-            }
             if (timeout > 0 && SDL_TICKS_PASSED(SDL_GetTicks(), expiration)) {
                 /* Timeout expired and no events */
                 return 0;
@@ -1004,13 +1034,6 @@ SDL_WaitEventTimeout(SDL_Event * event, int timeout)
             SDL_Delay(1);
             break;
         default:
-            if (timeout == 0 && SDL_GetEventState(SDL_POLLSENTINEL) == SDL_ENABLE) {
-                /* We are at the start of a poll cycle with at least one new event.
-                   Add a sentinel event to mark the end of the cycle. */
-                SDL_Event sentinel;
-                sentinel.type = SDL_POLLSENTINEL;
-                SDL_PushEvent(&sentinel);
-            }
             /* Has events */
             return 1;
         }

[necessarily-equal]

The issue is still present on SDL main branch.

[necessarily-equal]

@Viech do you have time to check this SDL patch? This seems to fix my problem. Hopefully it fixes yours. I would be surprised though.

diff --git a/src/events/SDL_mouse.c b/src/events/SDL_mouse.c
index a30c3dbc0..3a9b7a83b 100644
--- a/src/events/SDL_mouse.c
+++ b/src/events/SDL_mouse.c
@@ -383,6 +383,7 @@ SDL_PrivateSendMouseMotion(SDL_Window * window, SDL_MouseID mouseID, int relativ
         mouse->x = x;
         mouse->y = y;
         mouse->has_position = SDL_TRUE;
+        return 0;
     } else if (!xrel && !yrel) {  /* Drop events that don't change state */
 #ifdef DEBUG_MOUSE
         printf("Mouse event didn't change state - dropped!\n");

[Viech]

@Viech do you have time to check this SDL patch? This seems to fix my problem. Hopefully it fixes yours. I would be surprised though.

Given the chance that something as large as SDL compiles without expert knowledge, not anytime soon. 😕

I can confirm that I have sdl2 2.0.20-2 installed.

I don't recall anything about the blaster in particular. The typical problem for me is that certain keys do not register as released (so that I have e.g. an auto-walk effect in some direction) while other keys seem to not register as pressed (not certain about this; they could also just be ignored for conflicting with a key that falsly remains pressed).

[necessarily-equal]

I don't really think we have seen the same bug. I would recommend you having a look at the https://wiki.archlinux.org/title/Arch_Build_System page. IIRC, an important part of the building process is checking out the source using svn, switching to the right directory, then running `makepkg -si` that should build and install the package. To add the patch, I think you can put the above text in a .patch file, add it in srcs and add a 'SKIP' line in the checksums at the bottom. I've never built an Arch package though, so don't trust me too much on that :P

…

On Wed, Mar 9 2022 at 12:22:48 -0800, Maximilian Stahlberg ***@***.***> wrote: > @Viech do you have time to check this SDL patch? This seems to fix > my problem. Hopefully it fixes yours. I would be surprised though. > Given the chance that something as large as SDL compiles without expert knowledge, not anytime soon. 😕 I can confirm that I have sdl2 2.0.20-2 installed. I don't recall anything about the blaster in particular. The typical problem for me is that certain keys do not register as released (so that I have e.g. an auto-walk effect in some direction) while other keys seem to not register as pressed (not certain about this; they could also just be ignored for conflicting with a key that falsly remains pressed). — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you authored the thread.Message ID: ***@***.***>

[necessarily-equal]

Fixed upstream by libsdl-org/SDL@3318590

[necessarily-equal]

@Viech I think your bug is a different one unfortunately. I think you'll want to make a bug report in order to get this fixed.

[necessarily-equal]

The "Fixed event pump starvation" that improves it is part of SDL2.20, and the "Fixed mouse warping" commit is part of SDL2.16. Meaning that every version of SDL2.x where 16≤x≤20 is known bad. @illwieckz do you think we should do something about this by making a release note, by checking the version in cmake and printing a warning?

Viech's bug may be still be around on SDL main, if it isn't fixed yet, it would be nice to fix it before the next SDL release

[illwieckz]

I consider this is not solved, see:

• Only center the mouse if we get a mouse event with relative motion #624

[necessarily-equal]

Apparently people on Arch Linux are reporting that daemon master isn't fixed for them, both with SDL2.0.20 and SDL2.0.22. While SDL2.0.22 being broken is not too surprising (cough my fault cough), SDL2.0.20 is supposed to be working. On Arch, the issue seems to also be causing lost input keyboard keys, as this is what both @Viech and cu-kai reported.

On my computer (NixOS), I can reproduce the issue with SDL2.0.22, but 2.0.20 is fixed by @slouken's PR for me.

Setting in_nograb to 1 is known to "fix" this, in that the bug disappear but the mouse will switch to other monitors (and other windows if not in fullscreen). This is a variation of the bug @slouken fixed in #624, as it is specific to this call for IN_CenterMouse().

I'd say we have to find a workaround before the next release, at least for SDL2.0.20.

[slipher]

We will be using a statically linked SDL 2.0.12 for the release build, so this is at least not release-blocking. I decided not to upgrade SDL because the latest version also introduced new bugs on Mac.

[slouken]

I decided not to upgrade SDL because the latest version also introduced new bugs on Mac.

What bugs were introduced on macOS?

[slipher]

What bugs were introduced on macOS?

#628. I'm not saying it's necessarily an SDL bug, but our program doesn't do what we want it to.

[IngarKCT]

#634 fixes the Arch Linux issue for me. The original issue looked like the mouse was pushed to my secondary monitor and the primary monitor running unvanquished going blank.
After the patch I can actually get into the main menu. I can still move my mouse to the secondary monitor though. Playing works fine.

I'm seeing similar issue with some Steam Games (e.g. Stellaris): mouse isn't grabbed properly and escaping to the secondary monitor.