2.0.22 regression: Portal 2 and Team Fortress 2 mouse warping

[smcv]

Versions:

• Arch Linux, up to date as of 2022-04-22, on AMD hardware with Mesa 22.0.1
• KDE Plasma Desktop, in either X11 or Wayland mode
• Do not force any specific video driver (SDL uses X11, possibly via Xwayland)
• Good configuration: Portal 2 launched under Steam public beta, without using "Steam Linux Runtime"; Steam Runtime 'scout' v0.20220315.0; no 32-bit SDL installed system-wide; Portal 2 uses 32-bit SDL 2.0.20 (libSDL2-2.0.so.0.18.2) from the scout runtime
• Bad configuration: replace the scout runtime with one that includes SDL 2.0.22rc2 (note for @slouken and other Valve people: this is the steam-runtime.tar.xz from scout internal build smcv-task106-0.20220422.0, please ask me for a copy if you are having trouble reproducing the issue)

Steps to reproduce:

• Load Portal 2
• Load a saved game (in case it matters, mine is shortly after leaving the cell from Portal 1, before you get the portal gun)
• Move the mouse horizontally at a constant moderate-to-fast speed

Expected result:

• Camera direction changes smoothly

Actual result:

• In the good configurations (with SDL 2.0.20), the camera direction changes smoothly
• When moving the mouse slowly, the camera direction changes smoothly
• When moving the mouse at a moderate-to-fast speed with 2.0.22rc2, the camera periodically snaps back to a previous position

Additional notes: I don't know whether this would be considered to be a bug in SDL or whether Portal 2's engine is known to be doing something wrong here, but either way it's a regression triggered by SDL 2.0.22.

[smcv]

Team Fortress 2 (Source 1 engine) has similar behaviour.

CS:GO (also Source 1 engine?) does not - maybe that version has been fixed?

[smcv]

From bisecting, it looks like 3318590 is the commit that made this regress. Unfortunately, that commit was done to fix Unvanquished.

[smcv]

Looking at the code affected by 3318590, I wonder whether it would be sufficient to solve the Unvanquished issue by skipping the actual SDL_PushEvent, but still do the rest of the function, which would allow SDL_WINDOW_MOUSE_CAPTURE to take effect?

[slouken]

Can you try it and submit a PR if it looks good?

[smcv]

@necessarily-equal, is there a straightforward/reliable way to reproduce the issue fixed by #5387?

[smcv]

I can reproduce something a bit like what's described in #5387 by LD_PRELOADing the Steam Runtime's SDL 2.0.20 into the Flatpak version of Unvanquished. Starting a LAN game and running around the level for a minute or so gives me Com_QueueEvent: overflow console messages, but no other obvious ill effects. Is there a user-visible effect that I should be looking for?

[smcv]

It looks like Unvanquished is calling SDL_WarpMouseInWindow() to the midpoint of its window, once per SDL_MOUSEMOTION event - effectively the same as what SDL does when SDL_HINT_MOUSE_RELATIVE_MODE_WARP is true.

The problem that #5387 solved seems to be that SDL would normally ignore mouse events with xrel == 0 && yrel == 0, but between cf092ec "mouse: Save initial position yet even if xrel and yrel are 0" and 3318590 "Fix relative mouse input for Unvanquished" (#5387), it would always process the first mouse event after a SDL_WarpMouseInWindow() or SDL_SetMouseFocus(), and convert it into a SDL_MOUSEMOTION event to be dispatched to user code. Because Unvanquished warps the mouse in response to each SDL_MOUSEMOTION event, every call to SDL_PrivateSendMouseMotion() would quickly be followed by a new SDL_MOUSEMOTION event (even if the displacement from the previous coordinates is zero), overflowing Unvanquished's event queue.

However, I think #5387 might be solving that by applying a bigger hammer than is really needed: it's completely ignoring any mouse event that is the first one after a warp, even if it has a non-zero displacement from the (last_x, last_y) position. I don't have any visibility into the TF2 and Portal 2 code, but presumably this is making the input processing in TF2/Portal 2 go wrong.

[smcv]

With approximately 2.0.22rc2 (more precisely d9b5805), Unvanquished seems to work well. Portal 2 still has the bug described above with this version.

With d9b5805 modified by a straightforward revert of commit 3318590, I can reproduce the bad behaviour in Unvanquished (worse than in 2.0.20). Rapid mouse movements briefly "look away" from my initial aim point, but then jump back to the initial aim point; and keyboard input doesn't always register, presumably because Unvanquished has a single event queue which has filled up with mouse events. Portal 2 seems to work well with this version, though.

I also tried skipping just the SDL_PushEvent as I suggested above. In Unvanquished, that no longer overflows the event queue, but I still get the same symptom of my view briefly "looking away" from my initial aim point but then snapping back to the initial aim point. Again, Portal 2 seems to work well with this version. I've pushed this to smcv@issue5569-try1 for reference, but I would not recommend applying this change.

To reproduce the issue with Unvanquished, do your SDL builds in a sufficiently old container (I used Steam Runtime 2 'soldier', based on Debian 10), then rsync --copy-links the 64-bit build/.libs/libSDL2-2.0.so.0 onto a test system and do something like this:

$ flatpak install flathub net.unvanquished.Unvanquished
$ flatpak run --filesystem=$HOME/tmp:ro --command=bash net.unvanquished.Unvanquished
[net.unvanquished.Unvanquished ~]$ LD_PRELOAD=$HOME/tmp/your-sdl-build.so unvanquished

and start a LAN game (listen server).

To reproduce the issue with Portal 2, LD_PRELOAD a 32-bit SDL build into it.

[necessarily-equal]

[unvanquished with SDL 2.20] gives me Com_QueueEvent: overflow console messages, but no other obvious ill effects

This is because SDL sends in one more events each time, presumably one more every frame. This means that after some time like 30s, the game has to process 1000 events per frame, and after 5 minutes, 10000 events per frame. This increases te CPU usage then reduces the FPS count significatively. This is mostly due to the fact that Unvanquished is using an engine "syscall" for each of the input, with smart pointers used in the event queue to handle the C++ constructors/destructors, so it should be possible to do `if (xrel != 0 && yrel != 0) syscall(event);` to filter out the noise and hide the performance issue. Although maybe SDL shouldn't send 1000 events every frame instead? From looking a the SDL code it seemed like those empty events were wrongly kept in te queue and this is why I did this PR. What got me to think those events should be dropped with a `return 0;` was the code right below the line added in my PR (that is, the `else if`).

[smcv]

Although maybe SDL shouldn't send 1000 events every frame instead?

If I'm understanding the code correctly (I am not an expert on game event loops), the only reason it's doing that is because Unvanquished responds to each one by warping the pointer with SDL_WarpMouseInWindow(), which results in another event, which results in another pointer warp, and so on.

The code that existed before #5387 doesn't seem ideal, but #5387 seems like it went too far the other way, and is now suppressing events that were necessary for some other games (in particular Portal 2 and TF2). Presumably the right thing to do is somewhere between those extremes, which is what I tried to do in smcv@issue5569-try1, but that commit doesn't seem to be the right thing either.

What got me to think those events should be dropped with a return 0; was the code right below the line added in my PR (that is, the else if)

I can see the logic there, but the else if clause is only dropping "empty" events that have a relative movement of (0,0), whereas the change in #5387 is dropping all events that immediately follow a warp (even if they are non-empty).

[smcv]

I also tried smcv@issue5569-try2, but that has the same results as my first attempt: fixes Portal 2, but regresses Unvanquished.

[slouken]

So the defined behavior of a mouse warp is that there must be a mouse event following the warp that contains the position of the warp. If Unvanquished is warping the mouse in response to that event, then it's broken. I've fixed this bug in several projects over the years, and it's easy to write.

I haven't dug into Unvanquished, but I know that the Valve games rely on this behavior to function properly, which it sounds like is what @smcv is seeing.

[slouken]

So this is the bug in Unvanquished:
https://github.com/DaemonEngine/Daemon/blob/master/src/engine/sys/sdl_input.cpp#L1091

This code needs to check to see if the mouse motion is already centered, before centering again.

[slouken]

I'm going to back out 3318590 for 2.0.22 and update DaemonEngine/Daemon#600 with the fix.

[slouken]

So I can reproduce this issue with this test program:

#include "SDL.h"
 
SDL_Window *window;

void IN_CenterMouse()
{
	int w, h;
	SDL_GetWindowSize( window, &w, &h );
	SDL_WarpMouseInWindow( window, w / 2, h / 2 );
}

int main(int argc, char ** argv)
{
	SDL_Event event;
	int quit = 0;
	int frame = 0;
 
	SDL_Init(SDL_INIT_VIDEO);
	window = SDL_CreateWindow("Test", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 640, 480, 0);
 
	SDL_SetRelativeMouseMode(SDL_TRUE);
	IN_CenterMouse();
	while (!quit)
	{
		while (SDL_PollEvent(&event) > 0) {
			switch (event.type)
			{
			case SDL_QUIT:
				quit = 1;
				break;
			case SDL_KEYUP:
				if (event.key.keysym.scancode == SDL_SCANCODE_ESCAPE)
				{
					quit = 1;
				}
				break;
			case SDL_MOUSEMOTION:
				IN_CenterMouse();
				break;
			}
		}
		SDL_Log("Frame %d\n", ++frame);
		SDL_Delay(16);
	}
 
	SDL_DestroyWindow(window);
	SDL_Quit();
 
	return 0;
}

With 2.0.18, this loop runs forever without printing a single frame. With 2.0.22, this loop will exit, but because each mouse motion generates another mouse motion, each real mouse motion will build up until the SDL event queue is full - which is the behavior reported in DaemonEngine/Daemon#600.

[slouken]

And here is the fix for the test program:

                        case SDL_MOUSEMOTION:
                                SDL_Log("Motion: %d,%d\n", event.motion.xrel, event.motion.yrel);
                                if (event.motion.xrel || event.motion.yrel) {
                                        IN_CenterMouse();
                                }
                                break;

[slouken]

@smcv, can you verify that fa073ed fixes the regression with Source 1 games?

[smcv]

@smcv, can you verify that fa073ed fixes the regression with Source 1 games?

I tested a straightforward git revert 3318590 in #5569 (comment), and it fixed Portal 2, while breaking Unvanquished. I'll double-check with your commit next week, but I'm pretty sure it's the same as what I already tested.

[slouken]

@smcv, can you verify that fa073ed fixes the regression with Source 1 games?

I tested a straightforward git revert 3318590 in #5569 (comment), and it fixed Portal 2, while breaking Unvanquished. I'll double-check with your commit next week, but I'm pretty sure it's the same as what I already tested.

Okay, my change was just a git revert, so I’ll take that as confirmation. Thanks!