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

Reduce errors in src_c/surface.c when compiling with SDL3 #3215

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gresm
Copy link
Contributor

@gresm gresm commented Nov 6, 2024

Not a full port.

Things left to do:

  • Logic for palettes.
  • RLEACCEL
  • alphablit
  • surface_fill
  • simd_blitters_avx2
  • simd_blitters_sse2
  • simd_surface_fill_avx2
  • simd_surface_fill_sse2

Unchecked points will be done in a different PR / different PRs.

@gresm gresm requested a review from a team as a code owner November 6, 2024 19:30
@yunline yunline added Code quality/robustness Code quality and resilience to changes sdl3 labels Nov 7, 2024
src_c/_pygame.h Outdated
#define PG_GetSurfacePalette SDL_GetSurfacePalette
#define PG_SurfaceMapRGBA(surf, r, g, b, a) \
SDL_MapSurfaceRGBA(surf, r, g, b, a)
#define PG_GetSurfaceClipRect(surf, rect) SDL_GetSurfaceClipRect(surf, rect)
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 approach is fine for now, but eventually we may have to look into how this can be improved. For now we have no idea how cheap or how expensive these functions are, and they are called in tight loops in performance critical sections.
Many of those places probably need to be refactored to call this method once outside any tight loops and then use that result wherever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely something to remember in the future ports, but in the src_c/surface.c it seems like only surf_get_bounding_rect falls in this category.

src_c/_pygame.h Outdated Show resolved Hide resolved
@Starbuck5
Copy link
Member

Hello.

Looking through this I see some parts that won't work for us. SDL_GetPixelFormatDetails and SDL_GetSurfaceClipRect need to be error checked, for one thing. Also when mapping to or from the same surface over and over again getting the format every time is going to be a large hit to performance, which doesn't come up a bunch in surface.c but does apply at least to surf_get_bounding_rect.

@gresm
Copy link
Contributor Author

gresm commented Nov 12, 2024

SDL_GetPixelFormatDetails and SDL_GetSurfaceClipRect need to be error checked, for one thing.

Oh right... SDL_GetSurfaceClipRect errors only when invalid surface is passes, so I assumed that it would be checked, but in many cases it doesn't seem like it. And for SDL_GetPixelFormatDetails, didn't notice that.

which doesn't come up a bunch in surface.c but does apply at least to surf_get_bounding_rect.

You're right, but currently seems like it's only for surf_get_bounding_rect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality/robustness Code quality and resilience to changes sdl3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants