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

glTF conformance: khronos-TextureLinearInterpolationTest #22483

Closed
elalish opened this issue Sep 1, 2021 · 16 comments · Fixed by #22551 or #22952
Closed

glTF conformance: khronos-TextureLinearInterpolationTest #22483

elalish opened this issue Sep 1, 2021 · 16 comments · Fixed by #22551 or #22952
Labels

Comments

@elalish
Copy link
Contributor

elalish commented Sep 1, 2021

I've finally updated our fidelity tests and three.js is looking quite respectable (with the exception of sheen), but I did notice a clear failure: https://modelviewer.dev/fidelity/#khronos-TextureLinearInterpolationTest

This warning looks like it may be directed at us. I also notice many of our renders are subtly darker than the others if you look through these fidelity tests. Considering the exposures and tone mappings have been carefully matched, I wonder if this might be the cause?

@donmccurdy
Copy link
Collaborator

I've been a little skeptical of this test (KhronosGroup/glTF-Sample-Models#297 (comment)) and wrote the warning above (KhronosGroup/glTF-Sample-Models#313), but if it really is making our renders subtly darker than baselines that would be much more motivating... 🤔

The workaround is to use EXT_sRGB in WebGL 1.0 (on by default in WebGL 2.0) instead of doing texture colorspace conversions in the fragment shader... I'm not sure what the support looks like for that extension, but I think we'd at least need to keep a fallback to the current GLSL for as long as we support IE11, is that worth the effort? Or is there a target date we might drop IE11 support? (EOL is currently June 15, 2022).

@mrdoob
Copy link
Owner

mrdoob commented Sep 2, 2021

Not sure texture colorspaces conversion is the (only) issue the renders are darker.

I can see many samples that don't use textures and are still dark:

https://modelviewer.dev/fidelity/#khronos-BrainStem
https://modelviewer.dev/fidelity/#khronos-Fox
https://modelviewer.dev/fidelity/#khronos-GearboxAssy
https://modelviewer.dev/fidelity/#khronos-ReciprocatingSaw
https://modelviewer.dev/fidelity/#khronos-RiggedFigure
https://modelviewer.dev/fidelity/#khronos-BoxAnimated

And on this one the shine is still too bright:

https://modelviewer.dev/fidelity/#khronos-RiggedSimple

@WestLangley
Copy link
Collaborator

three.js is not glTF-compliant. AFAIK, I am the only one working on trying to make it so.

It is too early for conformance testing... Maybe in a month, assuming an organized and concerted effort.

@mrdoob
Copy link
Owner

mrdoob commented Sep 2, 2021

AFAIK, I am the only one working on trying to make it so.

I'm trying to solve alphaMode 💪

@donmccurdy
Copy link
Collaborator

Can someone please explain why the usage of EXT_sRGB produces a different result than the GLSL inline sRGB conversion?

There's a difference in whether color interpolation happens before or after sRGB decoding...

The second sphere samples its color as an interpolated value from a 2x1 texture. When interpolation happens after sRGB decoding, the final emissive value should also be about 0.5 green.

... it's clear in this example (the example is designed for that) but I'm not sure how much visual effect it has generally.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 6, 2021

I've tried adding support for gl.EXT_sRGB today but noticed something unfortunate. With WebGL 1, it is not possible to use the extension in combination with mipmap generation which makes gl.EXT_sRGB impractical. The following error occurs:

[.WebGL-000075BE002A8600] GL_INVALID_OPERATION: Texture format does not support mipmap generation.

Mipmap generation is supported with WebGL 2 but only with RGBA textures (meaning gl.SRGB8_ALPHA8). It does not work when using RGB textures which results in gl.SRGB8. I believe the reason for this is the fact that SRGB8_ALPHA8 is color-renderable but SRGB8 isn't. And gl.generateMipmap() only works with texture filterable AND color-renderable textures.

So unless the engine always defines color textures as RGBA during a sRGB workflow, the sRGB texture format will be hard to use.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 6, 2021

I've created a branch with my code: https://github.com/Mugen87/three.js/tree/dev17

With gl.SRGB8_ALPHA8: https://raw.githack.com/Mugen87/three.js/dev17/examples/webgl_loader_gltf.html
Current: https://threejs.org/examples/webgl_loader_gltf

The examples look different. The gl.SRGB8_ALPHA8 looks brighter in certain areas.

@WestLangley
Copy link
Collaborator

@Mugen87 Those examples are very helpful. It looks like the subtle differences are related to the emissive map... somehow...

One thing I did not try was to turn off ACESFilmic.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 16, 2021

After investigating the issue more closely, it seems that using LinearMipmapLinearFilter or LinearFilter darkens the texture. The better the filtering, the darker the output. Meaning LinearMipmapLinearFilter produces the darkest image:

NearestFilter, sRGB: https://jsfiddle.net/d0xs7wnr/
LinearFilter, sRGB: https://jsfiddle.net/fyu70vm2/1/
LinearMipmapLinearFilter, sRGB: https://jsfiddle.net/9dL5rhm4/

This also happens when using linear color space (although this setup is not valid in the first place since the grass texture is a sRGB texture).

NearestFilter, linear: https://jsfiddle.net/c35txr9v/
LinearFilter, linear: https://jsfiddle.net/08r2qzeb/
LinearMipmapLinearFilter, linear: https://jsfiddle.net/f6g97a5c/

When using gl.SRGB8_ALPHA8, this effect does not appear. The brightness is consistent:

NearestFilter, sRGB, SRGB8_ALPHA8: https://jsfiddle.net/we2v9xob/
LinearFilter, sRGB, SRGB8_ALPHA8: https://jsfiddle.net/1fy9xed6/
LinearMipmapLinearFilter, sRGB, SRGB8_ALPHA8: https://jsfiddle.net/pwk48n7h/

@WestLangley
Copy link
Collaborator

It seems to me that this code block

vec4 emissiveColor = texture2D( emissiveMap, vUv );
emissiveColor.rgb = emissiveMapTexelToLinear( emissiveColor ).rgb;

is doing precisely what the Linear Interpolation Test hopes to prevent.

three.js interpolates first and decodes second -- and does so for all nonlinear maps.

Perhaps the code injection can be removed for WebGL 2.

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2021

Perhaps the code injection can be removed for WebGL 2.

That makes sense to me 👍

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2021

Hmm, but what are we supposed to do with all the other encodings?

function getEncodingComponents( encoding ) {
switch ( encoding ) {
case LinearEncoding:
return [ 'Linear', '( value )' ];
case sRGBEncoding:
return [ 'sRGB', '( value )' ];
case RGBEEncoding:
return [ 'RGBE', '( value )' ];
case RGBM7Encoding:
return [ 'RGBM', '( value, 7.0 )' ];
case RGBM16Encoding:
return [ 'RGBM', '( value, 16.0 )' ];
case RGBDEncoding:
return [ 'RGBD', '( value, 256.0 )' ];
case GammaEncoding:
return [ 'Gamma', '( value, float( GAMMA_FACTOR ) )' ];
case LogLuvEncoding:
return [ 'LogLuv', '( value )' ];
default:
console.warn( 'THREE.WebGLProgram: Unsupported encoding:', encoding );
return [ 'Linear', '( value )' ];
}
}

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2021

In the first step, I would suggest to only bypass the inline decode for sRGB encoded RGBA textures in WebGL 2 (and use gl.SRGB8_ALPHA8 instead). We also have to support more special cases when e.g. a cube camera produces an sRGB encoded render target which is later used in the scene.

Encodings like RGBE or RGBM can't be handled by WebGL so without an inline decode in GLSL the texture data have to be converted to linear space before they are uploaded to the GPU. Eventually there will be different code paths for sRGB and all other encodings. Assuming we want to use the WebGL sRGB features.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2021

I guess I have something ready for a closer review: Mugen87@c767a1f

One requirement for using gl.SRGB8_ALPHA8 is the usage of RGBAFormat. So various places had to be changed where RGBFormat was previously used. Considering #22293 (comment), it's probably a nice simplification anyway.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 17, 2021

With WebGL 1, it is not possible to use the extension in combination with mipmap generation which makes gl.EXT_sRGB impractical.

Ouch... does this also occur in WebGL 1 with gl.SRGB8_ALPHA8? Never mind, confirmed. WebGL 2 only, then. 😕

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 10, 2021

Since SRGB8_ALPHA8 will not be used anymore with r134, we should reopen this issue.

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