Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Start getting base module ready for SDL3 #3171

Merged
merged 3 commits into from
Nov 2, 2024
Merged

Start getting base module ready for SDL3 #3171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d6a8e3
Rework SDL_INIT_TIMER for SDL3
Starbuck5 Oct 13, 2024
5827924
Rework pg_EnvShouldBlendAlphaSDL2 for SDL3
Starbuck5 Oct 13, 2024
cc260b3
Adjust pg_(Get/Set)DefaultConvertFormat for SDL3
Starbuck5 Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src_c/_pygame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@
#define PG_CreateSurface SDL_CreateSurface
#define PG_CreateSurfaceFrom SDL_CreateSurfaceFrom
#define PG_ConvertSurface SDL_ConvertSurface
#define PG_ConvertSurfaceFormat SDL_ConvertSurfaceFormat
#define PG_ConvertSurfaceFormat SDL_ConvertSurface

#define PG_PixelFormatEnum SDL_PixelFormat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the addition of this macro. I believe defining another macro like

#if SDL_VERSION_ATLEAST(3, 0, 0)
#define PG_SURF_format(s) ((s)->format)
#else
#define PG_SURF_format(s) ((s)->format->format)
#endif

would also be useful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to wait a bit on that, see what abstractions evolve out of porting other modules for that case. My preliminary work has shown a big challenge in getting colorkey data and keeping it with the format, so maybe the bare format enum will be a rarity.


#define PG_SurfaceHasRLE SDL_SurfaceHasRLE

Expand Down Expand Up @@ -115,6 +117,8 @@ PG_UnlockMutex(SDL_mutex *mutex)
#define PG_FIND_VNUM_MINOR(ver) SDL_VERSIONNUM_MINOR(ver)
#define PG_FIND_VNUM_MICRO(ver) SDL_VERSIONNUM_MICRO(ver)

#define PG_INIT_TIMER 0

#else /* ~SDL_VERSION_ATLEAST(3, 0, 0)*/
#define PG_ShowCursor() SDL_ShowCursor(SDL_ENABLE)
#define PG_HideCursor() SDL_ShowCursor(SDL_DISABLE)
Expand Down Expand Up @@ -144,6 +148,8 @@ PG_UnlockMutex(SDL_mutex *mutex)
#define PG_ConvertSurfaceFormat(src, pixel_format) \
SDL_ConvertSurfaceFormat(src, pixel_format, 0)

#define PG_PixelFormatEnum SDL_PixelFormatEnum

#define PG_SoftStretchNearest(src, srcrect, dst, dstrect) \
SDL_SoftStretch(src, srcrect, dst, dstrect)

Expand Down Expand Up @@ -180,6 +186,8 @@ PG_UnlockMutex(SDL_mutex *mutex)
#define PG_FIND_VNUM_MINOR(ver) ver.minor
#define PG_FIND_VNUM_MICRO(ver) ver.patch

#define PG_INIT_TIMER SDL_INIT_TIMER

#if SDL_VERSION_ATLEAST(2, 0, 14)
#define PG_SurfaceHasRLE SDL_HasSurfaceRLE
#else
Expand Down
30 changes: 15 additions & 15 deletions src_c/base.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ static int pg_is_init = 0;
static int pg_sdl_was_init = 0;
SDL_Window *pg_default_window = NULL;
pgSurfaceObject *pg_default_screen = NULL;
static char *pg_env_blend_alpha_SDL2 = NULL;
static int pg_env_blend_alpha_SDL2 = 0;

static void
pg_install_parachute(void);
Expand Down Expand Up @@ -170,7 +170,7 @@ static pgSurfaceObject *
pg_GetDefaultWindowSurface(void);
static void
pg_SetDefaultWindowSurface(pgSurfaceObject *);
static char *
static int
pg_EnvShouldBlendAlphaSDL2(void);

/* compare compiled to linked, raise python error on incompatibility */
Expand Down Expand Up @@ -341,13 +341,13 @@ pg_init(PyObject *self, PyObject *_null)

/*nice to initialize timer, so startup time will reflec pg_init() time*/
#if defined(WITH_THREAD) && !defined(MS_WIN32) && defined(SDL_INIT_EVENTTHREAD)
pg_sdl_was_init = SDL_Init(SDL_INIT_EVENTTHREAD | SDL_INIT_TIMER |
pg_sdl_was_init = SDL_Init(SDL_INIT_EVENTTHREAD | PG_INIT_TIMER |
PG_INIT_NOPARACHUTE) == 0;
#else
pg_sdl_was_init = SDL_Init(SDL_INIT_TIMER | PG_INIT_NOPARACHUTE) == 0;
pg_sdl_was_init = SDL_Init(PG_INIT_TIMER | PG_INIT_NOPARACHUTE) == 0;
#endif

pg_env_blend_alpha_SDL2 = SDL_getenv("PYGAME_BLEND_ALPHA_SDL2");
pg_env_blend_alpha_SDL2 = SDL_getenv("PYGAME_BLEND_ALPHA_SDL2") != NULL;

/* initialize all pygame modules */
for (i = 0; modnames[i]; i++) {
Expand Down Expand Up @@ -2075,28 +2075,28 @@ pg_SetDefaultWindowSurface(pgSurfaceObject *screen)
pg_default_screen = screen;
}

SDL_PixelFormat *pg_default_convert_format = NULL;
PG_PixelFormatEnum pg_default_convert_format = 0;

static SDL_PixelFormat *
static PG_PixelFormatEnum
pg_GetDefaultConvertFormat(void)
{
if (pg_default_screen) {
#if SDL_VERSION_ATLEAST(3, 0, 0)
return pg_default_screen->surf->format;
#else
return pg_default_screen->surf->format->format;
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block of code can be return PG_SURF_format(pg_default_screen->surf); after implementing the macro

}
return pg_default_convert_format;
}

static SDL_PixelFormat *
pg_SetDefaultConvertFormat(Uint32 format)
static void
pg_SetDefaultConvertFormat(PG_PixelFormatEnum format)
{
if (pg_default_convert_format != NULL) {
SDL_FreeFormat(pg_default_convert_format);
}
pg_default_convert_format = SDL_AllocFormat(format);
return pg_default_convert_format; // returns for NULL error checking
pg_default_convert_format = format;
}

static char *
static int
pg_EnvShouldBlendAlphaSDL2(void)
{
return pg_env_blend_alpha_SDL2;
Expand Down
6 changes: 3 additions & 3 deletions src_c/include/_pygame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,13 +181,13 @@ typedef struct pg_bufferinfo_s {
(*(void (*)(pgSurfaceObject *))PYGAMEAPI_GET_SLOT(base, 22))

#define pg_EnvShouldBlendAlphaSDL2 \
(*(char *(*)(void))PYGAMEAPI_GET_SLOT(base, 23))
(*(int (*)(void))PYGAMEAPI_GET_SLOT(base, 23))

#define pg_GetDefaultConvertFormat \
(*(SDL_PixelFormat * (*)(void)) PYGAMEAPI_GET_SLOT(base, 27))
(*(PG_PixelFormatEnum(*)(void))PYGAMEAPI_GET_SLOT(base, 27))

#define pg_SetDefaultConvertFormat \
(*(SDL_PixelFormat * (*)(Uint32)) PYGAMEAPI_GET_SLOT(base, 28))
(*(void (*)(Uint32))PYGAMEAPI_GET_SLOT(base, 28))

#define import_pygame_base() IMPORT_PYGAME_MODULE(base)
#endif /* ~PYGAMEAPI_BASE_INTERNAL */
Expand Down
3 changes: 0 additions & 3 deletions src_c/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
# first the "required" modules

# TODO: support SDL3
if sdl_api != 3
base = py.extension_module(
'base',
'base.c',
Expand All @@ -10,7 +8,6 @@ base = py.extension_module(
install: true,
subdir: pg,
)
endif

color = py.extension_module(
'color',
Expand Down
64 changes: 26 additions & 38 deletions src_c/surface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1582,66 +1582,54 @@ surf_convert(pgSurfaceObject *self, PyObject *args)
static SDL_Surface *
pg_DisplayFormat(SDL_Surface *surface)
{
SDL_PixelFormat *default_format = pg_GetDefaultConvertFormat();
PG_PixelFormatEnum default_format = pg_GetDefaultConvertFormat();
if (!default_format) {
SDL_SetError(
"No convert format has been set, try display.set_mode()"
" or Window.get_surface().");
return NULL;
}
return PG_ConvertSurface(surface, default_format);
return PG_ConvertSurfaceFormat(surface, default_format);
}

static SDL_Surface *
pg_DisplayFormatAlpha(SDL_Surface *surface)
{
SDL_PixelFormat *dformat;
Uint32 pfe;
Uint32 amask = 0xff000000;
Uint32 rmask = 0x00ff0000;
Uint32 gmask = 0x0000ff00;
Uint32 bmask = 0x000000ff;

dformat = pg_GetDefaultConvertFormat();
PG_PixelFormatEnum pfe = SDL_PIXELFORMAT_ARGB8888;
PG_PixelFormatEnum dformat = pg_GetDefaultConvertFormat();
if (!dformat) {
SDL_SetError(
"No convert format has been set, try display.set_mode()"
" or Window.get_surface().");
return NULL;
}

switch (PG_FORMAT_BytesPerPixel(dformat)) {
case 2:
/* same behavior as SDL1 */
if ((dformat->Rmask == 0x1f) &&
(dformat->Bmask == 0xf800 || dformat->Bmask == 0x7c00)) {
rmask = 0xff;
bmask = 0xff0000;
}
switch (dformat) {
#if SDL_VERSION_ATLEAST(3, 0, 0)
case SDL_PIXELFORMAT_XBGR1555:
#else
case SDL_PIXELFORMAT_BGR555:
#endif
case SDL_PIXELFORMAT_ABGR1555:
case SDL_PIXELFORMAT_BGR565:
case PG_PIXELFORMAT_XBGR8888:
case SDL_PIXELFORMAT_ABGR8888:
pfe = SDL_PIXELFORMAT_ABGR8888;
break;
case 3:
case 4:
/* keep the format if the high bits are free */
if ((dformat->Rmask == 0xff) && (dformat->Bmask == 0xff0000)) {
rmask = 0xff;
bmask = 0xff0000;
}
else if (dformat->Rmask == 0xff00 &&
(dformat->Bmask == 0xff000000)) {
amask = 0x000000ff;
rmask = 0x0000ff00;
gmask = 0x00ff0000;
bmask = 0xff000000;
}

case SDL_PIXELFORMAT_BGRX8888:
case SDL_PIXELFORMAT_BGRA8888:
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
case SDL_PIXELFORMAT_BGR24:
#else
case SDL_PIXELFORMAT_RGB24:
#endif
pfe = SDL_PIXELFORMAT_BGRA8888;
break;
default: /* ARGB8888 */

default:
Copy link
Member

@ankith26 ankith26 Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the explanative diagram, with that I can see the equivalence of this code with the old code.

Now that we are moving to pixelformats here, we could probably further improve this logic to be simpler and more correct, by dealing with the SDL_PIXELORDER macro instead, though that would definitely be for a future PR, I think imma make an attempt at that after merging this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured my comment was not clear, and I got the itch to work on it now. So what I'm essentially proposing is:

static SDL_Surface *
pg_DisplayFormatAlpha(SDL_Surface *surface)
{
    PG_PixelFormatEnum pfe = SDL_PIXELFORMAT_ARGB8888;
    PG_PixelFormatEnum dformat = pg_GetDefaultConvertFormat();
    if (!dformat) {
        SDL_SetError(
            "No convert format has been set, try display.set_mode()"
            " or Window.get_surface().");
        return NULL;
    }

    if (SDL_ISPIXELFORMAT_ALPHA(dformat)) {
        /* Display surface already has alpha, use the exact same format */
        pfe = dformat;
    }
    else if (SDL_ISPIXELFORMAT_ARRAY(dformat)) {
        /* In SDL2 this path is only triggered for
         * SDL_PIXELFORMAT_RGB24/SDL_PIXELFORMAT_BGR24 */
        switch (SDL_PIXELORDER(dformat)) {
#if SDL_BYTEORDER == SDL_BIG_ENDIAN
            case SDL_ARRAYORDER_RGB:
                pfe = SDL_PIXELFORMAT_ARGB8888;
                break;
            case SDL_ARRAYORDER_BGR:
                pfe = SDL_PIXELFORMAT_BGRA8888;
                break;
#else
            case SDL_ARRAYORDER_RGB:
                pfe = SDL_PIXELFORMAT_BGRA8888;
                break;
            case SDL_ARRAYORDER_BGR:
                pfe = SDL_PIXELFORMAT_ARGB8888;
                break;
#endif
            default:
                break;
        }
    }
    else if (SDL_ISPIXELFORMAT_PACKED(dformat)) {
        switch (SDL_PIXELORDER(dformat)) {
            case SDL_PACKEDORDER_XRGB:
                pfe = SDL_PIXELFORMAT_ARGB8888;
                break;
            case SDL_PACKEDORDER_RGBX:
                pfe = SDL_PIXELFORMAT_RGBA8888;
                break;
            case SDL_PACKEDORDER_XBGR:
                pfe = SDL_PIXELFORMAT_ABGR8888;
                break;
            case SDL_PACKEDORDER_BGRX:
                pfe = SDL_PIXELFORMAT_BGRA8888;
                break;
            default:
                break;
        }
    }
    return PG_ConvertSurfaceFormat(surface, pfe);
}

Still gonna PR it later so it can be independently reviewed

break;
}
pfe = SDL_MasksToPixelFormatEnum(32, rmask, gmask, bmask, amask);
if (pfe == SDL_PIXELFORMAT_UNKNOWN) {
SDL_SetError("unknown pixel format");
return NULL;
}
return PG_ConvertSurfaceFormat(surface, pfe);
}

Expand Down
8 changes: 2 additions & 6 deletions src_c/window.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,8 @@ window_get_surface(pgWindowObject *self, PyObject *_null)
return RAISE(pgExc_SDLError, SDL_GetError());
}

if (pg_GetDefaultConvertFormat() == NULL) {
if (pg_SetDefaultConvertFormat(_surf->format->format) == NULL) {
/* This is very unlikely, I think only would happen if SDL runs
* out of memory when allocating the format. */
return RAISE(pgExc_SDLError, SDL_GetError());
}
if (pg_GetDefaultConvertFormat() == 0) {
pg_SetDefaultConvertFormat(_surf->format->format);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pg_SetDefaultConvertFormat(_surf->format->format);
pg_SetDefaultConvertFormat(PG_SURF_format(_surf));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is ready, except that this minor issue has not been addressed yet. Adding the abstraction would fix this code for the SDL3 case, now it would error.
I suggested the abstraction because I can see it being used twice in this PR itself, so I assumed it could be potentially useful in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in my defense this PR is to port base, not window.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, we can deal with it later then. Anyways it is an issue that a compiler can catch, so it won't go unnoticed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah when you left your comment I stared at the line and was like how did the compiler not catch that. It took me a sec to realize the compiler isn't catching it because Window.c is not being compiled.

}

if (self->surf == NULL) {
Expand Down
Loading