GLTF: Don't collapse non-joint leaf nodes when importing skeletons

[aaronfranke]

Fixes issue #75917, fixes issue #92447, supersedes PR #92452, supersedes PR #98516.

This PR fixes an issue encountered by the Blender Project DogWalk team where non-joint leaf glTF nodes were getting always collapsed into the skeleton instead of being imported as nodes. The file they exported from Blender was correctly able to be imported back into Blender, but did not work correctly in Godot. This PR fixes that.

When I initially opened this PR I tried to keep the changes minimal, but I kept running into edge case after edge case, and I finally decided I needed to completely refactor this code to make it work.

In SkinTool, the existing code essentially dug down into the node hierarchy of a skeleton root and assigned everything inside as a joint, but this isn't what we want. The correct behavior is that only joint nodes and their ancestors up to the skeleton root should be generated as bones, along with the first level of leaf bones. If a leaf glTF node is not a joint, we should allow it to stay as a node, following the skeleton's bones.

However, this leads to a problem where we generate too many nodes, so that's why this PR needs to refactor the main node generation function to make this work. We need to only generate nodes when needed, only generate bone attachments when needed, only generate the correct amount of bones, and ensure nested skeleton leaf node trees stay together. All of this is required to be consistent with the behavior in Blender.

Here is a minimal reproduction asset: skinD_09_modified.zip. This file is the "skinD" example in Animation_Skin_09.gltf which can be found here https://github.com/KhronosGroup/glTF-Asset-Generator/tree/main/Output/Positive/Animation_Skin which showcases most of the edge cases. I've modified this file to have a non-joint leaf node in the skeleton so that it showcases the problem this PR fixes. In master, this file imports like this:

With this PR, the leaf node does not get collapsed, and becomes a node, with a parent attached to the bone:

With this PR, Godot's import of the file matches the correct behavior of how Blender imports the same file.

• Bugsquad edit: Fixes Blender asset import creates a new bone for every object parented to same bone #75917, fixes Importing GLTF add fake bones to the skeleton #92447

[fire]

There are some github action errors. They look minor.

[fire]

Since we're changing the algorithm for the node tree structure and that breaks the scene tree when doing inherited scenes. We've tended to do a forward upgrade import boolean.

[fire]

It's been a long time since we've had one, but maybe @lyuma remember how we did it.

[aaronfranke]

The set of nodes generated with this PR should be a superset of the nodes generated in master.

[fire]

For example inherited nodes will have a different node path. Let's say when we place a magical rod in the bone attachment. So I'm worried this will shift if the Paths are not exactly the same. I can't experiment on this due to a pending trip.

[fire]

We have some old tests from https://github.com/godotengine/godot-tests/tree/master/tests/gltf_skeleton_tests

[fire]

Also, GLTF has no concept of skeleton root. So in theory any bone parent node up to the root node can be a skeleton root.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#_skin_skeleton

[lyuma]

If we opt to fix this, we should also fix mesh nodes. See how transformNode is both a node and a BoneAttachment + MeshInstance3D? Users also want those to become standalone nodes.

Our current behavior in Godot 4.x guarantees glTF compatibility, and Just Works without a sea of edge cases, but at the expense of usability for some users. As I understand it, though, your change is not fundamentally changing all aspects of the skeleton (these leaf nodes will still also be bones, but it adds a case where it is creating a bone attachment? it's a bit hard to follow what changed with the refactor)

Personally, I don't like all of the _end nodes creating BoneAttachments. That creates a gigantic mess on any model with hundreds of bones, and I am relieved to see those end nodes collapsed into the skeleton. I'm worried this sort of change will bring back messy skeletons, at least on FBX files (since Blender adds _end leaf bones by default).

We need to only generate nodes when needed, only generate bone attachments when needed, only generate the correct amount of bones, and ensure nested skeleton leaf node trees stay together.

I'm not sure I understand how you can know this. I guess if it came from Blender, you can make some assumptions since Blender always explicitly exports all skeleton bones as joints (this is how you know), but in general glTF has no way of communicating if a node is "when needed". The joints array isn't guaranteed to exist (and in the leaf bone case, I don't think Blender adds those to joints during .fbx export).

The correct behavior ...

Unless I'm mistaken, this isn't about correctness in the glTF spec: it is about matching the scene tree in Blender, right? I do like improving compatibility with Blender, but I'd be worried about introducing bugs with exports from other DCC applications (or even FBX, which also uses SkinTool)

I don't personally have the mental bandwidth to really dig into this problem, figure out which cases should be nodes and which should be bones, evaluate the edge cases, figure out animation bugs (remember that Godot stores bone and node transforms differently), and think about compatibility breakage, and then figure out how to rewrite all of my addons to support the new behavior (since having a mix of bones and nodes will break all of my skeleton modifier code). This is just too complex a problem for me to get into it for the foreseeable future... so I'd personally like to recuse myself from involvement in this PR.

The compatibility options can still be used where needed in projects that depend on the old behavior, so I understand the compatibility risk is minimal in this sense.

[aaronfranke]

If we opt to fix this, we should also fix mesh nodes. See how transformNode is both a node and a BoneAttachment + MeshInstance3D? Users also want those to become standalone nodes.

What do you mean? The transformNode node is not a skinned mesh, so it must be attached to the bone. This is required to be compliant with the example file from Khronos.

Personally, I don't like all of the _end nodes creating BoneAttachments. .. at least on FBX files since Blender adds

This does not happen with this PR, at least not with FBX files exported from Blender. _end bones don't get attachments.

I'm not sure I understand how you can know this.
Unless I'm mistaken, this isn't about correctness in the glTF spec: it is about matching the scene tree in Blender, right?

Yes, it's matching the behavior in Blender. Well, glTF doesn't specify how Godot or Blender should import these, but the existence of joints is pretty clearly defining which glTF nodes are bones in a skeleton.

but in general glTF has no way of communicating if a node is "when needed"

If we don't do this, people expecting to have nodes attached to skeletons (ex: the Blender Project DogWalk team) have no way to do this... almost. If we did keep this behavior, there is still a workaround that does require the refactor in my PR but undoing the SkinTool changes. Currently Godot doesn't consider GLTFDocumentExtension classes when deciding if a bone should have a node generated, but this is a mistake. However, I don't think we should require users to do this. People should be able to attach a plain node in Blender without glTF extensions and use that in Godot directly.

The joints array isn't guaranteed to exist

It is guaranteed to exist: https://github.com/KhronosGroup/glTF/blob/main/specification/2.0/schema/skin.schema.json#L33

I don't personally have the mental bandwidth to really dig into this problem,

Honestly I completely understand that. It has taken me days to do this. In fact, I thought I had it, so I opened this PR, but then I found more edge cases, so I had to put this PR as draft and rework things like 3 more times before I got it right. The old test files in the repo @fire sent were immensely useful because they contained so many edge cases. I believe this PR now works correctly for all of those.

The compatibility options can still be used where needed in projects that depend on the old behavior, so I understand the compatibility risk is minimal in this sense.

Yes, and I've updated this PR to add a new number to the glTF compatibility option.

[fire]

What's the state of this?

[aaronfranke]

@fire Probably all good now as far as I can tell, but given the impact and previous problems, we should do more testing first. Regardless I am confident this is better than the current master but ideally we find any other edge cases and fix them, if any more exist.

[fire]

Why is this still a draft pull request?

[aaronfranke]

@lyuma Yes, this PR has the correct behavior for round-tripping out of Godot and back into itself, and into Blender.

Correctly imported Godot scene from the test file in the PR description:

Correctly exported from Godot: exported.zip

The mesh on transformNode is given its own node on import, which is the correct behavior (remember that this is an edge case, we need to handle it to be spec-compliant but this is not possible to represent in either Godot or Blender), and then is given a unique name on export (not sure why 3 but again it's an edge case). The Skeleton3D's joint3 bone is a leaf bone, which is the correct behavior. It's interesting to see that the node order changed to be alphabetical, I'm not quite sure why, but well, either order is correct. The AnimationPlayer node should probably be excluded from export, but that's a separate issue from this PR (I tested that it happens on master).

Correctly imported Godot scene from the exported glTF file:

Correctly imported into Blender:

[fire]

I think it's good to go, but will poke @lyuma for an approval.

[TokageItLab]

I am concerned about the compatibility with the Import as Skeleton Bones option implemented by #88819. Since I remember it being like making all the Armature's child nodes (even if without weight) into bones.

[aaronfranke]

@TokageItLab Thanks, that was indeed not working. I've updated this PR to fix that option. The new parameter added to SkinTool::_determine_skeletons for whether to turn non-joint descendant nodes into bones or not needs to be true in the case of importing the whole scene as skeleton bones, and true in compat mode, but false if neither of those are the case.

if (p_state->get_import_as_skeleton_bones()) {
	err = SkinTool::_determine_skeletons(p_state->skins, p_state->nodes, p_state->skeletons, p_state->root_nodes, true);
} else {
	err = SkinTool::_determine_skeletons(p_state->skins, p_state->nodes, p_state->skeletons, Vector<GLTFNodeIndex>(), _naming_version < 2);
}

In a future compat breakage, we could consolidate the boolean into just checking if the root bone array is empty or not.

[lyuma]

Seems to mostly work well for the test cases, so I think it looks good.

There's a special case function _does_skinned_mesh_require_placeholder_node which I think is unnecessary and incorrect. It works well after I remove it.

[lyuma]

This function causes a regression in https://github.com/godotengine/godot-tests/tree/master/tests/gltf_skeleton_tests/gltf_tests: MeshNode becomes both a bone attachment, an empty and the mesh node is renamed to MeshNode2, even though the BoneAttachment is not needed.

MeshSkinnedToItselfAndOthers.glb

To fix it, I enitrely removed _does_skinned_mesh_require_placeholder_node (made it always return false), and it seemed to solve the rest of the corner cases. I tested with both the above gltf_skeleton_tests as well as the MRP from #67773 which is mentioned in the "Edge case" comment below, and I found it works correctly.

[aaronfranke]

Part of this function is an extracted version of the existing logic here

godot/modules/gltf/gltf_document.cpp

Line 6322 in 42c7f14

if (gltf_node->skin >= 0 && gltf_node->mesh >= 0 && !gltf_node->children.is_empty()) {

You added this in PR #72158 actually: https://github.com/godotengine/godot/pull/72158/files#diff-fdf33f6fcd2b129d540ebd0e70fd04982b68e279cebfd9e96036d2fd7561c31fR5643

The function checks for skinned meshes (existing check), checks for non-joint children (modified from the check for no children), and checks if child joints don't have their skeletons in the tree yet (new check, an edge case that becomes required to check for because of the modified second check). Replacing the function with return false; ignores part of the point of this PR, so that's the wrong move.

I think this explains it: The function is technically correct that we do need a placeholder in this case, but where we go wrong is by generating a new placeholder node. The subtlety here is that in this case, the Skeleton3D itself should be the placeholder. In some cases we need a new placeholder within a skeleton, in most cases we need to make a new Skeleton3D node that doesn't map to any glTF node, but in this case, these overlap, so we can feed two birds with one scone by using the Skeleton3D as the placeholder, inserting it into the tree immediately.

I just pushed this PR with this diff, which fixes the file you linked.

Additionally, this now means the generated tree matches what GLTFNode.get_scene_node_path expects with PR #104787 included (errors in the current master).

EDIT: It is also possible that user code generates a Skeleton3D node in a GLTFDocumentExtension, and this code also works for that case, so long as the user wants it named Skeleton3D or sets its name manually.

[lyuma]

It's hard for me to conceptualize enough of this code to be 100% certain, but you offer a reasonable explanation of the fix to the placeholder -> skinned mesh issue, and it seems to cover the test cases I have, so I'm satisfied.

I'll continue testing during beta, but I think it's good to merge. Nice work solving such a difficult round trip problem, Aaron.

[Repiteo]

Thanks!

[JonqsGames]

Sorry but the simple test project on my PR #98516 is not working as expected. I think it even worse since now parenting looks mixed up. Can you confirm that this PR was meant to fix the leaf bone created by godot when mesh is parented to a bone ?

[aaronfranke]

@JonqsGames Make sure you change the naming version to "Godot 4.5 or later" in the Import dock.