Merge pull request #5007 from malortie/fix_hl1_mdl_importer_bone_hierarchy

Fix Half-Life 1 MDL importer bone hierarchy.

code/AssetLib/MDL/HalfLife/HL1MDLLoader.cpp

@@ -470,14 +470,16 @@ void HL1MDLLoader::read_bones() {
 
     temp_bones_.resize(header_->numbones);
 
+    // Create the main 'bones' node that will contain all MDL root bones.
     aiNode *bones_node = new aiNode(AI_MDL_HL1_NODE_BONES);
     rootnode_children_.push_back(bones_node);
-    bones_node->mNumChildren = static_cast<unsigned int>(header_->numbones);
-    bones_node->mChildren = new aiNode *[bones_node->mNumChildren];
+
+    // Store roots bones IDs temporarily.
+    std::vector<int> roots;
 
     // Create bone matrices in local space.
     for (int i = 0; i < header_->numbones; ++i) {
-        aiNode *bone_node = temp_bones_[i].node = bones_node->mChildren[i] = new aiNode(unique_bones_names[i]);
+        aiNode *bone_node = temp_bones_[i].node = new aiNode(unique_bones_names[i]);
 
         aiVector3D angles(pbone[i].value[3], pbone[i].value[4], pbone[i].value[5]);
         temp_bones_[i].absolute_transform = bone_node->mTransformation =
@@ -485,9 +487,11 @@ void HL1MDLLoader::read_bones() {
                         aiVector3D(pbone[i].value[0], pbone[i].value[1], pbone[i].value[2]));
 
         if (pbone[i].parent == -1) {
-            bone_node->mParent = scene_->mRootNode;
+            bone_node->mParent = bones_node;
+            roots.push_back(i); // This bone has no parent. Add it to the roots list.
         } else {
-            bone_node->mParent = bones_node->mChildren[pbone[i].parent];
+            bone_node->mParent = temp_bones_[pbone[i].parent].node;
+            temp_bones_[pbone[i].parent].children.push_back(i); // Add this bone to the parent bone's children list.
 
             temp_bones_[i].absolute_transform =
                     temp_bones_[pbone[i].parent].absolute_transform * bone_node->mTransformation;
@@ -496,6 +500,36 @@ void HL1MDLLoader::read_bones() {
         temp_bones_[i].offset_matrix = temp_bones_[i].absolute_transform;
         temp_bones_[i].offset_matrix.Inverse();
     }
+
+    // Allocate memory for each MDL root bone.
+    bones_node->mNumChildren = static_cast<unsigned int>(roots.size());
+    bones_node->mChildren = new aiNode *[bones_node->mNumChildren];
+
+    // Build all bones children hierarchy starting from each MDL root bone.
+    for (size_t i = 0; i < roots.size(); ++i)
+    {
+        const TempBone &root_bone = temp_bones_[roots[i]];
+        bones_node->mChildren[i] = root_bone.node;
+        build_bone_children_hierarchy(root_bone);
+    }
+}
+
+void HL1MDLLoader::build_bone_children_hierarchy(const TempBone &bone)
+{
+    if (bone.children.empty())
+        return;
+
+    aiNode* bone_node = bone.node;
+    bone_node->mNumChildren = static_cast<unsigned int>(bone.children.size());
+    bone_node->mChildren = new aiNode *[bone_node->mNumChildren];
+
+    // Build each child bone's hierarchy recursively.
+    for (size_t i = 0; i < bone.children.size(); ++i)
+    {
+        const TempBone &child_bone = temp_bones_[bone.children[i]];
+        bone_node->mChildren[i] = child_bone.node;
+        build_bone_children_hierarchy(child_bone);
+    }
 }
 
 // ------------------------------------------------------------------------------------------------

code/AssetLib/MDL/HalfLife/HL1MDLLoader.h

@@ -143,6 +143,14 @@ private:
      */
     static bool get_num_blend_controllers(const int num_blend_animations, int &num_blend_controllers);
 
+    /**
+     *  \brief Build a bone's node children hierarchy.
+     *
+     * \param[in] bone The bone for which we must build all children hierarchy.
+     */
+    struct TempBone;
+    void build_bone_children_hierarchy(const TempBone& bone);
+
     /** Output scene to be filled */
     aiScene *scene_;
 
@@ -198,11 +206,13 @@ private:
         TempBone() :
             node(nullptr),
             absolute_transform(),
-            offset_matrix() {}
+            offset_matrix(),
+            children() {}
 
         aiNode *node;
         aiMatrix4x4 absolute_transform;
         aiMatrix4x4 offset_matrix;
+        std::vector<int> children; // Bone children
     };
 
     std::vector<TempBone> temp_bones_;

test/unit/ImportExport/MDL/utMDLImporter_HL1_Nodes.cpp

@@ -55,6 +55,17 @@ using namespace Assimp;
 
 class utMDLImporter_HL1_Nodes : public ::testing::Test {
 
+    /**
+    * @note Represents a flattened node hierarchy where each item is a pair
+    * containing the node level and it's name.
+    */
+    using Hierarchy = std::vector<std::pair<unsigned int, std::string>>;
+
+    /**
+    * @note A vector of strings. Used for symplifying syntax.
+    */
+    using StringVector = std::vector<std::string>;
+
 public:
     /**
     * @note The following tests require a basic understanding
@@ -63,6 +74,51 @@ public:
     * (Valve Developer Community).
     */
 
+    // Given a model, verify that the bones nodes hierarchy is correctly formed.
+    void checkBoneHierarchy() {
+        Assimp::Importer importer;
+        const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "multiple_roots.mdl", aiProcess_ValidateDataStructure);
+        ASSERT_NE(nullptr, scene);
+        ASSERT_NE(nullptr, scene->mRootNode);
+
+        // First, check that "<MDL_root>" and "<MDL_bones>" are linked.
+        const aiNode* node_MDL_root = scene->mRootNode->FindNode(AI_MDL_HL1_NODE_ROOT);
+        ASSERT_NE(nullptr, node_MDL_root);
+
+        const aiNode *node_MDL_bones = scene->mRootNode->FindNode(AI_MDL_HL1_NODE_BONES);
+        ASSERT_NE(nullptr, node_MDL_bones);
+        ASSERT_NE(nullptr, node_MDL_bones->mParent);
+        ASSERT_EQ(node_MDL_root, node_MDL_bones->mParent);
+
+        // Second, verify "<MDL_bones>" hierarchy.
+        const Hierarchy expected_hierarchy = {
+            { 0, AI_MDL_HL1_NODE_BONES },
+                { 1, "root1_bone1" },
+                    { 2, "root1_bone2" },
+                        { 3, "root1_bone4" },
+                        { 3, "root1_bone5" },
+                    { 2, "root1_bone3" },
+                        { 3, "root1_bone6" },
+                { 1, "root2_bone1" },
+                    { 2, "root2_bone2" },
+                    { 2, "root2_bone3" },
+                        { 3, "root2_bone5" },
+                    { 2, "root2_bone4" },
+                        { 3, "root2_bone6" },
+                { 1, "root3_bone1" },
+                    { 2, "root3_bone2" },
+                    { 2, "root3_bone3" },
+                    { 2, "root3_bone4" },
+                        { 3, "root3_bone5" },
+                            { 4, "root3_bone6" },
+                            { 4, "root3_bone7" },
+        };
+
+        Hierarchy actual_hierarchy;
+        flatten_hierarchy(node_MDL_bones, actual_hierarchy);
+        ASSERT_EQ(expected_hierarchy, actual_hierarchy);
+    }
+
     /*  Given a model with bones that have empty names,
         verify that all the bones of the imported model
         have unique and no empty names.
@@ -82,7 +138,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "unnamed_bones.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_bones_names = {
+        const StringVector expected_bones_names = {
             "Bone",
             "Bone_0",
             "Bone_1",
@@ -94,7 +150,9 @@ public:
             "Bone_7"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_BONES, expected_bones_names);
+        StringVector actual_bones_names;
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_BONES), actual_bones_names);
+        ASSERT_EQ(expected_bones_names, actual_bones_names);
     }
 
     /*  Given a model with bodyparts that have empty names,
@@ -116,7 +174,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "unnamed_bodyparts.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_bodyparts_names = {
+        const StringVector expected_bodyparts_names = {
             "Bodypart",
             "Bodypart_1",
             "Bodypart_5",
@@ -128,7 +186,10 @@ public:
             "Bodypart_7"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_BODYPARTS, expected_bodyparts_names);
+        StringVector actual_bodyparts_names;
+        // Get the bodyparts names "without" the submodels.
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_BODYPARTS), actual_bodyparts_names, 0);
+        ASSERT_EQ(expected_bodyparts_names, actual_bodyparts_names);
     }
 
     /*  Given a model with bodyparts that have duplicate names,
@@ -150,7 +211,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "duplicate_bodyparts.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_bodyparts_names = {
+        const StringVector expected_bodyparts_names = {
             "Bodypart",
             "Bodypart_1",
             "Bodypart_2",
@@ -162,7 +223,10 @@ public:
             "Bodypart_4"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_BODYPARTS, expected_bodyparts_names);
+        StringVector actual_bodyparts_names;
+        // Get the bodyparts names "without" the submodels.
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_BODYPARTS), actual_bodyparts_names, 0);
+        ASSERT_EQ(expected_bodyparts_names, actual_bodyparts_names);
     }
 
     /*  Given a model with several bodyparts that contains multiple
@@ -192,7 +256,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "duplicate_submodels.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::vector<std::string>> expected_bodypart_sub_models_names = {
+        const std::vector<StringVector> expected_bodypart_sub_models_names = {
             {
                     "triangle",
                     "triangle_0",
@@ -210,9 +274,13 @@ public:
         const aiNode *bodyparts_node = scene->mRootNode->FindNode(AI_MDL_HL1_NODE_BODYPARTS);
         EXPECT_NE(nullptr, bodyparts_node);
         EXPECT_EQ(3u, bodyparts_node->mNumChildren);
-        for (unsigned int i = 0; i < bodyparts_node->mNumChildren; ++i) {
-            expect_named_children(bodyparts_node->mChildren[i],
-                    expected_bodypart_sub_models_names[i]);
+
+        StringVector actual_submodels_names;
+        for (unsigned int i = 0; i < bodyparts_node->mNumChildren; ++i)
+        {
+            actual_submodels_names.clear();
+            get_node_children_names(bodyparts_node->mChildren[i], actual_submodels_names);
+            ASSERT_EQ(expected_bodypart_sub_models_names[i], actual_submodels_names);
         }
     }
 
@@ -235,7 +303,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "duplicate_sequences.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_sequence_names = {
+        const StringVector expected_sequence_names = {
             "idle_1",
             "idle",
             "idle_2",
@@ -247,7 +315,9 @@ public:
             "idle_7"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_SEQUENCE_INFOS, expected_sequence_names);
+        StringVector actual_sequence_names;
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_SEQUENCE_INFOS), actual_sequence_names);
+        ASSERT_EQ(expected_sequence_names, actual_sequence_names);
     }
 
     /*  Given a model with sequences that have empty names, verify
@@ -269,7 +339,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "unnamed_sequences.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_sequence_names = {
+        const StringVector expected_sequence_names = {
             "Sequence",
             "Sequence_1",
             "Sequence_0",
@@ -281,7 +351,9 @@ public:
             "Sequence_6"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_SEQUENCE_INFOS, expected_sequence_names);
+        StringVector actual_sequence_names;
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_SEQUENCE_INFOS), actual_sequence_names);
+        ASSERT_EQ(expected_sequence_names, actual_sequence_names);
     }
 
     /*  Given a model with sequence groups that have duplicate names,
@@ -304,7 +376,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "duplicate_sequence_groups/duplicate_sequence_groups.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_sequence_names = {
+        const StringVector expected_sequence_names = {
             "default",
             "SequenceGroup",
             "SequenceGroup_1",
@@ -317,7 +389,9 @@ public:
             "SequenceGroup_2"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_SEQUENCE_GROUPS, expected_sequence_names);
+        StringVector actual_sequence_names;
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_SEQUENCE_GROUPS), actual_sequence_names);
+        ASSERT_EQ(expected_sequence_names, actual_sequence_names);
     }
 
     /*  Given a model with sequence groups that have empty names,
@@ -340,7 +414,7 @@ public:
         const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MDL_HL1_MODELS_DIR "unnamed_sequence_groups/unnamed_sequence_groups.mdl", aiProcess_ValidateDataStructure);
         EXPECT_NE(nullptr, scene);
 
-        const std::vector<std::string> expected_sequence_names = {
+        const StringVector expected_sequence_names = {
             "default",
             "SequenceGroup",
             "SequenceGroup_2",
@@ -353,7 +427,9 @@ public:
             "SequenceGroup_4"
         };
 
-        expect_named_children(scene, AI_MDL_HL1_NODE_SEQUENCE_GROUPS, expected_sequence_names);
+        StringVector actual_sequence_names;
+        get_node_children_names(scene->mRootNode->FindNode(AI_MDL_HL1_NODE_SEQUENCE_GROUPS), actual_sequence_names);
+        ASSERT_EQ(expected_sequence_names, actual_sequence_names);
     }
 
     /*  Verify that mOffsetMatrix applies the correct
@@ -398,26 +474,58 @@ public:
     }
 
 private:
-    void expect_named_children(const aiNode *parent_node, const std::vector<std::string> &expected_names) {
-        EXPECT_NE(nullptr, parent_node);
-        EXPECT_EQ(expected_names.size(), parent_node->mNumChildren);
+    void expect_equal_matrices(const aiMatrix4x4 &expected, const aiMatrix4x4 &actual, float abs_error) {
+        for (int i = 0; i < 4; ++i) {
+            for (int j = 0; j < 4; ++j)
+                EXPECT_NEAR(expected[i][j], actual[i][j], abs_error);
+        }
+    }
 
-        for (unsigned int i = 0; i < parent_node->mNumChildren; ++i)
-            EXPECT_EQ(expected_names[i], parent_node->mChildren[i]->mName.C_Str());
+    /** Get a flattened representation of a node's hierarchy.
+     * \param[in] node The node.
+     * \param[out] hierarchy The flattened node's hierarchy.
+     */
+    void flatten_hierarchy(const aiNode *node, Hierarchy &hierarchy)
+    {
+        flatten_hierarchy_impl(node, hierarchy, 0);
     }
 
-    void expect_named_children(const aiScene *scene, const char *node_name, const std::vector<std::string> &expected_names) {
-        expect_named_children(scene->mRootNode->FindNode(node_name), expected_names);
+    void flatten_hierarchy_impl(const aiNode *node, Hierarchy &hierarchy, unsigned int level)
+    {
+        hierarchy.push_back({ level, node->mName.C_Str() });
+        for (size_t i = 0; i < node->mNumChildren; ++i)
+        {
+            flatten_hierarchy_impl(node->mChildren[i], hierarchy, level + 1);
+        }
     }
 
-    void expect_equal_matrices(const aiMatrix4x4 &expected, const aiMatrix4x4 &actual, float abs_error) {
-        for (int i = 0; i < 4; ++i) {
-            for (int j = 0; j < 4; ++j)
-                EXPECT_NEAR(expected[i][j], actual[i][j], abs_error);
+    /** Get all node's children names beneath max_level.
+     * \param[in] node The parent node from which to get all children names.
+     * \param[out] names The list of children names.
+     * \param[in] max_level If set to -1, all children names will be collected.
+     */
+    void get_node_children_names(const aiNode *node, StringVector &names, const int max_level = -1)
+    {
+        get_node_children_names_impl(node, names, 0, max_level);
+    }
+
+    void get_node_children_names_impl(const aiNode *node, StringVector &names, int level, const int max_level = -1)
+    {
+        for (size_t i = 0; i < node->mNumChildren; ++i)
+        {
+            names.push_back(node->mChildren[i]->mName.C_Str());
+            if (max_level == -1 || level < max_level)
+            {
+                get_node_children_names_impl(node->mChildren[i], names, level + 1, max_level);
+            }
         }
     }
 };
 
+TEST_F(utMDLImporter_HL1_Nodes, checkBoneHierarchy) {
+    checkBoneHierarchy();
+}
+
 TEST_F(utMDLImporter_HL1_Nodes, emptyBonesNames) {
     emptyBonesNames();
 }