search

KhronosGroup / glTF-Asset-Generator

Tool for generating various glTF assets for importer validation
MIT License
161 stars 42 forks source link

Animation_Skin#7 describes multiple skins #543

Closed donmccurdy closed 5 years ago

donmccurdy commented 5 years ago

The description of Animation_Skin 7 says:

skinB which is made up of two skins. joint1 is referenced by both skins and is animating with a rotation.

From that description I'd expected to see two entries in the skins list, but there is only one:

https://github.com/KhronosGroup/glTF-Asset-Generator/blob/master/Output/Positive/Animation_Skin/Animation_Skin_07.gltf#L304-L312

Not sure if this is a mistake in the output, or in the description. I got here from https://github.com/KhronosGroup/glTF-Blender-IO/issues/566, hoping to find a test case where two skins reference some of the same joint nodes.

donmccurdy commented 5 years ago

Hm, actually this test does have what I expected from the description above: https://bghgary.github.io/glTF-Assets-Viewer/?manifest=https://raw.githubusercontent.com/KhronosGroup/glTF-Asset-Generator/v0.6.0/Output/Manifest.json&folder=2&model=8

Something might be mismatched here? 🤔

stevk commented 5 years ago

There are two skins in the current master, but they are both instances of "skin": 0 (check the nodes list). It would probably be less confusing if both skins were unique though. This was changed was introduced when instancing support was added, so release v0.6.1 and earlier have the non-instanced version of this model.

donmccurdy commented 5 years ago

It looked like each skin had different inverseBindMatrices, unless the accessor data was actually the same? This tests a different functionality with and without instancing; I do think it's important to have a test case where multiple distinct skins entries reference some of the same nodes.

There are two skins in the current master, but they are both instances of "skin": 0...

I get your meaning, but I don't think most users will interpret "two skins" this way, and the line joint1 is referenced by both skins seems meaningless here. If the model is kept as-is, I think a clearer framing might be: skinB is referenced by two skinned meshes

bghgary commented 5 years ago

Yeah, this definitely looks like a bug that we accidentally introduced. Thanks for catching this and filing the issue!