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

Workaround basis universal normal map issue for S3TC #74035

Closed
wants to merge 1 commit into from

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Feb 27, 2023

I'll preface this by saying I have no idea why this works, or how the `RA_AS_RG setting is supposed to work.

Fixes #61039

After this change, the MRP from Calinou shows correctly:
image

Unknown what happens if USE_RG_AS_RGBA is not defined, but it is currently forced on at the top of the file.
I have not tested ETC2 / mobile.

I did test the else case just to see, and it appeared to produce the equivalent of no normal map

} else {
//opengl most likely, bad for normal maps, nothing to do about this.
format = basist::transcoder_texture_format::cTFRGBA32;
imgfmt = Image::FORMAT_RGBA8;

image

This change probably ought to test and address the other cases

@lyuma lyuma requested a review from a team as a code owner February 27, 2023 09:49
@lyuma lyuma added this to the 4.1 milestone Feb 27, 2023
@Saul2022
Copy link

What the status of the pr? Maybe the issue is similar with the one that happens with mipmap #57981

@lyuma
Copy link
Contributor Author

lyuma commented May 17, 2023

@Saul2022

@lyuma lyuma closed this May 17, 2023
@lyuma lyuma reopened this May 17, 2023
@lyuma
Copy link
Contributor Author

lyuma commented May 17, 2023

(Sorry, I hit the Close button by mistake when clicking the autocomplete popup)

@Saul2022 This PR only affects basis universal. If you're hitting the same issue #57981, this is from my understanding not related to basis universal, so it may be a different issue.

Because the linked issue is from alpha and very old, I would appreciate if you are hitting this bug in your project, please create a Minimal Reproduction project (zip file), and indicate which precise version of Godot Engine your are using.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Looks like a widening operation from 2 channel to three. So I don't see how it can be harmful.

@clayjohn
Copy link
Member

From the looks of it, this is using a four channel texture instead of a two channel texture meaning that it will double the size when using normal maps. I think we need to figure out why BasisU is struggling to compress two channel textures instead of just falling back to a four channel texture.

@akien-mga
Copy link
Member

Related PR: #62718

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 16, 2023
@clayjohn
Copy link
Member

I carefully evaluated this against #62718 and I think that #62718 is the correct fix. BasisU expects the texture that is compressed/saved to disk to be in a "RA" format. When compressing we actually convert the image from "RG" to "RA", but we do the conversion after we have already copied the image into the basisu::image. #62718 fixes the issue by moving the mem copy to after the conversion. IMO that is the correct fix as it retains the RG_AS_RA format throughout the pipeline.

This PR doesn't change how the texture is stored on disk, instead it just treats the RG images as RGBA (because that is how they are stored on disk) which leaves everything in the right place for the rendering server.

Running both PRs locally, they both fix the problem (and appear to take the same amount of memory on disk), but #62718 should take up less space on the GPU as the "RG_AS_RA" format instructs the GPU that only two channels are needed, while the plain DXT5 will use all 4 channels.

@lyuma
Copy link
Contributor Author

lyuma commented Jun 16, 2023

I agree with the assessment by @clayjohn . I originally got confused because I didn't realize the image->convert... operation was happening in the wrong order relative to assigning the params.m_source_mipmap_images

I'll close this PR given that #62718 was approved.

@lyuma lyuma closed this Jun 16, 2023
@akien-mga akien-mga modified the milestones: 4.2, 4.x Jun 16, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Jul 24, 2023
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.

Normal map is incorrectly compressed when using Basis Universal compression mode
6 participants