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

Specular color and diffuse transmission color textures should be non-linear #596

Closed
andrewvarga opened this issue Jan 15, 2025 · 8 comments · Fixed by KhronosGroup/glTF-Sample-Renderer#19
Assignees

Comments

@andrewvarga
Copy link

I noticed that a particular .glb was rendering differently from all the other engines (three.js, babylon, playcanvas) - see attached glb:
SpecGlossVsMetalRough-converted.glb.zip

This is the SpecGlossVsMetalRough model dropped into gltf.report which converts the specular-glossiness material to metallic-roughness.

The rendered result of these should be the same but the converted material renders differently because of incorrect texture encoding (thanks @donmccurdy for pinpointing that):

Image

One solution is to set linear to false on the specular color texture (and diffuse transmission color texture). But I think both the specular color and the specular texture needs to be set to linear = false for this to take effect because this model uses the same image resource (and the same webgl texture) for these 2 texture slots, and SRGB8_ALPHA8_EXT internalFormat is not going to be used if specular texture is not set to linear = false.

Another approach is to switch to decoding in the shader but that would be a bigger refactor.

@UX3D-haertl
Copy link
Contributor

I do not think that this issue is related to texture encoding.
Since we needed to refactor the shader and not use the simplified version anymore, we cannot simply display specular glossiness anymore. We would need to convert to the correct metallic ratio for each fragment. It probably makes more sense to just convert specular glossiness to metallic roughness, specular and ior extensions while loading the asset. In this case we only need to convert the values once and not for every fragment for every frame.

@donmccurdy
Copy link

donmccurdy commented Jan 30, 2025

@UX3D-haertl in the attached file, the KHR_pbrSpecularGlossiness extension was already removed and replaced with metallic roughness, specular, and IOR. I believe the problem is in the support for KHR_materials_specular here:

https://github.com/KhronosGroup/glTF-Sample-Renderer/blob/63b7c128266cfd86bbd3f25caf8b3db3fe854015/source/gltf/material.js#L776-L781

The gltfTextureInfo class has linear=true as its default, which I assume should be overridden for specularColorTexture as is done for the baseColorTexture:

https://github.com/KhronosGroup/glTF-Sample-Renderer/blob/63b7c128266cfd86bbd3f25caf8b3db3fe854015/source/gltf/material.js#L597

@UX3D-haertl
Copy link
Contributor

We already set linear to false for specularColorTexture in
https://github.com/KhronosGroup/glTF-Sample-Renderer/blob/63b7c128266cfd86bbd3f25caf8b3db3fe854015/source/gltf/material.js#L340

specularTexture should be linear from the spec.
The asset from this issue uses the same texture for specularTexture and specularColorTexture. Therefore, one of the properties is in the wrong color space.

When I convert the asset with Gestaltor, I do not have any issues:

SpecGlossVsMetalRoughConverted.zip

@UX3D-haertl
Copy link
Contributor

I just tested the conversion with gltf.report and it seems to be indeed a bug on their end.
If you remove the specularTexture and only use the specularColorTexture, it looks correct.
Closing for now

@donmccurdy
Copy link

donmccurdy commented Jan 30, 2025

@UX3D-haertl specularTexture must point to a linear A channel within a texture. The alpha channel is not sRGB-encoded, only the RGB channels. This is perfectly valid. The reason that KHR_materials_specular was defined with RGB specularColorTexture and A specularTexture was so that the two could be packed together. The same is true for other texture types in the glTF specification.

I've made this choice intentionally in glTF Report and glTF Transform, and will not be splitting the two textures (doubling VRAM). I'm not aware of a viewer other than the sample renderer that fails on this case.

@UX3D-haertl
Copy link
Contributor

You're right. In this case we need to change the sampler format based on the material property and not the texture.

@UX3D-haertl
Copy link
Contributor

UX3D-haertl commented Jan 31, 2025

I opened a PR which now checks all TextureInfos to determine the final format of the texture.
With this change packed texture with RGBA or sRGB + linaer alpha are supported.
This avoids the need to recreate textures which would increase memory consumption and we still have a one to one mapping of glTF texture to WebGL texture.
Are there any usecases where these two formats are not enough for packed textures?

@donmccurdy
Copy link

donmccurdy commented Jan 31, 2025

Thank you @UX3D-haertl, that was fast!

Are there any usecases where these two formats are not enough for packed textures?

I have seen other cases. For example, baseColor (sRGB) and occlusion (linear) both using the RGB channels of the same texture. But — in my opinion — that is user error and not something engines must handle at runtime. Thoughts welcome on this though, the discussion is still open:

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

Successfully merging a pull request may close this issue.

3 participants