Port display.c to SDL3
#3428
Port display.c to SDL3
#3428
Conversation
ankith26
force-pushed
the
ankith26-display-sdl3
branch
4 times, most recently
from
7006314 to
6b66f16
Compare
|
This PR should be done from my side. Given that this is a kinda big PR to review, I have already organized everything into commits that take up stuff one at a time and focus on it fully. So, if it helps reviewing I can split this up into multiple PRs each with subsets of commits from here. |
| @@ -331,6 +331,10 @@ pg_window_set_fullscreen(SDL_Window *window, int desktop) | |||
| goto end; | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Basically while working on fullscreen changes in display I saw that SDL_SetWindowFullscreenMode in SDL3 has effect only if the window is already in fullscreen mode according to the docs. So I added a call here as well that does that first.
ankith26
force-pushed
the
ankith26-display-sdl3
branch
from
6b66f16 to
efb4dd4
Compare
ankith26
force-pushed
the
ankith26-display-sdl3
branch
from
efb4dd4 to
1ab8b97
Compare
| // could also take the size of the old display surface | ||
| SDL_GetWindowSize(win, &window_w, &window_h); | ||
| window_display = SDL_GetWindowDisplayIndex(win); | ||
| if (SDL_GetDesktopDisplayMode(window_display, &display_mode) != 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah it was just unused in this function. Just a relic of copy pasting stuff IG
There was a problem hiding this comment.
Thanks Ankith! I looked through the changes and it seems good.
This did take a significant amount of time to read through, maybe if some of the smaller commits were split into their own PRs they could be reviewed more quickly by others? Not sure the best move.
When I combine with my surface branch I'm unable to open a window, but this PR is for compiling and not necessarily for getting everything to run.
>>> screen = pygame.display.set_mode((500, 500))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
pygame.error: Invalid display
ankith26
force-pushed
the
ankith26-display-sdl3
branch
from
1ab8b97 to
e495f2c
Compare
|
I rebased this PR on main to get the latest surface compilation changes (hence the force push). While I was at it I also fixed a super minor return code related issue in an SDL3 specific part of this PR, and as a result *: With this PR we have some examples in a somewhat usable/runnable state on SDL3 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds SDL3 compatibility across display/window management alongside existing SDL2 paths, guarded by SDL version checks. Updates pixel format types under PG_SDL3, un-gates building the display module, and adjusts fullscreen logic to pre-enable fullscreen before setting mode. Gamma ramp APIs now error under SDL3. Public API signatures unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Display as Display module
participant SDL as SDL2/SDL3
App->>Display: set_mode()/resize/init
alt SDL3
Display->>SDL: SDL_GetDisplays / SDL_CreateWindowWithProperties
Display->>SDL: SDL_GetDesktopDisplayMode (const ptr)
Display->>SDL: SDL_SetRenderLogicalPresentation / flags
else SDL2
Display->>SDL: SDL_GetNumVideoDisplays / SDL_CreateWindow
Display->>SDL: SDL_GetDesktopDisplayMode (by value)
Display->>SDL: SDL_RenderSetIntegerScale
end
Display-->>App: window/display state
sequenceDiagram
participant Caller
participant Window as pg_window_set_fullscreen
participant SDL
Caller->>Window: request fullscreen (desktop/non-desktop)
Window->>SDL: SDL_SetWindowFullscreen(window, 1)
alt success
Window->>SDL: SDL_SetWindowFullscreenMode(window, mode)
Window-->>Caller: result
else failure
Window-->>Caller: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ 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
🧹 Nitpick comments (2)
src_c/window.c (1)
334-337: Verify error handling for fullscreen mode transitionThe function now calls
SDL_SetWindowFullscreen(window, 1)beforeSDL_SetWindowFullscreenMode. While the logic aligns with SDL3's requirements, the error handling could be improved for clarity.Apply this diff to improve error messaging:
- if (!SDL_SetWindowFullscreen(window, 1)) { - goto end; - } + if (!SDL_SetWindowFullscreen(window, 1)) { + SDL_SetError("Failed to enable fullscreen state before setting mode"); + goto end; + }src_c/display.c (1)
2977-3032: Creative solution for SDL3 window manager type detectionThe
get_syswm_type()function provides a workaround for the absence of SDL_SysWMinfo in SDL3 by detecting the video driver string. While functional, this approach is somewhat fragile.Consider adding a comment explaining why this approach is necessary and potentially logging a warning if an unknown driver is detected:
static SDL_SYSWM_TYPE get_syswm_type() { + /* SDL3 removed SDL_SysWMinfo, so we detect the window system + * by checking the video driver name. This is less reliable but + * currently the only available method. */ const char *driver = SDL_GetCurrentVideoDriver(); if (!driver) { return SDL_SYSWM_UNKNOWN; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src_c/display.c(71 hunks)src_c/include/_pygame.h(1 hunks)src_c/meson.build(0 hunks)src_c/window.c(1 hunks)
💤 Files with no reviewable changes (1)
- src_c/meson.build
🧰 Additional context used
🧬 Code Graph Analysis (1)
src_c/display.c (2)
src_c/base.c (1)
pg_GetDefaultWindow(2170-2174)src_c/_pygame.h (3)
PG_GetSurfaceFormat(128-132)PG_GetSurfaceFormat(235-239)PG_GetSurfacePalette(241-245)
🔇 Additional comments (17)
src_c/include/_pygame.h (1)
287-294: LGTM! Clean SDL3 compatibility for pg_VideoInfo structThe conditional compilation for SDL3 correctly updates the pixel format types from
SDL_PixelFormat*toSDL_PixelFormatDetails*, aligning with SDL3's new API.src_c/display.c (16)
34-36: LGTM! Proper SDL3 conditional exclusion of SDL_syswm.hThe header is correctly excluded for SDL3 builds since SDL_SysWMinfo is no longer available in SDL3.
867-883: LGTM! Well-designed integer scaling compatibility layerThe
PG_RenderSetIntegerScalefunction provides a clean abstraction over the differences between SDL2 and SDL3's rendering APIs. The SDL3 path correctly usesSDL_SetRenderLogicalPresentationwith appropriate presentation modes.
1052-1081: LGTM! Proper SDL3 display enumeration with memory managementThe SDL3 path correctly:
- Uses
SDL_GetDisplaysinstead ofSDL_GetNumVideoDisplays- Properly frees the allocated display array with
SDL_free- Uses SDL_DisplayID correctly for display identification
1122-1159: Well-structured fullscreen mode handling for SDL3The
PG_SetWindowFullscreenfunction properly implements SDL3's two-step fullscreen process:
- First calls
SDL_SetWindowFullscreento toggle fullscreen state- Then calls
SDL_SetWindowFullscreenModewith appropriate display mode- Properly handles memory cleanup with
SDL_free(modes)
1787-1790: LGTM! Correct SDL3 surface color mappingThe SDL3 path correctly uses
SDL_MapSurfaceRGBinstead ofSDL_MapRGB, properly adapting to SDL3's API changes.
2460-2477: SDL3 gamma ramp removal handled appropriatelyThe functions correctly return an error for SDL3 builds since gamma ramp functionality has been removed from SDL3. The error messages are clear and informative.
2385-2385: LGTM! Correct palette access using compatibility macroUses
PG_GetSurfacePalettemacro which provides proper abstraction for SDL2/SDL3 differences.
535-563: LGTM! Comprehensive SDL3 window properties accessThe Windows platform code correctly uses SDL3's new property system with
SDL_GetPointerPropertyto access window handles, device contexts, and instance pointers.
244-248: Proper SDL3 setenv handling for Windows compatibilityUses
SDL_setenv_unsafein SDL3 which is the appropriate replacement forSDL_setenv.
1642-1644: Verify SDL_CreateRenderer call for SDL3The SDL3 path correctly uses the simplified
SDL_CreateRenderer(win, NULL)API. The vsync is properly set afterwards withSDL_SetRenderVSync.
2714-2727: LGTM! Renderer info retrieval adapted for SDL3The function correctly handles the differences between SDL2's
SDL_RendererInfostruct and SDL3's simpler API that only provides the renderer name.
2809-2813: LGTM! Correct window flags handling for fullscreen detectionThe SDL3 path correctly uses
SDL_WINDOW_FULLSCREENflag alone, as SDL3 simplified the fullscreen flag system.
2864-2874: LGTM! Proper vsync detection for SDL3 OpenGL contextsThe SDL3 path correctly uses
SDL_GL_GetSwapIntervalwith an output parameter instead of a return value.
3664-3668: SDL3 message box button ID field name updatedCorrectly uses
buttonIDinstead ofbuttonidfor SDL3 compatibility.
1086-1118: LGTM! Clean window creation compatibility wrapperThe
PG_CreateWindowCompatfunction provides a good abstraction over SDL2'sSDL_CreateWindowand SDL3'sSDL_CreateWindowWithProperties, properly handling the property-based API in SDL3.
1283-1295: Const pointer usage in SDL3 display mode is consistent
All instances ofSDL_GetDesktopDisplayModein SDL3 code paths declare aconst SDL_DisplayMode*; the pattern is applied uniformly across the file. No changes required.
WIP to port
display.cto SDL3Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores