GLTF loader: handle warning NODE_SKINNED_MESH_WITHOUT_SKIN

[mockersf]

Objective

• According to the GLTF spec, it should not be possible to have a non skinned mesh on a skinned node

When the node contains skin, all mesh.primitives MUST contain JOINTS_0 and WEIGHTS_0 attributes
https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#reference-node

• However, the reverse (a skinned mesh on a non skinned node) is just a warning, see NODE_SKINNED_MESH_WITHOUT_SKIN in https://github.com/KhronosGroup/glTF-Validator/blob/main/ISSUES.md#linkerror
• This causes a crash in Bevy because the bind group layout is made from the mesh which is skinned, but filled from the entity which is not

thread '<unnamed>' panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 5, Metal)>`
    In a set_bind_group command
      note: bind group = `<BindGroup-(27, 1, Metal)>`
    Bind group 2 expects 2 dynamic offsets. However 1 dynamic offset were provided.

• Blender can export GLTF files with this kind of issues

Solution

• When a skinned mesh is only used on non skinned nodes, ignore skinned information from the mesh and warn the user (this is what three.js is doing)
• When a skinned mesh is used on both skinned and non skinned nodes, log an error

[nicopap]

I think the PR description has a wording issue.

… should not be possible to have a skinned mesh on a non skin node

… the reverse (a skinned mesh on a non skinned node)

hmm. So to reword I'd say

The glTF standard mandates that nodes with a skin field have JOINTS_N and WEIGHTS_N attributes on its mesh primitives.

But it doesn't mandates nodes without a skin field to not have JOINTS_N and WEIGHTS_N attributes on its mesh primitives.

So this PR iterates once over all nodes of the glTF to check for skinned nodes, before doing it again a second time.

In load_node at line 847, we already read the gltf_node.skin() and afaik load_node runs for all nodes already.

But since load_node is ran after we load the meshes, we can't track skin nodes this way.

However, is it possible to do the reverse? keep track of meshes with the skinning attributes and at line 847, have an else clause where we check our primitive isn't in the list of meshes with a skin?

[mockersf]

I think the PR description has a wording issue.

Oh 🤦
Fixed the negations, thanks!

However, is it possible to do the reverse? keep track of meshes with the skinning attributes and at line 847, have an else clause where we check our primitive isn't in the list of meshes with a skin?

It would be possible, but at that point you can't modify the already loaded mesh, so as a "fix" you could ignore that node, which is what some other GLTF validators I found do, but I think it's less good than removing the extra attributes from the mesh while we load it.

[nicopap]

I'll approve. Because the tradeoff to me seems positive. But I'll warn people that this adds a second iteration over all the nodes in a glTF file. And maybe we shouldn't.

[JMS55]

Can we add this check/loop only in debug mode?

[mockersf]

Can we add this check/loop only in debug mode?

Checking just in debug would mean still crashing in release on something that is valid. The loop doesn't just check, it applies a quick fix when possible.

[alice-i-cecile]

Ultimately I'd like to swap this sort of robustness check / correction to an automated / semi-automated asset preprocessing step. But that's a ton of extra work, and something that needs a much larger design. This fix seems good to me for now.

[cart]

Ultimately I'd like to swap this sort of robustness check / correction to an automated / semi-automated asset preprocessing step.

If we added a processor for GLTF assets (running on the outputs of this AssetLoader), the checks in this loader would not happen at runtime for the processed version.