gltf: Fix mesh nodes which are also bones.

Fix issue when two skeletons end up directly parented.
Prevent animating TRS for skinned Mesh node.
Fix animating weights on meshes with targets but no weights.

5 files changed, 121 insertions, 138 deletions

diff --git a/modules/gltf/doc_classes/GLTFNode.xml b/modules/gltf/doc_classes/GLTFNode.xml
index 5b7d4fadec..af7be55e4b 100644
--- a/modules/gltf/doc_classes/GLTFNode.xml
+++ b/modules/gltf/doc_classes/GLTFNode.xml
@@ -13,8 +13,6 @@
 		</member>
 		<member name="children" type="PackedInt32Array" setter="set_children" getter="get_children" default="PackedInt32Array(  )">
 		</member>
-		<member name="fake_joint_parent" type="int" setter="set_fake_joint_parent" getter="get_fake_joint_parent" default="-1">
-		</member>
 		<member name="height" type="int" setter="set_height" getter="get_height" default="-1">
 		</member>
 		<member name="joint" type="bool" setter="set_joint" getter="get_joint" default="false">
diff --git a/modules/gltf/gltf_document.cpp b/modules/gltf/gltf_document.cpp
index ac76506b87..abac0a2e17 100644
--- a/modules/gltf/gltf_document.cpp
+++ b/modules/gltf/gltf_document.cpp
@@ -2821,8 +2821,8 @@ Error GLTFDocument::_parse_meshes(Ref<GLTFState> state) {
 				}
 				blend_weights.write[j] = weights[j];
 			}
-			mesh->set_blend_weights(blend_weights);
 		}
+		mesh->set_blend_weights(blend_weights);
 		mesh->set_mesh(import_mesh);
 
 		state->meshes.push_back(mesh);
@@ -4108,81 +4108,10 @@ Error GLTFDocument::_reparent_non_joint_skeleton_subtrees(Ref<GLTFState> state,
 		subtree_set.get_members(subtree_nodes, subtree_root);
 
 		for (int subtree_i = 0; subtree_i < subtree_nodes.size(); ++subtree_i) {
-			ERR_FAIL_COND_V(_reparent_to_fake_joint(state, skeleton, subtree_nodes[subtree_i]), FAILED);
-
-			// We modified the tree, recompute all the heights
-			_compute_node_heights(state);
-		}
-	}
-
-	return OK;
-}
-
-Error GLTFDocument::_reparent_to_fake_joint(Ref<GLTFState> state, Ref<GLTFSkeleton> skeleton, const GLTFNodeIndex node_index) {
-	Ref<GLTFNode> node = state->nodes[node_index];
-
-	// Can we just "steal" this joint if it is just a spatial node?
-	if (node->skin < 0 && node->mesh < 0 && node->camera < 0) {
-		node->joint = true;
-		// Add the joint to the skeletons joints
-		skeleton->joints.push_back(node_index);
-		return OK;
-	}
-
-	GLTFNode *fake_joint = memnew(GLTFNode);
-	const GLTFNodeIndex fake_joint_index = state->nodes.size();
-	state->nodes.push_back(fake_joint);
-
-	// We better not be a joint, or we messed up in our logic
-	if (node->joint) {
-		return FAILED;
-	}
-
-	fake_joint->translation = node->translation;
-	fake_joint->rotation = node->rotation;
-	fake_joint->scale = node->scale;
-	fake_joint->xform = node->xform;
-	fake_joint->joint = true;
-
-	// We can use the exact same name here, because the joint will be inside a skeleton and not the scene
-	fake_joint->set_name(node->get_name());
-
-	// Clear the nodes transforms, since it will be parented to the fake joint
-	node->translation = Vector3(0, 0, 0);
-	node->rotation = Quat();
-	node->scale = Vector3(1, 1, 1);
-	node->xform = Transform();
-
-	// Transfer the node children to the fake joint
-	for (int child_i = 0; child_i < node->children.size(); ++child_i) {
-		Ref<GLTFNode> child = state->nodes[node->children[child_i]];
-		child->parent = fake_joint_index;
-	}
-
-	fake_joint->children = node->children;
-	node->children.clear();
-
-	// add the fake joint to the parent and remove the original joint
-	if (node->parent >= 0) {
-		Ref<GLTFNode> parent = state->nodes[node->parent];
-		parent->children.erase(node_index);
-		parent->children.push_back(fake_joint_index);
-		fake_joint->parent = node->parent;
-	}
-
-	// Add the node to the fake joint
-	fake_joint->children.push_back(node_index);
-	node->parent = fake_joint_index;
-	node->fake_joint_parent = fake_joint_index;
-
-	// Add the fake joint to the skeletons joints
-	skeleton->joints.push_back(fake_joint_index);
-
-	// Replace skin_skeletons with fake joints if we must.
-	for (GLTFSkinIndex skin_i = 0; skin_i < state->skins.size(); ++skin_i) {
-		Ref<GLTFSkin> skin = state->skins.write[skin_i];
-		if (skin->skin_root == node_index) {
-			skin->skin_root = fake_joint_index;
+			Ref<GLTFNode> node = state->nodes[subtree_nodes[subtree_i]];
+			node->joint = true;
+			// Add the joint to the skeletons joints
+			skeleton->joints.push_back(subtree_nodes[subtree_i]);
 		}
 	}
 
@@ -4920,10 +4849,9 @@ void GLTFDocument::_assign_scene_names(Ref<GLTFState> state) {
 	}
 }
 
-BoneAttachment3D *GLTFDocument::_generate_bone_attachment(Ref<GLTFState> state, Skeleton3D *skeleton, const GLTFNodeIndex node_index) {
+BoneAttachment3D *GLTFDocument::_generate_bone_attachment(Ref<GLTFState> state, Skeleton3D *skeleton, const GLTFNodeIndex node_index, const GLTFNodeIndex bone_index) {
 	Ref<GLTFNode> gltf_node = state->nodes[node_index];
-	Ref<GLTFNode> bone_node = state->nodes[gltf_node->parent];
-
+	Ref<GLTFNode> bone_node = state->nodes[bone_index];
 	BoneAttachment3D *bone_attachment = memnew(BoneAttachment3D);
 	print_verbose("glTF: Creating bone attachment for: " + gltf_node->get_name());
 
@@ -5008,7 +4936,7 @@ EditorSceneImporterMeshNode3D *GLTFDocument::_generate_mesh_instance(Ref<GLTFSta
 	return mi;
 }
 
-Light3D *GLTFDocument::_generate_light(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index) {
+Node3D *GLTFDocument::_generate_light(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index) {
 	Ref<GLTFNode> gltf_node = state->nodes[node_index];
 
 	ERR_FAIL_INDEX_V(gltf_node->light, state->lights.size(), nullptr);
@@ -5057,7 +4985,7 @@ Light3D *GLTFDocument::_generate_light(Ref<GLTFState> state, Node *scene_parent,
 		light->set_param(SpotLight3D::PARAM_SPOT_ATTENUATION, angle_attenuation);
 		return light;
 	}
-	return nullptr;
+	return memnew(Node3D);
 }
 
 Camera3D *GLTFDocument::_generate_camera(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index) {
@@ -5429,31 +5357,22 @@ void GLTFDocument::_convert_mesh_to_gltf(Node *p_scene_parent, Ref<GLTFState> st
 void GLTFDocument::_generate_scene_node(Ref<GLTFState> state, Node *scene_parent, Node3D *scene_root, const GLTFNodeIndex node_index) {
 	Ref<GLTFNode> gltf_node = state->nodes[node_index];
 
+	if (gltf_node->skeleton >= 0) {
+		_generate_skeleton_bone_node(state, scene_parent, scene_root, node_index);
+		return;
+	}
+
 	Node3D *current_node = nullptr;
 
 	// Is our parent a skeleton
 	Skeleton3D *active_skeleton = Object::cast_to<Skeleton3D>(scene_parent);
 
-	if (gltf_node->skeleton >= 0) {
-		Skeleton3D *skeleton = state->skeletons[gltf_node->skeleton]->godot_skeleton;
-
-		if (active_skeleton != skeleton) {
-			ERR_FAIL_COND_MSG(active_skeleton != nullptr, "glTF: Generating scene detected direct parented Skeletons");
-
-			// Add it to the scene if it has not already been added
-			if (skeleton->get_parent() == nullptr) {
-				scene_parent->add_child(skeleton);
-				skeleton->set_owner(scene_root);
-			}
-		}
-
-		active_skeleton = skeleton;
-		current_node = skeleton;
-	}
+	const bool non_bone_parented_to_skeleton = active_skeleton;
 
-	// If we have an active skeleton, and the node is node skinned, we need to create a bone attachment
-	if (current_node == nullptr && active_skeleton != nullptr && gltf_node->skin < 0) {
-		BoneAttachment3D *bone_attachment = _generate_bone_attachment(state, active_skeleton, node_index);
+	// skinned meshes must not be placed in a bone attachment.
+	if (non_bone_parented_to_skeleton && gltf_node->skin < 0) {
+		// Bone Attachment - Parent Case
+		BoneAttachment3D *bone_attachment = _generate_bone_attachment(state, active_skeleton, node_index, gltf_node->parent);
 
 		scene_parent->add_child(bone_attachment);
 		bone_attachment->set_owner(scene_root);
@@ -5467,7 +5386,86 @@ void GLTFDocument::_generate_scene_node(Ref<GLTFState> state, Node *scene_parent
 	}
 
 	// We still have not managed to make a node
-	if (current_node == nullptr) {
+	if (gltf_node->mesh >= 0) {
+		current_node = _generate_mesh_instance(state, scene_parent, node_index);
+	} else if (gltf_node->camera >= 0) {
+		current_node = _generate_camera(state, scene_parent, node_index);
+	} else if (gltf_node->light >= 0) {
+		current_node = _generate_light(state, scene_parent, node_index);
+	} else {
+		current_node = _generate_spatial(state, scene_parent, node_index);
+	}
+
+	scene_parent->add_child(current_node);
+	if (current_node != scene_root) {
+		current_node->set_owner(scene_root);
+	}
+	current_node->set_transform(gltf_node->xform);
+	current_node->set_name(gltf_node->get_name());
+
+	state->scene_nodes.insert(node_index, current_node);
+
+	for (int i = 0; i < gltf_node->children.size(); ++i) {
+		_generate_scene_node(state, current_node, scene_root, gltf_node->children[i]);
+	}
+}
+
+void GLTFDocument::_generate_skeleton_bone_node(Ref<GLTFState> state, Node *scene_parent, Node3D *scene_root, const GLTFNodeIndex node_index) {
+	Ref<GLTFNode> gltf_node = state->nodes[node_index];
+
+	Node3D *current_node = nullptr;
+
+	Skeleton3D *skeleton = state->skeletons[gltf_node->skeleton]->godot_skeleton;
+	// In this case, this node is already a bone in skeleton.
+	const bool is_skinned_mesh = (gltf_node->skin >= 0 && gltf_node->mesh >= 0);
+	const bool requires_extra_node = (gltf_node->mesh >= 0 || gltf_node->camera >= 0 || gltf_node->light >= 0);
+
+	Skeleton3D *active_skeleton = Object::cast_to<Skeleton3D>(scene_parent);
+	if (active_skeleton != skeleton) {
+		if (active_skeleton) {
+			// Bone Attachment - Direct Parented Skeleton Case
+			BoneAttachment3D *bone_attachment = _generate_bone_attachment(state, active_skeleton, node_index, gltf_node->parent);
+
+			scene_parent->add_child(bone_attachment);
+			bone_attachment->set_owner(scene_root);
+
+			// There is no gltf_node that represent this, so just directly create a unique name
+			bone_attachment->set_name(_gen_unique_name(state, "BoneAttachment3D"));
+
+			// We change the scene_parent to our bone attachment now. We do not set current_node because we want to make the node
+			// and attach it to the bone_attachment
+			scene_parent = bone_attachment;
+			WARN_PRINT(vformat("glTF: Generating scene detected direct parented Skeletons at node %d", node_index));
+		}
+
+		// Add it to the scene if it has not already been added
+		if (skeleton->get_parent() == nullptr) {
+			scene_parent->add_child(skeleton);
+			skeleton->set_owner(scene_root);
+		}
+	}
+
+	active_skeleton = skeleton;
+	current_node = skeleton;
+
+	if (requires_extra_node) {
+		// skinned meshes must not be placed in a bone attachment.
+		if (!is_skinned_mesh) {
+			// Bone Attachment - Same Node Case
+			BoneAttachment3D *bone_attachment = _generate_bone_attachment(state, active_skeleton, node_index, node_index);
+
+			scene_parent->add_child(bone_attachment);
+			bone_attachment->set_owner(scene_root);
+
+			// There is no gltf_node that represent this, so just directly create a unique name
+			bone_attachment->set_name(_gen_unique_name(state, "BoneAttachment3D"));
+
+			// We change the scene_parent to our bone attachment now. We do not set current_node because we want to make the node
+			// and attach it to the bone_attachment
+			scene_parent = bone_attachment;
+		}
+
+		// We still have not managed to make a node
 		if (gltf_node->mesh >= 0) {
 			current_node = _generate_mesh_instance(state, scene_parent, node_index);
 		} else if (gltf_node->camera >= 0) {
@@ -5476,22 +5474,18 @@ void GLTFDocument::_generate_scene_node(Ref<GLTFState> state, Node *scene_parent
 			current_node = _generate_light(state, scene_parent, node_index);
 		}
 
-		if (!current_node) {
-			current_node = _generate_spatial(state, scene_parent, node_index);
-		}
-
 		scene_parent->add_child(current_node);
 		if (current_node != scene_root) {
 			current_node->set_owner(scene_root);
 		}
-		current_node->set_transform(gltf_node->xform);
+		// Do not set transform here. Transform is already applied to our bone.
 		current_node->set_name(gltf_node->get_name());
 	}
 
 	state->scene_nodes.insert(node_index, current_node);
 
 	for (int i = 0; i < gltf_node->children.size(); ++i) {
-		_generate_scene_node(state, current_node, scene_root, gltf_node->children[i]);
+		_generate_scene_node(state, active_skeleton, scene_root, gltf_node->children[i]);
 	}
 }
 
@@ -5632,28 +5626,30 @@ void GLTFDocument::_import_animation(Ref<GLTFState> state, AnimationPlayer *ap,
 
 	for (Map<int, GLTFAnimation::Track>::Element *track_i = anim->get_tracks().front(); track_i; track_i = track_i->next()) {
 		const GLTFAnimation::Track &track = track_i->get();
-		//need to find the path
+		//need to find the path: for skeletons, weight tracks will affect the mesh
 		NodePath node_path;
+		//for skeletons, transform tracks always affect bones
+		NodePath transform_node_path;
 
 		GLTFNodeIndex node_index = track_i->key();
-		if (state->nodes[node_index]->fake_joint_parent >= 0) {
-			// Should be same as parent
-			node_index = state->nodes[node_index]->fake_joint_parent;
-		}
 
 		const Ref<GLTFNode> gltf_node = state->nodes[track_i->key()];
 
+		Node *root = ap->get_parent();
+		ERR_FAIL_COND(root == nullptr);
+		Map<GLTFNodeIndex, Node *>::Element *node_element = state->scene_nodes.find(node_index);
+		ERR_CONTINUE_MSG(node_element == nullptr, vformat("Unable to find node %d for animation", node_index));
+		node_path = root->get_path_to(node_element->get());
+
 		if (gltf_node->skeleton >= 0) {
-			const Skeleton3D *sk = Object::cast_to<Skeleton3D>(state->scene_nodes.find(node_index)->get());
+			const Skeleton3D *sk = state->skeletons[gltf_node->skeleton]->godot_skeleton;
 			ERR_FAIL_COND(sk == nullptr);
 
 			const String path = ap->get_parent()->get_path_to(sk);
 			const String bone = gltf_node->get_name();
-			node_path = path + ":" + bone;
+			transform_node_path = path + ":" + bone;
 		} else {
-			Node *root = ap->get_parent();
-			Node *godot_node = state->scene_nodes.find(node_index)->get();
-			node_path = root->get_path_to(godot_node);
+			transform_node_path = node_path;
 		}
 
 		for (int i = 0; i < track.rotation_track.times.size(); i++) {
@@ -5672,11 +5668,13 @@ void GLTFDocument::_import_animation(Ref<GLTFState> state, AnimationPlayer *ap,
 			}
 		}
 
-		if (track.rotation_track.values.size() || track.translation_track.values.size() || track.scale_track.values.size()) {
+		// Animated TRS properties will not affect a skinned mesh.
+		const bool transform_affects_skinned_mesh_instance = gltf_node->skeleton < 0 && gltf_node->skin >= 0;
+		if ((track.rotation_track.values.size() || track.translation_track.values.size() || track.scale_track.values.size()) && !transform_affects_skinned_mesh_instance) {
 			//make transform track
 			int track_idx = animation->get_track_count();
 			animation->add_track(Animation::TYPE_TRANSFORM);
-			animation->track_set_path(track_idx, node_path);
+			animation->track_set_path(track_idx, transform_node_path);
 			//first determine animation length
 
 			const double increment = 1.0 / bake_fps;
diff --git a/modules/gltf/gltf_document.h b/modules/gltf/gltf_document.h
index bda1ce87d6..900c367010 100644
--- a/modules/gltf/gltf_document.h
+++ b/modules/gltf/gltf_document.h
@@ -260,11 +260,12 @@ private:
 	Error _serialize_animations(Ref<GLTFState> state);
 	BoneAttachment3D *_generate_bone_attachment(Ref<GLTFState> state,
 			Skeleton3D *skeleton,
-			const GLTFNodeIndex node_index);
+			const GLTFNodeIndex node_index,
+			const GLTFNodeIndex bone_index);
 	EditorSceneImporterMeshNode3D *_generate_mesh_instance(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index);
 	Camera3D *_generate_camera(Ref<GLTFState> state, Node *scene_parent,
 			const GLTFNodeIndex node_index);
-	Light3D *_generate_light(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index);
+	Node3D *_generate_light(Ref<GLTFState> state, Node *scene_parent, const GLTFNodeIndex node_index);
 	Node3D *_generate_spatial(Ref<GLTFState> state, Node *scene_parent,
 			const GLTFNodeIndex node_index);
 	void _assign_scene_names(Ref<GLTFState> state);
@@ -365,6 +366,7 @@ public:
 	void _generate_scene_node(Ref<GLTFState> state, Node *scene_parent,
 			Node3D *scene_root,
 			const GLTFNodeIndex node_index);
+	void _generate_skeleton_bone_node(Ref<GLTFState> state, Node *scene_parent, Node3D *scene_root, const GLTFNodeIndex node_index);
 	void _import_animation(Ref<GLTFState> state, AnimationPlayer *ap,
 			const GLTFAnimationIndex index, const int bake_fps);
 	GLTFMeshIndex _convert_mesh_instance(Ref<GLTFState> state,
diff --git a/modules/gltf/gltf_node.cpp b/modules/gltf/gltf_node.cpp
index 777c6fbd9a..f6f33ef009 100644
--- a/modules/gltf/gltf_node.cpp
+++ b/modules/gltf/gltf_node.cpp
@@ -55,8 +55,6 @@ void GLTFNode::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("set_scale", "scale"), &GLTFNode::set_scale);
 	ClassDB::bind_method(D_METHOD("get_children"), &GLTFNode::get_children);
 	ClassDB::bind_method(D_METHOD("set_children", "children"), &GLTFNode::set_children);
-	ClassDB::bind_method(D_METHOD("get_fake_joint_parent"), &GLTFNode::get_fake_joint_parent);
-	ClassDB::bind_method(D_METHOD("set_fake_joint_parent", "fake_joint_parent"), &GLTFNode::set_fake_joint_parent);
 	ClassDB::bind_method(D_METHOD("get_light"), &GLTFNode::get_light);
 	ClassDB::bind_method(D_METHOD("set_light", "light"), &GLTFNode::set_light);
 
@@ -72,7 +70,6 @@ void GLTFNode::_bind_methods() {
 	ADD_PROPERTY(PropertyInfo(Variant::QUAT, "rotation"), "set_rotation", "get_rotation"); // Quat
 	ADD_PROPERTY(PropertyInfo(Variant::VECTOR3, "scale"), "set_scale", "get_scale"); // Vector3
 	ADD_PROPERTY(PropertyInfo(Variant::PACKED_INT32_ARRAY, "children"), "set_children", "get_children"); // Vector<int>
-	ADD_PROPERTY(PropertyInfo(Variant::INT, "fake_joint_parent"), "set_fake_joint_parent", "get_fake_joint_parent"); // GLTFNodeIndex
 	ADD_PROPERTY(PropertyInfo(Variant::INT, "light"), "set_light", "get_light"); // GLTFLightIndex
 }
 
@@ -172,14 +169,6 @@ void GLTFNode::set_children(Vector<int> p_children) {
 	children = p_children;
 }
 
-GLTFNodeIndex GLTFNode::get_fake_joint_parent() {
-	return fake_joint_parent;
-}
-
-void GLTFNode::set_fake_joint_parent(GLTFNodeIndex p_fake_joint_parent) {
-	fake_joint_parent = p_fake_joint_parent;
-}
-
 GLTFLightIndex GLTFNode::get_light() {
 	return light;
 }
diff --git a/modules/gltf/gltf_node.h b/modules/gltf/gltf_node.h
index ce8aff8944..3a5689d004 100644
--- a/modules/gltf/gltf_node.h
+++ b/modules/gltf/gltf_node.h
@@ -53,7 +53,6 @@ private:
 	Quat rotation;
 	Vector3 scale = Vector3(1, 1, 1);
 	Vector<int> children;
-	GLTFNodeIndex fake_joint_parent = -1;
 	GLTFLightIndex light = -1;
 
 protected:
@@ -96,9 +95,6 @@ public:
 	Vector<int> get_children();
 	void set_children(Vector<int> p_children);
 
-	GLTFNodeIndex get_fake_joint_parent();
-	void set_fake_joint_parent(GLTFNodeIndex p_fake_joint_parent);
-
 	GLTFLightIndex get_light();
 	void set_light(GLTFLightIndex p_light);
 };