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 #9876: MacBook Touch Bar crash / render issues w/ 32bpp graphics #10060

Merged
merged 3 commits into from Oct 16, 2022

Conversation

Bouke
Copy link
Contributor

@Bouke Bouke commented Sep 28, 2022

Motivation / Problem

Rendering of the MacBook Touch Bar is incorrect on SSE-based blitters when using 32bpp base graphics and software rendering. The following problems related to Touch Bar rendering have been found:

Blitter Result
32bpp-sse2 wrong (washed out colors)
32bpp-sse2-anim correct (doesn't use Blitter_32bppSSE_Base::Encode)
32bpp-sse4 wrong (black buttons)
32bpp-sse4-anim crash: #9876
32bpp-ssse3 wrong (black buttons)

Description

The crash using 32bpp-sse4 is caused by trying to use the animation buffer while not rendering to the screen. Similar to Blitter_40bppAnim::Draw we need to forward to a non-animating Draw(). Blitter_32bppSSE4_Anim encodes sprites using Blitter_32bppSSE_Base::Encode but didn't inherit from a non-animating blitter, so it now inherits from Blitter_32bppSSE4 to allow calling Blitter_32bppSSE4::Draw when a non-animated sprite is requested.

To fix the incorrect rendering @JGRennison had a look at the alpha channel rendering in the SSE blitters. My original patch (incorrectly) covered over the problems by disabling translucency altogether. The SSE blitters didn't set the alpha values to anything in particular. AlphaBlendTwoPixels are now adjusted so that the alpha output is as expected (i.e. it matches ComposeColourRGBA).

32bpp-simple (reference):
32bpp-simple

32bpp-sse2 (washed out):
32bpp-sse2

32bpp-ssse3 (black):
32bpp-ssse3

32bpp-ssse3 (with fix):
32bpp-ssse3-fixed-jgr

@glx22, @JGRennison and @frosch123 contributed to this PR by debugging and/or blitter / C++ support.

Limitations

?

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 touches english.txt or translations? Check the guidelines
  • 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')

@Bouke Bouke changed the title Fix #9876: Fix MacBook Touch Bar crash / render issues on 32bpp Fix #9876: MacBook Touch Bar crash / render issues on 32bpp Sep 28, 2022
@Bouke Bouke changed the title Fix #9876: MacBook Touch Bar crash / render issues on 32bpp Fix #9876: MacBook Touch Bar crash / render issues w/ 32bpp graphics Sep 28, 2022
JGRennison added a commit to JGRennison/OpenTTD-patches that referenced this pull request Sep 28, 2022
Bouke pushed a commit to Bouke/OpenTTD that referenced this pull request Sep 29, 2022
Bouke pushed a commit to Bouke/OpenTTD that referenced this pull request Sep 29, 2022
@JGRennison
Copy link
Contributor

I had another look at the AlphaBlendTwoPixels commit and was able to make the instruction sequences slightly shorter: JGRennison/OpenTTD-patches@8d8765e

@Bouke
Copy link
Contributor Author

Bouke commented Sep 29, 2022

I had another look at the AlphaBlendTwoPixels commit and was able to make the instruction sequences slightly shorter: JGRennison/OpenTTD-patches@8d8765e

This is great! I've tested these changes and there appears to be no changes to the rendered Touch Bar 👍. I've included that commit in this PR.

@michicc
Copy link
Member

michicc commented Oct 16, 2022

Commit messages need to be fixed.

For the Preprocessor hash is put into the first column, before the tab indentation message, fixing it would require reformatting the complete file as the pre-existing contents is "wrongly" formatted as well. Maybe someone with commit super-powers can just ignore that and force this PR.

@Bouke
Copy link
Contributor Author

Bouke commented Oct 16, 2022

I have fixed the commit messages. I haven't changed the indentation as that would require formatting the whole file as you've pointed out.

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

4 participants