glTF skins bound per Mesh and deduplicated by joint names break RecursiveSkeletons and shared-skin models

[mvaligursky]

Description

While investigating #4611 (whose premise turned out to be wrong: the transform of a skinned mesh node is already ignored, see the closing comment there) I found three real defects in how glb skins are bound. All three were verified by running the actual GlbParser + renderer on NullGraphicsDevice and comparing, per skinned mesh instance and bone, what the vertex shader applies (node.worldTransform * skinInstance.matrices[i]) against the glTF spec value (jointWorldTransform[i] * inverseBindMatrix[i]) computed independently from the source file.

Asset	glTF skins	engine Skins	SkinInstances	skinned mesh instances	wrong
Khronos RecursiveSkeletons	84	84	1	84	83
glTF-Asset-Generator Animation_Skin_11	2	1	1	2	1
Animation_Skin_05 + translation on 2nd node (file below)	1	1	1	2	1
Animation_Skin_00/02/05/07, BrainStem (59 mesh instances, 1 skin)					0

1. The skin is stored on the shared Mesh, so a mesh reused by several nodes with different skins gets the last skin for all of them

This is the architectural problem reported in #5680 (Mesh.skin prevents instantiating several skins on one mesh). The assessment there that the impact is limited does not hold: it breaks a Khronos sample asset and is a regression, see below.

linkSkins in glb-parser.js does mesh.skin = skins[gltfNode.skin] for every node that references the mesh, so the last node wins. GlbContainerResource.instantiateRenderEntity then creates the skin instance from meshInstance.mesh.skin instead of the node's skin. RecursiveSkeletons has 1 mesh referenced by 84 nodes, each with its own skin: every one of the 84 mesh instances ends up sharing a single SkinInstance bound to skin 83, so all tentacle segments collapse onto the last skeleton section.

This is a regression from #3350 (v1.45.2, July 2021): before it, instantiateRenderEntity passed skins[gltfNode.skin] for each node to the skin instance cache; #3350 replaced that with meshInstance.mesh.skin and added linkSkins. The legacy Model path (GlbContainerResource.createModel) still looks up skins[gltfNode.skin] per node and binds all 84 correctly. Mesh._initBoneAabbs also sizes the bone AABBs from this.skin.boneNames, so a mesh shared by skins with different joint counts would get wrong culling bounds even after fixing the binding.

2. createSkin deduplicates skins by joined joint names only

The glbSkins cache (added in #2912) keys on boneNames.join('#'). Two glTF skins with the same joint names become one engine Skin carrying the first skin's inverse bind matrices, so differing IBMs are silently dropped. Bone resolution is also by name (rootBone.findByName), so two skeletons that reuse joint names collapse onto the first one. Duplicate node names are legal in glTF. Animation_Skin_11 (two skinA instances, both using joints named joint0/joint1) renders the second plane driven by the first skeleton.

3. A shared SkinInstance assumes all its mesh instances share one node transform

SkinInstanceCache shares one SkinInstance per (skin, rootBone). Renderer.updateCpuSkinMatrices computes matrices[i] = inv(node.world) * bone.world * IBM[i] once per frame using the first mesh instance's node and skips the rest via _skinUpdateIndex, but every mesh instance still uploads its own matrix_model. The inverse only cancels for the first one; the others render offset by nodeB.world * inv(nodeA.world). Which mesh instance is "first" depends on layer order, so removing or disabling one skinned part shifts the others, which is the symptom described in #5329 (possibly the same defect). Per spec the skinned node's transform must be ignored, so exporters can legitimately leave any transform there. No stock Khronos asset has two differently-transformed nodes on one skin; the spec-valid file below (Animation_Skin_05 with translation: [1,0,0] on plane2) reproduces it, with plane2 offset by exactly 1 unit.

Steps to Reproduce

1. Load RecursiveSkeletons.glb as a container asset and call instantiateRenderEntity(). Expected: 84 skinned segments along 4 recursive skeletons. Actual: everything is bound to the last skin.
2. Load Animation_Skin_11.gltf. Expected: each plane follows its own skeleton. Actual: both follow the first.
3. Load the file below. Expected: both planes coincide (node transforms on skinned nodes are ignored). Actual: plane2 is offset by 1 on X.

Animation_Skin_05_offset.gltf (self-contained, buffers embedded)

{"accessors":[{"bufferView":0,"componentType":5126,"count":6,"type":"VEC3","max":[0.25,0,0.2],"min":[-0.25,0,-0.2],"name":"Positions Accessor"},{"bufferView":1,"componentType":5125,"count":12,"type":"SCALAR","name":"Indices Accessor"},{"bufferView":2,"componentType":5126,"count":6,"type":"VEC4","name":"weights accessor"},{"bufferView":3,"componentType":5123,"count":6,"type":"VEC4","name":"joint indices accessor"},{"bufferView":4,"componentType":5126,"count":2,"type":"MAT4","name":"IBM"},{"bufferView":5,"componentType":5126,"count":6,"type":"VEC3","max":[0.85,0,0.2],"min":[0.35000002,0,-0.2],"name":"Positions Accessor"}],"asset":{"generator":"glTF-Asset-Generator Animation_Skin_05 + translation on node 3 (plane2)","version":"2.0"},"buffers":[{"uri":"data:application/octet-stream;base64,AACAvgAAAADNzEy+AACAPgAAAADNzEy+AACAvgAAAAAAAAAAAACAPgAAAAAAAAAAAACAvgAAAADNzEw+AACAPgAAAADNzEw+AAAAAAEAAAACAAAAAgAAAAEAAAADAAAAAgAAAAMAAAAEAAAABAAAAAMAAAAFAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAQAAAAAAAAABAAAAAAAAAAEAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAAAAAAAAgD8AAAAAAAAAAAAAAADNzEw+AACAPwAAgD8AAAAAAAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAAAAAAIA/AAAAAAAAAAAAAAAAAAAAgAAAgD80M7M+AAAAAM3MTL6amVk/AAAAAM3MTL40M7M+AAAAAAAAAACamVk/AAAAAAAAAAA0M7M+AAAAAM3MTD6amVk/AAAAAM3MTD4=","byteLength":464}],"bufferViews":[{"buffer":0,"byteLength":72,"name":"Positions"},{"buffer":0,"byteOffset":72,"byteLength":48,"name":"Indices"},{"buffer":0,"byteOffset":120,"byteLength":96,"name":"weights buffer view"},{"buffer":0,"byteOffset":216,"byteLength":48,"name":"joint indices buffer view"},{"buffer":0,"byteOffset":264,"byteLength":128,"name":"Inverse Bind Matrix"},{"buffer":0,"byteOffset":392,"byteLength":72,"name":"Positions"}],"materials":[{"pbrMetallicRoughness":{"baseColorFactor":[0.8,0.8,0.8,1]},"doubleSided":true},{"pbrMetallicRoughness":{"baseColorFactor":[0,0,1,1]},"doubleSided":true}],"meshes":[{"primitives":[{"attributes":{"POSITION":0,"WEIGHTS_0":2,"JOINTS_0":3},"indices":1,"material":0}]},{"primitives":[{"attributes":{"POSITION":5,"WEIGHTS_0":2,"JOINTS_0":3},"indices":1,"material":1}]}],"nodes":[{"skin":0,"mesh":0,"name":"plane"},{"children":[2],"rotation":[-0.7071067,0,0,0.70710677],"translation":[0,-0.2,0],"name":"joint0"},{"rotation":[-0.25881904,0,0,0.9659258],"translation":[0,0,0.2],"name":"joint1"},{"skin":0,"mesh":1,"name":"plane2","translation":[1,0,0]}],"scene":0,"scenes":[{"nodes":[0,1,3]}],"skins":[{"inverseBindMatrices":4,"joints":[1,2],"name":"skinA"}]}

Suggested fixes

1. Bind the skin per glTF node in instantiateRenderEntity (skins[gltfNode.skin], as createModel does) and treat mesh.skin only as the default for the render-asset path; make bone AABBs per skin rather than per mesh.
2. Key the skin dedupe on joint node indices plus the IBM accessor (or drop it and rely on the SkinInstance cache), and resolve bones by node identity rather than by name when instantiating from a glb.
3. Compute the palette relative to the cache key (rootBone) instead of meshInstance.node, and upload the root bone's world transform as matrix_model for skinned draws (the skinned AABB path in mesh-instance.js must use the same frame). Alternatively include the mesh node in the cache key, at the cost of the sharing Render component is sharing and properly releasing skin instances #2912 introduced.

Engine main at 63b1937. The math is backend independent.

[mvaligursky]

Fix options

Notes from looking at how each of the three defects could be addressed, in case someone picks this up.

1. Skin stored on the shared Mesh (RecursiveSkeletons, #5680)

• Minimal, non-breaking. instantiateRenderEntity already has the glTF node when it creates the mesh instance, so it can bind skins[gltfNode.skin] per node instead of meshInstance.mesh.skin (this is what the code did before [Fix] Fixes skining using render component meshes imported using hierarchy in Editor #3350). The MeshInstance needs its own skin reference that falls back to mesh.skin, because RenderComponent._cloneSkinInstances rebuilds skin instances from mesh.skin whenever rootBone changes or the component is re-enabled. Bone AABBs are not a blocker: Mesh._initBoneAabbs only reads blend weights/indices from the vertex data and uses the skin for the bone count, so boneAabb can stay on the mesh, sized from the skin instance or from the highest joint index.
• Full fix (Architectural Issue: The skin is defined within the mesh. #5680). Remove Mesh.skin and make the skin a MeshInstance-level concept. Readers that would break: gltf-exporter.js (3 sites), json-model.js, scripts/esm/vat/vat-converter.mjs, and the Editor. Needs a deprecation cycle, so V3.
• Editor limitation either way. A template gets one render asset per glTF mesh and finds the skin through it, so expressing many skins on one mesh via the Editor would also need a skin reference on the render component. The engine-side fix covers container loading at runtime, which is where the Khronos model breaks.

2. Skins deduplicated by joint names

• Key on structure, not names. Dedupe on the joint node indices plus the inverseBindMatrices accessor index. Keeps the sharing benefit for exporters that emit identical skins per mesh part, stops merging skins that only share names.
• Dropping the dedupe loses bone-texture sharing entirely, since SkinInstanceCache keys on Skin identity. Not worth it.
• Duplicate joint names across skeletons. The container knows the exact node-to-entity mapping, so it can resolve bones by identity at instantiation instead of rootBone.findByName. The residual gap is a later rootBone reassignment, which re-resolves by name; storing relative paths in Skin instead of flat names would close it at the cost of breaking on hierarchy edits. Probably better documented as a limitation.

3. Shared SkinInstance assumes one node frame

• Preferred: compute the palette relative to the root bone (the cache key) instead of meshInstance.node, and for skinned draws upload the root bone's world/normal matrices as matrix_model. One change site in Renderer.setMeshInstanceMatrices, plus the skinned branch of MeshInstance._updateAabb, which must use the same frame. Every mesh instance sharing a skin instance shares the root bone by construction, so the cancellation is exact, the node transform is ignored as the spec requires, and the local-space precision from Local space skinning #1097 is preserved. Skin instances without a root bone (legacy Model path) keep using the node.
• Add the node to the cache key. Trivial and correct, but gives up the sharing Render component is sharing and properly releasing skin instances #2912 introduced: multi-part characters pay one bone texture and palette update per part.
• World-space palette (drop the inverse). Simplest math, but float32 precision degrades away from the origin and unnormalized weights produce worse artifacts; Local space skinning #1097 moved away from this deliberately.
• Recomputing per node is not viable because the bone texture is shared.

Suggested order

The root-bone frame change and the structural dedupe key are small and independent, and together fix Animation_Skin_11 and the offset case. Per-node binding with a MeshInstance-level skin fallback comes next and fixes RecursiveSkeletons without an API break. Removing Mesh.skin waits for V3. Each step should land with tests built from the Khronos assets: the null-device harness used above (GlbParser.parse + instantiateRenderEntity + app.render(), then compare node.worldTransform * skinInstance.matrices[i] against jointWorld[i] * IBM[i] from the source file) runs the parser and renderer end to end and catches all three.