Add support for KHR_materials_unlit #6977

OmarShehata · 2018-08-30T17:48:07Z

Resolves #6507.

As described in the spec, any glTF that has this extension defined as shown below will be rendered just using its base color.

"materials": [
    {
        "name": "unlit_with_fallback",
        "pbrMetallicRoughness": {
            "baseColorFactor": [ 1.0, 1.0, 1.0, 1.0 ],
            "baseColorTexture": { "index": 0 },
            "roughnessFactor": 0.9,
            "metallicFactor": 0.0
        },
        "extensions": {
            "KHR_materials_unlit": {}
        }
    }
]

I implemented this by simply setting hasNormals and hasTangents, which generates a simple shader with no lighting.

Here is Suzanne with PBR.

And with the unlit extension.

Unlit materials are affected by shadows. This is viewer.shadows = true.

@lilleyse any feedback is welcome! This is the current shader that gets generated:

void main(void) 
{
    vec4 baseColorWithAlpha = u_baseColorFactor;
    vec3 baseColor = baseColorWithAlpha.rgb;
    vec3 color = baseColor;
    color += u_emissiveFactor;
    color = LINEARtoSRGB(color);
    gl_FragColor = vec4(color, 1.0);
}

Should it be using emissive factor?
Are there other potential code paths to test?
I couldn't find any tests for processPbrMetallicRoughness in this branch.

cesium-concierge · 2018-08-30T17:48:08Z

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.

🌍 🌎 🌏

emackey · 2018-08-30T18:30:22Z

@OmarShehata Unlit materials should not apply emissive. KhronosGroup/glTF#1391

emackey · 2018-08-30T18:36:58Z

Also KHR_materials_unlit should be added to the list of extensions allowed to appear in extensionsRequired. See ModelUtility.checkSupportedExtensions.

OmarShehata · 2018-08-30T21:01:12Z

Thanks for the link @emackey ! I changed it to ignore emissive and occlusion as suggested there. The new shader looks like:

void main(void) 
{
    vec4 baseColorWithAlpha = u_baseColorFactor;
    vec3 baseColor = baseColorWithAlpha.rgb;
    vec3 color = baseColor;
    color = LINEARtoSRGB(color);
    gl_FragColor = vec4(color, 1.0);
}

Also KHR_materials_unlit should be added to the list of extensions allowed to appear in extensionsRequired. See ModelUtility.checkSupportedExtensions.

It should already be in there !

ggetz · 2018-08-30T21:04:22Z

@OmarShehata There's no explcit tests for the processX functions as it was just moved out of gltf-pipeline, but ModelSpec.js tests for rendering pbr materials.

emackey · 2018-08-30T21:07:22Z

@OmarShehata This looks good to me. One last thing to be aware of, currently Cesium is known to not be following the glTF spec which calls for calculating missing normals (issue #6506). If/when that bug gets fixed, we should make sure that normals are not calculated for unlit materials.

mramato · 2018-08-31T00:21:23Z

Source/Scene/processPbrMetallicRoughness.js

@@ -660,6 +670,8 @@ define([
        }
        fragmentShader += '}\n';

+        console.log(fragmentShader);


Accidental debug code?

Yes, sorry. Thanks for catching that!

mramato · 2018-08-31T00:22:26Z

Source/Scene/processPbrMetallicRoughness.js

            }
+            else if (defined(generatedMaterialValues.u_emissiveFactor)) {
+                    fragmentShader += '    color += u_emissiveFactor;\n';
+                }


Please run the auto-formatter on your blocks of changed code so that the source file stays consistently formatted.

mramato · 2018-08-31T00:23:33Z

Source/Scene/processPbrMetallicRoughness.js

            }
+            else if (defined(generatedMaterialValues.u_emissiveFactor)) {


else and else if blocks go on the same line as the previous brace } else if Refer to the https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide

OmarShehata · 2018-08-31T14:32:07Z

Thanks for the review Matt! @ggetz I added a test there to verify that it correctly renders the base color when using the unlit extension. I added a new glTF to test on. Since it was just a modified Box I put it in the same directory but let me know if that should go somewhere else.

ggetz

Thanks @OmarShehata, just those comments.

ggetz · 2018-08-31T14:41:13Z

Specs/Scene/ModelSpec.js

@@ -2692,6 +2693,21 @@ defineSuite([
        });
    });

+    fit('renders with the unlit extension', function() {


Remove the fit

ggetz · 2018-08-31T14:42:45Z

Specs/Scene/ModelSpec.js

@@ -120,6 +120,7 @@ defineSuite([

    var boomBoxUrl = './Data/Models/PBR/BoomBox/BoomBox.gltf';
    var boxPbrUrl = './Data/Models/PBR/Box/Box.gltf';
+    var boxPbrUnlitUrl = './Data/Models/PBR/Box/Box-Unlit.gltf';
    var boxAnimatedPbrUrl = './Data/Models/PBR/BoxAnimated/BoxAnimated.gltf';


Put the model in it's own folder like we do here. Also use camel case like we do for the other models. The path should be ./Data/Models/PBR/BoxUnlit/BoxUnlit.gltf. Just for consistency's sake.

OmarShehata · 2018-08-31T18:52:03Z

I also tested this with a base texture instead of a base color and it seems to correctly render the unlit texture. Here's the produced shader in that case.

void main(void) 
{
    vec4 baseColorWithAlpha = SRGBtoLINEAR4(texture2D(u_baseColorTexture, v_texcoord_0));
    baseColorWithAlpha *= u_baseColorFactor;
    vec3 baseColor = baseColorWithAlpha.rgb;
    vec3 color = baseColor;
    color = LINEARtoSRGB(color);
    gl_FragColor = vec4(color, 1.0);
}

emackey · 2018-08-31T20:34:52Z

Base texture shader logic looks good, thanks.

OmarShehata · 2018-09-04T18:42:16Z

@ggetz I've just verified that the CI errors in this branch are also erroring in the base branch. It's also peculiar because the test I added passes when ran individually but fails when ran together with the other tests in ModelSpec (I tried this with a few other tests in that file, so it's not just mine).

Do you know why/is this expected?

lilleyse · 2018-09-04T19:04:29Z

@OmarShehata the model tests can be a little weird at times and you may have missed resetting a model's show or something like that. Look at what other tests do to reset the state after they are finished.

ggetz · 2018-09-04T19:20:35Z

@OmarShehata There's an error in a previous test for Model.js in that branch which causes all subsequent tests to fail. I saw your test pass individually so you should be fine.

emackey · 2018-09-04T20:13:15Z

CHANGES.md

@@ -10,6 +10,7 @@ Change Log
 * Improved support for polygon entities using `perPositionHeight`, including supporting vertical polygons.  This also improves KML compatibility. [#6791](https://github.com/AnalyticalGraphicsInc/cesium/pull/6791)
 * Added `Cartesian3.midpoint` to compute the midpoint between two `Cartesian3` positions [#6836](https://github.com/AnalyticalGraphicsInc/cesium/pull/6836)
 * Added `equalsEpsilon` methods to `OrthographicFrustum`, `PerspectiveFrustum`, `OrthographicOffCenterFrustum` and `PerspectiveOffCenterFrustum`.
+* Added support for glTF extension [KHR_materials_unlit](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_materials_unlit) [#6977](https://github.com/AnalyticalGraphicsInc/cesium/pull/6977).


Looks like this isn't targeting master, as the changelog is 2 versions out of date as of today's release.

Yeah I'm merging into #6805 since that PR moves gltfPipeline from ThirdParty into Cesium core.

@OmarShehata Create a section for October's release and move your changes there. Thanks!

Oh, I see the problem now. I moved it into a new section for October. Thanks for pointing this out!

lilleyse · 2018-09-05T22:28:42Z

Took a quick look as well, looks good!

Omar Shehata added 2 commits August 30, 2018 12:22

Add KHR_materials_unlit to list of supported extensions

ccad16c

Turn off lighting when detects unlit extension

b136052

Updated changes.md

7a9faa7

Ignore unlit and occlusion when unlit

c56a339

emackey mentioned this pull request Aug 30, 2018

Generate normals automatically for glTF 2.0 models #6506

Open

mramato reviewed Aug 31, 2018

View reviewed changes

Omar Shehata added 2 commits August 31, 2018 10:01

Remove log and fix formatting

21caa93

Added test for unlit

7c28d2a

ggetz requested changes Aug 31, 2018

View reviewed changes

Moved BoxUnlit to its own folder

ba4d840

ggetz approved these changes Sep 4, 2018

View reviewed changes

emackey reviewed Sep 4, 2018

View reviewed changes

Shehata and others added 2 commits September 5, 2018 09:28

Moved changes.md text into October release

f3afd01

Merge branch 'gltf-update-pipeline' into gltf-unlit

974af0d

lilleyse merged commit 604400d into CesiumGS:gltf-update-pipeline Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for KHR_materials_unlit #6977

Add support for KHR_materials_unlit #6977

OmarShehata commented Aug 30, 2018

cesium-concierge commented Aug 30, 2018

emackey commented Aug 30, 2018

emackey commented Aug 30, 2018

OmarShehata commented Aug 30, 2018

ggetz commented Aug 30, 2018

emackey commented Aug 30, 2018

mramato Aug 31, 2018

OmarShehata Aug 31, 2018

mramato Aug 31, 2018

mramato Aug 31, 2018

OmarShehata commented Aug 31, 2018

ggetz left a comment

ggetz Aug 31, 2018

ggetz Aug 31, 2018

OmarShehata commented Aug 31, 2018

emackey commented Aug 31, 2018

OmarShehata commented Sep 4, 2018

lilleyse commented Sep 4, 2018

ggetz commented Sep 4, 2018

emackey Sep 4, 2018

OmarShehata Sep 5, 2018

ggetz Sep 5, 2018

OmarShehata Sep 5, 2018

lilleyse commented Sep 5, 2018

		}
		else if (defined(generatedMaterialValues.u_emissiveFactor)) {

Add support for KHR_materials_unlit #6977

Add support for KHR_materials_unlit #6977

Conversation

OmarShehata commented Aug 30, 2018

cesium-concierge commented Aug 30, 2018

emackey commented Aug 30, 2018

emackey commented Aug 30, 2018

OmarShehata commented Aug 30, 2018

ggetz commented Aug 30, 2018

emackey commented Aug 30, 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

OmarShehata commented Aug 31, 2018

ggetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Aug 31, 2018

emackey commented Aug 31, 2018

OmarShehata commented Sep 4, 2018

lilleyse commented Sep 4, 2018

ggetz commented Sep 4, 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

lilleyse commented Sep 5, 2018