Added SimpleSkin example

[javagl]

A simple example for vertex skinning.

This is essentially the model that is explained in full detail in the tutorial at https://github.com/javagl/glTF-Tutorials/blob/master/gltfTutorial/gltfTutorial_019_SimpleSkin.md

It is rendered and animated properly in https://gltf-viewer.donmccurdy.com/ and https://sandbox.babylonjs.com/

It is not rendered properly in https://pissang.github.io/clay-viewer/editor/ , though...

The shortcut to the model in the branch in the fork is https://github.com/javagl/glTF-Sample-Models/tree/SimpleSkin/2.0/SimpleSkin

Further reviews or comments by skinning experts would be appreciated...

[cx20]

I tried this model with gltf-test.
The current test results are as follows.

Some libraries seem to have the same problem. ClayGL, PlayCanvas, GLBoost, etc.

[bghgary]

Some libraries seem to have the same problem. ClayGL, PlayCanvas, GLBoost, etc.

It will be interesting to find out what the culprit is so that we can possibly add a test case to the skin models in asset generator.

[javagl]

@cx20 Thanks for the tests!

I only manually validated it in 3 viewers, and noticed that it worked in 2 of them, but didn't work properly in ClayGL.

It's really interesting to see that the cases where it does not work all seem to be "wrong in the same way". This is a strong hint that there's some critical ambiguity in the spec.

(Which still doesn't say anything about whether "the model itself" is correct - maybe Babylon and Three.js are wrong...?)

I'll try to allocate some more time for that, maybe I can figure out the reason.

(I think it should not be related to KhronosGroup/glTF#1504 (comment) , though - but I'll try to add some example, maybe a "SimpleSkinTest" or so, where the effect of using the skeleton root vs. the node transform becomes obvious. I basically thought about actually adding a screenshot/GIF showing the wrong result, and saying "If you can see this, you made this-and-that error, and you should fix it like this-and-that"...)

[cx20]

@javagl I think it's good to illustrate common mistakes.

The following list is a list of libraries that support skinning animation (For example, CesiumMan). It was unexpected that Cesium.js could not be processed correctly.

Model	Simple Skin	remarks
Three.js	✅
Babylon.js	✅
Cesium.js	❌	Cannot read property 'parents' of undefined
Grimoire.js	❌	No error is output but it does not work
minimal-gltf-loader	❌	Cannot read property 'slice' of undefined
ClayGL	❌	Part of the model does not work
Hilo3d	✅
PlayCanvas	❌	Part of the model does not work
CZPG.js	❌	No error is output but it does not work
GLBoost	❌	Part of the model does not work
RedCube.js	❌	No error is output but it does not work
RedGL	✅
Ashes	✅
Unity	❌	Uncaught abort() at Error
pex-renderer	✅
Filament	✅

[emackey]

I think the Cesium issue is due to CesiumGS/cesium#8175. This doesn't need to be resolved before merging this.

@javagl With the latest validator, I see two warning messages here. There aren't any errors, so this could be merged as-is. But I wanted to ask, do you expect these warnings from this example?

{
    "uri": "2.0/SimpleSkin/glTF/SimpleSkin.gltf",
    "mimeType": "model/gltf+json",
    "validatorVersion": "2.0.0-dev.3.2",
    "issues": {
        "numErrors": 0,
        "numWarnings": 2,
        "numInfos": 0,
        "numHints": 0,
        "messages": [
            {
                "code": "ACCESSOR_JOINTS_USED_ZERO_WEIGHT",
                "message": "Joints accessor element at index 1 (component index 1) is used with zero weight but has non-zero value (1).",
                "severity": 1,
                "pointer": "/meshes/0/primitives/0/attributes/JOINTS_0"
            },
            {
                "code": "ACCESSOR_JOINTS_USED_ZERO_WEIGHT",
                "message": "Joints accessor element at index 5 (component index 1) is used with zero weight but has non-zero value (1).",
                "severity": 1,
                "pointer": "/meshes/0/primitives/0/attributes/JOINTS_0"
            }
        ],
        "truncated": false
    },
...

[lexaknyazev]

@emackey
We had similar issue with the asset generator, see KhronosGroup/glTF-Asset-Generator#559 (comment) for details.

A few nits (just to improve readability, they do not affect the rendering issues in some engines).

• Make material double-sided, so the model can be rotated (sometimes viewers enable auto-rotation by default).
• Remove unneeded min/max properties for accessors (except where they are required).
• Remove zero byte offsets for accessors and buffer views (as they are zero by default).

[lexaknyazev]

FWIW, removing skin.inverseBindMatrices property distorts the rendering as expected in Babylon and three. At the same time, it does not affect the rendering in ClayGL.

[javagl]

The validation warning seem to be new: IIRC, the online version did not show warnings - so I assume that the drag-and-drop validator is/was not up to date at the time I tested it. I'll have another look at the warnings and the data, and see how they can be fixed (also, the other minor fixes).

If I understood it correctly, then removing the skin.inverseBindMatrices property causes the rendering to be distorted in the same way as in ClayGL? This would be a strong hint that these viewers do not take this property into account properly...

[lexaknyazev]

distorted in the same way as in ClayGL

No, that distorts differently.

[emackey]

@javagl There's a newer version of the validator (2.0.0-dev.3.2) that among other things added a number of new checks and requirements for skinned models.

[emackey]

Finally coming back around to this, as I'm working on my own implementation of skinning.

I can see why the sample data is the way it is (for simplicity), and I can see why the validator thinks it's worth issuing a warning, but not an error. I think it's OK to let this one go for the sake of readability of the raw data.

Any other concerns before this gets merged?

[javagl]

@emackey Things like this tend to be buried under other (usually more pressing) tasks. But the improvements that lexaknyazev mentioned (and resolving the warnings) are things that I'd certainly tackle. I'll set myself a deadline: If it's not revised until Thursday, it can be merged (and the shame of having contributed a model with warnings will haunt me :-/ )

[javagl]

The model has been updated

• Made material double-sided
• Removed unneeded min/max properties from accessors
• Removed offsets where they are zero by default
• Fixed ACCESSOR_JOINTS_USED_ZERO_WEIGHT warnings

There are no warnings with the current validator, and the resulting model (embedded and non-embedded) can be displayed in https://gltf-viewer.donmccurdy.com/

(An unrelated aside: Figuring out where exactly the min/max are required for an accessor was not trivial: The spec says

While these properties are not required for all accessor usages, there are cases when minimum and maximum must be defined. Refer to other sections of this specification for details.

but this sounds a bit like "Your Princess is in Another Castle"...)

[cx20]

I tested it again with gltf-test and found a library with an interesting bug.
That library seems to fail if only doubleSided is used for the material.
Probably most of the provided samples use pbrMetallicRoughness for the material and there is no single case for doubleSided.
I think it would be nice to have another test case to test the material.

[javagl]

@cx20 That's interesting.

In fact, I hesitated a bit to add the material to the sample: It then is no longer a test for (only) skinning, but also for material handling. And it may be a very special case in that it only defines the doubleSided property, and no pbrMetallicRoughness.

But loaders should be able to cope with that. From a quick glimpse at the console, it is not directly caused by the doubleSided flag, but rather by the fact that the material does not have a pbrMetallicRoughness in general, and in this case, the default values should be used (as of https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#reference-material ). As such, it's rather an issue with the specific viewer.

There once also was a SimpleMaterial test, but it eventually got buried. We might consider adding such a test, or even more broadly, systematic tests for default values, but there would be a certain overlap with https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Positive/Material/README.md ...

[cx20]

@javagl Thank you for your reply.

As such, it's rather an issue with the specific viewer.

Yes. Since this is a specific library issue, I have made a separate issue report.

There once also was a SimpleMaterial test, but it eventually got buried. We might consider adding such a test, or even more broadly, systematic tests for default values

I think it makes sense that the samples in glTF-Tutorial would be in glTF-Sample-Models as a basic use case. The case where only doubleSided material is used is a bit of a special test case, so I think the glTF-Asset-Generator is more suitable.

[emackey]

I'm fine with removing doubleSided and the material. Sounds like it makes the example more focused on the topic.

[javagl]

So I have removed the material (tests still passing). Although it may not look "nice" in some viewers (as Alexey said: They often have this auto-rotation enabled), it may be more suitable as a test that is supposed to be focused only on skinning. The topic of skinning is rather complex anyhow, and I still want to create an example that allows to test the proper handling of skeleton roots...

[bghgary]

You could also add more geometry instead of making it double-sided, but that may also make this example more complex than it needs to be.

[javagl]

There seem to be no other issues with that, so I think it's OK to merge.

[emackey]

Thanks @javagl!