libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
10.01k stars 1.85k forks source link

[Windows] Toggling SetRelativeMouseMode can cause incorrect SDL_MOUSEMOTION occasionally #4165

Closed peppy closed 3 years ago

peppy commented 3 years ago

I am working on a project where it is important to seamlessly move the mouse in and out of the window. While in the window, we wish for the mouse to be in relative mode to benefit from raw input and mouse capture. This means we are toggling the relative mouse state when approaching the window border (note that this is just to describe our use case and does not contribute to the reproducibility of the issue).

Our expectation is that when switching relative mode on / off, there should be no resultant SDL_MOUSEMOTION generated that was not a result of user input. On rare occasions this is not the case.


using System;
using System.Diagnostics;
using SDL2;

namespace SDL2IsolatedRelativeModeTest
{
    class Program
    {
        static bool relative;

        static void Main(string[] args)
        {
            SDL.SDL_Init(SDL.SDL_INIT_VIDEO);
            SDL.SDL_SetHint(SDL.SDL_HINT_EVENT_LOGGING, "2");

            SDL.SDL_CreateWindow("test window", 100, 100, 500, 500, SDL.SDL_WindowFlags.SDL_WINDOW_RESIZABLE);

            var stopwatch = new Stopwatch();
            stopwatch.Start();

            while (stopwatch.ElapsedMilliseconds < 10000)
            {
                while (SDL.SDL_PollEvent(out var e) > 0)
                {
                    if (e.type == SDL.SDL_EventType.SDL_MOUSEMOTION)
                    {
                        // switch relative mode every time an input event is received.
                        relative = !relative;
                        SDL.SDL_SetRelativeMouseMode(relative ? SDL.SDL_bool.SDL_TRUE : SDL.SDL_bool.SDL_FALSE);
                        Console.WriteLine($"Relative mode {relative}");
                    }
                }
            }
        }
    }
}

Doing small circles with the mouse away from the centre of the window works fine most of the time (and potentially forever if the toggle rate is lower) but can fail with rapid toggles. I have not yet ascertained whether the rate of toggle is important, or whether this can always occur at a low reproduction change, but based on loose testing with our game it looks to be the second of these cases.

https://user-images.githubusercontent.com/191335/110595947-07d57400-81c2-11eb-8c0f-5efb88edfbc0.mp4

Note the high delta suddenly appearing in the log output:


Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5669 windowid=1 which=0 state=0 x=424 y=436 xrel=1 yrel=2)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5676 windowid=1 which=0 state=0 x=424 y=438 xrel=0 yrel=2)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5685 windowid=1 which=0 state=0 x=424 y=439 xrel=0 yrel=1)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5701 windowid=1 which=0 state=0 x=424 y=440 xrel=0 yrel=1)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5719 windowid=1 which=0 state=0 x=423 y=440 xrel=-1 yrel=0)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5724 windowid=1 which=0 state=0 x=422 y=441 xrel=-1 yrel=1)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5734 windowid=1 which=0 state=0 x=420 y=441 xrel=-2 yrel=0)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5741 windowid=1 which=0 state=0 x=417 y=441 xrel=-3 yrel=0)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5749 windowid=1 which=0 state=0 x=415 y=441 xrel=-2 yrel=0)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5758 windowid=1 which=0 state=0 x=412 y=441 xrel=-3 yrel=0)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5765 windowid=1 which=0 state=0 x=410 y=441 xrel=-2 yrel=0)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5774 windowid=1 which=0 state=0 x=407 y=439 xrel=-3 yrel=-2)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5778 windowid=1 which=0 state=0 x=249 y=237 xrel=-158 yrel=-202)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5782 windowid=1 which=0 state=0 x=247 y=236 xrel=-2 yrel=-1)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5792 windowid=1 which=0 state=0 x=246 y=233 xrel=-1 yrel=-3)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5797 windowid=1 which=0 state=0 x=245 y=230 xrel=-1 yrel=-3)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5808 windowid=1 which=0 state=0 x=244 y=228 xrel=-1 yrel=-2)
Relative mode True
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5812 windowid=1 which=0 state=0 x=244 y=226 xrel=0 yrel=-2)
Relative mode False
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=5820 windowid=1 which=0 state=0 x=244 y=224 xrel=0 yrel=-2)

I've already spent considerable time trying to figure out why this is happening, but thought I would open an issue in the mean time. It would seem the bad event is arriving via

https://github.com/libsdl-org/SDL/blob/3a1317ed47698c268574e1d0018edd6485f5dbc4/src/video/windows/SDL_windowsevents.c#L635

aka the non-rawinput flow. It looks to be some kind of race condition or timing issue. I am aware that this is a pain point on windows (and there are already weird workaround required for recent windows updates, although that looks specific to the alternate warp-mode) but this is one of the main issues we are facing with getting our game working as a user would expect.

Any direction or feedback would be appreciated.

peppy commented 3 years ago

I'm looking at a solution for this. First of all, let me better explain the interactions at play here:

The crux of this issue looks to be that WM_MOUSEMOVE does not arrive immediately. The failing flow is:

SDL_SetRelativeMouseMode(TRUE) // application call
    // warps mouse to centre (#1)
    WIN_WarpMouse
    SetCursorPos

SDL_SetRelativeMouseMode(FALSE) // application call
    // warps mouse to true location (#2)
    WIN_WarpMouse
    SetCursorPos

// WM_MOUSEMOVE arrives for above SetCursorPos #1
INFO: SDL EVENT: SDL_MOUSEMOTION (timestamp=116167 windowid=1 which=0 state=0 x=250 y=238 xrel=-210 yrel=-200)

SDL_SetRelativeMouseMode(TRUE) // application call, before #2 MOUSEMOVE arrives

// WM_MOUSEMOVE never arrives for above SetCursorPos #2 (but why?)
// if the second event does arrive (can happen depending on timing) there is an equal relative jump back to correct position.

I have two proposed directions to fix this.

First is that we are only running into this issue due to the necessity of toggling relative mode off when the user reaches the edge of the window. This is because in our scenario, we don't implicitly want mouse "grab" to enabled when relative mode is. Currently this is enforced:

https://github.com/libsdl-org/SDL/blob/2689e844e0ab60e48052504f87efcbd6e766b227/src/video/SDL_video.c#L2657-L2661

By changing the logic here to make mouse grab optional would remove the need for us to handle this manually via a full SetRelativeMode toggle from our end. Of course, this needs API consideration, as it may overlap with existing exposed method SDL_SetWindowGrab (already usable for this purpose, but overwritten by the next call to SDL_UpdateWindowGrab).

Second would be to remove all "warp to center" logic in the case that relative_mode_warp is not set. This includes a "custom" implementation of ClipCursor which uses a 2x2 pixel area at the center of the window (which even on blaming back to the beginning, I can't find reasoning for its presence). I have done this and it looks to be working quite well, so will PR this solution for further discussion.

slouken commented 3 years ago

FYI, here's the equivalent C code in case this needs to be tested again in the future:

#include "SDL.h"

int main( int argc, char *argv[] )
{
    SDL_bool relative = SDL_FALSE;
    SDL_Event e;
    Uint32 then;

    SDL_Init( SDL_INIT_VIDEO );
    SDL_SetHint( SDL_HINT_EVENT_LOGGING, "2" );

    SDL_CreateWindow( "test window", 100, 100, 500, 500, SDL_WINDOW_RESIZABLE );

    then = SDL_GetTicks();
    while ( ( SDL_GetTicks() - then ) < 10000 )
    {
        while ( SDL_PollEvent( &e ) > 0 )
        {
            if ( e.type == SDL_MOUSEMOTION )
            {
                // switch relative mode every time an input event is received.
                relative = !relative;
                SDL_Log( "Relative mode %s\n", relative ? "true" : "false" );
                SDL_SetRelativeMouseMode( relative ? SDL_TRUE : SDL_FALSE );
            }
        }
    }
    return 0;
}
slouken commented 3 years ago

@peppy, I'm curious why you need to do this? It sounds like you have the mouse cursor visible and you want to allow free motion off the window, but still get relative mouse motion. But this is the case if you just don't enable relative mouse mode - you still get relative mouse motion, it just stops when the mouse leaves the window.

peppy commented 3 years ago

We use relative mode to get "raw" input, ie. bypassing windows mouse acceleration and applying our own. But when the "virtual" cursor reaches the edge of the window, we want to allow it to exit the window (if in game menus).

I can make a video showing our use case if you'd like (or you can test it out at https://github.com/ppy/osu if you don't mind downloading or building the project). You can find "high precision cursor" under settings, which toggles relative mode under the hood.

peppy commented 3 years ago

This has regressed with recent relative mode changes. I haven't looked into this, but it does mean for the time being we can't track upstream changes with our fork any more (as the related relative mouse code has been changed quite a lot).

On a quick tests with a similar scenario to the isolated one reported in this thread:

https://user-images.githubusercontent.com/191335/136323585-f3f06091-8923-46d0-963e-93f5084e0cdc.mp4

For reference, here's how it looked prior to recent changes:

https://user-images.githubusercontent.com/191335/136323362-d2c7c53b-6ed7-4f49-9d48-6be40cd826e7.mp4

slouken commented 3 years ago

The addition of the border inside the window where the mouse is warped to the opposite side is intentional. This makes sure that applications that have the mouse hidden get continuous motion events while the mouse is moving in a single direction, indefinitely.

I haven't tried it, but it seems like tracking your rendered mouse position and then warping to that location after turning off relative mode is the right way to handle your use case. I'll run your test in the original bug report and see if anything obvious is happening unexpectedly here.

slouken commented 3 years ago

Your original test program still works fine here. I'll write a quick test that shows the approach I described above and we can see if it works, and whether you're seeing the same result.

slouken commented 3 years ago

Here is an example program implementing what I described. Most of the time it works fine, but occasionally the mouse warp to the opposite side of the window is reflected in the app mouse coordinates. I'll take a look at why that's happening.

#include "SDL.h"

int
main(int argc, char *argv[])
{
    int w = 1024;
    int h = 768;
    SDL_Window *window;
    SDL_Renderer *renderer;
    SDL_Surface *image;
    SDL_Texture *sprite;
    SDL_Rect rect;
    SDL_bool done = SDL_FALSE;
    SDL_bool was_relative = SDL_FALSE;
    SDL_bool relative = SDL_FALSE;

    if (SDL_CreateWindowAndRenderer(w, h, SDL_WINDOW_SHOWN, &window, &renderer) < 0) {
        return 1;
    }

    image = SDL_LoadBMP("icon.bmp");
    if (!image) {
        return 2;
    }
    rect.w = image->w;
    rect.h = image->h;

    sprite = SDL_CreateTextureFromSurface(renderer, image);
    if (!sprite) {
        return 3;
    }
    SDL_FreeSurface(image);

    while (!done) {
        SDL_Event e;

        while (SDL_PollEvent(&e) == 1) {
            if (e.type == SDL_QUIT) {
                done = 1;
            }
        }

        SDL_GetMouseState(&rect.x, &rect.y);
        if (rect.x == 0 || rect.x == (w - 1) ||
            rect.y == 0 || rect.y == (h - 1)) {
            relative = SDL_FALSE;
        } else {
            relative = SDL_TRUE;
        }
        if (relative != was_relative) {
            SDL_SetRelativeMouseMode(relative);
            if (!relative) {
                SDL_WarpMouseInWindow(window, rect.x, rect.y);
            }
            was_relative = relative;
        }

        SDL_RenderClear(renderer);
        rect.x -= rect.w / 2;
        rect.y -= rect.h / 2;
        SDL_RenderCopy(renderer, sprite, NULL, &rect);
        SDL_RenderPresent(renderer);
    }
    SDL_Quit();
    return 0;
}
slouken commented 3 years ago

I'm not sure how to fix this. The problem is that the mouse motion for the warp and real mouse motion are combined and delivered some time after the warp is requested, and occasionally, after relative mode is turned off. We need to warp, otherwise the mouse pointer will potentially overlap the task bar or floating notifications and clicking on them will remove focus from the application, even though the visible mouse cursor is nowhere near them.

slouken commented 3 years ago

Flushing pending mouse motion when we warp takes care of this. Can you try the latest code and let me know if there are other issues?

peppy commented 3 years ago

Firstly thanks for your dedication.

Unfortunately I'm still seeing the mouse warp occasionally (I'm testing on a different system this time so I can't be certain but it does look to happen less often). If the original test code is working here for you then I can look to created and isolated repro case.

The reasoning you mention for warping to centre (to avoid focus loss) makes some sense, but also likely isn't infallible if there happened to be a window popup around the centre region. But this also wouldn't be a case we care about when at the game menus (where we don't want to confine mouse to the window). If fixing this seems like something that is potentially out of reach, a means to toggle the mouse confining (as proposed in https://github.com/libsdl-org/SDL/issues/4165#issuecomment-812301154) may be a better path forward.

slouken commented 3 years ago

Yes, a new test case would be helpful. Both the test programs above work well for me with the latest code.

slouken commented 3 years ago

Actually, I'm still seeing an occasional mouse warp as well.

I was able to simplify the test program since SDL internally handles the warping and duplicate relative mode states:

#include "SDL.h"

int
main(int argc, char *argv[])
{
    int w = 1024;
    int h = 768;
    SDL_Window *window;
    SDL_Renderer *renderer;
    SDL_Surface *image;
    SDL_Texture *sprite;
    SDL_Rect rect;
    SDL_bool done = SDL_FALSE;
    SDL_bool relative = SDL_FALSE;

    if (SDL_CreateWindowAndRenderer(w, h, SDL_WINDOW_SHOWN, &window, &renderer) < 0) {
        return 1;
    }

    image = SDL_LoadBMP("icon.bmp");
    if (!image) {
        return 2;
    }
    rect.w = image->w;
    rect.h = image->h;

    sprite = SDL_CreateTextureFromSurface(renderer, image);
    if (!sprite) {
        return 3;
    }
    SDL_FreeSurface(image);

    while (!done) {
        SDL_Event e;

        while (SDL_PollEvent(&e) == 1) {
            if (e.type == SDL_QUIT) {
                done = 1;
            }
        }

        SDL_GetMouseState(&rect.x, &rect.y);
        if (rect.x == 0 || rect.x == (w - 1) ||
            rect.y == 0 || rect.y == (h - 1)) {
            relative = SDL_FALSE;
        } else {
            relative = SDL_TRUE;
        }
        SDL_SetRelativeMouseMode(relative);

        SDL_RenderClear(renderer);
        rect.x -= rect.w / 2;
        rect.y -= rect.h / 2;
        SDL_RenderCopy(renderer, sprite, NULL, &rect);
        SDL_RenderPresent(renderer);
    }
    SDL_Quit();
    return 0;
}
slouken commented 3 years ago

I think this takes care of the unexpected mouse motion from the warp: https://github.com/libsdl-org/SDL/commit/16aeb8d0f5e550ac124096c7b5e03ce756762e12

slouken commented 3 years ago

Does the latest code work well for you?

peppy commented 3 years ago

Initial testing shows it doesn't. I'll try and isolate a repro of what we are doing (and ensure we aren't adding any new variables to the puzzle). Also of note is a similar issue (https://github.com/libsdl-org/SDL/issues/4339) which may or may not be related to the remaining problems we're finding.

peppy commented 3 years ago

As an update, I can confirm that my initial test code in this thread does not fail in latest HEAD. Will update when I figure why we are still seeing weird behaviour in our actual game implementation.

peppy commented 3 years ago

The remaining issue (and missing piece of the puzzle) is the we are using SDL_WarpMouseInWindow to "correctly" position the cursor on focus loss or reaching the edge of the game window (in windowed relative mouse mode). My suspicion is that SDL is performing warps which are unexpected by our usage and causing feedback (as the mouse re-enters the window). If I remove our usage of warp, issues no longer occur.

So before I go further, let me attempt to re-explain our use case (and why we need to use warp in the first place) in case you have a tip on how we should be doing things:

Right now we are using a warp call to reposition the windows cursor in these edge cases (and only these cases). In other words, we don't care what SDL does to the cursor when relative mouse is turned on, but after we turn it off, we want to be able to ensure the OS cursor is correct. (in fact, sdl warping the cursor to keep it within the bounds may be helpful when a user sets their sensitivity <1.0x, although seemingly not required).

This shows the behaviour we are going for. Hopefully the effect of the sensitivity adjustments are obvious from the large jumpiness of mouse updates:

https://user-images.githubusercontent.com/191335/136524360-7b5eace0-e0aa-495b-a514-d36d8c078238.mp4

Also, when alt-tabbing:

https://user-images.githubusercontent.com/191335/136524723-330022a6-c8e2-497a-8c9b-de8c838f081c.mp4

We transfer the cursor position to windows via a warp in order to allow seamless switching to non-relative mouse mode.

slouken commented 3 years ago

Is it important that you use a different sensitivity than the OS cursor movement when in the menu system? Usually when the mouse cursor is visible, the user expects it to move as quickly as they have configured (or simply are used to) on the desktop. My suggestion would be to simply turn off relative mode when in the menus. Maybe I'm being simplistic and your use case is more complex?

However, if your mouse sensitivity is a simple linear scaling, you can have SDL take care of it for you, by simply setting the hint SDL_HINT_MOUSE_RELATIVE_SPEED_SCALE. This dynamically adjusts the scaling applied to relative mouse motion, and when you turn off relative mode, SDL will warp the cursor to the current position for you. This has the advantage that your idea of where the mouse cursor is and SDL's is the same.

e.g. SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_SPEED_SCALE, "0.5")

peppy commented 3 years ago

The user expectation is unfortunately that the mouse should always move the same in the game regardless of where they are (that's how it has worked until now in our non-SDL-based older version, and no one has really complained).

I actually remember seeing that hint added and thinking it would be worthwhile to give it a go. I'll try that out within the next day and get back you , thanks!

slouken commented 3 years ago

I set the scale in the test code to 0.8, and run the mouse back and forth over the top left corner, and I'm still occasionally seeing the mouse pop over to the lower right corner. As far as I can tell, occasionally the SetCursorPos() call is simply ignored by Windows, as we see in https://github.com/libsdl-org/SDL/issues/4339.

Here's a more verbose trace of the occurrence, in this case with a single SetCursorPos() call:

INFO: WM_MOUSEMOVE: 596,511
INFO: WM_INPUT: -3,1
INFO: SendMouseMotion: 0,248
INFO: WM_MOUSEMOVE: 594,512
INFO: ToggleRawInput: DISABLED
INFO: SetCursorPos: 0,248 (WARP)
INFO: WM_MOUSEMOVE: 592,512 (IGNORED)
INFO: WM_MOUSEMOVE: 590,512 (IGNORED)
INFO: WM_MOUSEMOVE: 588,513 (IGNORED)
INFO: WM_MOUSEMOVE: 587,513 (IGNORED)
INFO: WM_MOUSEMOVE: 585,514 (IGNORED)
INFO: WM_MOUSEMOVE: 584,514 (IGNORED)
INFO: WM_MOUSEMOVE: 583,514 (IGNORED)
INFO: WM_MOUSEMOVE: 582,514 (IGNORED)
INFO: WM_MOUSEMOVE: 582,515 (AFTER WARP)
INFO: WM_MOUSEMOVE: 582,515
INFO: SendMouseMotion: 582,515
INFO: ToggleRawInput: ENABLED
INFO: WM_INPUT: -2,0
INFO: SendMouseMotion: 581,515
slouken commented 3 years ago

I think using the hint is still an improvement because it seems like Windows more frequently ignores the SetCursorPos() call when there are two in a row, e.g. SDL warps and then you warp.

slouken commented 3 years ago

I think this is finally fixed. Can you try it now?

Either way, I still recommend using the hint to reduce load on the window system with double-warping.

peppy commented 3 years ago

Interesting solution, but is in line with what I was seeing in my original investigation of the issue. Can confirm things are looking perfect now, as long as we do our existing warp calls.

If I remove the warp and apply the sensitivity hint as suggested, while the movement and positioning does match 1:1 with our expectations, the mouse does end up in the incorrect locations as above. Are you able to repro that case? This happens even with sensitivity set to 1.

https://user-images.githubusercontent.com/191335/136647704-1a855fe9-3439-4b09-baf7-56f78fe6b427.mp4

DomGries commented 3 years ago

I think the regression mentioned by @peppy https://github.com/libsdl-org/SDL/issues/4165#issuecomment-937450601 was caused by https://github.com/libsdl-org/SDL/commit/db68af8032e1fb988d08b7253839b17e532e1db7 because of the added WarpWithinBoundsRect while in relative mode however as @0x1F9F1 mentioned there this could be better achieved by WIN_UpdateClipCursor since we already clip the cursor anyway (just need to shrink the clip area).

I believe with this change we could then undo the recent commits (https://github.com/libsdl-org/SDL/commit/40ed9f75c9e1ed1dd99ee699ff4f678438ac3662, https://github.com/libsdl-org/SDL/commit/16aeb8d0f5e550ac124096c7b5e03ce756762e12 and https://github.com/libsdl-org/SDL/commit/649466f4915b68cfa75e802bef7ee192b2192684) which to me personally look like they reduce performance and code clarity.

0x1F9F1 commented 3 years ago

This is hopefully fixed by #4831. Any testing would be appreciated.

slouken commented 3 years ago

Please retest using the latest SDL snapshot at commit https://github.com/libsdl-org/SDL/commit/a1fabca162091b50d6f7dd71879d028319e09d80: http://www.libsdl.org/tmp/SDL-2.0.zip

Thanks!

peppy commented 3 years ago

In some extensive testing, I was only able to make the mouse warp once (over about 5 minutes of manual movement). That may have just been a fluke. I think it's still in a good state from my end.

slouken commented 3 years ago

When using a Razer Viper 8K mouse, I can get the mouse warp to be ignored semi-regularly. The mouse event sequence from Windows is OLD OLD OLD NEW OLD OLD OLD.

I think what's happening is that there is a race condition in Windows where if a mouse report is coming in at exactly the right time, it will clobber the SetCursorPos() call.

I think we've made it as robust as possible, so I'll close it as-is for now. Thanks for the help!