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

Fix #8774: Black screenshots when using 40bpp-blitter. #8791

Merged
merged 1 commit into from Mar 2, 2021

Conversation

michicc
Copy link
Member

@michicc michicc commented Mar 1, 2021

This affected all screenshot types that render to an off-screen
buffer and don't copy the actual screen contents.

Motivation / Problem

Screenshot types that take their contents from an newly rendered off-screen buffer were black when using 8bpp graphics with the 40bpp-anim blitter.

Description

The 40bpp-blitter does not resolve palette values when encoding a sprite as palette resolving is done by OpenGL in the end. Off-screen screenshots render using 32bpp-optimized though, which expects resolved RGB values.

We fix this by adding a special mode to the 32bpp drawing code that resolves the palette during blitting.

Limitations

Rendering these screenshots will take longer with 40bpp-anim than with a 32bpp blitter, as it incurs an additional palette lookup for each pixel. The 40bpp-anim blitter also takes a slight performance hit during sprite encoding as it has to calculate the max brightness value just for the screenshots.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

This affected all screenshot types that render to an off-screen
buffer and don't copy the actual screen contents.
@LordAro
Copy link
Member

@LordAro LordAro commented Mar 1, 2021

How much longer, approximately speaking?

@michicc
Copy link
Member Author

@michicc michicc commented Mar 1, 2021

Really hard to properly test and measure. Its probably going to highly depend on things like cache hits/misses, but something like 5-10% it seems.

There isn't any real alternative though, the whole point of 40bpp-anim is to resolve palette as late as possible so we can in fact properly blend things like RGB with palette mask.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 1, 2021

What part is 5-10% slower, the normal operation of the 40bpp-anim blitter, or the screenshots? (or both?) :D

@michicc
Copy link
Member Author

@michicc michicc commented Mar 1, 2021

These three specific broken screenshot types. As long as your sprite cache size is normally big (i.e. default), the sprite encoding change should not be measureable.

@michicc michicc merged commit 937d60f into OpenTTD:master Mar 2, 2021
10 checks passed
@michicc michicc deleted the pr/gl_screenshot branch Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants