GLTFLoader KHR_materials_unlit extension support. #13136

robertlong · 2018-01-19T02:38:39Z

Adds support for the pending KHR_materials_unlit glTF extension.

When this extension exists on a material the THREE.MeshBasicMaterial is used. The .map and .color properties are set appropriately.

I've also added an example model from mozilla/mr-social-client that shows off the extension. When selecting glTF-MaterialsUnlit in the dropdown a model with the extension is loaded and THREE.MeshBasicMaterial is used. When you select glTF a model without the extension is loaded and THREE.StandardMaterial is used with the correct fallback properties.

I could use some feedback on how to handle occlusion, emmisive, and normal maps. Exporters are expected to exclude these maps. I can ensure that these maps are removed from the material or I can leave it how it is and expect the exporters to remove them.

I also have a PR (facebookincubator/FBX2glTF#61) to export FBX models as unlit glTF models if you wish to test the loader with your own models.

donmccurdy · 2018-01-19T22:14:12Z

Thanks! The code looks good to me.

Here's a demo of my viewer running this version of GLTFLoader: https://khr-materials-unlit-ooddguuati.now.sh/

^ /cc @AurL would you want to try any of your examples to see if this looks reasonable? Here's one, edited by hand:

mrdoob · 2018-01-20T00:06:15Z

Are you adding two versions (gltf/bin and glb) of the same model intentionally?

donmccurdy · 2018-01-20T00:09:58Z

The gltf/bin model does not use the unlit extension, it is standard metal/rough for comparison. No preference on whether it should be included though.

mrdoob · 2018-01-20T00:14:04Z

As far as I understand glb also includes the textures, so we're adding the textures twice too...

donmccurdy · 2018-01-20T00:19:40Z

Yeah if keeping the repo size down is a concern, fine to remove glTF/bin version. Probably have more glTF examples than we need already. (cute robot though! 😊)

WestLangley · 2018-01-20T00:56:50Z

@donmccurdy Do you have a live example showing MeshStandardMaterial and the unlit material side-by-side?

donmccurdy · 2018-01-20T01:18:11Z

@WestLangley sure —

Robot:

FTM (original)

unlit
standard (need to turn off envMap in lighting settings)

Main expected use cases here are improved performance on mobile devices for VR, and photogrammetry.

EDIT: Oh, I don't have a demo showing side-by-side in the same scene though.

mrdoob · 2018-01-20T01:33:45Z

Yes please, lets remove the standard metal/rough version from the PR.

WestLangley · 2018-01-20T01:42:11Z

I must be missing something... Don't you want to approximate a PBR material with an unlit one?

If that is the case, I think you should be using MeshBasicMaterial with an enviornment map and dialing the reflectivity based on the metalness. You will have to select the best combine value, too.

donmccurdy · 2018-01-20T02:30:33Z

Don't you want to approximate a PBR material with an unlit one?

@WestLangley that might be the goal for this particular robot model, I'm not sure... But broadly the KHR_materials_unlit extension is only to specify "the model should be unlit", and the rest is up to the user. So this could also apply to 3D scans or anime/drawing styles where we do not want to approximate PBR at all. See KhronosGroup/glTF#1215.

takahirox · 2018-01-20T13:32:30Z

Off topic, I often think when the best timing of supporting new glTF spec/extension for GLTFLoader is. (Maybe depending on the specific spec/extension tho).

Made PR/issue in glTF GitHub and started discussion
Likely almost fixed
Fixed and published in https://github.com/KhronosGroup/glTF/tree/master/specification/2.0 or https://github.com/KhronosGroup/glTF/tree/master/extensions

KHR_materials_unlit is in the phase 2?

donmccurdy · 2018-01-21T22:43:25Z

Yes, I would say KHR_materials_unlit is in "likely almost fixed" phase. The list of extensions in glTF/extensions are not necessarily farther along though, e.g. KHR_technique_webgl is still a "draft" and further from finished than this one, even though this one is still a PR.

As far as when to merge, I don't have a strong preference. I think I was too early with KHR_lights in glTF 2.0 as that one is still changing, but fewer moving parts in an unlit extension. :)

takahirox · 2018-01-22T08:52:42Z

I see the status, thanks.

How about merging for r91?

(If I guess right) seems like r90 will be released soon, and there seems be a discussion left a bit for KHR_materials_unlit KhronosGroup/glTF#1163 (comment)

Update: oops I've just realized that this PR has been already added to the r91 milestone

donmccurdy · 2018-01-22T19:00:30Z

Sure, r91 seems fine to me.

zellski · 2018-01-23T21:05:41Z

I believe if material.emissiveFactor or material.emissiveTexture are present in the glTF file, they will override materialParams.color and materialParams.map a little further down in the same function. The extension PR not explicit about what should happen if emissive properties are present (except to suggest they should not be), but it does request:

color = <baseColorTerm>

which suggests the emissive terms should be explicitly ignored in the presence of this extension. Thoughts?

donmccurdy · 2018-01-23T21:52:03Z

As the extension is written, agreed that emissive should be stripped. Would also be open to removing that stipulation in the spec, since emissive isn't necessarily incompatible with an unlit material.

zellski · 2018-01-23T22:19:14Z

I've come across the conflict only once, and it was clearly a mistake -- someone had left emissiveFactor: [0, 0, 0] in place, and they couldn't work out why everything was pitch black even though KHR_materials_unlit was in place, and the baseColor* properties were correct. While it was a little perplexing that emissive was still relevant, this is clearly an edge case.

Are there good reasons to actually include emissive elements in the fallback material? I know I should ask this in the extension PR, but I'm terrified of disrupting the fragile tranquility that we've achieved there. :-)

donmccurdy · 2018-01-23T22:34:02Z

I can't think of a reason that emissive would be used in the fallback material, since it would necessarily be added to baseColor anyway... The use case I envisioned was that someone might want an emissive map on their (supported) unlit material, e.g. an unlit VR mobile scene with glowy lights. But choosing unlit for performance and then also wanting post processing might be an unlikely combination, I'm not sure...

zellski · 2018-01-24T05:18:58Z

examples/js/loaders/GLTFLoader.js

+		materialParams.color = new THREE.Color( 1.0, 1.0, 1.0 );
+		materialParams.opacity = 1.0;
+
+		if ( Array.isArray( metallicRoughness.baseColorFactor ) ) {


At this point, we don't actually know that metallicRoughness is defined, do we? The material.pbrMetallicRoughness !== undefined test in the calling function happens after the call to this extension. I think this would NPE in a (weird but valid) material lacking that particular subobject.=

donmccurdy · 2018-01-24T16:09:20Z

About emissive, a bit of old code left over from glTF 1.0:

if ( materialDef.emissiveFactor !== undefined ) {

  if ( materialType === THREE.MeshBasicMaterial ) {

    materialParams.color = new THREE.Color().fromArray( materialDef.emissiveFactor );

  } else {

    materialParams.emissive = new THREE.Color().fromArray( materialDef.emissiveFactor );

  }

}

overwriting materialParams.color is not what we want, for now I think that should be:

if ( materialDef.emissiveFactor !== undefined ) {

  materialParams.emissive = new THREE.Color().fromArray( materialDef.emissiveFactor );

}

Whether emissive should be stripped or allowed I will ask on the extension PR.

EDIT: oh, MeshBasicMaterial does not support emissive at all...

WestLangley · 2018-01-24T16:32:08Z

MeshBasicMaterial does not support emissive at all...

We could discuss adding it, but first see if you can reasonably avoid needing this feature.

donmccurdy · 2018-01-24T20:25:51Z

We can avoid needing the feature.

In that case I propose changing the above code to:

if ( materialDef.emissiveFactor !== undefined && materialType !== THREE.MeshBasicMaterial ) {

  materialParams.emissive = new THREE.Color().fromArray( materialDef.emissiveFactor );

}

robertlong · 2018-01-26T03:10:02Z

@donmccurdy I made the changes you recommended and also removed setting occlusion and normal maps on the MeshBasicMaterial.

I also removed the .glb model. Is the example correct now? It feels a bit odd that nothing displays until you select the unlit material extension from the dropdown.

donmccurdy · 2018-01-26T04:04:13Z

Thanks!

I also removed the .glb model. Is the example correct now? It feels a bit odd that nothing displays until you select the unlit material extension from the dropdown.

Yeah, really the demo should default to the first available extension but that's a bug in our demo setup, and we can fix it later.

donmccurdy · 2018-01-26T04:12:02Z

Updated demo: https://khr-materials-unlit-ooddguuati.now.sh/

Vertex colors and (lack of) emissive in Quill example look good now:

zellski · 2018-01-27T05:39:13Z

One tiny nit: if you are leaving KHR_materials_common support in for now, given that its days are probably numbered, does it make sense to check for KHR_materials_unlit first? It seems both the more specific request on behalf of the author, and also the more likely-to-survive-the-coming-culling way to go.

donmccurdy · 2018-01-27T07:31:50Z

Will remove it shortly: #13169

donmccurdy · 2018-02-19T19:11:41Z

I think the KHR_materials_unlit extension is stable enough to support in three.js now. Here is a change set on the Blender exporter for testing: KhronosGroup/glTF-Blender-Exporter#160

@robertlong could you rebase this PR?

…al maps.

robertlong · 2018-02-21T18:57:34Z

I rebased the PR and made the glTF extensions example default to the model of the first available extension.

I think this should be good to go now.

donmccurdy · 2018-02-21T18:59:56Z

Oops, I think the rebase merging has brought back the (deleted) GLTFMaterialsCommonExtension.

robertlong · 2018-02-21T19:08:13Z

Oops, yeah I didn't see that the GLTFMaterialsCommonExtension made it's way back in. I've removed it. Thanks!

donmccurdy · 2018-02-21T19:29:50Z

examples/webgl_loader_gltf_extensions.html

+					objectRotation: new THREE.Euler(0, 0, 0),
+					addLights:true,
+					extensions: ['glTF-MaterialsUnlit'],
+					addEnvMap: true


nit: No need for addEnvMap: true or addLights: true on this one.

donmccurdy · 2018-02-21T19:30:50Z

Thanks! Looks good to me. 🚀

mrdoob · 2018-02-21T22:37:37Z

Thanks!

takahirox · 2018-03-04T21:41:21Z

IMO, the unlit example is a bit hard to see. Better to change background?

robertlong mentioned this pull request Jan 19, 2018

KHR_materials_unlit KhronosGroup/glTF#1163

Merged

mrdoob requested a review from donmccurdy January 19, 2018 03:20

mrdoob added this to the r91 milestone Jan 19, 2018

zellski reviewed Jan 24, 2018

View reviewed changes

donmccurdy mentioned this pull request Jan 24, 2018

GLTFLoader: Remove old or unstable extensions. #13169

Merged

robertlong added 4 commits February 20, 2018 18:29

GLTFLoader KHR_materials_unlit extension support.

a14ecde

Check for pbrMetallicRoughness and prevent applying additional materi…

d3c65c1

…al maps.

Remove .glb model. Update .gltf model. Update example.

f12f1fe

Default to first available extension.

1a08472

robertlong force-pushed the feature/gltf-KHR_materials_unlit branch from 6679cba to 1a08472 Compare February 21, 2018 02:57

Remove KHR_materials_common

eb9b990

donmccurdy reviewed Feb 21, 2018

View reviewed changes

donmccurdy approved these changes Feb 21, 2018

View reviewed changes

mrdoob merged commit fc102a3 into mrdoob:dev Feb 21, 2018

GLTFLoader KHR_materials_unlit extension support. #13136

GLTFLoader KHR_materials_unlit extension support. #13136

Conversation

robertlong commented Jan 19, 2018

donmccurdy commented Jan 19, 2018 • edited Loading

mrdoob commented Jan 20, 2018

donmccurdy commented Jan 20, 2018

mrdoob commented Jan 20, 2018

donmccurdy commented Jan 20, 2018

WestLangley commented Jan 20, 2018

donmccurdy commented Jan 20, 2018 • edited Loading

mrdoob commented Jan 20, 2018

WestLangley commented Jan 20, 2018

donmccurdy commented Jan 20, 2018 • edited Loading

takahirox commented Jan 20, 2018 • edited Loading

donmccurdy commented Jan 21, 2018 • edited Loading

takahirox commented Jan 22, 2018 • edited Loading

donmccurdy commented Jan 22, 2018

zellski commented Jan 23, 2018

donmccurdy commented Jan 23, 2018

zellski commented Jan 23, 2018

donmccurdy commented Jan 23, 2018

zellski Jan 24, 2018

Choose a reason for hiding this comment

donmccurdy commented Jan 24, 2018 • edited Loading

WestLangley commented Jan 24, 2018

donmccurdy commented Jan 24, 2018

robertlong commented Jan 26, 2018

donmccurdy commented Jan 26, 2018

donmccurdy commented Jan 26, 2018

zellski commented Jan 27, 2018

donmccurdy commented Jan 27, 2018

donmccurdy commented Feb 19, 2018

robertlong commented Feb 21, 2018

donmccurdy commented Feb 21, 2018

robertlong commented Feb 21, 2018

donmccurdy Feb 21, 2018

Choose a reason for hiding this comment

donmccurdy commented Feb 21, 2018

mrdoob commented Feb 21, 2018

takahirox commented Mar 4, 2018

donmccurdy commented Jan 19, 2018 •

edited

Loading

donmccurdy commented Jan 20, 2018 •

edited

Loading

donmccurdy commented Jan 20, 2018 •

edited

Loading

takahirox commented Jan 20, 2018 •

edited

Loading

donmccurdy commented Jan 21, 2018 •

edited

Loading

takahirox commented Jan 22, 2018 •

edited

Loading

donmccurdy commented Jan 24, 2018 •

edited

Loading