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

Support unspecified linear size in DDS files #86336

Merged
merged 1 commit into from Dec 20, 2023

Conversation

LunaticInAHat
Copy link
Contributor

@LunaticInAHat LunaticInAHat commented Dec 19, 2023

This PR adjusts the DDS image loader, to properly handle compressed DDS images which were created by tools that elected not to specify the size of the top-level image, in the DDS header.

In cases where the exporter did specify a size, we compare it against our locally-calculated size. In cases where the exporter did not specify, we check that they populated a 0 in the associated header field, to minimize the risk of trying to parse corrupt / malformed files.

This PR fixes a part of #86330. The image supplied in that bug report's reproduction project still does not import successfully, due to the Image subsystem expecting more mipmaps than are actually present in the file, however that appears to be a separate issue, not intrinsically related to the changes being made here (i.e., while that particular image is not fully fixed by this change, there exist other images that are)

ERR_FAIL_COND_V(!(flags & DDSD_LINEARSIZE), Ref<Resource>());
if (flags & DDSD_LINEARSIZE) {
// Image exporter specified a size for the top-level image; it should match our calculation
ERR_FAIL_COND_V(size != pitch, Ref<Resource>());
Copy link
Member

Choose a reason for hiding this comment

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

As said in the issue this needs to be contextualised so it's clear what's wrong, as it's not clear from this message that it depends on the flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I guess it wasn't clear to me what you meant by that comment. I wasn't previously aware of the existence of ERR_FAIL_COND_V_MSG(), which is what I'm assuming you want me to do?

Copy link
Member

Choose a reason for hiding this comment

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

In this case yeah, sorry for unclear, tried to make the same point in there 🙂

Copy link
Member

Choose a reason for hiding this comment

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

The wording sounds good 🙂

@clayjohn
Copy link
Member

CC @BlueCube3310 Who has been doing a lot of work on DDS importing recently

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.

This change makes sense to me. It would be good to have @BlueCube3310's opinion too

Copy link
Contributor

@BlueCube3310 BlueCube3310 left a comment

Choose a reason for hiding this comment

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

Fixes importing compressed non-power of 2 textures created by DirectX Texture Tool, great work!

@LunaticInAHat
Copy link
Contributor Author

@clayjohn If you don't mind, I'd like to get your thoughts about how to address the second half of #86330. The DDS file in question is 1024x1024, and (as I recall) contains 8 mipmap levels, down to 4x4. Unfortunately, the Image class does not allow callers to specify how many mipmap levels they're providing, when initializing an image, and it tries to mip all the way down to 1x1 (10 levels). That results in it expecting a handful of bytes more data than are present in the DDS, for those last couple mipmap levels.

I spent some time tinkering with that, and got Image to a point where it could be told how many mip levels were coming along when images were being set up, however that just resulted in making the renderer unhappy, because it also wanted image data sized for mipping all the way down to 1x1.

I am not sure what a viable path forward would be. Is it practical to plumb the number of mipmap levels all the way down to the renderer? Or should we try to detect cases where an image with pre-generated mipmaps (e.g., DDS) hasn't been mipmap'd down to 1x1, and just ignore the mipmaps (and generate our own, internally?)

modules/dds/texture_loader_dds.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed discussion labels Dec 19, 2023
Not all exporters choose to populate that (optional) header field.
@clayjohn
Copy link
Member

I am not sure what a viable path forward would be. Is it practical to plumb the number of mipmap levels all the way down to the renderer? Or should we try to detect cases where an image with pre-generated mipmaps (e.g., DDS) hasn't been mipmap'd down to 1x1, and just ignore the mipmaps (and generate our own, internally?)

I would go with the second approach and have the importer complete the mip map chain in cases where it isn't fully supplied. That will be the easiest "fix" for this problem.

@LunaticInAHat
Copy link
Contributor Author

I am not sure what a viable path forward would be. Is it practical to plumb the number of mipmap levels all the way down to the renderer? Or should we try to detect cases where an image with pre-generated mipmaps (e.g., DDS) hasn't been mipmap'd down to 1x1, and just ignore the mipmaps (and generate our own, internally?)

I would go with the second approach and have the importer complete the mip map chain in cases where it isn't fully supplied. That will be the easiest "fix" for this problem.

Hmmm... easy in concept, perhaps more complicated in practice, since we're dealing with compressed images. Image does mipmap generation, but it just punts on the issue of dealing with compressed images. In order to properly generate the missing levels, we would have to decompress the pixel data into some temporary buffer, resample it, and then compress the resulting mipmap data using the original encoding scheme, to append onto the original file's contents.

That seems like a lot of work to do, just to generate mips for when a 512x512 texture has been crushed down to a 2x2 square on the screen... The missing mips are so close to the point of "I am a single pixel on the screen", that I wonder if we might get acceptable results by just duplicating the initial block of the last provided mipmap level.

For example, the image from #86330 is 512x512, and the mipmap chain in the DDS ends at 8x8. That is 4 (2x2) blocks of DXT data. The 4x4 mipmap will be a single block, and then 2x2 and 1x1 aren't even fully utilized blocks. Maybe just take the top-left block of data from the 8x8 mipmap and copy it three times? It will be wrong for strong color gradients that have different colors in all four corners, but might yield acceptable results for less pathological cases.

@clayjohn
Copy link
Member

@LunaticInAHat that could work. A similar idea would be to extract the lowest mip level of the compressed texture. Copy that into an image, decompress, generate mipmaps, compress, then copy the source and generated mipmaps into a new image

@Calinou
Copy link
Member

Calinou commented Dec 20, 2023

Unfortunately, the Image class does not allow callers to specify how many mipmap levels they're providing, when initializing an image, and it tries to mip all the way down to 1x1 (10 levels). That results in it expecting a handful of bytes more data than are present in the DDS, for those last couple mipmap levels.

This reminds me of the Mipmaps > Limit option having no effect in the Import dock.

@YuriSizov YuriSizov merged commit c28a091 into godotengine:master Dec 20, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks, and congrats on your first merged Godot contribution!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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.

None yet

6 participants