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

get_array_stride sometimes incorrectly assumes that mip levels have byte-size 1/4 of the previous level #5

Open
w-flo opened this issue Oct 28, 2021 · 4 comments · Fixed by #6

Comments

@w-flo
Copy link
Contributor

w-flo commented Oct 28, 2021

That assumption is not always true when using block compression. Most cases are correctly handled via the "min_mipmap_size" param, but there are a few cases where this won't work:

For example, a 16x4 texture uses four 4x4 blocks and takes up 4xblock_size bytes. The next mip level is 8x2 pixels, which uses two blocks, so it takes up 2xblock_size bytes, so only half of the size instead of 1/4. But since the "min_mipmap_size" value is just 1*block_size, this gives an incorrect result.

It works fine for e.g. 8x4 textures thanks to the minimum block size. I only noticed this today after switching to a more space-efficient texture/rect packer that manages to end up with a 4096x1024 texture instead of the previous "textures are all over the place in my texture atlas so that atlas texture is a full 4096x4096 square". :-)

So the calculation needs to take dimensions into account. I'll try to fix it and send a PR, but I can only test using the unit tests and what my own app does, so it might break other stuff.

w-flo added a commit to w-flo/ddsfile that referenced this issue Oct 28, 2021
The next mipmap level of a 16x4 texture does not take up 1/4 the number
of bytes when using block compression. It takes up half the number of
bytes, because 8x2 textures are stored as two blocks (total dimensions
of 8x4 where half of those stored pixels are ignored). This is more than
the "min mipmap size" so the calculation would give an incorrect result
previously.

Fixes SiegeEngine#5.
@mikedilger
Copy link
Contributor

To be honest I haven't been using my own library for a very long time. I'm glad for your efforts here and would welcome giving you authorship/push rights if you wanted to move it forward. For now I'll just merge your PRs (as I don't have a lot of time to even study them)

@w-flo
Copy link
Contributor Author

w-flo commented Oct 30, 2021

Oh, don't worry about it. I doubt there will be any more PRs coming from my side because everything is now working perfectly. ddsfile was really helpful for my pet project, and I was able to generate atlas textures on load successfully. It appears to be working perfectly now! The latest code happens to produce 1:1, 2:1 or 1:2 aspect ratio textures only, which should probably work even without this patch.

So thanks for sharing that code! And taking into account the fact that I only have two rather specific ddsfile use cases and no way to test that second PR in other use cases, I do wonder if my second PR should be reverted until someone with more dds experience/knowledge than me finds time to review it properly. The issue that this fixes is rather obscure and I really don't want to be the guy that broke dds in wgpu because my ddsfile patch introduced some nasty bugs. :-)

@mikedilger
Copy link
Contributor

Ok. I'll look closer at that patch or revert it in the coming days. Glad my code helped you.

@mikedilger
Copy link
Contributor

I'll re-open this so it's a known open issue.

@mikedilger mikedilger reopened this Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants