-
-
Notifications
You must be signed in to change notification settings - Fork 230
[Part 2] SDL3: display+window: runtime fixes #3577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,19 @@ _display_state_cleanup(_DisplayState *state) | |
| #endif | ||
| } | ||
|
|
||
| /* This macro defines the pixel format we use for pg_renderer. | ||
| * On our SDL2 code we used to use SDL_PIXELFORMAT_ARGB8888 but on SDL3 it | ||
| * gives incorrect results because it includes alpha when we don't need it, so | ||
| * we use the XRGB variant. | ||
| * TODO: can consider using XRGB unconditionally on both SDL2 and SDL3 | ||
| * codepaths | ||
| */ | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| #define PG_TEXTURE_FMT SDL_PIXELFORMAT_XRGB8888 | ||
| #else | ||
| #define PG_TEXTURE_FMT SDL_PIXELFORMAT_ARGB8888 | ||
| #endif | ||
|
|
||
| // prevent this code block from being linked twice | ||
| // (this code block is copied by window.c) | ||
| #ifndef BUILD_STATIC | ||
|
|
@@ -464,7 +477,12 @@ pg_GetVideoInfo(pg_VideoInfo *info) | |
| } | ||
| else { | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| if ((mode_ptr = SDL_GetCurrentDisplayMode(0))) { | ||
| SDL_DisplayID primary_display = SDL_GetPrimaryDisplay(); | ||
| if (primary_display == 0) { | ||
| PyErr_SetString(pgExc_SDLError, SDL_GetError()); | ||
| return (pg_VideoInfo *)NULL; | ||
| } | ||
| if ((mode_ptr = SDL_GetCurrentDisplayMode(primary_display))) { | ||
| info->current_w = mode_ptr->w; | ||
| info->current_h = mode_ptr->h; | ||
| formatenum = mode_ptr->format; | ||
|
|
@@ -942,9 +960,8 @@ pg_ResizeEventWatch(void *userdata, SDL_Event *event) | |
|
|
||
| SDL_DestroyTexture(pg_texture); | ||
|
|
||
| pg_texture = | ||
| SDL_CreateTexture(pg_renderer, SDL_PIXELFORMAT_ARGB8888, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| pg_texture = SDL_CreateTexture(pg_renderer, PG_TEXTURE_FMT, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
@@ -1166,6 +1183,7 @@ PG_SetWindowFullscreen(SDL_Window *window, bool fullscreen, | |
| } | ||
| } | ||
|
|
||
| SDL_SyncWindow(window); | ||
| ret = true; | ||
| end: | ||
| SDL_free(modes); | ||
|
|
@@ -1261,6 +1279,16 @@ pg_set_mode(PyObject *self, PyObject *arg, PyObject *kwds) | |
| } | ||
| } | ||
|
|
||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| /* In SDL2, display == 0 meant primary display, so compat code for it */ | ||
| if (display == 0) { | ||
| display = SDL_GetPrimaryDisplay(); | ||
| if (display == 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| } | ||
| #endif | ||
|
Comment on lines
+1282
to
+1290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Short answer: SDL display IDs are unique instance IDs for the lifetime of the application but not a guaranteed dense/sequential index. They are stable while the display remains connected (and never reused during the app run), but they can change if a display is disconnected/reconnected or between separate program launches. The SDL docs do not promise any particular ordering for the array returned by SDL_GetDisplays. References: [1], [2]. Sources
[1] SDL_DisplayID docs. 🏁 Script executed: # First, let's understand the context - search for display-related code in the file
cd src_c && grep -n "display" display.c | head -30Repository: pygame-community/pygame-ce Length of output: 1506 🏁 Script executed: # Check for display parameter documentation in pygame-ce
rg -n "display.*=" src_c/display.c -B 2 -A 2 | head -50Repository: pygame-community/pygame-ce Length of output: 2058 🏁 Script executed: # Look for SDL2 display index handling to understand the original behavior
rg -n "SDL2.*display" src_c/ -i --type cRepository: pygame-community/pygame-ce Length of output: 363 🏁 Script executed: # Check if there's documentation about display parameters in the codebase
rg -n "display.*index\|display.*id" src_c/ --type c -i | head -20Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # View the full function context around lines 1282-1290
sed -n '1270,1310p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 1270 🏁 Script executed: # Find the function name and full implementation
sed -n '1250,1320p' src_c/display.c | cat -nRepository: pygame-community/pygame-ce Length of output: 2640 🏁 Script executed: # Search for other display parameter handling in display.c to see if indices are converted
rg -n "display_id\|SDL_GetDisplays\|display.*index" src_c/display.c -A 3 -B 1Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Check if there's any documentation about display parameter semantics in pygame
rg -n "display.*parameter\|display.*index\|display.*id" src_c/doc/ --type c --type txt 2>/dev/null | head -20Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Look for function signatures that accept a display parameter to understand intent
ast-grep --pattern 'int.*display.*{
$$$
}'Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Find the function name containing line 1282
sed -n '1200,1290p' src_c/display.c | grep -n "^[a-zA-Z_].*(" | head -5Repository: pygame-community/pygame-ce Length of output: 130 🏁 Script executed: # Get function signature and parameters
sed -n '1240,1290p' src_c/display.c | head -60Repository: pygame-community/pygame-ce Length of output: 1400 🏁 Script executed: # Search for where display parameter comes from - look for function definition
rg -B 20 "In SDL2, display == 0 meant primary" src_c/display.c | head -40Repository: pygame-community/pygame-ce Length of output: 1071 🏁 Script executed: # Check if there's logic to convert display indices to IDs anywhere in display.c
rg -n "SDL_GetDisplays\|display_id\|displayIndex" src_c/display.cRepository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Look for all instances of "display" assignment to understand data flow
sed -n '1200,2200p' src_c/display.c | grep -n "display\s*=" | head -20Repository: pygame-community/pygame-ce Length of output: 455 🏁 Script executed: # Check the full pg_set_mode function signature and parameter handling
sed -n '1240,1260p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 687 🏁 Script executed: # Look for where display_index is used and if it's converted to display ID
rg -n "display_index\|display.*id" src_c/display.c -B 2 -A 2 | head -50Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Check if there's any conversion logic between index and ID for non-zero indices
sed -n '1280,1350p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 2102 🏁 Script executed: # Search for proper index-to-ID conversion pattern used elsewhere in the codebase
rg -n "SDL_GetDisplays" src_c/display.c -B 5 -A 10Repository: pygame-community/pygame-ce Length of output: 2183 🏁 Script executed: # Check if there's a helper function or conversion logic for display index to ID
rg -n "display.*index\|index.*display" src_c/display.c | grep -v "display_index = 0"Repository: pygame-community/pygame-ce Length of output: 52 🏁 Script executed: # Look at the full context of pg_set_mode to see parameter declaration and all uses
sed -n '1200,1295p' src_c/display.c | cat -nRepository: pygame-community/pygame-ce Length of output: 3610 🏁 Script executed: # Check if display indices > 0 are ever validated or converted before use
sed -n '1280,1400p' src_c/display.c | cat -nRepository: pygame-community/pygame-ce Length of output: 4705 🏁 Script executed: # Look for the other two locations (2038, 2121) to see if they have the same pattern
sed -n '2030,2050p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 896 🏁 Script executed: sed -n '2110,2130p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 765 Fix broken multi-display API compatibility: display indices 1+ are not converted to valid SDL3 IDs In SDL3, display IDs are not guaranteed to be sequential 0-based indices like SDL2. Passing To maintain backwards compatibility with SDL2's 0-based index semantics, non-zero display values need conversion via |
||
|
|
||
| if ((vsync == -1) && ((flags & PGS_OPENGL) == 0)) { | ||
| return RAISE(PyExc_ValueError, | ||
| "requested adaptive vsync without OpenGL"); | ||
|
|
@@ -1751,9 +1779,9 @@ pg_set_mode(PyObject *self, PyObject *arg, PyObject *kwds) | |
| } | ||
| #endif | ||
|
|
||
| pg_texture = SDL_CreateTexture( | ||
| pg_renderer, SDL_PIXELFORMAT_ARGB8888, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| pg_texture = | ||
| SDL_CreateTexture(pg_renderer, PG_TEXTURE_FMT, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| } | ||
| surf = PG_CreateSurface(w, h, SDL_PIXELFORMAT_XRGB8888); | ||
| newownedsurf = surf; | ||
|
|
@@ -1875,6 +1903,9 @@ pg_set_mode(PyObject *self, PyObject *arg, PyObject *kwds) | |
| } | ||
| } | ||
| } | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| SDL_SyncWindow(win); | ||
| #endif | ||
|
|
||
| /*return the window's surface (screen)*/ | ||
| Py_INCREF(surface); | ||
|
|
@@ -1973,6 +2004,9 @@ pg_set_window_position(PyObject *self, PyObject *arg) | |
| if (win) { | ||
| /* Will raise errors with SDL 3, deal with it during the porting */ | ||
| SDL_SetWindowPosition(win, x, y); | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| SDL_SyncWindow(win); | ||
| #endif | ||
| } | ||
|
|
||
| Py_RETURN_NONE; | ||
|
|
@@ -1999,7 +2033,16 @@ pg_mode_ok(PyObject *self, PyObject *args, PyObject *kwds) | |
| &display_index)) { | ||
| return NULL; | ||
| } | ||
| #if !SDL_VERSION_ATLEAST(3, 0, 0) | ||
|
|
||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| /* In SDL2, display == 0 meant primary display, so compat code for it */ | ||
| if (display_index == 0) { | ||
| display_index = SDL_GetPrimaryDisplay(); | ||
| if (display_index == 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| } | ||
| #else | ||
| /* Display ID is not bounded by number of displays in SDL3 */ | ||
| if (display_index < 0 || display_index >= SDL_GetNumVideoDisplays()) { | ||
| return RAISE(PyExc_ValueError, | ||
|
|
@@ -2026,6 +2069,15 @@ pg_mode_ok(PyObject *self, PyObject *args, PyObject *kwds) | |
| } | ||
|
|
||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| /* Compat with SDL2 behaviour */ | ||
| const SDL_DisplayMode *curmode; | ||
| if (!(curmode = SDL_GetCurrentDisplayMode(display_index))) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| if (curmode->w == desired.w && curmode->h == desired.h) { | ||
| return PyLong_FromLong(SDL_BITSPERPIXEL(curmode->format)); | ||
| } | ||
|
Comment on lines
+2072
to
+2079
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let me find the header files with pixel format helpers
fd -e "h" "pygame" | head -20Repository: pygame-community/pygame-ce Length of output: 264 🏁 Script executed: # Search for PG_SURF_BitsPerPixel and related helpers in the codebase
rg "PG_SURF_BitsPerPixel|PG_FORMAT_BitsPerPixel|PG_PixelFormat" --type c --type hRepository: pygame-community/pygame-ce Length of output: 17548 🏁 Script executed: # Look at the specific lines mentioned in display.c
sed -n '2070,2100p' src_c/display.cRepository: pygame-community/pygame-ce Length of output: 1163 🏁 Script executed: # Check what's defined in _pygame.h around pixel format helpers
cat src_c/_pygame.h | grep -A 20 -B 5 "BitsPerPixel"Repository: pygame-community/pygame-ce Length of output: 2581 🏁 Script executed: # Check for SDL_AllocFormat and SDL_FreeFormat usage patterns
rg "SDL_AllocFormat|SDL_FreeFormat" --type c -B 2 -A 5Repository: pygame-community/pygame-ce Length of output: 3539 🏁 Script executed: # Check how display.c gets BitsPerPixel from formats elsewhere
rg "BitsPerPixel|BITSPERPIXEL" src_c/display.c -B 2 -A 2Repository: pygame-community/pygame-ce Length of output: 1388 🏁 Script executed: # Look for other format allocation patterns in codebase
rg "SDL_AllocFormat" --type cRepository: pygame-community/pygame-ce Length of output: 366 SDL2/SDL3 BitsPerPixel mismatch in display mode checks needs fixing (but suggested fix has memory leak) The issue is real: The suggested fix has a critical flaw: it allocates format with 🤖 Prompt for AI Agents |
||
|
|
||
| if (!SDL_GetClosestFullscreenDisplayMode(display_index, desired.w, | ||
| desired.h, desired.refresh_rate, | ||
| 1, &closest)) { | ||
|
|
@@ -2065,7 +2117,15 @@ pg_list_modes(PyObject *self, PyObject *args, PyObject *kwds) | |
| return NULL; | ||
| } | ||
|
|
||
| #if !SDL_VERSION_ATLEAST(3, 0, 0) | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| /* In SDL2, display == 0 meant primary display, so compat code for it */ | ||
| if (display_index == 0) { | ||
| display_index = SDL_GetPrimaryDisplay(); | ||
| if (display_index == 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| } | ||
| #else | ||
| /* Display ID is not bounded by number of displays in SDL3 */ | ||
| if (display_index < 0 || display_index >= SDL_GetNumVideoDisplays()) { | ||
| return RAISE(PyExc_ValueError, | ||
|
|
@@ -2077,19 +2137,31 @@ pg_list_modes(PyObject *self, PyObject *args, PyObject *kwds) | |
| #pragma PG_WARN(Ignoring flags) | ||
|
|
||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| const SDL_DisplayMode *curmode; | ||
| if (!(curmode = SDL_GetCurrentDisplayMode(display_index))) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| if (bpp == 0) { | ||
| const SDL_DisplayMode *curmode; | ||
| if (!(curmode = SDL_GetCurrentDisplayMode(display_index))) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
| bpp = SDL_BITSPERPIXEL(curmode->format); | ||
| } | ||
|
|
||
| SDL_DisplayMode **modes = | ||
| SDL_GetFullscreenDisplayModes(display_index, &nummodes); | ||
| if (nummodes < 0) { | ||
| if (!modes || nummodes < 0) { | ||
| return RAISE(pgExc_SDLError, SDL_GetError()); | ||
| } | ||
|
|
||
| /* SDL3 can return empty list here but SDL2 didn't. In that case, use | ||
| * curmode. */ | ||
| if (nummodes == 0) { | ||
| SDL_free(modes); | ||
| modes = SDL_malloc(sizeof(SDL_DisplayMode **)); | ||
| if (!modes) { | ||
| return PyErr_NoMemory(); | ||
| } | ||
| modes[0] = (SDL_DisplayMode *)curmode; | ||
| nummodes = 1; | ||
| } | ||
| #else | ||
| if (bpp == 0) { | ||
| SDL_DisplayMode curmode; | ||
|
|
@@ -2726,6 +2798,9 @@ pg_iconify(PyObject *self, PyObject *_null) | |
| return RAISE(pgExc_SDLError, "No open window"); | ||
| } | ||
| SDL_MinimizeWindow(win); | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| SDL_SyncWindow(win); | ||
| #endif | ||
| return PyBool_FromLong(1); | ||
| } | ||
|
|
||
|
|
@@ -3219,7 +3294,7 @@ pg_toggle_fullscreen(PyObject *self, PyObject *_null) | |
| SDL_CreateRenderer(win, -1, SDL_RENDERER_SOFTWARE); | ||
| #endif | ||
| pg_texture = | ||
| SDL_CreateTexture(pg_renderer, SDL_PIXELFORMAT_ARGB8888, | ||
| SDL_CreateTexture(pg_renderer, PG_TEXTURE_FMT, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| } | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
|
|
@@ -3372,7 +3447,7 @@ pg_toggle_fullscreen(PyObject *self, PyObject *_null) | |
| SDL_CreateRenderer(win, -1, SDL_RENDERER_SOFTWARE); | ||
| #endif | ||
| pg_texture = | ||
| SDL_CreateTexture(pg_renderer, SDL_PIXELFORMAT_ARGB8888, | ||
| SDL_CreateTexture(pg_renderer, PG_TEXTURE_FMT, | ||
| SDL_TEXTUREACCESS_STREAMING, w, h); | ||
| } | ||
|
|
||
|
|
@@ -3453,6 +3528,9 @@ pg_toggle_fullscreen(PyObject *self, PyObject *_null) | |
| } | ||
| } | ||
| } | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| SDL_SyncWindow(win); | ||
| #endif | ||
| return PyLong_FromLong(1); | ||
| } | ||
|
|
||
|
|
@@ -3531,6 +3609,9 @@ pg_display_resize_event(PyObject *self, PyObject *event) | |
| /* do not do anything that would invalidate a display surface! */ | ||
| return PyLong_FromLong(-1); | ||
| } | ||
| #if SDL_VERSION_ATLEAST(3, 0, 0) | ||
| SDL_SyncWindow(win); | ||
| #endif | ||
| Py_RETURN_FALSE; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
SDL3 SDL_SyncWindow SDL_GetPrimaryDisplay API availability version history💡 Result:
Both functions are part of SDL3 (header <SDL3/SDL_video.h>) and were introduced in the SDL 3.2.0 series:
Sources:
[1] SDL_GetPrimaryDisplay manpage (libsdl3-doc / Debian).
[2] SDL_SyncWindow — SDL Wiki.
[3] SDL_SyncWindow manpage (SDL3-devel-doc / openSUSE).
Align version guard to stable SDL3 release (3.2.0) since key APIs are only available from that version onward
The
PG_TEXTURE_FMTabstraction is clean, but switching fromSDL_VERSION_ATLEAST(3, 0, 0)toSDL_VERSION_ATLEAST(3, 2, 0)is important:SDL_SyncWindowandSDL_GetPrimaryDisplay(used in other sections of this file) are only available starting in SDL 3.2.0, which is the first stable SDL3 release. Aligning this guard ensures consistency across the module and prevents accidental use of preview SDK features.🤖 Prompt for AI Agents