Update gltf Pipeline and support KHR_techniques_webgl and KHR_blend #6805

ggetz · 2018-07-16T21:14:39Z

Part of (fixes?) CesiumGS/gltf-pipeline#330

Update ThirdParty/gltfPipeline files to use the 2.0 branch
Support the KHR_techniques_webgl extension
Support the KHR_blend extension
Fixes Remove pipeline extras for glTF #5776, removes added pipeline extras.

Ran through the Sandcastle examples to check the new updates, and the one issue I see is with classification (in this sandcastle demo). The classification model is still based on glTF 1.0, and the indices seem to be wrong. Any ideas?

Incorporates #6803 to fix bounding spheres, merge that first.

TODO:

Include fix from Update sample tilesets to version 1.0 #6857 (comment)
Fix Specs
Additional Specs
Update CHANGES.md

…lend extensions

cesium-concierge · 2018-07-16T21:14:41Z

Thanks for the pull request @ggetz!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Changes to third party files were made.
- Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

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

Overall looks pretty good. I like the organizational and cleanup changes, the more the better since it's not often that Model is updated so heavily.

For testing I would recommend:

Run all model and 3D Tiles Sandcastle demos
Test changing a model's color or clipping planes to make sure shader regeneration still works
Load all 1.0 and 2.0 models in https://github.com/KhronosGroup/glTF-Sample-Models and Polly

Some other changes to make:

Some small edits have been made in gltf-pipeline since this PR was opened - make sure all files are up to date before we merge this.
Handle the ALPHA_MASK uniform for KHR_techniques_webgl (I saw there was a TODO for that)

To be safe, let's merge this after August 1st.

lilleyse · 2018-07-18T14:50:39Z

Source/Scene/ClassificationModel.js

@@ -124,25 +124,25 @@ define([



It's probably worth doing a pass over the documentation to fix some of the existing problems like:

References the old KHR_binary_glTF extension

Some errors that are old

@exception {DeveloperError} bgltf is not a valid Binary glTF file.

@exception {DeveloperError} Only glTF Binary version 1 is supported.

@param {Object|ArrayBuffer|Uint8Array} options.gltf The object for the glTF JSON or an arraybuffer of Binary glTF defined by the KHR_binary_glTF extension. - only binary glTF is supported.

Also for Model and Model.fromGltf.

lilleyse · 2018-07-18T14:53:15Z

Source/Scene/ClassificationModel.js

-        var meshId = gltfNode.mesh;
-        if (gltfNodes.length !== 1 || !defined(meshId)) {
+        var gltfNode = gltfNodes.rootNode;
+        if (!defined(gltfNode) || !defined(gltfNode.meshes)) {


The classification model is still based on glTF 1.0, and the indices seem to be wrong. Any ideas?

It looks like parseBinaryGltf used to also update the version but parseGlb does not. Make sure to call updateVersion and addDefaults after parseGlb.

After fixing that I suspect a lot of the other code changes below may not be needed.

lilleyse · 2018-07-18T15:01:14Z

Source/Scene/ClassificationModel.js

@@ -646,7 +646,9 @@ define([
    }


Above in parseBuffers, and throughout - keep an eye out for ways to clean up the code using gltf-pipeline helper functions. For example, parseBuffers can use ForEach.buffer. Also it may not need the extras._pipeline lines.

buffer.extras = defaultValue(buffer.extras, {}); buffer.extras._pipeline = defaultValue(buffer.extras._pipeline, {});

Similarly ForEach.meshPrimitiveAttribute in createVertexArray.

lilleyse · 2018-07-18T15:13:04Z

Source/Scene/ClassificationModel.js

-                var parameterName = uniforms[name];
+        ForEach.technique(model.gltf, function(technique) {
+            var parameters = technique.parameters;
+            ForEach.techniqueUniform(technique, function(parameterName, uniformName) {


This looks like another area that is depending on the glTF being 1.0. Normally a 2.0 model with the techniques extension would not have parameters.

lilleyse · 2018-07-18T19:48:50Z

Source/Scene/Model.js

                    updateVersion(this.gltf);
                    if (defined(this.gltf.asset) && defined(this.gltf.asset.extras) &&
-                        this.gltf.asset.extras.gltf_pipeline_upgrade_10to20) {
-                            this._forwardAxis = Axis.X;
+                            this.gltf.asset.extras.gltf_pipeline_upgrade_10to20) {


This is one thing that got lost between 2.0-cesium and 2.0 in gltf-pipeline.

CesiumGS/gltf-pipeline#371

One change - add gltf_pipeline_upgrade_10to20 to gltf.extras._pipeline instead of gltf.asset.extras

lilleyse · 2018-07-23T13:30:49Z

Source/Scene/Model.js

@@ -2250,7 +2265,7 @@ define([
        var tx;


Above in createSamplers, use ForEach.sampler.

lilleyse · 2018-07-23T13:54:26Z

Source/Scene/Model.js

@@ -2528,7 +2545,7 @@ define([
                stopTime : stopTime,
                channelEvaluators : channelEvaluators
            };
-        }
+        });
    }

    function createVertexArrays(model, context) {


Below in createVertexArrays, use ForEach.mesh, ForEach.meshPrimitive, and ForEach.meshPrimitiveAttribute

lilleyse · 2018-07-23T13:58:31Z

Source/Scene/Model.js

+            // semantic or targeted via a source (for animation) are not
+            // targetable for material animations.  Is this too strict?
+            //
+            // https://github.com/KhronosGroup/glTF/issues/142


Are these GLTF_SPEC comments here (and others throughout) still relevant?

lilleyse · 2018-07-23T14:06:19Z

Source/Scene/Model.js

-                shader = modifyShader(shader, id, model._vertexShaderLoaded);
-                toClipCoordinates[id] = ModelUtility.toClipCoordinatesGLSL(model.gltf, shader);
+                shader = modifyShader(shader, programId, model._vertexShaderLoaded);
+                toClipCoordinates[programId] = ModelUtility.toClipCoordinatesGLSL(model.gltf, shader);
            }
        }



Right below - model.gltf.glExtensionsUsed - this is now part of the techniques_webgl extension.

lilleyse · 2018-07-23T14:24:38Z

Source/Scene/ModelUtility.js

-        return undefined;
+
+        return function(parameterName, attributeOrUniformName) {
+            var attributeOrUniform = technique.parameters[parameterName];


Is this block needed? The KHR_techniques_webgl extension should be defined.

lilleyse · 2018-07-31T22:16:38Z

@ggetz when this is ready can you publish a new version of the power plant tileset to ion?

I checked that upgrade converts it correctly and it loads in this branch. That's really good because it uses pretty complicated glTF techniques and shaders.

lilleyse · 2018-08-09T01:27:25Z

Related to #6805 (comment), since gltf-pipeline no longer adds pipeline extras to everything there is now a bug when a model has an unused material. A few areas of the code try to access material.extras._pipeline.primitive when extras._pipeline doesn't exist.

CUBE.gltf.txt

This model reveals another bug related to primitive.material being undefined. I'll open a gltf-pipeline fix for that.

ggetz · 2018-08-20T20:36:32Z

@lilleyse Other than specs which I'm working on now, this should be updated. I handle ALPHA_MASK and have the latest build of gltf-pipeline here.

I removed pipeline extras where applicable, but they are needed for updateVersion. Also fixed the bug related to material.extras._pipeline.primitive

Tested the sample models as well as Polly.

lilleyse · 2018-08-22T17:15:33Z

Thanks @ggetz, I hope to look at this later today.

lilleyse · 2018-08-26T22:29:58Z

Source/Scene/ClassificationModel.js

@@ -103,17 +94,15 @@ define([
     * @private
     *
     * @param {Object} [options] Object with the following properties:
-     * @param {Object|ArrayBuffer|Uint8Array} options.gltf The object for the glTF JSON or an arraybuffer of Binary glTF defined by the KHR_binary_glTF extension.
-     * @param {Resource|String} [options.basePath=''] The base path that paths in the glTF JSON are relative to.
+     * @param {ArrayBuffer|Uint8Array} [options.gltf] A binary glTF buffer.


options and options.gltf are required

lilleyse · 2018-08-26T22:33:03Z

Source/Scene/Model.js

@@ -242,13 +225,15 @@ define([
     * Cesium supports glTF assets with the following extensions:
     * <ul>
     * <li>
-     * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/1.0/Khronos/KHR_binary_glTF/README.md|KHR_binary_glTF}
-     * </li><li>


Technically this can be left in since we do support that extension.

lilleyse · 2018-08-26T22:36:00Z

Source/Scene/Model.js

        });
    }

    function parsePrograms(model) {
-        ForEach.program(model.gltf, function(program, id) {
-            model._loadResources.programsToCreate.enqueue(id);
+        var sourcePrograms = model._sourcePrograms = {};


It would be better to initialize _sourcePrograms in the constructor where _sourceTechniques and _sourceShaders are also initialized.

lilleyse · 2018-08-26T22:41:48Z

Source/Scene/Model.js

@@ -1499,6 +1373,9 @@ define([
        var buffers = gltf.buffers;
        var bufferViews = gltf.bufferViews;
        ForEach.shader(gltf, function(shader, id) {
+            // retain reference to source shader text
+            var sourceShader = model._sourceShaders[id] = clone(shader.extras._pipeline.source);


I don't think clone is needed for a string.

lilleyse · 2018-08-26T22:43:57Z

Source/Scene/Model.js

-                    if (defined(this.gltf.asset) && defined(this.gltf.asset.extras) &&
-                        this.gltf.asset.extras.gltf_pipeline_upgrade_10to20) {
-                            this._forwardAxis = Axis.X;
+                    if (this.gltf.extras._pipeline.gltf_pipeline_upgrade_10to20) {


There will need to be a small PR into gltf-pipeline that adds this property in updateVersion.

This was from merging in the previous forward axis fixes. I would rather do a check of the original version number before the update than update gltf-pipeline itself.

Ah, that sounds like a better approach.

lilleyse · 2018-08-26T22:55:43Z

Source/Scene/Model.js

-        rendererRenderStates[id] = RenderState.fromCache({
-            frontFace : defined(statesFunctions.frontFace) ? statesFunctions.frontFace[0] : WebGLConstants.CCW,
+        var enableCulling = !material.doubleSided;
+        var blendingEnabled = (material.alphaMode !== 'OPAQUE');


See #6805 (comment)

I think this should actually be material.alphaMode === 'BLEND'); since MASK does not require blending to be enabled, just that discard is called in the shader.

It still needs the depth states, I will make a separate path though for MASK.

MASK can use the same depth states as OPAQUE.

Sorry, my mistake, it was blending that needs to be enabled if alphaMode is MASK. Otherwise this model will not render correctly:

Ok I thought the PBR shader was calling discard if the alpha value was below the cutoff, but it looks like it isn't. Tweaking the shader to use discard would be preferable to enabling blending for MASK.

lilleyse · 2018-08-26T23:03:31Z

Source/Scene/Model.js

@@ -4314,6 +4101,8 @@ define([
            }

            if (loadResources.finished()) {
+                ModelUtility.removePipelineExtras(this.gltf);


Is ModelUtility.removePipelineExtras still needed? Can gltf-pipeline's removePipelineExtras be used now instead?

ggetz · 2018-09-13T21:36:30Z

@lilleyse, this should be ready for another look. I've incorporated the changes made via gltf-pipeline, fixed a bug I noticed with model caching and the forward axis, fixed the current specs, and added additional specs for loading 2.0 models and 2.0 models using the techniques extension.

lilleyse · 2018-09-17T17:42:22Z

Thanks @ggetz, I will do some final testing later today.

Support glTF Specular Glossiness Extension

lilleyse · 2018-09-18T00:10:26Z

Source/Scene/Model.js

+                    if (!this._loadRendererResourcesFromCache) {
+                        var gltf = this.gltf;
+                        // Add the original version so it remains cached
+                        gltf.extras.sourceVersion = gltf.asset.version;


If it still works, maybe it is cleaner to store the source version as a property on model rather than extras?

I'll have to set up a new cache just for the version number since it's the processed gltf object which becomes cached. That seems messier than adding it to extras to me, but I can if we really want to avoid adding it.

Ok, that makes sense. The method here sounds a lot easier for caching than my suggestion.

lilleyse · 2018-09-18T00:12:08Z

Source/Scene/Model.js

+                    if (!this._loadRendererResourcesFromCache) {
+                        var gltf = this.gltf;
+                        // Add the original version so it remains cached
+                        gltf.extras.sourceVersion = gltf.asset.version;


I ran into an old tileset that doesn't actually set gltf.asset which is technically valid for gltf 1.0. The code here and in ModelUtility should account for that and assume 1.0 if asset is undefined.

lilleyse · 2018-09-18T00:22:57Z

Source/Scene/processPbrMaterials.js

+        // so we should apply PBR unless no materials are found.
+        if (gltf.materials.length === 0) {
+            return gltf;
+        }


Hit a weird edge case where a gltf didn't have any meshes so it didn't even create any default materials and failed at this line. I guess a quick check that gltf.materials is defined should fix this. CC @OmarShehata.

lilleyse · 2018-09-18T00:34:56Z

I'm seeing three test failures:

Scene/ModelInstanceCollection renders rigged figure
Scene/ModelInstanceCollection renders with animations when instancing is disabled
Scene/Model renders with a silhouette

The last two pass when run individually.

lilleyse · 2018-09-18T00:39:02Z

Source/Scene/Model.js

+     * </li><li>
+     * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_techniques_webgl/README.md|KHR_techniques_webgl}
+     * </li><li>
+     * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_blend/README.md|KHR_blend}


Add KHR_materials_unlit and KHR_materials_pbrSpecularGlossiness to this list. Forgot to add those in the other PRs.

lilleyse · 2018-09-18T00:39:22Z

Source/Scene/Model.js

+     * </li><li>
+     * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_techniques_webgl/README.md|KHR_techniques_webgl}
+     * </li><li>
+     * {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_blend/README.md|KHR_blend}


Also add those two extensions here.

lilleyse · 2018-09-18T00:55:44Z

I think that's it from me. Besides those edge cases I haven't seen any other issues. 👍

ggetz · 2018-09-18T19:42:26Z

Specs/Scene/ModelSpec.js

+
+            scene.renderForSpecs();
+        });
+    });


@OmarShehata The above 3 tests have no expectations and don't remove the model. This is causing the last of the failing tests in this branch. Can you take a look soon? We're trying to get this merged early so it has some time in master before the release.

Fix tests in #6805

ggetz · 2018-09-19T17:20:25Z

Thanks @lilleyse! Tests are all cleaned up, and your feedback has been addressed.

lilleyse · 2018-09-19T18:04:57Z

Thanks @ggetz. Awesome to get this in!

emackey · 2018-09-19T19:06:38Z

Congrats @ggetz, @OmarShehata, and everyone involved. It's great to have the PBR shader generation moved into core Cesium like this.

ggetz added 2 commits July 16, 2018 16:13

Update gltf pipeline files and support KHR_techniques_webgl and KHR_b…

be6fffa

…lend extensions

Cleanup

9d907df

ggetz requested a review from lilleyse July 16, 2018 21:14

Doc edits

e82b4b6

lilleyse reviewed Jul 23, 2018

View reviewed changes

lilleyse and others added 2 commits July 23, 2018 17:48

Minor cleanup

395eb34

Cleanup model with new gltf-pipeline

173f4b5

This was referenced Jul 30, 2018

glTF 2.0 in samples generator CesiumGS/3d-tiles-validator#139

Merged

Update sample tilesets to version 1.0 #6857

Merged

Update ion tilesets to 3D Tiles 1.0 #6862

Merged

ggetz added 6 commits August 2, 2018 16:50

Simplify render states

923874b

Fix ClassificationModel, factor out duplicate code

7abc5c7

Various gltf cleanup

1e5d67f

Merge branch 'master' into gltf-update-pipeline

58f99b4

alphaCutoff uniform, attribute location fix

6de38fe

Update gltf-pipeline

5651d79

lilleyse mentioned this pull request Aug 10, 2018

Fix model crash by adding default pbr material #6906

Merged

ggetz added the priority - next release label Aug 16, 2018

This was referenced Aug 18, 2018

KHR_techniques_webgl missing after updating model from gltf v1.0 to gltf v2.0 CesiumGS/gltf-pipeline#417

Closed

Unsupported extension: KHR_techniques_webgl KhronosGroup/glTF-Validator#95

Closed

ggetz added 2 commits August 20, 2018 14:59

Update pipeline, resolve material errors

c098115

Remove pipeline extras where possible

be83cc2

This was referenced Aug 20, 2018

Cesium no longer adds a default material for gltf 2.0 as needed #6891

Closed

An error occurred while rendering. Rendering has stopped #6940

Closed

lilleyse reviewed Aug 26, 2018

View reviewed changes

Shehata and others added 4 commits September 13, 2018 16:05

Fix merge conflict

05ef68d

Fix failing tests

0e63533

Merge branch 'master' into gltf-update-pipeline

3cad9ea

Update CHANGES.md

b137ab4

Omar Shehata and others added 3 commits September 17, 2018 14:24

Simplify check for PBR materials

2de8d69

No need to account for glTF 1.0

4d5d53f

Merge pull request #7006 from OmarShehata/specgloss-ext

e88ac9d

Support glTF Specular Glossiness Extension

lilleyse reviewed Sep 18, 2018

View reviewed changes

ggetz added 3 commits September 18, 2018 10:25

Fix glTF 1.0 asset version and no mesh edge cases

88ebec6

Fix modelInstanceCollection spec

4c168a9

Update model specs

015e700

ggetz commented Sep 19, 2018

View reviewed changes

Omar Shehata added 2 commits September 19, 2018 10:12

Clean up after test

e5e495c

Added expectations to spec gloss tests

f78b4f5

OmarShehata mentioned this pull request Sep 19, 2018

Fix tests in #6805 #7050

Merged

Omar Shehata and others added 3 commits September 19, 2018 10:42

Use defaultValue

125c36f

2 -> 2.0

fb69626

Merge pull request #7050 from OmarShehata/gltf-update-pipeline

c0eaf28

Fix tests in #6805

lilleyse merged commit 0ffd273 into master Sep 19, 2018

lilleyse deleted the gltf-update-pipeline branch September 19, 2018 18:05

OmarShehata mentioned this pull request Dec 17, 2018

Re-support glTF 1.0 premultipliedAlpha #7419

Closed

ggetz mentioned this pull request Jan 28, 2019

KHR_blend KhronosGroup/glTF#1302

Closed

4 tasks

Update gltf Pipeline and support KHR_techniques_webgl and KHR_blend #6805

Update gltf Pipeline and support KHR_techniques_webgl and KHR_blend #6805

Conversation

ggetz commented Jul 16, 2018 • edited

cesium-concierge commented Jul 16, 2018

lilleyse left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jul 31, 2018

lilleyse commented Aug 9, 2018

ggetz commented Aug 20, 2018

lilleyse commented Aug 22, 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

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

Choose a reason for hiding this comment

lilleyse Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Sep 13, 2018

lilleyse commented Sep 17, 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

Choose a reason for hiding this comment

lilleyse commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Sep 18, 2018

Choose a reason for hiding this comment

ggetz commented Sep 19, 2018

lilleyse commented Sep 19, 2018

emackey commented Sep 19, 2018

ggetz commented Jul 16, 2018 •

edited

lilleyse Aug 27, 2018 •

edited