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

WebGLRenderer: Make use of SRGB8_ALPHA8. #22551

Merged
merged 12 commits into from
Sep 20, 2021
Merged

WebGLRenderer: Make use of SRGB8_ALPHA8. #22551

merged 12 commits into from
Sep 20, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Sep 18, 2021

Related issue: Fixed #22483

Description

This PR introduces the usage of SRGB8_ALPHA8. That means sRGB encoded RGBA textures are now decoded via the WebGL API and not in the shader anymore. Now the decode into linear color space happens first and then the texture filtering is applied which produces more correct renderings.

The engine only uses SRGB8_ALPHA8 with WebGL 2 and only with RGBA textures. This combination ensures mipmaps can be generated. Since RGBA is required and RGB textures are often emulated by WebGL and are in fact RGBA textures (#22293 (comment)), RGBA is now always used in various loaders.

It was necessary to update some screenshots since the visuals are a bit different than before in some cases. A good example is webgl_loader_gltf_variants.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Fascinating...

Comment on lines -26 to -29
// JPEGs can't have an alpha channel, so memory can be saved by storing them as RGB.
const isJPEG = url.search( /\.jpe?g($|\?)/i ) > 0 || url.search( /^data\:image\/jpeg/ ) === 0;

texture.format = isJPEG ? RGBFormat : RGBAFormat;
Copy link
Owner

Choose a reason for hiding this comment

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

@Oletus FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an acceptable compromise to ensure generateMipmap works, devices tend to have a lot more memory these days compared to when this change was made.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Is this expected?

image

/cc @WestLangley

Edit: False alarm.

@mrdoob mrdoob added this to the r133 milestone Sep 18, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

This one seems strange too:

image

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

I had some problems with generating the E2E screenshots so I had to remove --use-gl=swiftshader from the config. It seems the existing setup does not work with SRGB8_ALPHA8 on my machine. Without using swiftshader, some other screenshots were not correctly produced. It seems I've overlooked webgl_materials_physical_transmission.

The light probe example indeed renders differently.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Uh oh, force-pushing messes with the Pull Request conversation.. 😶

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

Sorry, I push additional commits from now on 👍 .

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

I had some problems with generating the E2E screenshots so I had to remove --use-gl=swiftshader from the config. It seems the existing setup does not work with the extensions.

Hmm, which extensions?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

I mean SRGB8_ALPHA8 . The E2E screenshots showed black textures in the examples which rely on SRGB8_ALPHA8.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Oh, so SwiftShader doesn't support SRGB8_ALPHA8?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

At least I could not get it to work on my iMac. But it seems the GitHub backend has no problems with it.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

This one is also weird:

image

Seems like the horses receiveShadow is not working.

Edit: Seems to be unrelated to this PR.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

At least I could not get it to work on my iMac. But it seems the GitHub backend has no problems with it.

Have you tried doing npm update in the /test folder?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

I'll give it a try.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

The black edges are finally gone on this one! 🎉

image

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

It seems it does not solve the issue. The webgl_animation_skinning_blending screenshot looks like so now (although it renders correctly in the browser).

webgl_animation_skinning_blending

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

@Mugen87 Oh well. Thanks for testing 🙏

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

The black edges are finally gone on this one! 🎉

image

Although I wonder if this is related to this PR or if it was a previous change and the E2E didn't catch it.

@Mugen87 Could you include the builds in the PR so we can compare raw.githack links with current dev?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Another thing that would help is...

Instead of updating all the screenshots, can you update only the screenshots that fail in the E2E?

Edit: I'm saying this because I don't see a different between these two:

https://raw.githack.com/Mugen87/three.js/dev17/examples/index.html?q=car#webgl_materials_car
https://raw.githack.com/mrdoob/three.js/dev/examples/index.html?q=car#webgl_materials_car

But the screenshots are different.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

It seems the SSR examples look wrong now. However, they do something which is not expected in the post-processing demos.

renderer.outputEncoding = THREE.sRGBEncoding;

When using post-processing, sRGB should be normally applied via GammaCorrectionShader at the end of the pass chain. It seems SSRPass and SSRrPass have an encoding logic that is not consistent regarding this policy.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

Okay, I've just updated the screenshots that failed. Although I'm not understand the logic why some test fail and others not. E.g. webgl_loader_gltf_variants passes although the is a noticeable hue shift.

https://raw.githack.com/Mugen87/three.js/dev17/examples/webgl_loader_gltf_variants.html
https://threejs.org/examples/webgl_loader_gltf_variants

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 18, 2021

When using post-processing, sRGB should be normally applied via GammaCorrectionShader at the end of the pass chain. It seems SSRPass and SSRrPass have an encoding logic that is not consistent regarding this policy.

I've updated the PR with the respective change.

@mrdoob
Copy link
Owner

mrdoob commented Sep 18, 2021

Although I'm not understand the logic why some test fail and others not. E.g. webgl_loader_gltf_variants passes although the is a noticeable hue shift.

Yes, that is its own issue. Better not to go into that rabbit hole right now. I've been there 😬

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 19, 2021

The light probe example indeed renders differently.

That was indeed a bug in PMREMGenerator. Its shaders also perform inline decodes/encodes which are not necessary when working with SRGB8_ALPHA8.

@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

Okay, I've just updated the screenshots that failed. Although I'm not understand the logic why some test fail and others not. E.g. webgl_loader_gltf_variants passes although the is a noticeable hue shift.

https://raw.githack.com/Mugen87/three.js/dev17/examples/webgl_loader_gltf_variants.html
https://threejs.org/examples/webgl_loader_gltf_variants

I think that is unrelated to this PR. I can see the hue shift already happening in dev:

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_loader_gltf_variants.html

My guess is this was due the HDR clamping fix: #22451

@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

Okay, I've gone through the examples and everything looks good!👌

Can you remove the builds so this can be merged?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 20, 2021

Can you remove the builds so this can be merged?

I have reverted the build commits 👍

@mrdoob mrdoob merged commit cba9457 into mrdoob:dev Sep 20, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glTF conformance: khronos-TextureLinearInterpolationTest
3 participants