Convert glTF 1.0 techniques to PBR materials by default

[lilleyse]

Previously we would convert glTF 1.0 to 2.0 with the KHR_techniques_webgl extension which preserves the techniques and shaders but is not widely supported by runtimes because of its implementation complexity.

This PR changes this behavior and tries to do a best effort upgrade from glTF 1.0 to glTF 2.0 with PBR materials. It does this by looking for common parameter names like "diffuse", "tex", etc and maps those to PBR material properties. This won't work for glTF 1.0 assets that rely on shader code for special lighting, billboard effects, or custom quantization/oct-decoding, but it works for most glTF 1.0 in the wild including all the sample models in https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/1.0

This whole process started because CesiumJS's glTF implementation is changing and can no longer support embedded shaders, but we still want to support as much of glTF 1.0 as possible. CesiumJS uses gltf-pipeline to upgrade glTF 1.0 to 2.0 already so it made sense to make the changes here. See https://github.com/CesiumGS/cesium/pull/10414 and https://github.com/CesiumGS/cesium/issues/10613 for more background.

For the previous behavior just pass in --keepLegacyExtensions

[lilleyse]

I still need to test this with more assets, but @javagl maybe you would like to review?

[lilleyse]

For testing - I wrote a script to convert all the glTF 1.0 sample models to 2.0: gltfConverter.js.zip

And used this sandcastle to compare the results in CesiumJS: sandcastle. The sandcastle is partially auto-generated by the script.

[lilleyse]

And just to point some the more noticeable breakages in the sandcastle:

• BoxSemantics color no longer has a blue tint - maybe something in the original shader is turning it blue?
• SmilingFace is now black - I think this is because of the presence of vertex colors, which are ignored in the glTF 1.0 shader
• Animation issues - being tracked in https://github.com/CesiumGS/cesium/issues/10621

[javagl]

The animation for BoxAnimated showed the error that is mentioned in https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BoxAnimated#common-problems .

I'm a bit stumped by this one, because ...

1. all versions in the test show the same behavior

2. Looking at the glTF 2, non-legacy output of the converter in ThreeJS shows some obscure result:

3. Looking at the same output in my JglTF viewer looks correct:

(except for the horrible PBR rendering... I never managed to allocate enough time for that one...)

4. Looking at the original glTF 2.0 model in the gltf-test looks correct:

https://cx20.github.io/gltf-test/examples/cesium/index.html?model=BoxAnimated

---

So there does not seem to be a fundamental problem with the animation handling in general, but the conversion ... does something that causes some viewers to glitch out...

This might, in fact, be related to (or have the same source as) https://github.com/CesiumGS/cesium/issues/10621

---

I just started tests, will go though other models and then drop another note here. I haven't started with an actual "code review" yet (and depending on the expected depth of that, may have to read a bit of code beyond the changes here), but can try to focus on the actual changes during the review.

[javagl]

Further test remarks:

---

BoxSemantics color no longer has a blue tint - maybe something in the original shader is turning it blue?

The color changed when rotating the model. Unfortunately, the README does not say what exacty is tested with this model, but apparently, it was only a test for semantics handling, by basically mixing a bunch of semantic-based values into the color ...

---

SmilingFace is now black - I think this is because of the presence of vertex colors, which are ignored in the glTF 1.0 shader

This might warrant a second look: The result of converting the glTF-Embedded and glTF-Binary version of this model does have the right colors (while the plain glTF one is indeed black)

---

I noticed a strange effect for the CesiumMan. It's tempting to think that this is related to the animation, but it looks more like some sort of z-Buffer issue (both for glTF and Model/glTF2):

This might be an issue of the conversion, but I wonder what could be causing this effect.

[lilleyse]

The Cesium Man artifact is related to https://github.com/CesiumGS/cesium/issues/6447#issuecomment-385552614. I think the glTF 2.0 version was fixed.

[javagl]

Yeah, it's difficult: The CesiumMan 1.0 apparently contains errors that cause this artifact (related to vertex weights, according to https://github.com/BabylonJS/Babylon.js/issues/4240#issuecomment-385524302 ), and of course, the converter will also produce invalid outputs then. Specifically, the converted CesiumMan has 1090 errors. But more generally, one could spend a considerable amount of time looking at the validation reports...

Assets with errors:
        C:\output\gltf-2.0\Avocado\glTF-Binary\Avocado.gltf
        C:\output\gltf-2.0\Avocado\glTF-Embedded\Avocado.gltf
        C:\output\gltf-2.0\BarramundiFish\glTF-Embedded\BarramundiFish.gltf
        C:\output\gltf-2.0\BarramundiFish\glTF-Binary\BarramundiFish.gltf
        C:\output\gltf-2.0\Avocado\glTF\Avocado.gltf
        C:\output\gltf-2.0\BarramundiFish\glTF\BarramundiFish.gltf
        C:\output\gltf-2.0\2CylinderEngine\glTF-Embedded\2CylinderEngine.gltf
        C:\output\gltf-2.0\2CylinderEngine\glTF-MaterialsCommon\2CylinderEngine.gltf
        C:\output\gltf-2.0\2CylinderEngine\glTF\2CylinderEngine.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-Embedded\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-Binary\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-MaterialsCommon\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMilkTruck\glTF\CesiumMilkTruck.gltf
        C:\output\gltf-2.0\Duck\glTF\Duck.gltf
        C:\output\gltf-2.0\Duck\glTF-Embedded\Duck_1_noScene.gltf
        C:\output\gltf-2.0\Duck\glTF-Embedded\Duck.gltf
        C:\output\gltf-2.0\Duck\glTF-MaterialsCommon\Duck.gltf
        C:\output\gltf-2.0\CesiumMilkTruck\glTF-Embedded\CesiumMilkTruck.gltf
        C:\output\gltf-2.0\CesiumMilkTruck\glTF-MaterialsCommon\CesiumMilkTruck.gltf
        C:\output\gltf-2.0\Buggy\glTF\Buggy.gltf
        C:\output\gltf-2.0\Monster\glTF\Monster.gltf
        C:\output\gltf-2.0\GearboxAssy\glTF\GearboxAssy.gltf
        C:\output\gltf-2.0\Buggy\glTF-Embedded\buggy.gltf
        C:\output\gltf-2.0\Monster\glTF-Binary\Monster.gltf
        C:\output\gltf-2.0\GearboxAssy\glTF-MaterialsCommon\GearboxAssy.gltf
        C:\output\gltf-2.0\Buggy\glTF-MaterialsCommon\Buggy.gltf
        C:\output\gltf-2.0\GearboxAssy\glTF-Embedded\GearboxAssy.gltf
        C:\output\gltf-2.0\Monster\glTF-Embedded\Monster.gltf
        C:\output\gltf-2.0\Monster\glTF-MaterialsCommon\Monster.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-Binary\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedSimple\glTF\RiggedSimple.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-Embedded\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-MaterialsCommon\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedSimple\glTF-Embedded\RiggedSimple.gltf
        C:\output\gltf-2.0\RiggedSimple\glTF-MaterialsCommon\RiggedSimple.gltf
        C:\output\gltf-2.0\SmilingFace\glTF\SmilingFace.gltf
        C:\output\gltf-2.0\SmilingFace\glTF-Binary\SmilingFace.gltf
        C:\output\gltf-2.0\SmilingFace\glTF-Embedded\SmilingFace.gltf
        C:\output\gltf-2.0\VC\glTF\VC.gltf
        C:\output\gltf-2.0\VC\glTF-Binary\VC.gltf
        C:\output\gltf-2.0\ReciprocatingSaw\glTF\ReciprocatingSaw.gltf
        C:\output\gltf-2.0\VC\glTF-Embedded\VC.gltf
        C:\output\gltf-2.0\ReciprocatingSaw\glTF-MaterialsCommon\ReciprocatingSaw.gltf
        C:\output\gltf-2.0\ReciprocatingSaw\glTF-Embedded\ReciprocatingSaw.gltf
        C:\output\gltf-2.0\VC\glTF-MaterialsCommon\VC.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-Binary\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-Embedded\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-MaterialsCommon\WalkingLady.gltf
        C:\output\gltf-2.0\BrainStem\glTF\BrainStem.gltf
        C:\output\gltf-2.0\BrainStem\glTF-Binary\BrainStem.gltf
        C:\output\gltf-2.0\BrainStem\glTF-MaterialsCommon\BrainStem.gltf
        C:\output\gltf-2.0\BrainStem\glTF-Embedded\BrainStem.gltf

... and try to figure out which of these errors are caused by errors in the input data, and which ones are caused by the converter ...

[javagl]

I'm still looking through the code, but there seems to have a test failure at

.  compressDracoMeshes
F    × applies quantization bits
      - Expected 214 to be less than 213.

[javagl]

Trying to zoom a bit into the BoxAnimated version: The following shows the structure of

• The glTF 1.0 input
• The converted glTF 2.0 output
• The current glTF 2.0 sample model version

The 1.0 version contains two animations. The converter converts this into two animations. In contrast to that, the current sample model version contains one animation (with two channels and two samplers).

This might be a structural change that cannot be handled by the converter. (I'd have to re-read the 'animations' parts of the 1.0 spec and the 2.0 spec to be sure about the semantics and intended behavior of both...)

(An aside: I verified that the relevant accessors contain all the same data in both version, so that's not the issue)

[javagl]

I was surprised to see that even the converted Duck.gltf caused validation errors, and had a closer look: The errors are from imprecise accessor.min/max values:

            {
                "code": "ACCESSOR_MIN_MISMATCH",
                "message": "Declared minimum value for this component (0.09929370135068893) does not match actual minimum (0.09929369390010834).",
                "severity": 0,
                "pointer": "/accessors/1/min/1"
            },

(several of that kind).

I assume that this is the reason for many of the validation errors that I summarized above.

I drafted a PR https://github.com/CesiumGS/gltf-pipeline/pull/620 that calls a function validatePresentAccessorMinMax, which updates the min/max values (iff they are present) with the ones that are freshly computed.

For the case of the Duck.gltf, this fixes the validation errors.

I can imagine that calling this function might be costly for larger models (and maybe it should be possible to enable/disable this step with a command line parameter or so...), and I'm not 100% sure whether that's the right place of calling it and so. But maybe it's worth considering this, just for the validity sake...

[lilleyse]

@javagl I think this is good to merge, but I just wanted to make sure I didn't miss anything actionable from your comments

[javagl]

Sorry, I should have tried to not comment so much as I went along, but rather try to write one, final, concise summary.

What's left:

Maybe low priority:

SmilingFace is now black - I think this is because of the presence of vertex colors, which are ignored in the glTF 1.0 shader

This might warrant a second look: The result of converting the glTF-Embedded and glTF-Binary version of this model does have the right colors (while the plain glTF one is indeed black)

What may be higher priority:

A test failure at

.  compressDracoMeshes
F    × applies quantization bits
      - Expected 214 to be less than 213.

[lilleyse]

This might warrant a second look: The result of converting the glTF-Embedded and glTF-Binary version of this model does have the right colors (while the plain glTF one is indeed black)

For whatever reason the glTF-Embeded and glTF-Binary versions do not have vertex colors, but the plain glTF does

[lilleyse]

. compressDracoMeshes F × applies quantization bits

• Expected 214 to be less than 213.

Ah, I knew I was missing something...

Strange that CI didn't pick up on this. Maybe node_modules is cached? I didn't get this test failure until I reinstalled the packages, which points to it being a recent change in Draco.

I just changed the test so it quantizes texcoords as well :shrug:

[javagl]

I tried the latest state, and the test passes (although I won't claim to have understood what the test was testing).

So I think that this PR could be merged.

---

The following is probably unrelated to this PR:

The validation on the output now yields

Assets with errors:
        C:\output\gltf-2.0\CesiumMan\glTF\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-Embedded\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-Binary\CesiumMan.gltf
        C:\output\gltf-2.0\CesiumMan\glTF-MaterialsCommon\CesiumMan.gltf
        C:\output\gltf-2.0\Monster\glTF\Monster.gltf
        C:\output\gltf-2.0\Monster\glTF-Embedded\Monster.gltf
        C:\output\gltf-2.0\Monster\glTF-Binary\Monster.gltf
        C:\output\gltf-2.0\Monster\glTF-MaterialsCommon\Monster.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-MaterialsCommon\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-Embedded\RiggedFigure.gltf
        C:\output\gltf-2.0\RiggedFigure\glTF-Binary\RiggedFigure.gltf
        C:\output\gltf-2.0\VC\glTF-Binary\VC.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-Binary\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-MaterialsCommon\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF\WalkingLady.gltf
        C:\output\gltf-2.0\WalkingLady\glTF-Embedded\WalkingLady.gltf

From a quick look each asset (in one particular version), the errors seem to be of the categories

            {
                "code": "EMPTY_ENTITY",
                "message": "Entity cannot be empty.",
                "severity": 0,
                "pointer": "/animations/1/channels"
            },
            {
                "code": "UNEXPECTED_PROPERTY",
                "message": "Unexpected property.",
                "severity": 1,
                "pointer": "/skins/0/bindShapeMatrix"
            },
            {
                "code": "NODE_SKINNED_MESH_NON_ROOT",
                "message": "Node with a skinned mesh is not root. Parent transforms will not affect a skinned mesh.",
                "severity": 1,
                "pointer": "/nodes/33"
            },
            {
                "code": "ACCESSOR_WEIGHTS_NON_NORMALIZED",
                "message": "Weights accessor elements (at indices 168..171) have non-normalized sum: 0.23690563440322876.",
                "severity": 0,
                "pointer": "/meshes/0/primitives/0/attributes/WEIGHTS_0"
            },

These might all be due to the difficulties that arise from translating glTF 1.0 animations/skinning to the proper glTF 2.0 animations/skinning. Subtle details have changed there, and it's not even clear whether they can be updated automatically. So I assume that this is not considered to be an issue for now...

[lilleyse]

Thanks @javagl, yeah, I would leave those are future work.