SDL3 runtime fixes from Ankith's PRs #3661
Conversation
📝 WalkthroughWalkthroughThis PR refactors SDL3 compatibility across multiple modules, introducing wrapper functions for consistent boolean return values (PG_GL_SetSwapInterval, PG_BlitSurface, PG_SetWindowFullscreen), unifies palette access via PG_GetSurfacePalette, updates display and window fullscreen handling logic, and corrects documentation for pgSurface_Blit return values. Test conditionals skip SDL version ranges where RLE handling was temporarily broken. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 (beta)
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
📒 Files selected for processing (9)
docs/reST/c_api/surface.rst(1 hunks)src_c/_pygame.h(2 hunks)src_c/display.c(25 hunks)src_c/font.c(1 hunks)src_c/pixelcopy.c(1 hunks)src_c/surface.c(5 hunks)src_c/surface_fill.c(1 hunks)src_c/window.c(2 hunks)test/surface_test.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 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.
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 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
Repo: pygame-community/pygame-ce PR: 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.
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 0
File: :0-0
Timestamp: 2025-11-03T19:49:28.177Z
Learning: For translation pull requests in the pygame-ce repository: Focus only on verifying direct translation accuracy from the English source. Do not suggest new content or enhancements during translation PRs. New content should be added to the English version first in a separate pull request.
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
📚 Learning: 2025-08-30T21:11:00.240Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 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/window.csrc_c/display.csrc_c/pixelcopy.ctest/surface_test.pysrc_c/surface.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 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/window.csrc_c/surface_fill.cdocs/reST/c_api/surface.rstsrc_c/_pygame.hsrc_c/display.csrc_c/pixelcopy.csrc_c/font.ctest/surface_test.pysrc_c/surface.c
📚 Learning: 2025-09-01T20:22:31.010Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 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:
docs/reST/c_api/surface.rstsrc_c/_pygame.hsrc_c/display.csrc_c/pixelcopy.csrc_c/font.csrc_c/surface.c
📚 Learning: 2025-10-17T08:53:32.449Z
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3219
File: src_c/window.c:11-13
Timestamp: 2025-10-17T08:53:32.449Z
Learning: SDL3's first stable release is version 3.2.0 (released January 21, 2025). SDL 3.0.x and 3.1.x were preview/pre-release versions not intended for production use, so version checks for SDL3 features in pygame-ce should use SDL_VERSION_ATLEAST(3, 2, 0) to target the first stable release.
Applied to files:
src_c/display.c
🧬 Code graph analysis (4)
src_c/display.c (2)
src_c/base.c (1)
pg_GetDefaultWindow(1925-1929)src_c/_pygame.h (1)
PG_GL_SetSwapInterval(406-410)
src_c/pixelcopy.c (1)
src_c/_pygame.h (1)
PG_GetSurfacePalette(273-277)
src_c/font.c (1)
src_c/_pygame.h (1)
PG_GetSurfacePalette(273-277)
src_c/surface.c (3)
src_c/base.c (1)
pg_EnvShouldBlendAlphaSDL2(1978-1982)src_c/transform.c (1)
_PgSurface_SrcAlpha(94-116)src_c/alphablit.c (1)
pygame_Blit(1582-1687)
⏰ 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). (19)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (windows-latest)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.10.17)
🔇 Additional comments (26)
src_c/surface_fill.c (1)
1145-1149: Good: avoid early-return so surface unlock path is preserved.
Setting the SDL error +result = -1and breaking keeps cleanup consistent and prevents lock leaks.src_c/font.c (1)
718-726: Looks good; same SDL3 palette-access verification as pixelcopy.c.
The null-check is correct and preserves behavior. Just ensurePG_GetSurfacePalette()is the right abstraction for SDL3 builds.src_c/pixelcopy.c (1)
1195-1210: No action needed. PG_GetSurfacePalette is correctly implemented with proper SDL2/SDL3 handling: it resolves to SDL_GetSurfacePalette for SDL3 builds (line 162) and provides a static inline wrapper returning surf->format->palette for SDL2 (lines 273-277). The call at line 1200 is safe and appropriately guarded by the SDL_ISPIXELFORMAT_INDEXED check.src_c/window.c (2)
304-312: SDL3/SDL2 return value handling looks correct.The SDL3 path correctly checks
!SDL_SetWindowFullscreen(...)since SDL3 returnstrueon success andfalseon failure, while SDL2 returns0on success and non-zero on failure. This follows the same pattern used elsewhere in the codebase for SDL3 compatibility.
380-380: Type change fromSDL_booltointis appropriate.Using
intis correct here becausePyArg_ParseTupleAndKeywordswith the"p"format specifier expects anint*pointer, notSDL_bool*. This avoids potential type mismatch issues.src_c/_pygame.h (2)
195-196: SDL3 macro alias is correct.In SDL3,
SDL_GL_SetSwapIntervalalready returnsbool(true on success), so a direct alias works correctly.
405-410: SDL2 wrapper correctly converts return semantics to bool.The wrapper correctly converts SDL2's
SDL_GL_SetSwapIntervalreturn value (0 on success, -1 on failure) to a bool (true on success, false on failure), providing a consistent interface with SDL3.src_c/surface.c (7)
1547-1554: Formatting change in RLE stripping hack looks fine.The logic remains the same; only minor formatting adjustments were made to the SDL_Rect initialization.
4450-4518: PG_BlitSurface wrapper correctly handles SDL3/SDL2 differences.This wrapper addresses SDL3's change where
SDL_BlitSurfaceno longer modifies the destination rectangle. The SDL3 path:
- Performs manual rectangle clipping similar to SDL2's internal logic
- Updates
dstrectwith the clipped result- Returns 0 on success, -1 on failure (SDL2-compatible semantics)
The return value conversion at line 4508 (
SDL_BlitSurface(...) ? 0 : -1) correctly maps SDL3's bool return (true=success) to SDL2-style int return (0=success).
4532-4535: Error return value change is correct.The
pgSurface_Blitfunction returns 0 on success and non-zero on error (as seen at line 4661:return result != 0;). Changing the early error return fromreturn 0;(which would incorrectly indicate success) toreturn 1;correctly signals an error condition.
4594-4621: SDL3 surface format conversion path looks correct.The SDL3-specific code properly:
- Gets pixel format details using
SDL_GetPixelFormatDetails- Creates a new format without alpha mask using
SDL_GetPixelFormatForMasks- Converts the surface and performs the blit via
PG_BlitSurface- Handles the NULL cases appropriately
The refactored structure with unified error handling (
result = -1whensrcis NULL) is cleaner than the previous SDL2-only code.
4632-4632: SDL_MUSTLOCK condition for RLE check.This condition checks
!SDL_MUSTLOCK(src) && !SDL_MUSTLOCK(dst)to avoid the pygame_Blit path when surfaces have RLE. As noted earlier, SDL3'sSDL_MUSTLOCKchecksSDL_SURFACE_LOCKEDrather than RLE status, which may affect this logic on SDL3. The behavior should be verified with SDL3.Based on learnings, Starbuck5 prefers maintaining exact existing behaviors during SDL2 to SDL3 compatibility changes.
4640-4640: Using PG_BlitSurface wrapper is correct.This correctly uses the new wrapper function that handles SDL3's different behavior where
dstrectis not modified bySDL_BlitSurface.
3010-3018: UsePG_SurfaceHasRLEinstead ofSDL_MUSTLOCKto detect actual RLE status on SDL3.The code at line 3016 uses
SDL_MUSTLOCK(surf)to determine if RLE is actually enabled, but on SDL3 this macro checks theSDL_SURFACE_LOCKEDflag (surface lock state), not RLE acceleration status. Since line 3011 already usesPG_SurfaceHasRLEwhich properly abstracts SDL2/SDL3 differences, line 3016 should use the same abstraction rather than relying onSDL_MUSTLOCKfor RLE detection.src_c/display.c (12)
282-286: LGTM! Improved defensive programming.The explicit null check with early return and clear variable declarations improve code clarity and maintainability.
828-836: Consistent with SDL_GL_SetAttribute - verify return semantics.This follows the same boolean return pattern as
SDL_GL_SetAttribute. The verification request in the previous comment applies here as well.
1212-1212: LGTM! Appropriate fullscreen mode selection.The
non_desktop_fullscreenflag correctly defaults to exclusive fullscreen (value 1) and switches to desktop/borderless fullscreen (value 0) when usingSCALEDmode or when window dimensions match the desktop resolution. This aligns with the expected behavior of avoiding physical resolution changes when possible.Also applies to: 1337-1337, 1347-1347
1466-1475: LGTM! Correct error handling for both SDL versions.The error handling correctly adapts to SDL3's boolean return (checking
!result) and SDL2's integer return (checking!= 0). Both paths properly raise exceptions on failure.
1564-1568: LGTM! Correct usage of updated PG_SetWindowFullscreen API.The call correctly treats
PG_SetWindowFullscreenas returning a boolean and checks for failure with the!operator. Error handling is appropriate.
1626-1646: PG_GL_SetSwapInterval usage mostly correct, verify line 1645 intent.Lines 1626 and 1636 correctly check for failure when enabling vsync. However, line 1645 calls
PG_GL_SetSwapInterval(0)to disable vsync without checking the return value.Question: Is it intentional to ignore errors when disabling vsync (line 1645)? While this might be acceptable as a best-effort operation, it's inconsistent with the error handling on lines 1626 and 1636.
1720-1725: LGTM! Proper vsync validation.The condition correctly validates that when vsync is requested, it was successfully enabled. This ensures the user's explicit vsync requirement is met or an error is raised.
3192-3193: LGTM! Consistent PG_SetWindowFullscreen usage throughout toggle_fullscreen.All 11 calls to
PG_SetWindowFullscreenconsistently:
- Check for boolean failure with the
!operator- Raise appropriate errors on failure
- Use correct parameters for toggling fullscreen on/off and choosing desktop vs. non-desktop modes
The usage is correct and uniform across all code paths.
Also applies to: 3206-3207, 3246-3247, 3282-3283, 3315-3316, 3350-3351, 3355-3356, 3388-3389, 3413-3414, 3430-3431, 3442-3443
3456-3456: Verify intentional behavior change: unconditional success return.The function now unconditionally returns
PyLong_FromLong(1)at the end, indicating success whenever this line is reached. This represents a behavioral change from potential intermediate failure states.Observation: All error paths either raise exceptions (returning NULL) or return -1 early. If execution reaches line 3456, the operation has succeeded.
Question: Is this change intentional and properly documented? Previously, the function may have returned different success codes (0 vs 1) to indicate partial success or specific outcomes.
Based on learnings, as per Starbuck5's preference for maintaining exact behaviors during SDL compatibility changes, please verify this behavioral change was intentional.
1135-1186: LGTM! Good API unification across SDL versions.The function signature change to
static boolprovides consistent return semantics across SDL2 and SDL3. The SDL2 wrapper correctly converts the integer return (== 0) to boolean, and the SDL3 implementation properly handles the boolean return fromSDL_SetWindowFullscreen.
808-816: No action needed. The code correctly implements SDL3 compatibility:SDL_GL_SetAttributereturnsboolin SDL3 (true on success, false on failure), and the conditionif (!SDL_GL_SetAttribute(flag, value))properly handles the false return value by raising an error.
3766-3770: LGTM! Correct SDL_ShowMessageBox error handling for both versions.The error handling correctly adapts to SDL3's boolean return (checking
!resultfor false on line 3767) and SDL2's integer return (checkingif (result)for non-zero error code on line 3769). SDL3 returnstrueon success andfalseon failure, while SDL2 returns0on success and a negative error code on failure. Both code paths properly raise exceptions on error.
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
Fair enough, these changes are relatively safer to merge and I would also like to get them in earlier.
Feels weird to approve my own work but this approval is really yours 😉
I will merge this in now and rebase my own PRs on top of the new main
2 participants
Ankith has had 2 PRs stalled for a while because he and I are currently not in agreement on some of the details, like window syncing and how to create palettes. #3577 and #3578.
In this PR I took his changes and removed some things I didn't have the time to review, didn't understand, or disagreed with, in an effort to keep moving this effort forward. So this PR is basically an approval for some bits of the code.
I specifically edited the changes so that Ankith remains the primary author in git.