[Part 2] SDL3: display+window: runtime fixes #3577
Conversation
ankith26
commented
Sep 1, 2025
ankith26
commented
- display index compat code
- fullscreen compat code unified
- syncwindow calls so that changes take immediate affect like sdl2
📝 WalkthroughWalkthroughAdds an SDL3 boolean GL swap-interval wrapper, migrates display APIs to query the primary display, unifies fullscreen handling to a boolean-returning helper, and adds SDL_SyncWindow calls after many SDL3 window/display state changes. SDL2 paths remain guarded and unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Display as Display API
participant SDL as SDL (SDL3)
participant Win as SDL_Window
rect rgba(210,240,255,0.25)
note over App,Display: Toggle fullscreen / set mode (SDL3)
App->>Display: pg_toggle_fullscreen(...) / pg_set_mode(...)
Display->>SDL: PG_SetWindowFullscreen(Win, fullscreen, non_desktop)
SDL-->>Display: bool success/failure
alt success
Display->>SDL: SDL_SyncWindow(Win)
end
Display-->>App: result (bool)
end
rect rgba(220,255,220,0.2)
note over App,Display: VSync / swap interval (SDL3)
App->>Display: set_vsync(interval)
Display->>SDL: PG_GL_SetSwapInterval(interval)
SDL-->>Display: bool success/failure
Display-->>App: result (bool)
end
rect rgba(255,245,210,0.2)
note over App,Display: Query modes / video info (SDL3)
App->>Display: get_video_info() / list_modes()
Display->>SDL: SDL_GetPrimaryDisplay()
SDL-->>Display: primary_display
Display->>SDL: SDL_GetDisplayMode(primary_display) / SDL_GetCurrentDisplayMode(...)
SDL-->>Display: modes/info or error
Display-->>App: modes/info or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 1
📜 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 (3)
src_c/_pygame.h(2 hunks)src_c/display.c(31 hunks)src_c/window.c(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
📚 Learning: 2025-08-30T21:11:00.196Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.196Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Applied to files:
src_c/display.c
🧬 Code graph analysis (1)
src_c/display.c (2)
src_c/base.c (1)
pg_GetDefaultWindow(1916-1920)src_c/_pygame.h (1)
PG_GL_SetSwapInterval(384-388)
🔇 Additional comments (22)
src_c/window.c (4)
304-313: LGTM! Proper SDL3 boolean return handlingThe conversion to handle SDL3's boolean return value for
SDL_SetWindowFullscreenis correct, including the addition ofSDL_SyncWindowfor immediate effect.
349-349: Good addition of window sync for SDL3Adding
SDL_SyncWindowafter fullscreen setup ensures the changes take immediate effect, maintaining consistency with SDL2 behavior.
382-382: Correct type change for SDL3 compatibilityChanging
input_onlyfromSDL_booltointis appropriate since it's now used as a Python boolean viaPyArg_ParseTupleAndKeywords.
434-436: Consistent SDL_SyncWindow additionsAll the
SDL_SyncWindowcalls are properly guarded withSDL_VERSION_ATLEAST(3, 0, 0)and placed after window state changes. This ensures immediate synchronization with the windowing system in SDL3.Based on the learnings provided, this follows the preferred approach of maintaining exact existing behaviors for SDL2 to SDL3 compatibility changes.
Also applies to: 444-446, 454-456, 759-761, 814-816, 857-859, 891-893
src_c/display.c (18)
282-286: Proper null window handlingGood defensive programming - checking if the window exists before accessing its flags prevents potential crashes.
467-472: Correct primary display handling for SDL3The code properly handles SDL3's primary display API where display ID 0 has special meaning. The error checking when
SDL_GetPrimaryDisplay()returns 0 is essential.
813-821: SDL3 boolean return value handling looks correctThe GL attribute setter properly handles SDL3's boolean return convention.
833-841: GL getter boolean handling is correctThe GL attribute getter properly handles SDL3's boolean return convention.
1140-1192: Comprehensive fullscreen wrapper implementationThe
PG_SetWindowFullscreenfunction properly abstracts the differences between SDL2 and SDL3 fullscreen handling, including the boolean return values and window synchronization.
1270-1278: Primary display compatibility for SDL3Good implementation of backwards compatibility - SDL2's display index 0 is properly mapped to SDL3's primary display.
1483-1491: Display bounds API adaptationCorrectly handles the different return conventions between SDL2 and SDL3 for
SDL_GetDisplayUsableBounds.
1580-1584: Consistent use of PG_SetWindowFullscreen wrapperAll fullscreen state changes properly use the new wrapper function, maintaining consistency throughout the codebase.
1642-1661: Swap interval wrapper usageGood use of the
PG_GL_SetSwapIntervalwrapper to handle SDL3's boolean return convention for vsync control.
1899-1901: Window synchronization pointsAll SDL_SyncWindow calls are properly placed after window state changes and correctly guarded with SDL3 version checks.
Also applies to: 2000-2002, 2794-2796
2030-2038: Primary display handling in mode_okConsistent implementation of the primary display compatibility layer for SDL3.
2065-2073: Smart compatibility fallbackThe code cleverly uses the current display mode when the resolution matches, providing better SDL2 compatibility. This is a nice touch that maintains expected behavior.
2113-2121: Primary display handling in list_modesConsistent primary display compatibility implementation.
2133-2139: Proper bpp fallback using current modeGood use of current display mode for bits-per-pixel when not specified, maintaining SDL2 behavior.
2143-2157: Robust empty list handlingExcellent defensive programming - when SDL3 returns an empty display modes list (which SDL2 didn't), the code falls back to using the current mode. This maintains backwards compatibility.
3260-3527: Comprehensive fullscreen toggle implementationThe toggle_fullscreen function properly uses
PG_SetWindowFullscreenthroughout and addsSDL_SyncWindowat the end for SDL3. The implementation maintains compatibility across different rendering backends (GL, SDL_Renderer, software).
3605-3607: Display resize event synchronizationProper addition of
SDL_SyncWindowafter window size changes in the resize event handler.
3841-3844: Message box return value handlingCorrectly inverts the return check for SDL3's
SDL_ShowMessageBoxwhich returns true on success instead of 0.
I don't feel any particular need to add this, does this impact any of our unit tests? I don't think people are relying on the synchronicity. Plus when we're only on SDL3 I feel like we shouldn't have this. |
| if (0 != PG_SetWindowFullscreen( | ||
| win, sdl_flags & SDL_WINDOW_FULLSCREEN, | ||
| non_desktop_fullscreen)) { | ||
| if (!PG_SetWindowFullscreen(win, | ||
| sdl_flags & SDL_WINDOW_FULLSCREEN, | ||
| non_desktop_fullscreen)) { |
There was a problem hiding this comment.
This block and the SDL2 block below could be consolidated together with PG_SetWindowFullscreen.
There was a problem hiding this comment.
I think the current code is cleaner. Consolidating this is hard because non_desktop_fullscreen variable is only defined in the SDL3 path - and not on SDL2, whereas SDL_WINDOW_FULLSCREEN_DESKTOP is only defined on SDL2 and not SDL3. The consolidation would require refactoring to the SDL2 codepath, which I have tried to avoid to the best extent in my porting PRs.
Without doing this a lot of our tests fail. I feel like people may rely on synchronicity, especially given that without this call, SDL makes no guarantee how early or late the sync may actually happen (and it may also be a very platform dependent thing). I believe calling this explicitly keeps stuff more inter-platform consistent and is a good thing to do. |
ankith26
force-pushed
the
ankith26-sdl3-2
branch
from
9aeca32 to
943f46c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src_c/display.c (4)
915-923: Fix: early-return filters out the very window events you needAs written, the function returns immediately for window events, so resize/maximize/restore handling below never runs.
Apply:
-#if SDL_VERSION_ATLEAST(3, 0, 0) - if (event->type >= SDL_EVENT_WINDOW_FIRST && - event->type <= SDL_EVENT_WINDOW_LAST) -#else - if (event->type == SDL_WINDOWEVENT) -#endif - { - return 0; - } +#if SDL_VERSION_ATLEAST(3, 0, 0) + if (event->type < SDL_EVENT_WINDOW_FIRST || + event->type > SDL_EVENT_WINDOW_LAST) { + return 0; + } +#else + if (event->type != SDL_WINDOWEVENT) { + return 0; + } +#endif
1140-1180: Fullscreen mode selection: pass and validate count from SDL_GetFullscreenDisplayModesIndexing modes[0] without retrieving count risks OOB when zero modes are returned.
Apply:
- SDL_DisplayMode **modes = NULL; + SDL_DisplayMode **modes = NULL; + int nummodes = 0; @@ - modes = SDL_GetFullscreenDisplayModes(disp, NULL); - if (!modes) { + modes = SDL_GetFullscreenDisplayModes(disp, &nummodes); + if (!modes || nummodes <= 0) { goto end; } - chosen_mode = modes[0]; + chosen_mode = modes[0];
1734-1741: Fix: vsync check inverted for SDL3 rendererThis errors when vsync is actually enabled. Should error when vsync requested but not enabled.
Apply:
- if (vsync && has_vsync) { + if (vsync && !has_vsync) { PyErr_SetString(pgExc_SDLError, "could not enable vsync"); _display_state_cleanup(state); goto DESTROY_WINDOW; }
1884-1901: Use window flags, not surface flags, when checking fullscreenSDL_WINDOW_* flags are properties of SDL_Window; checking surface->surf->flags is incorrect. Fix the check in src_c/display.c:1884-1886:
-#if SDL_VERSION_ATLEAST(3, 0, 0) - ((surface->surf->flags & SDL_WINDOW_FULLSCREEN) != 0)) { -#else - ((surface->surf->flags & SDL_WINDOW_FULLSCREEN_DESKTOP) != 0)) { -#endif +#if SDL_VERSION_ATLEAST(3, 0, 0) + (SDL_GetWindowFlags(win) & SDL_WINDOW_FULLSCREEN)) { +#else + (SDL_GetWindowFlags(win) & + (SDL_WINDOW_FULLSCREEN | SDL_WINDOW_FULLSCREEN_DESKTOP))) { +#endif
🧹 Nitpick comments (2)
src_c/display.c (2)
2290-2294: Minor: ease debugging num displaysStoring the array before freeing clarifies intent and allows NULL checks.
- int ret = -1; - SDL_free(SDL_GetDisplays(&ret)); + int ret = -1; + SDL_DisplayID *disps = SDL_GetDisplays(&ret); + SDL_free(disps);
2133-2157: *list_modes: robust empty‑list fallback; prefer sizeof(modes) for clarity (optional nit)sizeof(SDL_DisplayMode **) is functionally equivalent here but confusing — use sizeof(*modes) to show you're allocating one element.
- modes = SDL_malloc(sizeof(SDL_DisplayMode **)); + modes = SDL_malloc(sizeof(*modes));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src_c/_pygame.h(2 hunks)src_c/display.c(31 hunks)src_c/window.c(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src_c/window.c
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
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.
📚 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.hsrc_c/display.c
📚 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.hsrc_c/display.c
📚 Learning: 2025-08-30T21:11:00.240Z
Learnt from: Starbuck5
PR: pygame-community/pygame-ce#3573
File: src_c/_camera.c:129-131
Timestamp: 2025-08-30T21:11:00.240Z
Learning: When doing SDL2 to SDL3 compatibility changes, Starbuck5 prefers to maintain exact existing behaviors even when other improvements could be made, focusing solely on the compatibility aspect rather than mixing in behavioral fixes.
Applied to files:
src_c/display.c
🧬 Code graph analysis (1)
src_c/display.c (2)
src_c/base.c (1)
pg_GetDefaultWindow(1916-1920)src_c/_pygame.h (1)
PG_GL_SetSwapInterval(384-388)
⏰ 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). (17)
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (15)
src_c/_pygame.h (2)
169-170: PG_GL_SetSwapInterval macro in SDL3 path is fineMapping directly to SDL3’s boolean-returning SDL_GL_SetSwapInterval matches the bool contract used by call sites.
383-388: Good bool wrapper for SDL2 path — callers verifiedrg shows only boolean-appropriate uses in src_c/display.c (lines 1642, 1652, 1661); no int-return semantics remain.
src_c/display.c (13)
282-287: get_active: uses window presence + flags correctlyChecks default window before querying flags; SDL3 logic (not hidden and not minimized) is correct.
467-472: Primary display fallbackUsing SDL_GetPrimaryDisplay when no window exists preserves SDL2’s “display 0 = primary” behavior.
805-817: gl_set_attribute: correct SDL3 boolean handlingRaising on false from SDL_GL_SetAttribute aligns with SDL3’s return semantics.
828-843: gl_get_attribute: correct SDL3 boolean handlingRaising on false from SDL_GL_GetAttribute is correct.
1483-1492: Usable bounds: correct SDL3 error checkReturning error when SDL_GetDisplayUsableBounds fails is correct; nice parity with SDL2 branch.
1580-1583: SDL3 fullscreen transition via helperSwitch to PG_SetWindowFullscreen keeps behavior unified across SDL2/SDL3.
1642-1662: OpenGL swap interval: consistent bool APIAdaptive/regular/off paths correctly use the new bool-returning PG_GL_SetSwapInterval.
1999-2003: Sync after window moveSDL_SyncWindow after SDL_SetWindowPosition matches the PR goal of stable, SDL2‑like immediacy.
2030-2038: mode_ok: display 0 → primary mappingGood SDL2 compatibility shim with proper error propagation if primary retrieval fails.
2794-2797: Sync after minimizeSDL_SyncWindow following SDL_MinimizeWindow improves cross‑platform consistency.
3525-3528: Sync after toggle_fullscreenMatches the PR’s stated objective; helps tests that assume synchronous state.
3606-3607: Sync after resize event pathGood for eliminating timing flakiness.
3841-3844: message_box: correct SDL3/SDL2 success conditionsCondition inversion between SDL3 (bool) and SDL2 (int) is handled correctly.
