Page MenuHomeWildfire Games

Fix logic issue with the DXT1a texture format
ClosedPublic

Authored by Itms on Dec 29 2019, 8:31 PM.

Details

Summary

I made a mistake when I updated tex_dds.cpp in rP23305.

The previous code had an unreachable line 464. Indeed at line 436 DDPF_ALPHAPIXELS (without DDPF_RGB excluded from the previous conditional) was used to determine whether the file was uncompressed greyscale. Thus the condition at line 463 could never be true.
As a consequence, before the upgrade, no DXT1a file would have been properly decoded: they would all be mistaken for a 8bpp greyscale, and would throw an error line 445 (DXT1a is 4bpp (actually dwRGBBitCount is not even guaranteed to be valid with compressed formats)).

This never happened since rP14015 so we probably never use DXT1a and do not need to support it.

When I updated our code to take into account the DDPF_ALPHA flag which describes uncompressed 8bpp greyscale, I kept the logic of that section intact without noticing that some code was unreachable. That let me to misunderstand the meaning of DDPF_ALPHA and to write a wrong comment.
As a consequence, right now, DXT1a files are still not supported, however loading them will fail silently: they will be mistaken for DXT1 files and their 1bpp alpha will be ignored.

This patch fixes the logic and allows us to properly decode DXT1a.

Test Plan

As mentioned above, we never encountered a DXT1a file in 6 years so there is no file on which we could test this. We could find one maybe.

We can however verify the logic.

Event Timeline

Itms created this revision.Dec 29 2019, 8:31 PM
Stan added a subscriber: Stan.Dec 29 2019, 8:33 PM

Maybe we could have such a file and a test?

Stan awarded a token.Dec 29 2019, 8:34 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/910/display/redirect

Itms added a comment.Dec 29 2019, 8:39 PM
In D2522#105360, @Stan wrote:

Maybe we could have such a file and a test?

Well actually I wanted to have an artist opinion about whether we could ever need the DXT1a format. See the description of the DXT formats here: https://docs.microsoft.com/fr-fr/windows/win32/direct3d9/compressed-texture-formats

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1426/display/redirect

Vulcan requested changes to this revision.Dec 30 2019, 12:01 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/2/display/redirect

This revision now requires changes to proceed.Dec 30 2019, 12:01 AM
Itms removed a reviewer: Vulcan.Dec 30 2019, 12:25 AM
This revision now requires review to proceed.Dec 30 2019, 12:25 AM
Itms added inline comments.Dec 30 2019, 10:09 AM
source/lib/tex/tex_dds.cpp
307

I should replace this address by https://docs.microsoft.com/fr-fr/windows/win32/direct3ddds/dx-graphics-dds-reference, since redirection doesn't happen.

Itms updated this revision to Diff 10827.Dec 30 2019, 11:34 AM

This is mainly to test the new macOS patch building. Sorry in advance for the noise...

Itms added inline comments.Dec 30 2019, 11:37 AM
source/lib/tex/tex_dds.cpp
1

I think this is safe to assume...

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/913/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/tex/tex_dds.cpp
|   1| /*·Copyright·(C)·2020·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2020"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1429/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/7/display/redirect

wraitii accepted this revision.Jun 7 2020, 9:00 AM
wraitii added a subscriber: wraitii.

Fits the Microsoft docs and makes logical sense.

This revision is now accepted and ready to land.Jun 7 2020, 9:00 AM
This revision was automatically updated to reflect the committed changes.