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

Re-allow destsurf scale w/ diff but compatible pixelformats #2172

Merged

Conversation

Starbuck5
Copy link
Member

@Starbuck5 Starbuck5 commented May 18, 2023

Fixes #2116

So, in #1884 I replaced our bespoke scaling code with a call to SDL_SoftStretch. I also removed our explicit source bitdepth == dest bitdepth check when a dest surface is provided, because I knew SDL_SoftStretch would do that error for us.

What I failed to take into account is that there are in fact times when a dest surface would be used (with the same bitdepth) but with a different pixelformat.

For example, scaling a RGBA image onto an RGBX destination surface. It was able to handle that before, and a few people used that functionality.

So I had a few ideas:

  • Revert my previous changes (This would get rid of the speedups too, and people like speedups)
  • Trick SDL_SoftStretch into thinking it had a surface of one type when it didn't, by overwriting the pixelformat and then replacing it later (Hacky)
  • Give SDL_SoftStretch a real surface to modify with the equivalent pixelformat.

I went with the third option, using CreateSurfaceFrom to share the pixel data with the existing destination surface.

@Starbuck5 Starbuck5 requested a review from a team as a code owner May 18, 2023 05:10
@Starbuck5 Starbuck5 added the transform pygame.transform label May 18, 2023
@Starbuck5 Starbuck5 added this to the 2.3 milestone May 20, 2023
@Starbuck5 Starbuck5 added the bugfix PR that fixes bug label May 21, 2023
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.

cool fix, thankss 🌟

Copy link
Member

@Temmie3754 Temmie3754 left a comment

Choose a reason for hiding this comment

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

Whilst this does solve the case of RGBX sources it does not fix BGRA cases (and possibly others) which the method prior to 2.2.1 handled fine so it will likely need to be modified to support those as well.

@Starbuck5
Copy link
Member Author

What kind of BGRA cases?

If you're trying to scale a BGRA surface onto a RGBA surface it should fail. It "worked" before but would produce incorrect output.

Copy link
Member

@Temmie3754 Temmie3754 left a comment

Choose a reason for hiding this comment

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

Cases where the destination format was BGRA which would result in altered colours in the old method.
Though if that was unintentional behaviour of the old function then everything else should be good to go.

@Starbuck5
Copy link
Member Author

My thought is that it’s better to explicitly fail in that case rather than give such weird output.

@Temmie3754
Copy link
Member

It likely is, I can't see many scenarios where someone would intentionally make use of the scaling methods to also change the displayed colour at the same time, there's a greater chance someone would accidentally cause that problem when scaling which the error message would help with.

@Starbuck5 Starbuck5 force-pushed the allow-heterogenous-dest-surface branch from cca990d to e6bef67 Compare May 28, 2023 01:48
@Starbuck5 Starbuck5 merged commit d868bc8 into pygame-community:main May 28, 2023
29 checks passed
@Starbuck5 Starbuck5 deleted the allow-heterogenous-dest-surface branch May 28, 2023 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR that fixes bug transform pygame.transform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when I scale a surface to destSurface
3 participants