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.

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

SDL3: restructure cliprect handling in draw.c #3278

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

Starbuck5
Copy link
Member

In SDL2, a Surface's clip rect can be accessed with surf->clip_rect, or with SDL_GetClipRect (function with no error returning).

In SDL3, this information only comes from SDL_GetSurfaceClipRect, a function which can return false on error.

In draw.c, we have a lot of accesses of the clip rect in the SDL2 style. These cannot be replaced "in place" straightforwardly because our internal draw functions aren't set up to return errors. Also a function call is more expensive than a struct access, so replacing in place would be a slowdown.

Therefore, in this PR, I bubbled up the clip rect acquisition to the overarching Python-interfacing functions (where it can fail and raise an error if needed), and then passed the rect down through however many layers of functions needed it.

This gets the draw module significantly closer to compiling properly on SDL3.

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

My (untested) hunch is that it is probably slightly more performant to pass the clip rect as a pointer SDL_Rect *surf_clip_rect, instead of what is being done here.

Normally I wouldn't nitpick performance things like this, but I think draw is one of the places we have to keep these things in mind.

@Starbuck5
Copy link
Member Author

Because passing a pointer is smaller than passing a struct?

It’s not much smaller. Ints are usually 32 bit, 4 ints = 128 bits vs a 64 bit pointer. Plus if it compiles the way it is written the pointer dereference would be a memory access vs how it is now it might be in registers. But who knows what the compiler will actually do, for all we know the functions in draw.c are all inlined.

If anything I’d think it’s written more “efficiently” now than before because it doesn’t have so many surface dereferences, but again maybe an optimizing compiler does stuff to it.

I haven’t tested performance but I expect any impact either way to be extremely minimal such that it can’t be reliably measured.

@ankith26
Copy link
Member

ankith26 commented Jan 4, 2025

I just did some basic performance testing with the strategy I mentioned, and as you said I didn't really see a measurable and consistent performance impact either way.

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR 🎉

Tested compilation on SDL3 and can confirm that I see no clip rect related compilation errors.

@ankith26 ankith26 added this to the 2.5.3 milestone Jan 5, 2025
Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@zoldalma999 zoldalma999 merged commit 0ddbe21 into pygame-community:main Jan 5, 2025
25 checks passed
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Alright, LGTM 👍

@Starbuck5 Starbuck5 deleted the draw-sdl3 branch January 14, 2025 07:46
@Starbuck5 Starbuck5 mentioned this pull request Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draw pygame.draw sdl3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants