libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.07k stars 1.71k forks source link

SDL_BlitSurface enhancements #6631

Closed icculus closed 1 month ago

icculus commented 1 year ago

If it's not too backwards-looking, I'd really like an SDL_BlitSurfaceEx() function (or change to SDL_BlitSurface()) to add a flags field. In particular, it'd be nice to have these options (ignore the made-up-on-the-spot names):

  • SDL_BLIT_FLIPHORZ: Flip the surface horizontally while blitting. DirectDraw provides this, and it's a pain to have to open-code a blitter just to use it.
  • SDL_BLIT_FLIPVERT: Flip the surface vertically. More for consistancy than anything else.
  • SDL_BLIT_IGNOREPALETTE: When blitting from a paletted surface to another paletted surface, just copy the raw indices across, don't try to find the equivalent colours if the palettes don't match. Not only does this match DirectDraw's behaviour, it also fits really well with almost every paletted app, which tend to only want to set a palette for the final displayed surface, not for every intermediate one. Can be easily hacked around, but still…

So if adding more features to the old Surface API isn't out-of-the-question, those are the things which crop up all-the-time when using the surface API to port older stuff.

Originally posted by @sulix in https://github.com/libsdl-org/SDL/issues/3519#issuecomment-1313630422

Lucretia commented 1 year ago

Is there any point in keeping the SDL1/2 surface api past SDL2? Surely old SDL1 apps have been ported by now?

icculus commented 1 year ago

It's still useful (even if lots of things are going to use the GPU)

slouken commented 9 months ago

Sounds good!

sulix commented 5 months ago

I started playing around with this, and ended up wondering whether indexed surfaces should require palettes at all. Currently, every indexed surface has a palette (which, if the user doesn't specify one, is all black, except for 1-bit surfaces, where it has one black colour, and one white).

I have a super-hacky proof of concept which makes the palette NULL by default, and has blits between surfaces with NULL palettes just use the identity map: https://github.com/sulix/SDL/commit/nopalsurface

(It only handles the 8-bit → 8-bit case properly at this point, so is definitely not production ready.)

Does this make sense from an API point of view, either instead of or in addition to a flag for SDL_BlitSurface()? Personally, I quite like how this makes creating and using a surface 'just work' unless palettes are manually specified (though there are some nasty corner cases when, e.g., SDL_LoadBMP() is used).

slouken commented 5 months ago

I started playing around with this, and ended up wondering whether indexed surfaces should require palettes at all. Currently, every indexed surface has a palette (which, if the user doesn't specify one, is all black, except for 1-bit surfaces, where it has one black colour, and one white).

I have a super-hacky proof of concept which makes the palette NULL by default, and has blits between surfaces with NULL palettes just use the identity map: sulix@nopalsurface

(It only handles the 8-bit → 8-bit case properly at this point, so is definitely not production ready.)

Does this make sense from an API point of view, either instead of or in addition to a flag for SDL_BlitSurface()? Personally, I quite like how this makes creating and using a surface 'just work' unless palettes are manually specified (though there are some nasty corner cases when, e.g., SDL_LoadBMP() is used).

This seems totally reasonable for SDL 3.0. When expanding to truecolor we can fail the blit, or have a NULL palette map to black. Feel free to create a PR when you're ready!

slouken commented 1 month ago

Adding flipping into all the various blit combinations would be code and test case explosion. Instead you can just use SDL_FlipSurface() once your blit is complete.

I went ahead and made the palette change in https://github.com/libsdl-org/SDL/pull/10226.

slouken commented 1 month ago

@sulix, I went ahead and merged your change for 8-bit palettes. Could you do the followup work to handle multiple bit-depths appropriately? Are there other corner cases that need tidying up as well?

Feel free to update surface_testPalette() or add additional automated palette tests to cover whatever you need.

slouken commented 1 month ago

Okay, I think this is in a good place now.

Copying between 8-bit indexed surfaces that don't have a palette will just do a memcpy, copying between indexed surfaces that don't have a palette and RGB surfaces will fail, and < 8 bit surfaces aren't supported as copy targets, as always.