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

Refactor DDS loading code #80900

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

BlueCube3310
Copy link
Contributor

@BlueCube3310 BlueCube3310 commented Aug 22, 2023

First of all, #80862 should be merged before this, as this PR is still requires some testing.

After testing, this PR doesn't seem to cause issues, and the changes done haven't altered the code in a way that would affect existing projects, so it most likely doesn't break compatibility.

The code for the DDS loading module hasn't been updated much since Godot 1.0, which makes it difficult to fix/add new features. This PR attempts to clean it up a little.

The biggest issue is the large amount of unnecessary repeated conditions while checking the format. For instance, the code checks for the FourCC flag six times in total, and for RGB flags - seven times. This not only makes the code difficult to evaluate, but also adds unneeded repeated checks.

There is also some leftover code intended to be used for palette-based image formats, but given that this feature has been broken for a long time and the indexed format isn't considered official, removing it for now seems to be the best course of action.

Another, smaller issue is the amount of commented-out debug print lines, which also contribute to the code being hard to read.

The idea I have is to 'split' the format-checking code into 3 blocks: FourCC formats, channel-masked formats, and other miscellaneous formats via nested if-statements. This serves to make the code easier to read, and it also decreases the amount of checks the engine does, improving the performance.

Another addition is the DDSFourCC enum, which contains the possible FourCC values. In the future, this could make it easier to add support for more FourCC formats, such as DX10 or float-based formats which are referenced by number, rather than by signature.

As stated before, this PR is a work-in-progress, so it may cause issues with existing projects. Extensive testing is required in order to ensure it doesn't affect anything.

An MRP containing several DDS files with different formats for testing: dds_test_images.zip
All of these images can be imported correctly by 4.2-dev3 (save for l8 and l8a8 images, but that is fixed by #81134)

@BlueCube3310 BlueCube3310 requested a review from a team as a code owner August 22, 2023 15:38
@BlueCube3310 BlueCube3310 marked this pull request as draft August 22, 2023 15:39
@BlueCube3310 BlueCube3310 force-pushed the dds-loading-refactor branch 5 times, most recently from dd568c1 to 87813ee Compare August 23, 2023 10:37
@BlueCube3310
Copy link
Contributor Author

BlueCube3310 commented Aug 23, 2023

There's an issue with RGB10A2 files: Microsoft's official tools (DirectXTex, DirectX Texture Tool) set the files' bitmasks as R10G10B10A2, but write/read the data as B10G10R10A2 and vice-versa. Other tools such as GIMP save/read the data according to the bitmasks, which causes compatibility breakage between them.
out
The image on the left was converted into R10G10B10A2 using Microsoft's tools and opened in Paint.Net (which uses DirectXTex under the hood) while the one on the right was opened with GIMP.

Additionally, official documentation states the data should be laid out according to the bitmasks.

Should Godot support the DirectXTex version or the standard version (which it used previously)?

@BlueCube3310 BlueCube3310 force-pushed the dds-loading-refactor branch 2 times, most recently from 38c9cd7 to 508a0d9 Compare August 23, 2023 12:48
@BlueCube3310 BlueCube3310 marked this pull request as ready for review August 23, 2023 15:02
@BlueCube3310
Copy link
Contributor Author

After doing some testing I believe this PR doesn't break compatibility with existing projects. The aforementioned issue appears to only be caused by Microsoft's tools, so I think the code should follow the standard set by other programs and the documentation.

@Chaosus Chaosus added this to the 4.x milestone Aug 27, 2023
@BlueCube3310 BlueCube3310 requested review from a team as code owners August 29, 2023 19:06
@BlueCube3310 BlueCube3310 marked this pull request as draft August 29, 2023 19:07
@AThousandShips AThousandShips removed this from the 4.x milestone Aug 29, 2023
@AThousandShips AThousandShips added this to the 4.x milestone Aug 29, 2023
@BlueCube3310
Copy link
Contributor Author

Is there a way to reconnect the dds-loading-refactor branch to this PR or should I open a new one?

@YuriSizov
Copy link
Contributor

Just push to it :)

@BlueCube3310
Copy link
Contributor Author

I did, but it only updated the branch, not the PR

@YuriSizov
Copy link
Contributor

@BlueCube3310 Your last push doesn't contain any changes compared to the master branch. That's why the PR auto-closed when you pushed the branch.

@YuriSizov YuriSizov reopened this Aug 29, 2023
@YuriSizov
Copy link
Contributor

Hmm, interesting, your commit showed up once the PR was reopened. Well, all good then!

@BlueCube3310 BlueCube3310 force-pushed the dds-loading-refactor branch 3 times, most recently from 2a49c0a to 9ec9e35 Compare August 30, 2023 13:50
@BlueCube3310 BlueCube3310 marked this pull request as ready for review August 30, 2023 14:01
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested locally and confirm it looks the same as master with the provided textures. The new code is a significant improvement.

@clayjohn clayjohn modified the milestones: 4.x, 4.3 Dec 7, 2023
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Approving on style 🎉

@YuriSizov YuriSizov merged commit 6e8bba8 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@BlueCube3310 BlueCube3310 deleted the dds-loading-refactor branch April 25, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants