Give bone attachments in the GLTF importer stable and proper names, and deduplicate.

[mortarroad]

Same as #47270, but for master.

[fire]

I put this onto my bucket list. The changes to the import dialogue took my time for today.

[fire]

@mortarroad Since I'm here, can you give me a visual and an import project of before and after.

I want to approve without compiling the engine and importing.

Edited: I mean a regular test case project without .import / .godot.

[mortarroad]

You mean the project with an .import folder?
I've got nothing pretty, just a chain of bones with cubes, if that's enough.
And I'll have to recompile, too.

[mortarroad]

@fire If that's okay, I will only provide an example after I've fixed another issue that I found:

Currently, unique names for nodes are enforced globally, even though in Godot unique names are only required within each parent node.
Now, if a bone has the same name as a node (for example a "head" node parented to the "head" bone), one of them will be renamed, even though it is not required.

I think that should be fixed too, before this can be merged.

[mortarroad]

On master, the set of all unique names is part of the public interface.
But I'm not really sure that there is a reason to do this at all.
Node::add_child can already generate nice names, so that would even remove complexity.

Is there a reason why the name uniqueness is enforced in the importer?

[fire]

All gltf nodes must have unique names, so the gltf importer enforces it there.

[mortarroad]

Ah, so I suppose I can just get rid of it for BoneAttachments, since those aren't GLTF nodes anyway.

[mortarroad]

I will update the PR against 3.x soon.

Before:

After:

GLTFBoneNameTest-master.zip

GLTFBoneNameTest-fixed-bone-names.zip

[fire]

My opinion is that if we are auto-generating the names of the bone attachments we can make them unique in the entire scene. Having two nodes with the same name looks ugly :D

Although Godot only enforces uniqueness on sibling nodes, the fact Godot can also export a gltf2 scene means I want both import and export to be symmetrical.

Thoughts?

[mortarroad]

I think the name should stay identical to the name in the file, if possible.
For example, having a "head" MeshInstance attached to a BoneAttachment called "head2" doesn't make any sense.

[fire]

When I want to move the head do I animate the "head" or "head"/ "head" node in the animation player?

I also recall that the BoneAttachment3D has a relation to a bone. Maybe we can use that name? Like BoneNameBoneAttachment3D (original.bone_CasingBoneAttachment3D is fine too).

[mortarroad]

Hm, I had also thought about adding a prefix / postfix and I think it is acceptable, though I'd prefer a short one if possible.
I'd still prefer to avoid renaming, if it is not necessary.
And in the end, it's the users fault, if their object hierarchy is named in a confusing way :P

[fire]

I agree with Hm, I had also thought about adding a prefix / postfix and I think it is acceptable, though I'd prefer a short one if possible.. Want to modify the pr?

[mortarroad]

Hm, I'm not sure what the best prefix would be.
"Bone" would fit in the style, but only if the bone's name is in PascalCase, too, and I don't think we can assume that.
Otherwise something like "bone_" could be good.
Automatically detecting which prefix should be used would be possible, but I think it's not a great idea because it is less predictable.

What's also possible is converting the bone's name into PascalCase, then the "Bone" prefix could be used. (There seems to be a function for that, but it's local to the mono module and would have to be moved.)

[fire]

Is that camelcase_to_underscore()?

[mortarroad]

No, I meant the other way around, because nodes are usually in PascalCase.
It looks like this is the only function that does this:

godot/modules/mono/editor/bindings_generator.cpp

Line 114 in 8368f53

static String snake_to_pascal_case(const String &p_identifier, bool p_input_is_upper = false) {

[mortarroad]

I suppose that's overthinking it.
I'd either go with "Bone" or "Bone_" as the prefix.
Which would you prefer? I'll update the PR accordingly.

[mortarroad]

I think getting the prefix perfectly right isn't as big of an issue, since a user can always rename these using a post import script (see EditorScenePostImport).

[fire]

Can you help get this reviewed by rebasing? Thanks!

[mortarroad]

I'm a bit confused what the new code does; and I have no files to test it with.
I don't understand in what situation it would happen.

[fire]

I thought the design was name the boneattachments after the bone?

[mortarroad]

Yes.
I mean for rebasing. I'll have to take a closer look at what changed meanwhile; the changes are non-trivial.

[mortarroad]

PR that caused the conflict #48915
So perhaps @lyuma could review too?

The updated version assumes that _get_or_generate_bone_attachment will always be called with the same scene_parent and scene_root for the same bone_index.
I haven't tested very much. master is just very glitchy all around.

[mortarroad]

The iOS build failure looks spurious.

[fire]

The code has changed a bit, can you review?

[fire]

The bone attachment code was revised can you update and see if your changes apply? It's holiday season so things will be delayed.

[mortarroad]

I did a rebase, but I don't currently have the time to do testing.

[fire]

Will need to go through the tests @lyuma made with the odd skeletons.

https://github.com/godotengine/godot-tests/tree/master/tests/gltf_skeleton_tests

[fire]

Fixed by: #72162

[akien-mga]

Superseded by #72162.