Convert glTF 1.0 techniques to PBR materials by default #619

lilleyse · 2022-08-02T20:53:06Z

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 CesiumGS/cesium#10414 and CesiumGS/cesium#10613 for more background.

For the previous behavior just pass in --keepLegacyExtensions

… unit tests

lilleyse · 2022-08-02T20:55:50Z

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

lilleyse · 2022-08-02T21:02:29Z

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 · 2022-08-02T21:40:13Z

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 Skinning broken for glTF 1.0 models with ModelExperimental cesium#10621

javagl · 2022-08-03T20:55:04Z

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 ...

all versions in the test show the same behavior

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

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...)

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) CesiumGS/cesium#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 · 2022-08-03T21:31:43Z

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 · 2022-08-03T22:22:39Z

The Cesium Man artifact is related to CesiumGS/cesium#6447 (comment). I think the glTF 2.0 version was fixed.

javagl · 2022-08-04T12:24:44Z

Yeah, it's difficult: The CesiumMan 1.0 apparently contains errors that cause this artifact (related to vertex weights, according to BabylonJS/Babylon.js#4240 (comment) ), 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 · 2022-08-04T12:35:29Z

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

Some comments above may be worth another look.
Some comments inlined.

Beyond that, the code looks fine for me (as far as I can 'understand' it in the given time).

I did not take a really close look at the Specs code in particular, but ran the glTF-Sample-Models conversion and the tests. If the Specs should be reviewed (as in: "Do the inputs/outputs make sense?", or "Is 'every reasonable corner case' covered with the spec?"), just drop me a note. But considering the legacy nature of the inputs, defining a 'reasonable corner case' could be hard...

javagl · 2022-08-04T12:45:37Z

lib/addDefaults.js

@@ -101,6 +99,17 @@ function addDefaults(gltf) {
          values.shininess = defaultValue(values.shininess, 0.0);
        }
      }
+
+      // These actually exist on the extension object, not the values object despite what's shown in the spec


I'm actually considering to bring that up in an issue in the glTF repo. But it's probably considered to be "too legacy" to be worth the noise...

javagl · 2022-08-04T12:55:49Z

lib/updateVersion.js

+function initializePbrMaterial(material) {
+  material.pbrMetallicRoughness = defined(material.pbrMetallicRoughness)
+    ? material.pbrMetallicRoughness
+    : {};


I've seen the use of the ternary operator and defined at various places, e.g. things like

values.emission = defined(values.emission) ? values.emission : [0.0, 0.0, 0.0, 1.0];

and wonder whether there is a reason to not use

values.emission = defaultValue(values.emission, [0.0, 0.0, 0.0, 1.0]);

for this.

Only reason is to avoid the allocation if the left hand side is already defined. Though this code is only called once so defaultValue would not cause much of a GC hit. I might just leave it as-is though both approaches are fine here.

javagl · 2022-08-04T12:58:15Z

lib/updateVersion.js

+    : {};
+
+  material.pbrMetallicRoughness.roughnessFactor = 1.0;
+  material.pbrMetallicRoughness.metallicFactor = 0.0;


It also always bugged me a bit, but the default value for the metallicFactor is indeed 1.0 ( see https://github.com/KhronosGroup/glTF/blob/b6e0fcc6d8e9f83347aa8b2e3df085b81590a65c/specification/2.0/schema/material.pbrMetallicRoughness.schema.json#L30 ).

(Maybe the value of 0.0 is used intentionally here, but I wanted to mention it...)

I set it to 0.0 intentionally since I think it makes for a better default, especially since the conversion is disregarding the specular values.

javagl · 2022-08-04T13:24:16Z

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 · 2022-08-04T14:47:22Z

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 #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...

…minmax Validate accessor min/max values

lilleyse · 2022-08-04T18:24:27Z

@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 · 2022-08-05T15:12:49Z

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 · 2022-08-05T15:23:13Z

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 · 2022-08-05T15:31:24Z

. 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 🤷

javagl · 2022-08-05T16:03:25Z

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 · 2022-08-05T20:09:50Z

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

lilleyse added 5 commits August 1, 2022 20:16

Convert legacy glTF 1.0 extensions to PBR

b5bab27

Handle KHR_materials_common values correctly, srgb to linear, and fix…

aa1e424

… unit tests

Add tests

46fa8dc

Add comment

a65bd24

Future work notes

0713846

javagl reviewed Aug 4, 2022

View reviewed changes

Validate accessor min/max values

b4cccd7

javagl mentioned this pull request Aug 4, 2022

Validate accessor min/max values #620

Merged

ptrgags mentioned this pull request Aug 4, 2022

Remove Model and rename ModelExperimental -> Model CesiumGS/cesium#10637

Merged

Merge pull request #620 from javagl/convert-legacy-extensions-to-pbr-…

3f41a76

…minmax Validate accessor min/max values

Fix test failure by quantizing more attributes

e4da497

lilleyse merged commit 59ddcbc into main Aug 5, 2022

lilleyse deleted the convert-legacy-extensions-to-pbr branch August 5, 2022 20:21

ptrgags mentioned this pull request Aug 5, 2022

Replace glTF model implementation CesiumGS/cesium#10644

Merged

4 tasks

lilleyse mentioned this pull request Aug 5, 2022

Limited support for glTF 1.0 in the new model implementation CesiumGS/cesium#10648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert glTF 1.0 techniques to PBR materials by default #619

Convert glTF 1.0 techniques to PBR materials by default #619

lilleyse commented Aug 2, 2022 •

edited

Loading

lilleyse commented Aug 2, 2022

lilleyse commented Aug 2, 2022 •

edited

Loading

lilleyse commented Aug 2, 2022

javagl commented Aug 3, 2022

javagl commented Aug 3, 2022

lilleyse commented Aug 3, 2022

javagl commented Aug 4, 2022

javagl commented Aug 4, 2022

javagl left a comment •

edited

Loading

javagl Aug 4, 2022

javagl Aug 4, 2022

lilleyse Aug 4, 2022

javagl Aug 4, 2022

lilleyse Aug 4, 2022

javagl commented Aug 4, 2022

javagl commented Aug 4, 2022 •

edited

Loading

lilleyse commented Aug 4, 2022

javagl commented Aug 5, 2022

lilleyse commented Aug 5, 2022

lilleyse commented Aug 5, 2022

javagl commented Aug 5, 2022

lilleyse commented Aug 5, 2022

Convert glTF 1.0 techniques to PBR materials by default #619

Convert glTF 1.0 techniques to PBR materials by default #619

Conversation

lilleyse commented Aug 2, 2022 • edited Loading

lilleyse commented Aug 2, 2022

lilleyse commented Aug 2, 2022 • edited Loading

lilleyse commented Aug 2, 2022

javagl commented Aug 3, 2022

javagl commented Aug 3, 2022

lilleyse commented Aug 3, 2022

javagl commented Aug 4, 2022

javagl commented Aug 4, 2022

javagl left a comment • edited Loading

Choose a reason for hiding this comment

javagl Aug 4, 2022

Choose a reason for hiding this comment

javagl Aug 4, 2022

Choose a reason for hiding this comment

lilleyse Aug 4, 2022

Choose a reason for hiding this comment

javagl Aug 4, 2022

Choose a reason for hiding this comment

lilleyse Aug 4, 2022

Choose a reason for hiding this comment

javagl commented Aug 4, 2022

javagl commented Aug 4, 2022 • edited Loading

lilleyse commented Aug 4, 2022

javagl commented Aug 5, 2022

lilleyse commented Aug 5, 2022

lilleyse commented Aug 5, 2022

javagl commented Aug 5, 2022

lilleyse commented Aug 5, 2022

lilleyse commented Aug 2, 2022 •

edited

Loading

lilleyse commented Aug 2, 2022 •

edited

Loading

javagl left a comment •

edited

Loading

javagl commented Aug 4, 2022 •

edited

Loading