Fix and clean up skeletal animation (#15722)

* Fix attachments lagging behind their parents (#14818)
* Fix animation blending (#14817)
* Bring back cool guy as another .x smoke test
* Add .x mesh loader unittest
* Do bounding box & matrix calculation at proper point in time
* Remove obsolete `SAnimatedMesh`

src/client/content_cao.cpp

@@ -178,15 +178,6 @@ static void setBillboardTextureMatrix(scene::IBillboardSceneNode *bill,
 	matrix.setTextureScale(txs, tys);
 }
 
-// Evaluate transform chain recursively; irrlicht does not do this for us
-static void updatePositionRecursive(scene::ISceneNode *node)
-{
-	scene::ISceneNode *parent = node->getParent();
-	if (parent)
-		updatePositionRecursive(parent);
-	node->updateAbsolutePosition();
-}
-
 static bool logOnce(const std::ostringstream &from, std::ostream &log_to)
 {
 	thread_local std::vector<u64> logged;
@@ -682,7 +673,6 @@ void GenericCAO::addToScene(ITextureSource *tsrc, scene::ISceneManager *smgr)
 			m_animated_meshnode = m_smgr->addAnimatedMeshSceneNode(mesh, m_matrixnode);
 			m_animated_meshnode->grab();
 			mesh->drop(); // The scene node took hold of it
-			m_animated_meshnode->animateJoints(); // Needed for some animations
 			m_animated_meshnode->setScale(m_prop.visual_size);
 
 			// set vertex colors to ensure alpha is set
@@ -693,6 +683,21 @@ void GenericCAO::addToScene(ITextureSource *tsrc, scene::ISceneManager *smgr)
 			m_animated_meshnode->forEachMaterial([this] (auto &mat) {
 				mat.BackfaceCulling = m_prop.backface_culling;
 			});
+
+			m_animated_meshnode->setOnAnimateCallback([&](f32 dtime) {
+				for (auto &it : m_bone_override) {
+					auto* bone = m_animated_meshnode->getJointNode(it.first.c_str());
+					if (!bone)
+						continue;
+
+					BoneOverride &props = it.second;
+					props.dtime_passed += dtime;
+
+					bone->setPosition(props.getPosition(bone->getPosition()));
+					bone->setRotation(props.getRotationEulerDeg(bone->getRotation()));
+					bone->setScale(props.getScale(bone->getScale()));
+				}
+			});
 		} else
 			errorstream<<"GenericCAO::addToScene(): Could not load mesh "<<m_prop.mesh<<std::endl;
 		break;
@@ -783,7 +788,6 @@ void GenericCAO::addToScene(ITextureSource *tsrc, scene::ISceneManager *smgr)
 	updateMarker();
 	updateNodePos();
 	updateAnimation();
-	updateBones(.0f);
 	updateAttachments();
 	setNodeLight(m_last_light);
 	updateMeshCulling();
@@ -1174,18 +1178,6 @@ void GenericCAO::step(float dtime, ClientEnvironment *env)
 		rot_translator.val_current = m_rotation;
 		updateNodePos();
 	}
-
-	if (m_animated_meshnode) {
-		// Everything must be updated; the whole transform
-		// chain as well as the animated mesh node.
-		// Otherwise, bone attachments would be relative to
-		// a position that's one frame old.
-		if (m_matrixnode)
-			updatePositionRecursive(m_matrixnode);
-		m_animated_meshnode->updateAbsolutePosition();
-		m_animated_meshnode->animateJoints();
-		updateBones(dtime);
-	}
 }
 
 static void setMeshBufferTextureCoords(scene::IMeshBuffer *buf, const v2f *uv, u32 count)
@@ -1394,44 +1386,6 @@ void GenericCAO::updateAnimationSpeed()
 	m_animated_meshnode->setAnimationSpeed(m_animation_speed);
 }
 
-void GenericCAO::updateBones(f32 dtime)
-{
-	if (!m_animated_meshnode)
-		return;
-	if (m_bone_override.empty()) {
-		m_animated_meshnode->setJointMode(scene::EJUOR_NONE);
-		return;
-	}
-
-	m_animated_meshnode->setJointMode(scene::EJUOR_CONTROL); // To write positions to the mesh on render
-	for (auto &it : m_bone_override) {
-		std::string bone_name = it.first;
-		scene::IBoneSceneNode* bone = m_animated_meshnode->getJointNode(bone_name.c_str());
-		if (!bone)
-			continue;
-
-		BoneOverride &props = it.second;
-		props.dtime_passed += dtime;
-
-		bone->setPosition(props.getPosition(bone->getPosition()));
-		bone->setRotation(props.getRotationEulerDeg(bone->getRotation()));
-		bone->setScale(props.getScale(bone->getScale()));
-	}
-
-	// The following is needed for set_bone_pos to propagate to
-	// attached objects correctly.
-	// Irrlicht ought to do this, but doesn't when using EJUOR_CONTROL.
-	for (u32 i = 0; i < m_animated_meshnode->getJointCount(); ++i) {
-		auto bone = m_animated_meshnode->getJointNode(i);
-		// Look for the root bone.
-		if (bone && bone->getParent() == m_animated_meshnode) {
-			// Update entire skeleton.
-			bone->updateAbsolutePositionOfAllChildren();
-			break;
-		}
-	}
-}
-
 void GenericCAO::updateAttachments()
 {
 	ClientActiveObject *parent = getParent();
@@ -1747,7 +1701,6 @@ void GenericCAO::processMessage(const std::string &data)
 		} else {
 			m_bone_override[bone] = props;
 		}
-		// updateBones(); now called every step
 	} else if (cmd == AO_CMD_ATTACH_TO) {
 		u16 parent_id = readS16(is);
 		std::string bone = deSerializeString16(is);

src/client/content_cao.h

@@ -286,8 +286,6 @@ public:
 
 	void updateAnimationSpeed();
 
-	void updateBones(f32 dtime);
-
 	void processMessage(const std::string &data) override;
 
 	bool directReportPunch(v3f dir, const ItemStack *punchitem,

src/client/mesh.cpp

@@ -10,10 +10,9 @@
 #include <cmath>
 #include <iostream>
 #include <IAnimatedMesh.h>
-#include <SAnimatedMesh.h>
 #include <IAnimatedMeshSceneNode.h>
 #include "S3DVertex.h"
-#include "SMesh.h"
+#include <SMesh.h>
 #include "SMeshBuffer.h"
 
 inline static void applyShadeFactor(video::SColor& color, float factor)
@@ -97,11 +96,8 @@ scene::IAnimatedMesh* createCubeMesh(v3f scale)
 		mesh->addMeshBuffer(buf);
 		buf->drop();
 	}
-
-	scene::SAnimatedMesh *anim_mesh = new scene::SAnimatedMesh(mesh);
-	mesh->drop();
-	scaleMesh(anim_mesh, scale);  // also recalculates bounding box
-	return anim_mesh;
+	scaleMesh(mesh, scale);  // also recalculates bounding box
+	return mesh;
 }
 
 template<typename F>

src/nodedef.cpp

@@ -4,7 +4,6 @@
 
 #include "nodedef.h"
 
-#include "SAnimatedMesh.h"
 #include "itemdef.h"
 #if CHECK_CLIENT_BUILD()
 #include "client/mesh.h"
@@ -964,13 +963,6 @@ void ContentFeatures::updateTextures(ITextureSource *tsrc, IShaderSource *shdsrc
 		// Note: By freshly reading, we get an unencumbered mesh.
 		if (scene::IMesh *src_mesh = client->getMesh(mesh)) {
 			bool apply_bs = false;
-			// For frame-animated meshes, always get the first frame,
-			// which holds a model for which we can eventually get the static pose.
-			while (auto *src_meshes = dynamic_cast<scene::SAnimatedMesh *>(src_mesh)) {
-				src_mesh = src_meshes->getMesh(0.0f);
-				src_mesh->grab();
-				src_meshes->drop();
-			}
 			if (auto *skinned_mesh = dynamic_cast<scene::SkinnedMesh *>(src_mesh)) {
 				// Compatibility: Animated meshes, as well as static gltf meshes, are not scaled by BS.
 				// See https://github.com/luanti-org/luanti/pull/16112#issuecomment-2881860329

src/unittest/CMakeLists.txt

@@ -59,6 +59,8 @@ set (UNITTEST_CLIENT_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/test_eventmanager.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_gameui.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_gltf_mesh_loader.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_x_mesh_loader.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/test_irr_matrix4.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_mesh_compare.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/test_keycode.cpp
 	PARENT_SCOPE)

src/unittest/test_irr_gltf_mesh_loader.cpp

@@ -394,36 +394,40 @@ SECTION("simple skin")
 	const auto joints = csm->getAllJoints();
 	REQUIRE(joints.size() == 3);
 
-	const auto findJoint = [&](const std::function<bool(SkinnedMesh::SJoint*)> &predicate) {
-		for (std::size_t i = 0; i < joints.size(); ++i) {
-			if (predicate(joints[i])) {
-				return joints[i];
+	const auto findJoint = [&](const std::function<bool(const SkinnedMesh::SJoint*)> &predicate) {
+		for (const auto *joint : joints) {
+			if (predicate(joint)) {
+				return joint;
 			}
 		}
 		throw std::runtime_error("joint not found");
 	};
 
 	// Check the node hierarchy
-	const auto parent = findJoint([](auto joint) {
-		return !joint->Children.empty();
+	const auto child = findJoint([&](auto *joint) {
+		return !!joint->ParentJointID;
 	});
-	REQUIRE(parent->Children.size() == 1);
-	const auto child = parent->Children[0];
-	REQUIRE(child != parent);
+	const auto *parent = joints.at(*child->ParentJointID);
 
 	SECTION("transformations are correct")
 	{
-		CHECK(parent->Animatedposition == v3f(0, 0, 0));
-		CHECK(parent->Animatedrotation == irr::core::quaternion());
-		CHECK(parent->Animatedscale == v3f(1, 1, 1));
-		CHECK(parent->GlobalInversedMatrix == irr::core::matrix4());
-		const v3f childTranslation(0, 1, 0);
-		CHECK(child->Animatedposition == childTranslation);
-		CHECK(child->Animatedrotation == irr::core::quaternion());
-		CHECK(child->Animatedscale == v3f(1, 1, 1));
-		irr::core::matrix4 inverseBindMatrix;
-		inverseBindMatrix.setTranslation(-childTranslation);
-		CHECK(child->GlobalInversedMatrix == inverseBindMatrix);
+		{
+			const auto &transform = std::get<core::Transform>(parent->transform);
+			CHECK(transform.translation == v3f(0, 0, 0));
+			CHECK(transform.rotation == irr::core::quaternion());
+			CHECK(transform.scale == v3f(1, 1, 1));
+			CHECK(parent->GlobalInversedMatrix == irr::core::matrix4());
+		}
+		{
+			const auto &transform = std::get<core::Transform>(child->transform);
+			const v3f translation(0, 1, 0);
+			CHECK(transform.translation == translation);
+			CHECK(transform.rotation == irr::core::quaternion());
+			CHECK(transform.scale == v3f(1, 1, 1));
+			irr::core::matrix4 inverseBindMatrix;
+			inverseBindMatrix.setTranslation(-translation);
+			CHECK(child->GlobalInversedMatrix == inverseBindMatrix);
+		}
 	}
 
 	SECTION("weights are correct")

src/unittest/test_irr_x_mesh_loader.cpp

@@ -0,0 +1,111 @@
+// Luanti
+// SPDX-License-Identifier: LGPL-2.1-or-later
+
+#include "catch_amalgamated.hpp"
+#include "content/subgames.h"
+#include "filesys.h"
+
+#include "irrlichttypes.h"
+#include "irr_ptr.h"
+
+#include "EDriverTypes.h"
+#include "IFileSystem.h"
+#include "IReadFile.h"
+#include "ISceneManager.h"
+#include "SkinnedMesh.h"
+#include "irrlicht.h"
+
+#include "catch.h"
+#include "matrix4.h"
+
+TEST_CASE("x") {
+
+const auto gamespec = findSubgame("devtest");
+
+if (!gamespec.isValid())
+	SKIP();
+
+irr::SIrrlichtCreationParameters p;
+p.DriverType = video::EDT_NULL;
+auto *driver = irr::createDeviceEx(p);
+
+REQUIRE(driver);
+
+auto *smgr = driver->getSceneManager();
+const auto loadMesh = [&] (const io::path& filepath) {
+	irr_ptr<io::IReadFile> file(driver->getFileSystem()->createAndOpenFile(filepath));
+	REQUIRE(file);
+	return smgr->getMesh(file.get());
+};
+
+const static auto model_stem = gamespec.gamemods_path +
+		DIR_DELIM + "testentities" + DIR_DELIM + "models" + DIR_DELIM + "testentities_";
+
+SECTION("cool guy") {
+	const auto *mesh = dynamic_cast<irr::scene::SkinnedMesh*>(loadMesh(model_stem + "cool_guy.x"));
+	REQUIRE(mesh);
+	REQUIRE(mesh->getMeshBufferCount() == 1);
+
+	auto getJointId = [&](auto name) {
+		return mesh->getJointNumber(name).value();
+	};
+
+	const auto root = getJointId("Root");
+	const auto armature = getJointId("Armature");
+	const auto armature_body = getJointId("Armature_body");
+	const auto armature_arm_r = getJointId("Armature_arm_r");
+
+	std::vector<core::matrix4> matrices;
+	matrices.reserve(mesh->getJointCount());
+	for (auto *joint : mesh->getAllJoints()) {
+		if (const auto *matrix = std::get_if<core::matrix4>(&joint->transform))
+			matrices.push_back(*matrix);
+		else
+		 	matrices.push_back(std::get<core::Transform>(joint->transform).buildMatrix());
+	}
+	auto local_matrices = matrices;
+	mesh->calculateGlobalMatrices(matrices);
+
+	SECTION("joints are topologically sorted") {
+		REQUIRE(root < armature);
+		REQUIRE(armature < armature_body);
+		REQUIRE(armature_body < armature_arm_r);
+	}
+
+	SECTION("parents are correct") {
+		const auto get_parent = [&](auto id) {
+			return mesh->getAllJoints()[id]->ParentJointID;
+		};
+		REQUIRE(!get_parent(root));
+		REQUIRE(get_parent(armature).value() == root);
+		REQUIRE(get_parent(armature_body).value() == armature);
+		REQUIRE(get_parent(armature_arm_r).value() == armature_body);
+	}
+
+	SECTION("local matrices are correct") {
+		REQUIRE(local_matrices[root].equals(core::IdentityMatrix));
+		REQUIRE(local_matrices[armature].equals(core::IdentityMatrix));
+		REQUIRE(local_matrices[armature_body] == core::matrix4(
+			-1,0,0,0,
+			0,0,1,0,
+			0,1,0,0,
+			0,2.571201,0,1
+		));
+		REQUIRE(local_matrices[armature_arm_r] == core::matrix4(
+			-0.047733,0.997488,-0.05233,0,
+			0.901521,0.020464,-0.432251,0,
+			-0.430095,-0.067809,-0.900233,
+			0,-0.545315,0,1,1
+		));
+	}
+
+	SECTION("global matrices are correct") {
+		REQUIRE(matrices[armature_body] == local_matrices[armature_body]);
+		REQUIRE(matrices[armature_arm_r] ==
+			matrices[armature_body] * local_matrices[armature_arm_r]);
+	}
+}
+
+driver->closeDevice();
+driver->drop();
+}