Factorize bone attachment #98516

JonqsGames · 2024-10-25T09:37:16Z

Aiming to solve #75917
Just set things in order to avoid creating bones for nodes that are meshes, this also implied to rework BoneAttachement3D creation.
I factorized everything so meshes attached to the same bone are now sharing the same BoneAttachement3D.

Tested using a blender model, not sure if this will fit GLTF file coming from other modelization tool.

BEFORE	AFTER

fire · 2024-10-25T11:51:28Z

I am worried that because the code isn't elegant it would be hard to fix the possible bugs in this.

The node tree to skeleton tree is the most fragile part of the GLTFDocument code.

Recall we had to divide one function into three methods to make verification possible.

We'll have to run through the entire large suite of test cases in https://github.com/godotengine/godot-tests/tree/master/tests/gltf_skeleton_tests.

Also the export and import verification

JonqsGames · 2024-10-25T12:33:04Z

@fire Tried to keep the changes small to discuss feasibility, can you point at what is inelegant in this implementation ?
I could run a bunch of test if the solution looks ok.

fire · 2024-10-25T15:33:03Z

At the minimum, we’ll need a forward upgrade flag that enables by default but stays off for existing imports. An importer ~~property~~ option.

JonqsGames · 2024-10-25T16:50:44Z

@fire Not sure how to do that but i'll search for feature implementing this kind of flag in the past.
For the option i'm not sure how to name it since the previous implementation was an error of interpretation of the GTLF format (Adding new bones with offsets to the armature), maybe "Legacy attachement import" would do the trick.
Tested a bunch of model from the repo you linked, animation looks fines and most of the armature looks cleaner after this change.

fire · 2024-10-25T20:47:09Z

We tried an use_legacy_names option called #48058.

Will try to find more details. It's been several years since we had to do that procedure.

JonqsGames · 2024-10-26T16:37:48Z

I added a legacy flag in import settings.
Right now i had to keep the bone architecture for legacy to avoid adding more parameter to the 'SkinTool' class. Maybe the skeleton creation should be moved out of this class for clarity.
I have some issues to make the legacy behavior, i can't find a way to differenciate the case of a new import (should use default value : false) or an existing import file not containing the flag (in this case i want to set the flag to true).

fire · 2024-10-26T18:33:42Z

modules/gltf/gltf_document.cpp

@@ -5124,7 +5132,7 @@ BoneAttachment3D *GLTFDocument::_generate_bone_attachment(Ref<GLTFState> p_state
 	Ref<GLTFNode> gltf_node = p_state->nodes[p_node_index];
 	Ref<GLTFNode> bone_node = p_state->nodes[p_bone_index];
 	BoneAttachment3D *bone_attachment = memnew(BoneAttachment3D);
-	print_verbose("glTF: Creating bone attachment for: " + gltf_node->get_name());
+	print_verbose("glTF: Creating bone attachment for: " + _legacy_attachements_import ? gltf_node->get_name() : bone_node->get_name());


this is causing the cicd to fail.

Gonna remove the inline in next commit

fire · 2024-10-26T18:35:39Z

I have some issues to make the legacy behavior, i can't find a way to differenciate the case of a new import (should use default value : false) or an existing import file not containing the flag (in this case i want to set the flag to true).

@lyuma Do you remember how the old legacy system worked?

fire · 2024-10-26T19:56:16Z

I suspect this code also runs for FBX, hmm.

lyuma · 2024-10-27T00:40:33Z

Okay so the idea here is instead of trying to solve it for every case, it specifically addresses the case of bone attachments which are meshes, since AFAIK it is not possible to create these in Blender.

Because it is a very specific fix, it is less likely to break one of the test cases, and with a setting, users can change it if it does.

By the way, the option name misspells "attachments". Instead of calling it legacy, would a more specific name make sense the option determines if we should create bones for mesh nodes.

I suspect this code also runs for FBX

that's a good thing I think.

I'm a bit cautious, but I know meshes inside a skeleton also being bones is a common complaint, and this doesn't change that much code. Clearly the tests need to pass.

Do you remember how the old legacy system worked?

i think the toplevel scene import options always get serialized, so it is possible to have a different default from what is initially set in the .import --- we need to make sure project settings import defaults are respected too

JonqsGames · 2024-10-27T09:33:34Z

@lyuma I tried to find a way to have different default value but could'nt get it to work. Right now i think it's not possible to have this kind of legacy flag.

Not sure if the behavior of having mesh being bones is only linked to Blender.
My guess was it's a confusion of having SkeletonId in node means it's a bone. But it could also apply to this case : a mesh parented to the skeleton via a bone.
In this case i think this is more of a fix and justify the legacy flag, because the old way is not the expected behavior.

fire · 2024-10-29T00:24:35Z

Some documentation was changed breaking the tests and we can't merge with broken tests.

Mickeon · 2024-12-03T18:17:11Z

modules/gltf/doc_classes/GLTFDocument.xml

+		<member name="legacy_attachments_import" type="bool" setter="set_legacy_attachments_import" getter="get_legacy_attachments_import" default="true">
+			If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.
+		</member>


As @fire brought up, this is not exposed on GLTFDocument and should be removed.

Suggested change

<member name="legacy_attachments_import" type="bool" setter="set_legacy_attachments_import" getter="get_legacy_attachments_import" default="true">

If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.

</member>

Mickeon · 2024-12-03T18:19:40Z

doc/classes/ResourceImporterScene.xml

@@ -59,6 +59,9 @@
 		<member name="nodes/import_as_skeleton_bones" type="bool" setter="" getter="" default="false">
 			Treat all nodes in the imported scene as if they are bones within a single [Skeleton3D]. Can be used to guarantee that imported animations target skeleton bones rather than nodes. May also be used to assign the [code]"Root"[/code] bone in a [BoneMap]. See [url=$DOCS_URL/tutorials/assets_pipeline/retargeting_3d_skeletons.html]Retargeting 3D Skeletons[/url] for more information.
 		</member>
+		<member name="nodes/legacy_attachments_import" type="bool" setter="" getter="" default="true">
+			If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.


Suggested change

If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.

If [code]true[/code], uses the legacy import method, creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.

JonqsGames requested a review from a team as a code owner October 25, 2024 09:37

Chaosus added bug topic:import topic:3d labels Oct 25, 2024

Chaosus added this to the 4.4 milestone Oct 25, 2024

JonqsGames force-pushed the clean_gltf_skeleton_attach branch from 6853538 to beb3bab Compare October 25, 2024 09:58

Factorize bone attachament

d7e5466

JonqsGames force-pushed the clean_gltf_skeleton_attach branch from beb3bab to d7e5466 Compare October 25, 2024 10:01

JonqsGames changed the title ~~Factorize bone attachament~~ Factorize bone attachment Oct 25, 2024

JonqsGames changed the title ~~Factorize bone attachment~~ Factorize bone attachement Oct 25, 2024

fire added feature proposal enhancement and removed bug feature proposal labels Oct 25, 2024

JonqsGames requested review from a team as code owners October 26, 2024 16:31

JonqsGames force-pushed the clean_gltf_skeleton_attach branch 2 times, most recently from 05fc82e to 4e764f0 Compare October 26, 2024 16:45

Add legacy flag

045b7b1

JonqsGames force-pushed the clean_gltf_skeleton_attach branch from 4e764f0 to 045b7b1 Compare October 26, 2024 17:01

fire reviewed Oct 26, 2024

View reviewed changes

Fix misspell, change default value

1ea711a

JonqsGames changed the title ~~Factorize bone attachement~~ Factorize bone attachment Oct 27, 2024

Mickeon reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factorize bone attachment #98516

Factorize bone attachment #98516

JonqsGames commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

JonqsGames commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

JonqsGames commented Oct 25, 2024

fire commented Oct 25, 2024

JonqsGames commented Oct 26, 2024 •

edited

Loading

fire Oct 26, 2024

JonqsGames Oct 26, 2024

fire commented Oct 26, 2024

fire commented Oct 26, 2024

lyuma commented Oct 27, 2024

JonqsGames commented Oct 27, 2024 •

edited

Loading

fire commented Oct 29, 2024 •

edited

Loading

Mickeon Dec 3, 2024

Mickeon Dec 3, 2024

	<member name="legacy_attachments_import" type="bool" setter="set_legacy_attachments_import" getter="get_legacy_attachments_import" default="true">
	If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.
	</member>

	If [code]true[/code], use the legacy import method creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.
	If [code]true[/code], uses the legacy import method, creating a bone and a [BoneAttachment3D] for each mesh parented to a bone.

Factorize bone attachment #98516

Are you sure you want to change the base?

Factorize bone attachment #98516

Conversation

JonqsGames commented Oct 25, 2024 • edited Loading

fire commented Oct 25, 2024 • edited Loading

JonqsGames commented Oct 25, 2024 • edited Loading

fire commented Oct 25, 2024 • edited Loading

JonqsGames commented Oct 25, 2024

fire commented Oct 25, 2024

JonqsGames commented Oct 26, 2024 • edited Loading

fire Oct 26, 2024

Choose a reason for hiding this comment

JonqsGames Oct 26, 2024

Choose a reason for hiding this comment

fire commented Oct 26, 2024

fire commented Oct 26, 2024

lyuma commented Oct 27, 2024

JonqsGames commented Oct 27, 2024 • edited Loading

fire commented Oct 29, 2024 • edited Loading

Mickeon Dec 3, 2024

Choose a reason for hiding this comment

Mickeon Dec 3, 2024

Choose a reason for hiding this comment

JonqsGames commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

JonqsGames commented Oct 25, 2024 •

edited

Loading

fire commented Oct 25, 2024 •

edited

Loading

JonqsGames commented Oct 26, 2024 •

edited

Loading

JonqsGames commented Oct 27, 2024 •

edited

Loading

fire commented Oct 29, 2024 •

edited

Loading