[Part 1] SDL3: event: runtime issue fixes #3576
Conversation
📝 WalkthroughWalkthroughAdds an SDL3-aware gesture subsystem and integrates it into pygame’s event handling, updates SDL/version-gated event enums and constants, and adjusts event translation and auto-init/quit hooks for gesture support across SDL2/SDL3 paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Pygame as "Pygame Event Module"
participant SDL as "SDL Core"
participant Gesture as "Gesture Subsystem"
App->>Pygame: import/init
Pygame->>Gesture: Gesture_Init() [SDL3+]
Gesture-->>Pygame: initialized
note right of Pygame: event proxies & filters configured
App->>Pygame: quit
Pygame->>Gesture: Gesture_Quit()
Gesture-->>Pygame: cleaned up
sequenceDiagram
autonumber
actor TouchDevice
participant SDL as "SDL Event Loop"
participant Gesture as "Gesture Subsystem"
participant Pygame as "Pygame Event Module"
participant Py as "Python"
TouchDevice->>SDL: raw touch events
alt SDL3 with Gesture impl
SDL->>Gesture: feed raw touch data
Gesture-->>SDL: emit gesture events (MULTI/DOLLAR/RECORD)
end
SDL->>Pygame: SDL_* gesture / finger events
Pygame->>Pygame: translate to dict (SDL2/SDL3 branches)
Pygame-->>Py: post Python events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src_c/event.c (2)
493-506: Typo: _pg_event_psuedo_block → _pg_event_pseudo_blockMinor spelling fix improves searchability and consistency.
-static bool -_pg_event_psuedo_block(SDL_Event *event) +static bool +_pg_event_pseudo_block(SDL_Event *event) @@ - return true; + return true; @@ - return false; + return false;And update call sites:
- if (_pg_event_psuedo_block(event)) { + if (_pg_event_pseudo_block(event)) {(Apply in Line 514 and Line 748.)
818-822: Improve error reporting on Gesture_Init failureIf Gesture_Init doesn't set an SDL error, the raised message may be empty. Consider a fallback string.
- if (Gesture_Init() != 0) { - return RAISE(pgExc_SDLError, SDL_GetError()); - } + if (Gesture_Init() != 0) { + const char *err = SDL_GetError(); + return RAISE(pgExc_SDLError, (err && *err) ? err : "Gesture_Init failed"); + }src_c/include/SDL_gesture.h (1)
878-900: Consider cleaning up commented debug code.This section contains extensive commented-out debug code. Consider removing it entirely if it's no longer needed, or moving it to a debug-only block.
Either remove the commented code or wrap it in a debug macro:
- /* inTouch->gestureLast[j].dDist = dDist; - inTouch->gestureLast[j].dtheta = dtheta; - - printf("dDist = %f, dTheta = %f\n",dDist,dtheta); - gdtheta = gdtheta*.9 + dtheta*.1; - gdDist = gdDist*.9 + dDist*.1 - knob.r += dDist/numDownFingers; - knob.ang += dtheta; - printf("thetaSum = %f, distSum = %f\n",gdtheta,gdDist); - printf("id: %i dTheta = %f, dDist = %f\n",j,dtheta,dDist); */ +#ifdef DEBUG_GESTURES + /* Debug output for gesture processing */ + printf("dDist = %f, dTheta = %f\n", dDist, dtheta); +#endif
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (4)
src_c/_pygame.h(4 hunks)src_c/constants.c(2 hunks)src_c/event.c(7 hunks)src_c/include/SDL_gesture.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T06:44:48.360Z
Learnt from: ankith26
PR: pygame-community/pygame-ce#3427
File: src_c/_sdl2/touch.c:99-103
Timestamp: 2025-08-31T06:44:48.360Z
Learning: SDL_GetNumTouchFingers() returns 0 on failure, not as a valid "no fingers" state. A return value of 0 should be treated as an error condition and SDL_GetError() should be called to get the error message.
Applied to files:
src_c/include/SDL_gesture.h
🧬 Code graph analysis (1)
src_c/event.c (1)
src_c/include/SDL_gesture.h (3)
SDLCALL(204-209)Gesture_Quit(313-321)Gesture_Init(211-217)
⏰ 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: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: dev-check
- GitHub Check: x86_64
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (macos-14)
- GitHub Check: aarch64
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: i686
🔇 Additional comments (12)
src_c/constants.c (1)
263-263: Switch to SDL-native JOYBALLMOTION/MULTIGESTURE — verify SDL3 aliases existDEC_CONST maps to SDL_JOYBALLMOTION/SDL_MULTIGESTURE. Please confirm these are defined for SDL3 builds via pgcompat/pygame.h (or equivalent). Otherwise this will fail to compile on SDL3 and/or diverge from prior PG* placeholders. Also verify docs/tests no longer reference PG_JOYBALLMOTION/PG_MULTIGESTURE.
Also applies to: 305-305
src_c/event.c (5)
28-28: Single-TU gesture implementation — avoid duplicate definitionsDefining SDL_GESTURE_IMPLEMENTATION here compiles the implementation into this TU. Please verify no other TU defines it to prevent duplicate symbols at link time.
507-518: Event filter split looks goodReturn type gating for SDL2/SDL3 and delegating pseudo-block decisions is appropriate.
741-749: Correct: do not hard-block window events hereEarly return keeps window events visible to watchers; actual blocking is handled later.
783-786: Proper lifecycle: Gesture_Quit on AutoQuitGood pairing with init; avoids leaking the event watch/touch state.
1310-1341: Casting SDL_Event to Gesture_MultiGestureEvent risks UB; prefer copy/accessor**Strict-aliasing/ABI assumptions can break under optimization or platform ABIs. Please either memcpy into a local Gesture_MultiGestureEvent or add an accessor helper in SDL_gesture.h that fills an out struct. Add a size check, too.
Suggested patch for safer read (SDL3 branch):
- _pg_insobj(dict, "touch_id", - PyLong_FromLongLong( - ((Gesture_MultiGestureEvent *)event)->touchID)); - _pg_insobj( - dict, "x", - PyFloat_FromDouble(((Gesture_MultiGestureEvent *)event)->x)); - _pg_insobj( - dict, "y", - PyFloat_FromDouble(((Gesture_MultiGestureEvent *)event)->y)); - _pg_insobj(dict, "rotated", - PyFloat_FromDouble( - ((Gesture_MultiGestureEvent *)event)->dTheta)); - _pg_insobj(dict, "pinched", - PyFloat_FromDouble( - ((Gesture_MultiGestureEvent *)event)->dDist)); - _pg_insobj(dict, "num_fingers", - PyLong_FromLong( - ((Gesture_MultiGestureEvent *)event)->numFingers)); + Gesture_MultiGestureEvent ge; + memcpy(&ge, event, sizeof(ge)); + _pg_insobj(dict, "touch_id", PyLong_FromLongLong(ge.touchID)); + _pg_insobj(dict, "x", PyFloat_FromDouble(ge.x)); + _pg_insobj(dict, "y", PyFloat_FromDouble(ge.y)); + _pg_insobj(dict, "rotated", PyFloat_FromDouble(ge.dTheta)); + _pg_insobj(dict, "pinched", PyFloat_FromDouble(ge.dDist)); + _pg_insobj(dict, "num_fingers", PyLong_FromLong(ge.numFingers));Additionally, near the top of this file (or in SDL_gesture.h), add:
#if SDL_VERSION_ATLEAST(3, 0, 0) _Static_assert(sizeof(Gesture_MultiGestureEvent) <= sizeof(SDL_Event), "Gesture_MultiGestureEvent must fit into SDL_Event"); #endifsrc_c/_pygame.h (2)
53-59: SDL3 gesture aliases look goodIncluding SDL_gesture.h and aliasing SDL_* to GESTURE_* ensures the rest of the code can stay SDL2-style.
425-445: Verify WINDOWSIZECHANGED mapping semanticsPGE_WINDOWSIZECHANGED → SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED: confirm this matches prior behavior (logical vs pixel size). If callers expect logical size changes, consider documenting or mapping accordingly.
src_c/include/SDL_gesture.h (4)
1-24: LGTM! Proper licensing and context documentation.The file correctly includes the SDL license header and clearly documents that this is the SDL2 gesture implementation vendored for SDL3 compatibility.
872-876: Good defensive programming for division by zero.The code properly handles the edge case where
lDist == 0to avoid division by zero and resulting NaN values.
454-454: Verify SDL_TouchID signedness and use of sentinel
SDL_TouchID is defined in the external SDL2 headers, so please confirm whether it’s a signed type (e.g. Sint64) and adjust theif (touchID >= 0)check to explicitly compare against the proper invalid sentinel (negative) or use an SDL-defined invalid touch-ID constant rather than relying on signedness.
283-293: Confirm SDL_GetTouchDevices(NULL) API usage: Per SDL3 docs,SDL_GetTouchDevices(NULL)returns a 0-terminatedSDL_TouchIDarray (or NULL on failure), accepts a NULL count pointer, and must be freed withSDL_free(); the code correctly checks for NULL, iterates until the zero terminator, and frees the array (wiki.libsdl.org)
ankith26
force-pushed
the
ankith26-sdl3-1
branch
from
161264d to
da4db14
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src_c/include/SDL_gesture.h (2)
681-683: Check normalization result before recognition.If normalization fails (returns 0), recognition uses garbage points.
Apply:
- GestureDollarNormalize(path, points, false); + if (GestureDollarNormalize(path, points, false) == 0) { + return bestDiff; /* no valid recognition */ + }
493-500: Handle template add failures when broadcasting to all touches.Currently ignores
GestureAddDollarfailures;loaded++increments regardless.Apply:
- for (i = 0; i < GestureNumTouches; i++) { + int ok = 1; + for (i = 0; i < GestureNumTouches; i++) { touch = &GestureTouches[i]; - /* printf("Adding loaded gesture to + touches\n"); */ - /* TODO: What if this fails? */ - GestureAddDollar(touch, templ.path); + if (GestureAddDollar(touch, templ.path) < 0) { + ok = 0; + } } - loaded++; + if (ok) { + loaded++; + }src_c/_pygame.h (1)
425-446: SDL3 window event mapping: confirm semantics for “size changed”.
PGE_WINDOWSIZECHANGED = SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED—double-check this is the intended semantic (pixel-size vs logical size). If pygame expects logical size parity with SDL2’sSDL_WINDOWEVENT_SIZE_CHANGED, consider also mapping/logging logical changes.I can propose a small shim to coalesce both pixel/logical size changes if needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src_c/_pygame.h(4 hunks)src_c/constants.c(2 hunks)src_c/event.c(7 hunks)src_c/include/SDL_gesture.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src_c/event.c
- src_c/constants.c
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T06:44:48.383Z
Learnt from: ankith26
PR: pygame-community/pygame-ce#3427
File: src_c/_sdl2/touch.c:99-103
Timestamp: 2025-08-31T06:44:48.383Z
Learning: SDL_GetNumTouchFingers() returns 0 on failure, not as a valid "no fingers" state. A return value of 0 should be treated as an error condition and SDL_GetError() should be called to get the error message.
Applied to files:
src_c/include/SDL_gesture.h
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/_pygame.h
📚 Learning: 2025-09-01T20:22:31.010Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_pygame.h:115-126
Timestamp: 2025-09-01T20:22:31.010Z
Learning: In pygame-ce PR #3573, the PG_SURF_BitsPerPixel function implementation uses hardcoded return values of 32 for YUY2/UYVY/YVYU FOURCC formats because it's copying the exact logic from SDL's internal SDL_GetMasksForPixelFormat function. This ensures perfect compatibility between SDL2 and SDL3 behaviors rather than using the technically more accurate values that might be returned by SDL_GetPixelFormatDetails.
Applied to files:
src_c/_pygame.h
🧬 Code graph analysis (1)
src_c/include/SDL_gesture.h (1)
src_c/event.c (1)
event(2599-2670)
🔇 Additional comments (6)
src_c/include/SDL_gesture.h (5)
142-144: Doc: fix wrong\sareferences (use Gesture_ names).*The
\satags still reference SDL_* symbols; these should point to the public Gesture_* API.Apply:
- * \sa SDL_LoadDollarTemplates - * \sa SDL_SaveAllDollarTemplates + * \sa Gesture_LoadDollarTemplates + * \sa Gesture_SaveAllDollarTemplates
158-160: Doc: fix wrong\sareferences (use Gesture_ names).*Same issue here.
Apply:
- * \sa SDL_SaveAllDollarTemplates - * \sa SDL_SaveDollarTemplate + * \sa Gesture_SaveAllDollarTemplates + * \sa Gesture_SaveDollarTemplate
283-293: Verify SDL3 API assumption forSDL_GetTouchDevices(NULL).This code assumes a 0-terminated array when
num_devicesis NULL. Please confirm against the SDL3 API you’re building with.Would you like a quick script to grep the SDL headers in your build env to double‑check the signature?
295-307: Critical: “record all” logic inverted and persistent flag never reset.
- Code records-all for any nonzero
touchID(so a specific device also records-all).GestureRecordAllis never cleared after recording completes.Apply:
- if (touchID != 0) { - GestureRecordAll = true; /* !!! FIXME: this is never set back to false - anywhere, that's probably a bug. */ + if (touchID == (SDL_TouchID)-1) { /* -1 means all devices */ + GestureRecordAll = true; for (i = 0; i < GestureNumTouches; i++) { GestureTouches[i].recording = true; } } else { - GestureTouch *touch = GestureGetTouch(touchID); + GestureRecordAll = false; + GestureTouch *touch = GestureGetTouch(touchID); if (!touch) { return 0; /* bogus touchid */ } touch->recording = true; }And reset the flag when finishing a record-all (see FINGER_UP handler below).
782-787: ResetGestureRecordAllafter finishing a record-all session.Without this, subsequent recordings unintentionally apply to all devices.
Apply:
if (GestureRecordAll) { index = GestureAddDollar(NULL, path); for (i = 0; i < GestureNumTouches; i++) { GestureTouches[i].recording = false; } + GestureRecordAll = false; }src_c/_pygame.h (1)
54-59: Sanity-check: gesture macro re-aliasing on SDL3.Including
include/SDL_gesture.hand mappingSDL_*gesture constants toGESTURE_*looks fine; confirm there are no conflicting system SDL headers that also define these in your build matrix.
SDL_gesture.h