Support glTF Specular Glossiness Extension #7006

OmarShehata · 2018-09-05T15:34:41Z

This PR adds support for the KHR_materials_pbrSpecularGlossiness glTF extension. Resolves #6921.

I still have to write tests, but I'm opening it now to get some early feedback.

It's currently implemented in processPbrMetallicRoughness.js because it seems implementation-wise the only difference between Metallic Roughness and Specular Glossiness is how the BRDF inputs are computed. It'll check for this extension and modify the relevant part of the shader, keeping the rest of it intact.

Since processPbrMetallicRoughness.js would now be doing both metallic-roughness and specular-glossiness, should it be renamed to something more general like processPbrMaterials.js ? The other option would be to abstract away most of processPbrMetallicRoughness.js into a common class that could be shared. @emackey what do you think?

I added a test model with spec-gloss in Specs/Data/Models/PBR/BoomBoxSpecularGlossiness/. Here's a snippet to test it locally in Sandcastle, and to switch between the metallic-roughness and the specular-glossiness versions of the model.

var viewer = new Cesium.Viewer('cesiumContainer', {
    selectionIndicator : false,
});

viewer.scene.globe.enableLighting = true;
viewer.scene.globe.depthTestAgainstTerrain = true;

var position = new Cesium.Cartesian3(-1371108.1511167218, -5508684.080096612, 2901825);
var heading = Cesium.Math.toRadians(180);
var pitch = Cesium.Math.toRadians(2);
var roll = Cesium.Math.toRadians(-6);
var hpr = new Cesium.HeadingPitchRoll(heading, pitch, roll);
var orientation = Cesium.Transforms.headingPitchRollQuaternion(position, hpr);

var bareGLTF = viewer.entities.add({
    position : position,
    orientation : orientation,
    model : {
        uri : '../../../Specs/Data/Models/PBR/BoomBox/BoomBox.gltf',
        heightReference : Cesium.HeightReference.RELATIVE_TO_GROUND ,
        minimumPixelSize : 512,
        maximumScale : 100,
        scale : 8.0,
        runAnimations : false
    }
});

viewer.trackedEntity = bareGLTF;

var position2 = new Cesium.Cartesian3(-1371100.6511167218, -5508684.080096612, 2901825);

var specGlossGLTF = viewer.entities.add({
    position : position2,
    orientation : orientation,
    model : {
        uri :'../../../Specs/Data/Models/PBR/BoomBoxSpecularGlossiness/BoomBox.gltf',
        heightReference : Cesium.HeightReference.RELATIVE_TO_GROUND ,
        minimumPixelSize : 512,
        maximumScale : 100,
        scale : 8.0,
        runAnimations : false
    }
});


var options = [{
    text : 'glTF',
    onselect : function() {
        viewer.trackedEntity = bareGLTF;
    }
}, {
    text : 'glTF Spec Gloss',
    onselect : function() {
        viewer.trackedEntity = specGlossGLTF;
    }
}];

Sandcastle.addToolbarMenu(options);

For this model, they look identical which I think is intended.

Adding a specularFactor: [1.0, 0.5, 0.5] gives the specular a reddish tone:

For reference, this is how Threejs implements it.

The only other thing is that the spec says:

In addition to the material properties, if a primitive specifies a vertex color using the attribute semantic property COLOR_0, then this value acts as an additional linear multiplier to diffuse.

@lilleyse do you know if this is something that's already handled?

cesium-concierge · 2018-09-05T15:34:43Z

Thanks for the pull request @OmarShehata!

✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

lilleyse · 2018-09-05T15:40:30Z

Very cool @OmarShehata!

In addition to the material properties, if a primitive specifies a vertex color using the attribute semantic property COLOR_0, then this value acts as an additional linear multiplier to diffuse.

@lilleyse do you know if this is something that's already handled?

Yeah we handle that already, for metallic roughness at least - https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/ThirdParty/GltfPipeline/processPbrMetallicRoughness.js#L529

emackey · 2018-09-05T19:41:50Z

Source/Scene/processPbrMetallicRoughness.js

+                fragmentShader += '    float alpha = pow((1.0 - glossiness), 2.0);\n';
+                fragmentShader += '    vec3 diffuseColor = diffuse.rgb * (1.0 - max(max(specular.r, specular.g), specular.b));\n';
+                fragmentShader += '    vec3 specularColor = specular;\n';
+                fragmentShader += '    float roughness = 1.0 - glossiness;\n';


Do this above the alpha calculation on line 638 above, then you can avoid the pow and just have float alpha = roughness * roughness.

emackey · 2018-09-06T15:51:11Z

Source/Scene/processPbrMetallicRoughness.js

+                        fragmentShader += '    vec3 specular = vec3(1.0);\n';
+                    }
+                    if (defined(generatedMaterialValues.u_glossinessFactor)) {
+                        fragmentShader += '    float glossiness *= clamp(u_glossinessFactor, 0.0, 1.0);\n';


You have float glossiness *= here, that doesn't compile.

I tried fixing this locally by removing the *. This works and shows my SpecGlossVsMetalRough new test model correctly!

emackey · 2018-09-06T15:53:45Z

Source/Scene/processPbrMetallicRoughness.js

                } else {
-                    fragmentShader += '    float metalness = 1.0;\n';
+                    if (defined(generatedMaterialValues.u_diffuseFactor)) {


eslint is complaining here, make sure you're set up to run the code style tools.

emackey · 2018-09-06T16:03:09Z

Need to merge in gltf-update-pipeline and resolve conflicts.

Left some comments above, otherwise looks great!

OmarShehata · 2018-09-07T21:32:45Z

Thanks for the review @emackey, and for the water bottle test case!

It should be all good now. The test cases I added check for being able to render a model when all parameters are provided, when none are provided and when no textures are provided, so it would have caught the shader compile error earlier.

I also renamed the class to processPbrMaterials because I think that better reflects what it does now.

OmarShehata · 2018-09-10T18:47:25Z

@emackey did you want to give this another look for the new commits? (Biggest change is adding tests and renaming the class).

emackey · 2018-09-10T21:06:59Z

Sure. Can't today, will try to carve out time tomorrow.

emackey

Looks good to me. Anyone else want to review?

emackey · 2018-09-11T17:32:01Z

Travis is having a ton of problems, I have trouble understanding when that's a fluke vs not.

OmarShehata · 2018-09-11T17:38:17Z

Thanks Ed! And yeah, I believe those are because the base branch gltf-update-pipeline doesn't have updated tests yet.

pjcozzi · 2018-09-12T15:22:22Z

Does anyone else need to look at this before it is merged? And what is needed for AnalyticalGraphicsInc:gltf-update-pipeline to be merged into master?

lilleyse

Just one minor comment. Looks good to me.

lilleyse · 2018-09-13T02:27:11Z

Source/Scene/processPbrMaterials.js

        var uniformName;
        var pbrMetallicRoughness = material.pbrMetallicRoughness;
-        if (defined(pbrMetallicRoughness)) {
-            for (var parameterName in pbrMetallicRoughness) {
+        var parameterName;


Minor style note:

var uniformName; var parameterName; var pbrMetallicRoughness = material.pbrMetallicRoughness;

looks cleaner than

var uniformName; var pbrMetallicRoughness = material.pbrMetallicRoughness; var parameterName;

lilleyse · 2018-09-13T02:35:06Z

And what is needed for AnalyticalGraphicsInc:gltf-update-pipeline to be merged into master?

There was one failing model (CesiumAir.gltf), but we identified the fix in gltf-pipeline and it just needs to be updated back in Cesium. I believe that was the last thing holding up the PR. @ggetz can confirm.

lilleyse · 2018-09-13T02:43:17Z

Source/Scene/processPbrMaterials.js

@@ -88,7 +88,7 @@ define([
        return gltf;
    }

-    function isPbrMaterial(material) {
+    function isMetallicRoughnessMaterial(material) {


This should actually not be renamed. The function is meant to catch if PBR shaders need to be generated. It should be catching whether the material uses metallic roughness, spec gloss, unlit, or the top-level 2.0 material properties. The purpose is to turn away materials that are using KHR_techniques_webgl or something else processPbrMaterials doesn't recognize.

One way to test is with a material like:

{ "extensions": { "KHR_materials_pbrSpecularGlossiness": {} } }

Which I don't think will get shaders generated for it right now.

This check probably also belongs before the hasPbrMetallicRoughness check:

// No need to create new techniques if they already exist, // the shader should handle these values if (hasExtension(gltf, 'KHR_techniques_webgl')) { return gltf; }

I feel like a lot of the material existence checking was a little messy before this PR and could use some cleanup.

Ah, good catch! I think it's helpful to have those two functions separately, so I might just add:

function isPbrMaterial(material){ return isMetallicRoughnessMaterial(material) || isSpecularGlossinessMaterial(material); }

Sounds good - though make sure sure the extreme edge case works too. This should produce a default PBR shader.

"materials": [ { } ]

Wow, you're right. I didn't realize glTF materials were all PBR by default. That extreme case currently doesn't work in master. It should be good here now.

So isMetallicRoughnessMaterial() isn't needed at all then. If any material is found, then it is metallic roughness, unless spec-gloss is also found.

Yeah it might not be needed at all.

OmarShehata · 2018-09-13T20:35:44Z

Thanks for the cleanup suggestions @lilleyse ! It should be good to merge now.

OmarShehata · 2018-09-17T15:38:24Z

@lilleyse have you gotten a chance to look at this yet/see if it's ready to merge?

lilleyse

Just one small comment and this is ready to merge.

lilleyse · 2018-09-17T17:35:14Z

Source/Scene/processPbrMaterials.js

+
+        if (!hasPbrMaterial) {
+            return gltf;
+        }


Simpler to just return early if there are no materials.

OmarShehata · 2018-09-17T18:30:35Z

@lilleyse I simplified that check, but to make it compatible with both glTF 2.0 and 1.0 I had to make sure to check if it's an array vs an object.

lilleyse · 2018-09-17T18:36:28Z

@OmarShehata the 2.0 vs 1.0 distinction isn't necessary. The model will always be 2.0 coming into this function.

OmarShehata · 2018-09-17T19:45:28Z

Ah, that makes it even simpler. Should be good now.

lilleyse · 2018-09-17T19:50:13Z

Thanks @OmarShehata!

Omar Shehata added 4 commits September 4, 2018 09:31

Added KHR_materials_pbrSpecularGlossiness to supported extensions

db96ceb

Add support for spec gloss

ca4dc6c

Added BoomBox SpecGloss model

6401433

Updated old boom box model

3dad95a

Added entry to CHANGES.md

6d8e5dc

emackey reviewed Sep 5, 2018

View reviewed changes

emackey mentioned this pull request Sep 6, 2018

Spec gloss vs metal rough KhronosGroup/glTF-Sample-Models#192

Merged

emackey reviewed Sep 6, 2018

View reviewed changes

OmarShehata and others added 6 commits September 7, 2018 14:04

Merge branch 'gltf-update-pipeline' into specgloss-ext

602db48

Fixed linting

73db740

Emackey's feedback

5fb072e

Rename to processPbrMaterials.js

d97b899

Added spec gloss tests

fcd4433

Add one more test

e59593a

emackey approved these changes Sep 11, 2018

View reviewed changes

lilleyse reviewed Sep 13, 2018

View reviewed changes

Shehata added 2 commits September 13, 2018 15:54

All glTF materials are PBR by default

5b94577

Fix merge conflict

05ef68d

Fix failing tests

0e63533

lilleyse reviewed Sep 17, 2018

View reviewed changes

Simplify check for PBR materials

2de8d69

No need to account for glTF 1.0

4d5d53f

lilleyse merged commit e88ac9d into CesiumGS:gltf-update-pipeline Sep 17, 2018

shunter mentioned this pull request Oct 14, 2019

Look into why CesiumJS release zip grew significantly larger in the 1.50 release #8284

Closed

OmarShehata mentioned this pull request Nov 14, 2019

could cesium add the technique "_3DTILESDIFFUSE" when using extension "KHR_materials_pbrSpecularGlossiness" #8396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support glTF Specular Glossiness Extension #7006

Support glTF Specular Glossiness Extension #7006

OmarShehata commented Sep 5, 2018 •

edited

Loading

cesium-concierge commented Sep 5, 2018

lilleyse commented Sep 5, 2018

emackey Sep 5, 2018

emackey Sep 6, 2018

emackey Sep 6, 2018

emackey Sep 6, 2018

emackey commented Sep 6, 2018

OmarShehata commented Sep 7, 2018

OmarShehata commented Sep 10, 2018

emackey commented Sep 10, 2018 via email

emackey left a comment

emackey commented Sep 11, 2018

OmarShehata commented Sep 11, 2018

pjcozzi commented Sep 12, 2018

lilleyse left a comment

lilleyse Sep 13, 2018

lilleyse commented Sep 13, 2018

lilleyse Sep 13, 2018 •

edited

Loading

lilleyse Sep 13, 2018

OmarShehata Sep 13, 2018

lilleyse Sep 13, 2018

OmarShehata Sep 13, 2018

lilleyse Sep 13, 2018

OmarShehata commented Sep 13, 2018

OmarShehata commented Sep 17, 2018

lilleyse left a comment •

edited

Loading

lilleyse Sep 17, 2018

OmarShehata commented Sep 17, 2018

lilleyse commented Sep 17, 2018

OmarShehata commented Sep 17, 2018

lilleyse commented Sep 17, 2018

Support glTF Specular Glossiness Extension #7006

Support glTF Specular Glossiness Extension #7006

Conversation

OmarShehata commented Sep 5, 2018 • edited Loading

cesium-concierge commented Sep 5, 2018

lilleyse commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emackey commented Sep 6, 2018

OmarShehata commented Sep 7, 2018

OmarShehata commented Sep 10, 2018

emackey commented Sep 10, 2018 via email

emackey left a comment

Choose a reason for hiding this comment

emackey commented Sep 11, 2018

OmarShehata commented Sep 11, 2018

pjcozzi commented Sep 12, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Sep 13, 2018

lilleyse Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Sep 13, 2018

OmarShehata commented Sep 17, 2018

lilleyse left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Sep 17, 2018

lilleyse commented Sep 17, 2018

OmarShehata commented Sep 17, 2018

lilleyse commented Sep 17, 2018

OmarShehata commented Sep 5, 2018 •

edited

Loading

lilleyse Sep 13, 2018 •

edited

Loading

lilleyse left a comment •

edited

Loading