Skip to content

GET_PIXELVALS SDL3 compat #3343

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

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented Feb 22, 2025

Changes

  • Ports the macros GET_PIXELVALS, GET_PIXELVALS_1, and SET_PIXELVAL to SDL3, consolidating back in the pixelarray methods version which I introduced recently to port the pixelarray module only
  • Ports the non-SIMD premultiply to SDL3 (since it was using the macro)

These changes lead to surface_fill.c and alphablit.c fully compiling on SDL3.

@Starbuck5 Starbuck5 requested a review from a team as a code owner February 22, 2025 08:13
@Starbuck5 Starbuck5 marked this pull request as draft February 22, 2025 08:13
@Starbuck5 Starbuck5 added Surface pygame.Surface sdl3 labels Feb 22, 2025
Comment on lines -1701 to +1752
GET_PIXELVALS(dR, dG, dB, dA, pixel, dstfmt, dstfmt);
GET_PIXELVALS(dR, dG, dB, dA, pixel, dstfmt, dstpal,
dstppa);
Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike all other instances, this GET_PIXELVALS passes dstfmt into the macro twice. Once as the format, once as the "ppa" (per pixel alpha).

I believe this was a copy paste bug and have removed this behavior.

It now properly uses the per pixel alpha of the surface.

I'm classifying this as a super minor bugfix no one will notice. To notice you would have to be blit blend subtracting an 8 bit surface onto a 24 bit surface on a platform we don't do SIMD for (otherwise it would use a different implementation). I have not attempted to reproduce the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, and nice catch too.
(we should reaaaalllyyy constrain surfaces to 32bits in pygame 3, imagine how much less branching and focus towards what really matters)

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 understand where you're coming from but forcing everything 32 bit will be difficult and removes paths that people can currently use.

I would like to do a review of all pixel handling code for pygame-ce 3, I think there are inconsistencies between places that could be resolved with higher level macros perhaps.

One other thing is that SDL3 introduces more exotic types of surfaces than ever before, and we need to make sure our APIs properly refuse to handle them or can actually handle them. Because it will be quite challenging to make sure they never leak into pygame-ce from SDL. Like imagine using the camera API and it gives you a YUV surface, or you load a weird png and it's a 64 bit surface.

@Starbuck5 Starbuck5 marked this pull request as ready for review February 22, 2025 08:29
@damusss damusss added the bugfix PR that fixes bug label Feb 22, 2025
Copy link
Member

@damusss damusss left a comment

Choose a reason for hiding this comment

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

Damn, good job!
Also those files are basically made entirely of macros, just how big is the processed code 😭

@damusss damusss added this to the 2.5.4 milestone Feb 22, 2025
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 🎈

@ankith26 ankith26 merged commit 7b05323 into pygame-community:main Feb 22, 2025
25 checks passed
@Starbuck5 Starbuck5 deleted the getpixelvals-sdl3 branch February 22, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug sdl3 Surface pygame.Surface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants