Compile _sdl2.controller and _sdl2.touch with SDL3 #3427
Conversation
damusss
changed the title
|
As a final change I discarded the preparations changes for audio, since it might be ported differently and will be handled by another PR either way. |
| SDL_GameControllerRumble(self->controller, 0, 0, 1); | ||
| if (!SDL_GameControllerRumble(self->controller, 0, 0, 1)) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } |
There was a problem hiding this comment.
I think this breaks the SDL2 case. Why not leave this code as it is?
| SDL_free(joysticks); | ||
| #else | ||
| int joycount = SDL_NumJoysticks(); | ||
| #endif |
There was a problem hiding this comment.
given this is used in 2 places, does it make more sense to define this as a helper function?
WalkthroughAdds SDL3-compatible code paths across controller and touch subsystems, switches Cython SDL header inclusion to support SDL3, updates Meson build to always include _sdl2 while gating SDL2-only extensions, and gates Python-side _sdl2 submodule imports by SDL version. Public APIs remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python caller
participant C as controller.c
participant SDL as SDL (v2 / v3)
rect rgb(245,245,255)
note over Py,C: Module init — enable gamepad events
Py->>C: controller_module_init()
alt SDL >= 3.0.0
C->>SDL: SDL_SetGamepadEventsEnabled(SDL_TRUE)
SDL-->>C: void
else
C->>SDL: SDL_GameControllerEventState(SDL_ENABLE)
SDL-->>C: int
end
end
rect rgb(245,255,245)
note over Py,C: Get controller count
Py->>C: controller_module_get_count()
alt SDL >= 3.0.0
C->>SDL: SDL_GetJoysticks(&count)
SDL-->>C: SDL_JoystickID* or NULL
C->>SDL: SDL_free(list)
C-->>Py: count or error
else
C->>SDL: SDL_NumJoysticks()
SDL-->>C: int
C-->>Py: count
end
end
rect rgb(255,245,245)
note over Py,C: Rumble / LED
Py->>C: controller_rumble()/controller_set_led()
alt SDL >= 3.0.0
C->>SDL: SDL_GameControllerRumble / SDL_SetGamepadLED
SDL-->>C: SDL_bool
C-->>Py: bool (or raise on failure)
else
C->>SDL: SDL_GameControllerRumble / SDL_GameControllerSetLED
SDL-->>C: int
C-->>Py: bool
end
end
sequenceDiagram
autonumber
participant Py as Python caller
participant T as touch.c
participant SDL as SDL (v2 / v3)
rect rgb(245,255,245)
note over Py,T: Enumerate touch devices
Py->>T: pg_touch_num_devices()
alt SDL >= 3.0.0
T->>SDL: SDL_GetTouchDevices(&count)
SDL-->>T: SDL_TouchID* or NULL
T->>SDL: SDL_free(list)
T-->>Py: count or error
else
T->>SDL: SDL_GetNumTouchDevices()
SDL-->>T: int
T-->>Py: count
end
end
rect rgb(245,245,255)
note over Py,T: Get device / fingers
Py->>T: pg_touch_get_device(index)
alt SDL >= 3.0.0
T->>SDL: SDL_GetTouchDevices(&count)
SDL-->>T: SDL_TouchID* or NULL
T->>SDL: SDL_free(list)
T-->>Py: device id or error
else
T->>SDL: SDL_GetTouchDevice(index)
SDL-->>T: SDL_TouchID
T-->>Py: device id or error
end
Py->>T: pg_touch_num_fingers(device_id)
alt SDL >= 3.0.0
T->>SDL: SDL_GetTouchFingers(device_id, &n)
SDL-->>T: SDL_Finger** or NULL
T->>SDL: SDL_free(ptr)
T-->>Py: n or error
else
T->>SDL: SDL_GetNumTouchFingers(device_id)
SDL-->>T: int
T-->>Py: n
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src_c/_sdl2/controller.c (2)
411-413: SDL3 stop_rumble error handling improved.The SDL3 behavior change where
SDL_GameControllerRumblereturnsSDL_bool(true on success) is correctly handled by checking for false and raising an error.
496-505: Duplicated joystick enumeration code.The SDL3 joystick enumeration code is duplicated between
controller_module_get_countandcontroller_new. This violates the DRY principle.Consider extracting this into a helper function to avoid duplication:
+static int +get_joystick_count(void) +{ +#if SDL_VERSION_ATLEAST(3, 0, 0) + int count; + SDL_JoystickID *joysticks = SDL_GetJoysticks(&count); + if (!joysticks) { + return -1; + } + SDL_free(joysticks); + return count; +#else + return SDL_NumJoysticks(); +#endif +} static PyObject * controller_new(PyTypeObject *subtype, PyObject *args, PyObject *kwargs) { // ... CONTROLLER_INIT_CHECK(); -#if SDL_VERSION_ATLEAST(3, 0, 0) - int joycount; - SDL_JoystickID *joysticks = SDL_GetJoysticks(&joycount); - if (!joysticks) { - return RAISE(pgExc_SDLError, SDL_GetError()); - } - SDL_free(joysticks); -#else - int joycount = SDL_NumJoysticks(); -#endif + int joycount = get_joystick_count(); + if (joycount < 0) { + return RAISE(pgExc_SDLError, SDL_GetError()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src_c/_sdl2/controller.c(6 hunks)src_c/_sdl2/meson.build(3 hunks)src_c/_sdl2/touch.c(3 hunks)src_c/cython/pygame/_sdl2/sdl2.pxd(1 hunks)src_c/meson.build(0 hunks)src_py/_sdl2/__init__.py(1 hunks)
💤 Files with no reviewable changes (1)
- src_c/meson.build
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: arm64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: dev-check
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: build (windows-latest)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: AMD64
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
🔇 Additional comments (14)
src_c/_sdl2/meson.build (2)
3-41: SDL3 conditional exclusion looks good.The SDL2-specific modules (audio, video, controller_old, and mixer) are correctly excluded from SDL3 builds. The nested condition for SDL_mixer is properly preserved.
52-52: Comment correctly pluralized.The change from "This is not a cython file" to "These are not cython files" correctly reflects that both touch and controller modules are included.
src_py/_sdl2/__init__.py (2)
1-2: Good addition of SDL version export.Re-exporting SDL from pygame.version is appropriate for version-based conditional logic.
4-10: SDL3 compatibility through conditional imports.The gating of audio and video module imports based on SDL version aligns with the SDL3 porting strategy. The sdl2 module remains unconditionally imported as expected.
src_c/cython/pygame/_sdl2/sdl2.pxd (1)
7-17: SDL3 header compatibility properly implemented.The embedded C header correctly includes SDL3/SDL.h when PG_SDL3 is defined and provides stub definitions for SDL_INIT_* macros that don't exist in SDL3. This ensures backward compatibility for code using these constants.
src_c/_sdl2/controller.c (5)
33-37: SDL3 gamepad events properly enabled.The use of
SDL_SetGamepadEventsEnabled(SDL_TRUE)for SDL3 andSDL_GameControllerEventState(SDL_ENABLE)for SDL2 correctly handles the API differences.
306-310: SDL3 GUID formatting correctly handled.The use of
SDL_GUIDToStringfor SDL3 andSDL_JoystickGetGUIDStringfor SDL2 properly handles the API differences.
391-401: SDL3 rumble API changes correctly handled.The SDL3 version returns
SDL_booldirectly while SDL2 returns 0 on success. The conversion to Python bool is handled appropriately for both cases.
429-443: SDL3 LED control API changes handled correctly.The function rename from
SDL_GameControllerSetLEDtoSDL_SetGamepadLEDand the different return value semantics (SDL3 returns bool, SDL2 returns int) are properly handled. The SDL error clearing for SDL3 when the device lacks an addressable LED is a thoughtful touch.
94-103: Potential memory leak if count is negative.While SDL3's
SDL_GetJoysticksis unlikely to return a negative count with a non-NULL pointer, the error path at line 104-106 doesn't free the allocated joysticks array.Apply this diff to fix the potential memory leak:
#if SDL_VERSION_ATLEAST(3, 0, 0) int count; SDL_JoystickID *joysticks = SDL_GetJoysticks(&count); if (!joysticks) { return RAISE(pgExc_SDLError, SDL_GetError()); } - SDL_free(joysticks); -#else - int count = SDL_NumJoysticks(); -#endif if (count < 0) { + SDL_free(joysticks); return RAISE(pgExc_SDLError, SDL_GetError()); } + SDL_free(joysticks); +#else + int count = SDL_NumJoysticks(); + if (count < 0) { + return RAISE(pgExc_SDLError, SDL_GetError()); + } +#endifLikely an incorrect or invalid review comment.
src_c/_sdl2/touch.c (4)
28-39: SDL3 touch device enumeration correctly handled.The API change from
SDL_GetNumTouchDevices()toSDL_GetTouchDevices(&count)is properly handled with appropriate memory management.
43-75: SDL3 touch device retrieval with proper bounds checking.The function signature change and SDL3 API usage are correct. The bounds checking for SDL3 and proper memory management with
SDL_freeare well implemented.
92-103: SDL3 finger counting API correctly handled.The change from
SDL_GetNumTouchFingerstoSDL_GetTouchFingerswith proper NULL checking and memory management is correct.
137-153: SDL3 finger retrieval with bounds checking.The SDL3 API change for getting touch fingers is properly handled with appropriate bounds checking and memory management.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src_c/_sdl2/touch.c (1)
86-89: Good fix: switched device_id to SDL_TouchID.This resolves the earlier type-size truncation concern around PyLong_AsLongLong and aligns with SDL3’s Uint64 TouchID.
🧹 Nitpick comments (2)
src_c/_sdl2/touch.c (2)
43-69: Consider unifying out-of-range error semantics across SDL2/SDL3.In the SDL3 branch, out-of-bounds raises IndexError; in the SDL2 branch, an invalid index ultimately raises an SDLError (via touchid==0). For a stable Python API, pick one error type and keep it consistent across SDL versions. If preserving backward compatibility is the priority, prefer SDLError in both paths; if “Pythonic” API is preferred, migrate both to IndexError.
Do you want a patch that makes both branches raise SDLError (backward compatible), or a broader change to migrate SDL2 path to IndexError?
143-146: Optional: match SDL2 behavior for “finger not present”.In the SDL2 path, a missing finger returns None; in the SDL3 path we currently raise IndexError when index is out of bounds. If preserving public API behavior across SDL versions is desired, consider returning None here as well. If you want IndexError, consider migrating the SDL2 path accordingly and updating docs.
Apply this minimal change (if choosing to match SDL2/old behavior):
- if (index < 0 || index >= fingercount) { - SDL_free(fingers); - return RAISE(PyExc_IndexError, "index is out of bounds"); - } + if (index < 0 || index >= fingercount) { + SDL_free(fingers); + Py_RETURN_NONE; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src_c/_sdl2/touch.c(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: dev-check
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: AMD64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: x86
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
🔇 Additional comments (2)
src_c/_sdl2/touch.c (2)
28-39: LGTM: SDL3 device enumeration path and cleanup look correct.Count is obtained via SDL_GetTouchDevices, error handled, and memory freed. Return value uses a Python int; no issues here.
155-159: LGTM: defensive NULL-check before populating the dict.Early exit on allocation failure is clean.
| fingercount = SDL_GetNumTouchFingers(device_id); | ||
| if (fingercount == 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Do not treat “0 fingers” as an error in the SDL2 path.
SDL_GetNumTouchFingers returns a negative value on error; 0 is a valid “no active fingers” state. Current code raises SDLError when fingercount == 0, which is incorrect and will spuriously error on valid states. Fix by checking for < 0 instead of == 0. See SDL2 docs.
Apply this diff:
#else
- fingercount = SDL_GetNumTouchFingers(device_id);
- if (fingercount == 0) {
- return RAISE(pgExc_SDLError, SDL_GetError());
- }
+ fingercount = SDL_GetNumTouchFingers(device_id);
+ if (fingercount < 0) {
+ return RAISE(pgExc_SDLError, SDL_GetError());
+ }
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fingercount = SDL_GetNumTouchFingers(device_id); | |
| if (fingercount == 0) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif | |
| #else | |
| fingercount = SDL_GetNumTouchFingers(device_id); | |
| - if (fingercount == 0) { | |
| - return RAISE(pgExc_SDLError, SDL_GetError()); | |
| if (fingercount < 0) { | |
| return RAISE(pgExc_SDLError, SDL_GetError()); | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In src_c/_sdl2/touch.c around lines 99 to 103, the code incorrectly treats a
fingercount of 0 as an SDL error; change the check to treat only negative return
values as errors by testing for fingercount < 0 and calling
RAISE(pgExc_SDLError, SDL_GetError()) only in that case, allowing fingercount ==
0 to be treated as a valid "no active fingers" state.
| VIDEO_INIT_CHECK(); | ||
|
|
||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| int fingercount; | ||
| SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount); | ||
| if (fingers == NULL) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| if (index < 0 || index >= fingercount) { | ||
| SDL_free(fingers); | ||
| return RAISE(PyExc_IndexError, "index is out of bounds"); | ||
| } | ||
| finger = fingers[index]; | ||
| SDL_free(fingers); | ||
| #else |
There was a problem hiding this comment.
Fix use-after-free in pg_touch_get_finger (SDL3 path).
SDL_GetTouchFingers returns a single allocation (array + finger data) that must be freed with SDL_free; finger pointers become invalid after free. The code currently assigns finger = fingers[index], calls SDL_free(fingers), and then dereferences finger later to build the dict — this is a UAF bug. Copy the fields you need before freeing, and then build the Python dict from those copies. See SDL3 docs: returned array is a single allocation to free once.
Apply this diff:
#if SDL_VERSION_ATLEAST(3, 0, 0)
int fingercount;
SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount);
if (fingers == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
if (index < 0 || index >= fingercount) {
SDL_free(fingers);
return RAISE(PyExc_IndexError, "index is out of bounds");
}
- finger = fingers[index];
- SDL_free(fingers);
+ finger = fingers[index];
+ SDL_FingerID fid = finger->id;
+ float fx = finger->x;
+ float fy = finger->y;
+ float fpressure = finger->pressure;
+ SDL_free(fingers);
#else
- if (!(finger = SDL_GetTouchFinger(touchid, index))) {
+ if (!(finger = SDL_GetTouchFinger(touchid, index))) {
Py_RETURN_NONE;
}
+ SDL_FingerID fid = finger->id;
+ float fx = finger->x;
+ float fy = finger->y;
+ float fpressure = finger->pressure;
#endif
@@
- _pg_insobj(fingerobj, "id", PyLong_FromLongLong(finger->id));
- _pg_insobj(fingerobj, "x", PyFloat_FromDouble(finger->x));
- _pg_insobj(fingerobj, "y", PyFloat_FromDouble(finger->y));
- _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(finger->pressure));
+ _pg_insobj(fingerobj, "id", PyLong_FromLongLong(fid));
+ _pg_insobj(fingerobj, "x", PyFloat_FromDouble(fx));
+ _pg_insobj(fingerobj, "y", PyFloat_FromDouble(fy));
+ _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(fpressure));Also applies to: 160-163
I have started working on compiling
pygame._sdl2with SDL3. This PR is draft because I'd like to port touch and audio, I'm creating it early to get feedback and opinions, especially for the following submodules:Do you agree?
(also I want to see what the checks have to say about my changes)
EDIT: I compiled and ported controller and touch. I have made small edits to prepare audio, which I will do in the next PR, so this one is easier to approve.
Summary by CodeRabbit
New Features
Bug Fixes
Chores