cmake: print a warning if SDL version is between 2.16 and 2.20 included, ref #600

[illwieckz]

See:

• SDL input is buggy in version 2.16 and later #600

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.
-- @necessarily-equal

[illwieckz]

@necessarily-equal I tried that instead, but it doesn't work, I have no idea why…

    if (SDL2_VERSION VERSION_GREATER_EQUAL "2.16"
        AND SDL2_VERSION VERSION_LESS_EQUAL "2.20")
        message(WARNING "SDL is known to be buggy between version 2.16 and 2.20, see https://github.com/DaemonEngine/Daemon/issues/600")
    endif()

Edit: SDL2 instead of SDL.

[illwieckz]

I mean, there is no message VERSION_GREATER_EQUAL isn't supported (it's supported since 3.7 and we require 3.12), the test just doesn't catch the version when I do ≥ and ≤ instead of ≮ or ≯.

[illwieckz]

In fact SDL2_VERSION is empty…

[illwieckz]

The CONFIG keyword had to be added to the find_package call in order to get SDL2_VERSION filled:

-     find_package(SDL2 REQUIRED)
+     find_package(SDL2 REQUIRED CONFIG)

[illwieckz]

Meh, the CI fails:

CMake Error at CMakeLists.txt:626 (find_package):
  Could not find a package configuration file provided by "SDL2" with any of
  the following names:

    SDL2Config.cmake
    sdl2-config.cmake

  Add the installation prefix of "SDL2" to CMAKE_PREFIX_PATH or set
  "SDL2_DIR" to a directory containing one of the above files.  If "SDL2"
  provides a separate development package or SDK, be sure it has been
  installed.

[illwieckz]

This is how I do it, the first find_package will not be fatal (QUIET) if CONFIG isn't supported, and thenfind_package is called again with REQUIRED but without doing any unsupported version comparison. This happens if SDL2 can be found but CMake doesn't provide sdl2-config.cmake (or SDL2Config.cmake).

    find_package(SDL2 QUIET CONFIG)
    if(SDL2_VERSION)
        if (SDL2_VERSION VERSION_GREATER_EQUAL "2.16"
            AND SDL2_VERSION VERSION_LESS_EQUAL "2.20")
            message(WARNING "SDL ${SDL2_VERSION} between version 2.16 and 2.20 is known to be buggy, see https://github.com/DaemonEngine/Daemon/issues/600")
        endif()
    else()
        # CMake may be able to find SDL2 without supporting CONFIG
        # If sdl2-config.cmake or SDL2Config.cmake isn't provided.
        find_package(SDL2 REQUIRED)
        message(WARNING "SDL version is unknown, version can't be checked for known bugs")
    endif()

[slipher]

I can't test, but based on the comments I'd think we could actually work around the bug. Something like

diff --git a/src/engine/sys/sdl_input.cpp b/src/engine/sys/sdl_input.cpp
index e8a29a26..48270d49 100644
--- a/src/engine/sys/sdl_input.cpp
+++ b/src/engine/sys/sdl_input.cpp
@@ -1079,7 +1079,7 @@ static void IN_ProcessEvents( bool dropInput )
                                        {
                                                Com_QueueEvent( Util::make_unique<Sys::MousePosEvent>(e.motion.x, e.motion.y) );
                                        }
-                                       else
+                                       else if ( e.motion.xrel != 0 || e.motion.yrel != 0 )
                                        {
                                                Com_QueueEvent( Util::make_unique<Sys::MouseEvent>(e.motion.xrel, e.motion.yrel) );
 #if defined( __linux__ ) || defined( __BSD__ )

This patch filters out mouse deltas with 0 motion. It might be necessary to filter out absolute mouse position events with zero deltas too; someone would need to test that.

[necessarily-equal]

That workaround would alleviate the issue quite a bit for SDL2.20 I think, but doesn't fix it for SDL2.16 and 2.18.

…

On Sun, Apr 3 2022 at 14:20:04 -0700, slipher ***@***.***> wrote: I can't test, but based on the comments I'd think we could actually work around the bug. Something like diff --git a/src/engine/sys/sdl_input.cpp b/src/engine/sys/sdl_input.cpp index e8a29a26..48270d49 100644 --- a/src/engine/sys/sdl_input.cpp +++ b/src/engine/sys/sdl_input.cpp @@ -1079,7 +1079,7 @@ static void IN_ProcessEvents( bool dropInput ) { Com_QueueEvent( Util::make_unique<Sys::MousePosEvent>(e.motion.x, e.motion.y) ); } - else + else if ( e.motion.xrel != 0 || e.motion.yrel != 0 ) { Com_QueueEvent( Util::make_unique<Sys::MouseEvent>(e.motion.xrel, e.motion.yrel) ); #if defined( __linux__ ) || defined( __BSD__ ) This patch filters out mouse deltas with 0 motion. It might be necessary to filter out absolute mouse position events with zero deltas too; someone would need to test that. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[slipher]

Why not? Going on this:

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.

[necessarily-equal]

That's after the second commit, the one that kinda fixes it. Before (between the two related commit) there is basically no input for me.

…

On Sun, Apr 3 2022 at 14:25:40 -0700, slipher ***@***.***> wrote: Why not? Going on this: > 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. > — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[slipher]

Our version of daemon/cmake/FindSDL2.cmake doesn't attempt to find a version number.

[illwieckz]

Our version of daemon/cmake/FindSDL2.cmake doesn't attempt to find a version number.

On my end I can delete it and build the engine, is it a fallback for some systems?

It looks like CMake doesn't provide FindSDL2.cmake intentionally to begin with (1, 2, 3) but only sdl2-config.cmake for config mode (what I rely on for version checking).

[slipher]

Right, if there is no FindSDL2.cmake in our module path or the modules shipped with CMake, the next thing is to check for the config.cmake thing. The sdl-config.cmake from the SDL repository also lacks a version number, so it doesn't help us.

[necessarily-equal]

So this is cool and the CI pass, but it seems to print this on Windows, Mac and Linux:

CMake Warning at CMakeLists.txt:636 (message):
  SDL version is unknown, version can't be checked for known bugs

[illwieckz]

Well, maybe it only checks for version on computers of lucky peoples like me…

[slipher]

I don't think it should work for anyone. The cmake/FindSDL2.cmake script we have does not set a version number. If it appears to work for you, maybe you just set SDL2_VERSION in the variable cache while testing...

[illwieckz]

@slipher you seem to miss something important: FindSDL2.cmake is NOT expected to set a version number.

The file that sets a version number is sdl2-config.cmake, which is only used when CONFIG option is passed to find_package(), so to get a version number, you need that your cmake provides sdl2-config.cmake.

On my end:

$ find /usr/ -name sdl2-config.cmake
/usr/lib/x86_64-linux-gnu/cmake/SDL2/sdl2-config.cmake

That's why I am talking about “luck”, it depends on user's own environment.

My code first tries the CONFIG option and then, if there is an error (for example, there is no sdl2-config.cmake), it tries without CONFIG to rely onFindSDL2.cmake or I don't know why.

Edit: on my end (Ubuntu 21.10 focal), the sdl2-config.cmake file is provided by SDL within the libsdl2-dev package.

[slipher]

The sdl-config.cmake from the SDL repository also lacks a version number, so it doesn't help us.

This, as I said above, is true. But there is a second file that does help us! /usr/lib/x86_64-linux-gnu/cmake/SDL2/sdl2-version-config.cmake (note the version in the filename).

So this will only give us the version number on the subset of Linux distros which ship the version config file. (The binary releases from the SDL website don't include the cmake-config stuff.) But hopefully the mouse bug only occurs on Linux so it will be OK.

[illwieckz]

We can also raise a warning in source code when trying to compile against a broken SDL version, but given our build isn't warning free that can be missed.

[slipher]

If you change the warning to only print for Linux builds then LGTM

[illwieckz]

Do you mean the SDL issue only affects Linux systems?

[slipher]

Do you mean the SDL issue only affects Linux systems?

I would guess so but it doesn't matter. On Windows and Mac we provide the precompiled SDL, so no one is using an unapproved version. I don't want there to be useless warnings for every Windows or Mac build.

[illwieckz]

Ah, I see!

[illwieckz]

OK, now the SDL version is only checked on Linux and the message telling the version can't be checked is a status and not a warning.