gltf: Fix mesh nodes which are also bones.

[lyuma]

This resolves these errors related to skinned meshes with mesh nodes as bones:

ERROR: Condition "mi == nullptr" is true.
   at: GLTFDocument::_process_mesh_instances (modules\gltf\gltf_document.cpp:5969)
ERROR: Condition "sk == nullptr" is true.
   at: GLTFDocument::_import_animation (modules\gltf\gltf_document.cpp:5648)

I filed an issue specifically for what this PR solves at #49118
It affects both 3.x and master.

in the following glTF documents:
NestedSkeletonReproCaseV2Animated.glb
MeshSkinnedToItselfAndOthersV2.glb
NestedSkeletonReproV3.glb

However, it does not resolve the problems in the following glTF document. More extensive work will need to be done on models causing the ERROR: glTF: Generating scene detected direct parented Skeletons error:
DirectParentedSkeletons.glb
The following assume the more useful error messages implemented by #48912:

ERROR: glTF: Generating scene detected direct parented Skeletons
   at: (modules\gltf\gltf_document.cpp:5441)
ERROR: glTF: Generating scene detected direct parented Skeletons
   at: (modules\gltf\gltf_document.cpp:5441)
ERROR: glTF: Generating scene detected direct parented Skeletons
   at: (modules\gltf\gltf_document.cpp:5441)
ERROR: Unable to cast node 13 of type Skeleton3D to EditorSceneImporterMeshNode3D
   at: (modules\gltf\gltf_document.cpp:5987)
ERROR: Unable to find node 32
   at: (modules\gltf\gltf_document.cpp:5984)

Despite any reservations you may have about the usefulness of having mesh nodes as bones, The above glTF documents are conformant and can be generated from the UniGLTF unity plugin, and I have confirmed that this issue existed in the wild (in at least one model being sold for real money). Also, these models are confirmed to work in Blender, three.js (Don McCurty), Unity (UniGLTF) and Windows 3D Viewer.

I think we should test these fixes heavily. We don't want to break any working models. CC @fire

[fire]

There is a bug with mesh + skeletons animations. Will be debugging with Lyuma on this.

[lyuma]

Ok, I'm now pretty confident with this change. Here is my testing project with glTF files that trigger some of these bugs:

gltf_test.zip

The big change in this update is we now remove the fake_joints hack, and instead add a special case in _generate_godot_node where an extra bone attachment is created when a bone is also a mesh, camera or light.

There are still some cases where two skeletons are created which are directly parented to one another, but this change ensures that Godot behaves correctly when this happens. Whether or not two skeletons can be directly parented seems to depend on arbitrary conditions and not all test files reproduced the issue. Fixing this with the existing algorithm appears intractable to me...
The code in _determine_skeletons is supposed to detect this case in some big O(S * N^2) time complexity loop, but evidently it does not succeed in detecting them. It stands to reason that if an additional checking pass is added, perhaps that can create more direct-parent relationships.

Another approach to solve direct parented skeletons could be to recursively add all nodes to the Skeleton, which would solve the direct parented case, but this has other implications, including animation artifacts due to the inability to precisely represent bone pose animations against the rest pose.

[lyuma]

Note about the blend shape issue, it's a couple lines and possibly could be in its own PR.

It seems related to issue #38751, fixed last year: the fix addressed most of the issues, but the animation code assumed that the blend_weights array has the correct length. The two line change included with this PR ensures this.

There is a separate bug caused by EditorSceneImporterMesh that causes weights to be ignored. I have filed it here: #49005

[fire]

First pass review on code style, but haven't gotten to the meat of the algorithm.

So you add to the joints array? That works. This was described in @marstaik 's doc on the gltf spec.

Edited:

Can you also submit the file to the tests repo? https://github.com/godotengine/godot-tests

[fire]

After most of the afternoon I and Lyuma reviewed this pr in depth.

We made changes to make the functions more direct and easier to understand.

I approve of this pr to be merged. The steps to add the models godot-test should be a later step.

Edited:

Pending changes on comments and on an unused variable removal

[akien-mga]

Thanks!