Joystick: Update to SDL3 #13159
Joystick: Update to SDL3 #13159
Conversation
HTRamsey
force-pushed
the
dev-sdl3
branch
5 times, most recently
from
4e2006f to
dc8012d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
HTRamsey
force-pushed
the
dev-sdl3
branch
2 times, most recently
from
017fe00 to
d5b654f
Compare
HTRamsey
force-pushed
the
dev-sdl3
branch
2 times, most recently
from
35c0cbb to
c9de1c6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the joystick system from SDL2 to SDL3 to improve compatibility and functionality. The changes also enhance thread safety in the JoystickManager and reorganize the vehicle joystick handling code.
- Updates SDL library from version 2.32.4 to 3.2.16 with corresponding API changes
- Improves thread safety by adding mutex protection in JoystickManager operations
- Refactors Vehicle class to consolidate joystick-related code into a dedicated section
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Vehicle/Vehicle.h | Reorganizes joystick properties and methods into dedicated section at bottom of class |
| src/Vehicle/Vehicle.cc | Moves joystick implementation to dedicated section and updates method signatures |
| src/Joystick/JoystickSDL.h | Updates SDL struct types and function names for SDL3 compatibility |
| src/Joystick/JoystickSDL.cc | Implements SDL3 API calls and updates joystick discovery logic |
| src/Joystick/JoystickManager.h | Adds mutex for thread safety and converts timer pointer to member |
| src/Joystick/JoystickManager.cc | Implements thread-safe operations and updates SDL event handling |
| src/Joystick/CMakeLists.txt | Updates build configuration for SDL3 with new options and flags |
| cmake/CustomOptions.cmake | Adds SDL gamepad controller configuration option |
Comments suppressed due to low confidence (3)
src/Vehicle/Vehicle.cc:360
- The multiplication
1.0 * 1000.0is unnecessary and could be simplified to1000.0ffor better readability and to match the float type.
}
| if (SDL_GameControllerAddMapping(qPrintable(mapping)) == -1) { | ||
| qCWarning(JoystickSDLLog) << "Couldn't add GameController mapping:" << mapping << "Error:" << SDL_GetError(); | ||
| } | ||
| #ifdef SDL_GAMECONTROLLERCONFIG |
There was a problem hiding this comment.
Using #ifdef to check for a CMake-defined string value is incorrect. CMake always defines the macro even if the string is empty. Consider using #if defined(SDL_GAMECONTROLLERCONFIG) && (SDL_GAMECONTROLLERCONFIG[0] != '\0') or checking the string length at runtime.
| #ifdef SDL_GAMECONTROLLERCONFIG | |
| #if defined(SDL_GAMECONTROLLERCONFIG) && (SDL_GAMECONTROLLERCONFIG[0] != '\0') |
Copilot uses AI. Check for mistakes.
HTRamsey
force-pushed
the
dev-sdl3
branch
3 times, most recently
from
4eff9de to
d72d064
Compare
cmake/CustomOptions.cmake
Outdated
| @@ -34,6 +34,9 @@ option(QGC_ENABLE_GST_VIDEOSTREAMING "Enable GStreamer Video Backend" ON) | |||
| cmake_dependent_option(QGC_CUSTOM_GST_PACKAGE "Enable Using QGC Provided Custom GStreamer Packages" OFF "QGC_ENABLE_GST_VIDEOSTREAMING" OFF) | |||
| option(QGC_ENABLE_QT_VIDEOSTREAMING "Enable QtMultimedia Video Backend" OFF) # Qt6Multimedia_FOUND | |||
|
|
|||
| # Joystick | |||
| set(QGC_SDL_GAMECONTROLLERCONFIG "0300000009120000544f000011010000,OpenTX Radiomaster TX16S Joystick,leftx:a3,lefty:a2,rightx:a0,righty:a1,platform:Linux" CACHE STRING "Custom SDL Joystick Mappings") | |||
There was a problem hiding this comment.
The db entry is wrong or missing in the one we use? Or is this just an example for the override possibility?
There was a problem hiding this comment.
There have been a few issues related to this joystick, and somebody commented that this config fixes it. Haven't verified it myself tho
There was a problem hiding this comment.
Should we have our own override file we read in to tweak some set over the top of the gamecontrollerdb file? Seems kinda specific to override a single joystick which a compiler option.
There was a problem hiding this comment.
we can, although I'd be surprised if we ever actually have to do this again for another joystick. (And maybe SDL3 fixes it by itself anyways)
There was a problem hiding this comment.
I guess in reality if some commercial company hits a problem with there systems it's gonna be a specific single joystick that needs tweaking. So this way is probably fine then. No need to get fancier.
This may improve joystick compatibility & functionality
Also update thread safety in JoystickManager and cleanup vehicle joystick handling