From 521df3291e349c57d1da91eaf804c106c13223e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois?= <mockersf@gmail.com>
Date: Sat, 14 Oct 2023 00:40:28 +0200
Subject: [PATCH] GLTF loader: handle warning NODE_SKINNED_MESH_WITHOUT_SKIN
 (#9360)

# 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
---
 crates/bevy_gltf/src/loader.rs | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_gltf/src/loader.rs b/crates/bevy_gltf/src/loader.rs
index 842290e2d4fcd..87fb8c6318b44 100644
--- a/crates/bevy_gltf/src/loader.rs
+++ b/crates/bevy_gltf/src/loader.rs
@@ -6,7 +6,7 @@ use bevy_core::Name;
 use bevy_core_pipeline::prelude::Camera3dBundle;
 use bevy_ecs::{entity::Entity, world::World};
 use bevy_hierarchy::{BuildWorldChildren, WorldChildBuilder};
-use bevy_log::warn;
+use bevy_log::{error, warn};
 use bevy_math::{Mat4, Vec3};
 use bevy_pbr::{
     AlphaMode, DirectionalLight, DirectionalLightBundle, PbrBundle, PointLight, PointLightBundle,
@@ -36,7 +36,7 @@ use gltf::{
     accessor::Iter,
     mesh::{util::ReadIndices, Mode},
     texture::{MagFilter, MinFilter, WrappingMode},
-    Material, Node, Primitive,
+    Material, Node, Primitive, Semantic,
 };
 use serde::Deserialize;
 use std::{
@@ -329,6 +329,17 @@ async fn load_gltf<'a, 'b, 'c>(
 
     let mut meshes = vec![];
     let mut named_meshes = HashMap::default();
+    let mut meshes_on_skinned_nodes = HashSet::default();
+    let mut meshes_on_non_skinned_nodes = HashSet::default();
+    for gltf_node in gltf.nodes() {
+        if gltf_node.skin().is_some() {
+            if let Some(mesh) = gltf_node.mesh() {
+                meshes_on_skinned_nodes.insert(mesh.index());
+            }
+        } else if let Some(mesh) = gltf_node.mesh() {
+            meshes_on_non_skinned_nodes.insert(mesh.index());
+        }
+    }
     for gltf_mesh in gltf.meshes() {
         let mut primitives = vec![];
         for primitive in gltf_mesh.primitives() {
@@ -339,6 +350,18 @@ async fn load_gltf<'a, 'b, 'c>(
 
             // Read vertex attributes
             for (semantic, accessor) in primitive.attributes() {
+                if [Semantic::Joints(0), Semantic::Weights(0)].contains(&semantic) {
+                    if !meshes_on_skinned_nodes.contains(&gltf_mesh.index()) {
+                        warn!(
+                        "Ignoring attribute {:?} for skinned mesh {:?} used on non skinned nodes (NODE_SKINNED_MESH_WITHOUT_SKIN)",
+                        semantic,
+                        primitive_label
+                    );
+                        continue;
+                    } else if meshes_on_non_skinned_nodes.contains(&gltf_mesh.index()) {
+                        error!("Skinned mesh {:?} used on both skinned and non skin nodes, this is likely to cause an error (NODE_SKINNED_MESH_WITHOUT_SKIN)", primitive_label);
+                    }
+                }
                 match convert_attribute(
                     semantic,
                     accessor,