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

GLTFLoader: Fix color space for specular map. #23630

Merged
merged 3 commits into from
Mar 2, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 2, 2022

Related issue: #23627

Description

This PR defines the color space for specularMap. Only relevant for KHR_materials_pbrSpecularGlossiness. SpecGlossVsMetalRough.glb now renders as expected.

image

@donmccurdy
Copy link
Collaborator

Thanks! I wonder how this wasn't broken before. 🤔

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 2, 2022

The previously automatic inline decode only worked for specific textures of built-in materials. For all others (like the specularMap of GLTFMeshStandardSGMaterial) Texture.encoding could not be used and the custom shaders had to perform the inline decode manaully.

@Mugen87 Mugen87 added this to the r139 milestone Mar 2, 2022
@WestLangley
Copy link
Collaborator

So, KHR_materials_pbrSpecularGlossiness is still a thing?...

See KhronosGroup/glTF#2078

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 2, 2022

New glTF material features (like sheen, transmission, refraction, IOR, iridescence, subsurface scattering, ...) are only added to the metal/rough PBR workflow by Khronos now and moving forward, so the spec/gloss workflow in glTF provided by KHR_materials_pbrSpecularGlossiness is basically deprecated.

With extensions like KHR_materials_specular and KHR_materials_ior, the metal/rough workflow can losslessly represent all of the same materials as the spec/gloss workflow anyway, and there are some tools (like glTF Transform) to convert spec/gloss to metal/rough.

I think we could consider removing support for spec/gloss.

@elalish
Copy link
Contributor

elalish commented Mar 2, 2022

Thanks @Mugen87! Yeah, Spec-Gloss is deprecated, but only for about a year now and there are lots of assets floating around with the extension because many authoring tools didn't like the idea of a lossy conversion. I would wait until there is a high quality free tool for converting Spec-Gloss GLBs to MR before we remove support.

@donmccurdy
Copy link
Collaborator

Not intending to rush the process, but -

gltf-transform metalrough input.glb output.glb

The conversion is not lossy if the viewer supports the specular and IOR extensions.

@elalish
Copy link
Contributor

elalish commented Mar 2, 2022

Awesome, thanks @donmccurdy! Is this part of a web GUI anywhere yet? I don't know if you want to help build out the functionality of the model-viewer editor or build your own, but either way it would be awesome. I know, I know, in our copious spare time...

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 2, 2022

At some point I'm going to have https://gltf.report/ start doing the conversion by default (it'll need metal/rough for other reasons), but for now it can be accomplished by running this in the sidebar script tab:

import { metalRough } from '@gltf-transform/functions';
await document.transform(metalRough());

@mrdoob mrdoob merged commit c5e0fdf into mrdoob:dev Mar 2, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 2, 2022

Thanks!

mrdoob pushed a commit that referenced this pull request Mar 3, 2022
* GLTFLoader: Fix color space for specular map.

* GLTFLoader: Add more missing sRGBEncoding.

* Update GLTFLoader.js
@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2022

Cherry-picked into the master branch and included in 0.138.3.

@Mugen87 Mugen87 modified the milestones: r139, r138 Mar 4, 2022
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* GLTFLoader: Fix color space for specular map.

* GLTFLoader: Add more missing sRGBEncoding.

* Update GLTFLoader.js
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.

5 participants