Skip to content
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

SDL2 backend: Implement multiple gamepad support #3884

Conversation

lethal-guitar
Copy link
Contributor

This makes the SDL2 backend work correctly with multiple gamepads. Instead of
hard-coding gamepad index 0, we keep a list of all currently available
gamepads. When gamepads are added or removed, we update the list accordingly.
When processing gamepad input, we then iterate over this list and combine the
button and axis state of all gamepads.

Note that it's not necessary to enumerate gamepads during initialization of the backend, because SDL automatically sends a SDL_CONTROLLERDEVICEADDED event for every gamepad that was already present during initialization of SDL.

The main motivation for this change is that I can have multiple gamepads
connected to my system and grab any one of them at random, and have ImGui
recognize my inputs. Previously, this was not guaranteed, because the gamepad I
grabbed might not have index 0. It's also possible now to (for example) use the
D-pad on one gamepad and buttons on another one at the same time, although
that seems less useful in practice :)

Additionally, we now properly close all previously opened gamepads when
shutting down the backend.

@lethal-guitar lethal-guitar force-pushed the sdl-multi-controller-support branch 3 times, most recently from 2d4995a to b8f9dbc Compare March 5, 2021 13:01
@ocornut ocornut added backends nav keyboard/gamepad navigation labels Mar 5, 2021
@ocornut
Copy link
Owner

ocornut commented Mar 5, 2021

Thanks for the PR.

Some quick feedback:

  • please squash into single commit.
  • missing break statement in ImGui_ImplSDL2_ProcessEvent()
  • ImGuiBackendFlags_HasGamepad is now set in two places.
  • why exactly linking with Shell32.lib? and if so probably should be added elsewhere?
  • style: single statement scopes don't need braces.
  • style: you can use .Size member of ImVector.
  • not sure why you introduced new functions ImGui_ImplSDL2_MapAnalog() etc. that code is designed to be dense and out of the way, and is very unfrequently changed.
  • you can update the changelog at the top of the imgui_impl_sdl.cpp file.
  • can SDL_GameControllerOpen() return NULL and if so what will happen on SDL_GameControllerClose() ?

Thank you

I am not sure this change is desirable for all users but I'm open to try.
I imagine backends may need some forms of settings eventually.

@lethal-guitar
Copy link
Contributor Author

lethal-guitar commented Mar 5, 2021

Thanks for the quick feedback! I believe I've addressed most of your comments now.

please squash into single commit.

Done - I still kept the commit adding Shell32.lib separate, as I'm now wondering if that maybe should be its own PR?

So what happened was that I wanted to build the SDL2 example in order to preapre this PR - I had only built ImGui as part of my own project's build system before. But I then got a linker error when running build_win32.bat:

SDL2main.lib(SDL_windows_main.obj) : error LNK2019: unresolved external symbol __imp__CommandLineToArgvW@8 referenced in function _main_getcmdline

I then consulted MSDN for the referenced function CommandLineToArgvW and found that it's located in Shell32.lib, so I added that and the build succeeded.

My suspicion is that earlier versions of SDL (I'm using 2.0.14) did not require this lib, so the build worked fine, but some newer version now needs it. But I need to verify that.

Let me know if you'd prefer a separate PR for that change, as it is indeed unrelated to the rest of this PR.

missing break statement in ImGui_ImplSDL2_ProcessEvent()

Thanks, added!

ImGuiBackendFlags_HasGamepad is now set in two places.

Whoops, forgot to remove the old one, fixed now. Thanks!

style: single statement scopes don't need braces.
style: you can use .Size member of ImVector.

Both done ✔️

not sure why you introduced new functions ImGui_ImplSDL2_MapAnalog() etc. that code is designed to be dense and out of the way, and is very unfrequently changed.

I personally find the one-liners harder to read, but your point that this is unlikely to change frequently makes sense. And it also seems better to keep the code local to ImGui_ImplSDL2_UpdateGamepads. So I went back to keeping everything in the macros as one-liners now.

To explain how I ended up with these functions: When doing these changes in my project originally, I converted the macros to C++ 11 lambdas so that I would have an easier time understanding the code and making the changes (here's how that looks). Once I decided to upstream the changes, I then converted the lambdas to functions to avoid the need for C++ 11. But the aspect of keeping the code local is unfortunately lost that way.

you can update the changelog at the top of the imgui_impl_sdl.cpp file.

Done, let me know if that looks ok!

can SDL_GameControllerOpen() return NULL and if so what will happen on SDL_GameControllerClose() ?

Ah yes, good catch, it can indeed return NULL. I've changed the code to only add it to the list if the return value is non-null. Please let me know if doing this type of combined assignment & check in an if-statement is ok, or if you'd prefer to have the assignment as a separate statement.

@lethal-guitar lethal-guitar force-pushed the sdl-multi-controller-support branch 2 times, most recently from dae475e to f8a814e Compare March 5, 2021 17:31
@ocornut
Copy link
Owner

ocornut commented Mar 5, 2021

My suspicion is that earlier versions of SDL (I'm using 2.0.14) did not require this lib, so the build worked fine, but some newer version now needs it. But I need to verify that.
Let me know if you'd prefer a separate PR for that change, as it is indeed unrelated to the rest of this PR.

Yes I would like us to clarify that, I honestly think it would be surprising if SDL suddenly started requiring shell32 especially for such a a simple version it would usually prefer experimenting.

The spacing in the first two functions is also odd. We normally have this specced in the .editor_config file but this is not well handled by all IDE..

@lethal-guitar
Copy link
Contributor Author

Yup, I just tested with an older version of SDL, and indeed the linker error does not happen with that one. So it seems my suspicion was correct, but I'll try to track down more precisely in which version that changed and why..

In the meantime, I've removed the commit and also fixed the spacing (I'm using Vim 😄)

@lethal-guitar
Copy link
Contributor Author

Ok, I managed to track it down in the SDL source. The dependency on Shell32.lib was introduced in version 2.0.12, with this commit: libsdl-org/SDL@f8400cb

This makes the SDL2 backend work correctly with multiple gamepads. Instead of
hard-coding gamepad index 0, we keep a list of all currently available
gamepads. When gamepads are added or removed, we update the list accordingly.
When processing gamepad input, we then iterate over this list and combine the
button and axis state of all gamepads.

Note that it's not necessary to enumerate gamepads during initialization
of the backend, because SDL automatically sends a
`SDL_CONTROLLERDEVICEADDED` event for every gamepad that was already
present during initialization of SDL.

The main motivation for this change is that I can have multiple gamepads
connected to my system and grab any one of them at random, and have ImGui
recognize my inputs. Previously, this was not guaranteed, because the gamepad I
grabbed might not have index 0. It's also possible now to (for example) use the
D-pad on one gamepad and and buttons on another one at the same time, although
that seems less useful in practice :)

Additionally, we now properly close all previously opened gamepads when
shutting down the backend.
@ocornut ocornut added inputs and removed nav keyboard/gamepad navigation labels Jun 30, 2023
ocornut added a commit that referenced this pull request Feb 13, 2024
…unt. Added ImGui_ImplSDL2_SelectGamepadAuto()/ImGui_ImplSDL2_SelectGamepadExplicit(). (#3884, #6559, #6890)
@ocornut
Copy link
Owner

ocornut commented Feb 13, 2024

I have pushed bf1c96d+f966da1 which:

  • add support for disconnection/reconnection
  • auto mode picks first controller
  • add ImGui_ImplSDL2_SelectGamepadExplicit() to select a gamepad manually.

I am not entirely against an API for multiple-gamepad support but I'm not entirely sure this is what you actually truly needed (vs simply better handling or disconnection/reconnection).

@ocornut
Copy link
Owner

ocornut commented Feb 13, 2024

I reopened as I am going to work those API right now.
Support for multiple gamepads requires some rework as we now use io events API, so it needs to be done differently.

@ocornut
Copy link
Owner

ocornut commented Feb 13, 2024

Rework API and added support for multiple controllers: d15e410

IMGUI_IMPL_API void ImGui_ImplSDL2_SetGamepadModeAutoFirst();   // Use first available gamepad (default)
IMGUI_IMPL_API void ImGui_ImplSDL2_SetGamepadModeAutoAll();
IMGUI_IMPL_API void ImGui_ImplSDL2_SetGamepadModeManual(struct _SDL_GameController** gamepads_array, int gamepads_count);

@ocornut ocornut closed this Feb 13, 2024
@lethal-guitar lethal-guitar deleted the sdl-multi-controller-support branch February 13, 2024 19:57
@lethal-guitar
Copy link
Contributor Author

@ocornut nice, that new auto-all mode looks exactly like what I need. Thanks!

ocornut added a commit that referenced this pull request Feb 14, 2024
ocornut added a commit that referenced this pull request Feb 14, 2024
… support for multiple gamepads. Added ImGui_ImplSDL3_SetGamepadMode(). (#7180, #3884, #6559, #6890)
@ocornut
Copy link
Owner

ocornut commented Feb 14, 2024

Reworked API it is now:

enum ImGui_ImplSDL2_GamepadMode 
{ 
   ImGui_ImplSDL2_GamepadMode_AutoFirst, 
   ImGui_ImplSDL2_GamepadMode_AutoAll, 
   ImGui_ImplSDL2_GamepadMode_Manual 
};
IMGUI_IMPL_API void ImGui_ImplSDL2_SetGamepadMode(ImGui_ImplSDL2_GamepadMode mode, struct _SDL_GameController** manual_gamepads_array = NULL, int manual_gamepads_count = -1);

SDL3 has same API.
Used "Gamepad" in function names to match SDL3 code and make things easier to diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants