Merge pull request #2469 from muxanickms/topic/fbx_node_naming_optimization

Node names optimization and fixing non-unique name

code/FBXConverter.cpp

@@ -86,7 +86,6 @@ namespace Assimp {
         , textures_converted()
         , meshes_converted()
         , node_anim_chain_bits()
-        , mNodeNameInstances()
         , mNodeNames()
         , anim_fps()
         , out(out)
@@ -142,12 +141,46 @@ namespace Assimp {
 
         void FBXConverter::ConvertRootNode() {
             out->mRootNode = new aiNode();
-            out->mRootNode->mName.Set("RootNode");
+            std::string unique_name;
+            GetUniqueName("RootNode", unique_name);
+            out->mRootNode->mName.Set(unique_name);
 
             // root has ID 0
             ConvertNodes(0L, *out->mRootNode);
         }
 
+        static std::string getAncestorBaseName(const aiNode* node)
+        {
+            const char* nodeName = nullptr;
+            size_t length = 0;
+            while (node && (!nodeName || length == 0))
+            {
+                nodeName = node->mName.C_Str();
+                length = node->mName.length;
+                node = node->mParent;
+            }
+
+            if (!nodeName || length == 0)
+            {
+                return {};
+            }
+            // could be std::string_view if c++17 available
+            return std::string(nodeName, length);
+        }
+
+        // Make unique name
+        std::string FBXConverter::MakeUniqueNodeName(const Model* const model, const aiNode& parent)
+        {
+            std::string original_name = FixNodeName(model->Name());
+            if (original_name.empty())
+            {
+                original_name = getAncestorBaseName(&parent);
+            }
+            std::string unique_name;
+            GetUniqueName(original_name, unique_name);
+            return unique_name;
+        }
+
         void FBXConverter::ConvertNodes(uint64_t id, aiNode& parent, const aiMatrix4x4& parent_transform) {
             const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(id, "Model");
 
@@ -179,35 +212,18 @@ namespace Assimp {
 
                         aiMatrix4x4 new_abs_transform = parent_transform;
 
+                        std::string unique_name = MakeUniqueNodeName(model, parent);
+
                         // even though there is only a single input node, the design of
                         // assimp (or rather: the complicated transformation chain that
                         // is employed by fbx) means that we may need multiple aiNode's
                         // to represent a fbx node's transformation.
-                        GenerateTransformationNodeChain(*model, nodes_chain, post_nodes_chain);
+                        const bool need_additional_node = GenerateTransformationNodeChain(*model, unique_name, nodes_chain, post_nodes_chain);
 
                         ai_assert(nodes_chain.size());
 
-                        std::string original_name = FixNodeName(model->Name());
-
-                        // check if any of the nodes in the chain has the name the fbx node
-                        // is supposed to have. If there is none, add another node to
-                        // preserve the name - people might have scripts etc. that rely
-                        // on specific node names.
-                        aiNode* name_carrier = NULL;
-                        for (aiNode* prenode : nodes_chain) {
-                            if (!strcmp(prenode->mName.C_Str(), original_name.c_str())) {
-                                name_carrier = prenode;
-                                break;
-                            }
-                        }
-
-                        if (!name_carrier) {
-                            std::string old_original_name = original_name;
-                            GetUniqueName(old_original_name, original_name);
-                            nodes_chain.push_back(new aiNode(original_name));
-                        }
-                        else {
-                            original_name = nodes_chain.back()->mName.C_Str();
+                        if (need_additional_node) {
+                            nodes_chain.push_back(new aiNode(unique_name));
                         }
 
                         //setup metadata on newest node
@@ -269,11 +285,11 @@ namespace Assimp {
                         ConvertNodes(model->ID(), *last_parent, new_abs_transform);
 
                         if (doc.Settings().readLights) {
-                            ConvertLights(*model, original_name);
+                            ConvertLights(*model, unique_name);
                         }
 
                         if (doc.Settings().readCameras) {
-                            ConvertCameras(*model, original_name);
+                            ConvertCameras(*model, unique_name);
                         }
 
                         nodes.push_back(nodes_chain.front());
@@ -426,21 +442,16 @@ namespace Assimp {
         void FBXConverter::GetUniqueName(const std::string &name, std::string &uniqueName)
         {
             uniqueName = name;
-            int i = 0;
-            auto it = mNodeNameInstances.find(name); // duplicate node name instance count
-            if (it != mNodeNameInstances.end())
+            auto it_pair = mNodeNames.insert({ name, 0 }); // duplicate node name instance count
+            unsigned int& i = it_pair.first->second;
+            while (!it_pair.second)
             {
-                i = it->second;
-                while (mNodeNames.find(uniqueName) != mNodeNames.end())
-                {
-                    i++;
-                    std::stringstream ext;
-                    ext << name << std::setfill('0') << std::setw(3) << i;
-                    uniqueName = ext.str();
-                }
+                i++;
+                std::ostringstream ext;
+                ext << name << std::setfill('0') << std::setw(3) << i;
+                uniqueName = ext.str();
+                it_pair = mNodeNames.insert({ uniqueName, 0 });
             }
-            mNodeNameInstances[name] = i;
-            mNodeNames.insert(uniqueName);
         }
 
         const char* FBXConverter::NameTransformationComp(TransformationComp comp) {
@@ -672,7 +683,7 @@ namespace Assimp {
             return name + std::string(MAGIC_NODE_TAG) + "_" + NameTransformationComp(comp);
         }
 
-        void FBXConverter::GenerateTransformationNodeChain(const Model& model, std::vector<aiNode*>& output_nodes,
+        bool FBXConverter::GenerateTransformationNodeChain(const Model& model, const std::string& name, std::vector<aiNode*>& output_nodes,
             std::vector<aiNode*>& post_output_nodes) {
             const PropertyTable& props = model.Props();
             const Model::RotOrder rot = model.RotationOrder();
@@ -787,8 +798,6 @@ namespace Assimp {
             // not be guaranteed.
             ai_assert(NeedsComplexTransformationChain(model) == is_complex);
 
-            std::string name = FixNodeName(model.Name());
-
             // now, if we have more than just Translation, Scaling and Rotation,
             // we need to generate a full node chain to accommodate for assimp's
             // lack to express pivots and offsets.
@@ -830,20 +839,20 @@ namespace Assimp {
                 }
 
                 ai_assert(output_nodes.size());
-                return;
+                return true;
             }
 
             // else, we can just multiply the matrices together
             aiNode* nd = new aiNode();
             output_nodes.push_back(nd);
-            std::string uniqueName;
-            GetUniqueName(name, uniqueName);
 
-            nd->mName.Set(uniqueName);
+            // name passed to the method is already unique
+            nd->mName.Set(name);
 
             for (const auto &transform : chain) {
                 nd->mTransformation = nd->mTransformation * transform;
             }
+            return false;
         }
 
         void FBXConverter::SetupNodeMetadata(const Model& model, aiNode& nd)

code/FBXConverter.h

@@ -154,6 +154,11 @@ private:
     // while these would be allowed, they are a potential trouble spot so better not use them).
     const char* NameTransformationComp(TransformationComp comp);
 
+    // ------------------------------------------------------------------------------------------------
+    // Returns an unique name for a node or traverses up a hierarchy until a non-empty name is found and
+    // then makes this name unique
+    std::string MakeUniqueNodeName(const Model* const model, const aiNode& parent);
+
     // ------------------------------------------------------------------------------------------------
     // note: this returns the REAL fbx property names
     const char* NameTransformationCompProperty(TransformationComp comp);
@@ -177,7 +182,7 @@ private:
     /**
     *  note: memory for output_nodes will be managed by the caller
     */
-    void GenerateTransformationNodeChain(const Model& model, std::vector<aiNode*>& output_nodes, std::vector<aiNode*>& post_output_nodes);
+    bool GenerateTransformationNodeChain(const Model& model, const std::string& name, std::vector<aiNode*>& output_nodes, std::vector<aiNode*>& post_output_nodes);
 
     // ------------------------------------------------------------------------------------------------
     void SetupNodeMetadata(const Model& model, aiNode& nd);
@@ -457,10 +462,7 @@ private:
     NodeAnimBitMap node_anim_chain_bits;
 
     // number of nodes with the same name
-    using NodeAnimNameMap = std::unordered_map<std::string, unsigned int>;
-    NodeAnimNameMap mNodeNameInstances;
-
-    using NodeNameCache = std::unordered_set<std::string>;
+    using NodeNameCache = std::unordered_map<std::string, unsigned int>;
     NodeNameCache mNodeNames;
 
     double anim_fps;

test/unit/utFBXImporterExporter.cpp

@@ -76,6 +76,119 @@ TEST_F( utFBXImporterExporter, importBareBoxWithoutColorsAndTextureCoords ) {
     EXPECT_EQ(mesh->mNumVertices, 36);
 }
 
+TEST_F(utFBXImporterExporter, importCubesWithNoNames) {
+    Assimp::Importer importer;
+    const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/FBX/cubes_nonames.fbx", aiProcess_ValidateDataStructure);
+    ASSERT_TRUE(scene);
+
+    ASSERT_TRUE(scene->mRootNode);
+    const auto root = scene->mRootNode;
+    ASSERT_STREQ(root->mName.C_Str(), "RootNode");
+    ASSERT_TRUE(root->mChildren);
+    ASSERT_EQ(root->mNumChildren, 2);
+
+    const auto child0 = root->mChildren[0];
+    ASSERT_TRUE(child0);
+    ASSERT_STREQ(child0->mName.C_Str(), "RootNode001");
+    ASSERT_TRUE(child0->mChildren);
+    ASSERT_EQ(child0->mNumChildren, 1);
+
+    const auto child00 = child0->mChildren[0];
+    ASSERT_TRUE(child00);
+    ASSERT_STREQ(child00->mName.C_Str(), "RootNode001001");
+
+    const auto child1 = root->mChildren[1];
+    ASSERT_TRUE(child1);
+    ASSERT_STREQ(child1->mName.C_Str(), "RootNode002");
+    ASSERT_TRUE(child1->mChildren);
+    ASSERT_EQ(child1->mNumChildren, 1);
+
+    const auto child10 = child1->mChildren[0];
+    ASSERT_TRUE(child10);
+    ASSERT_STREQ(child10->mName.C_Str(), "RootNode002001");
+}
+
+TEST_F(utFBXImporterExporter, importCubesWithUnicodeDuplicatedNames) {
+    Assimp::Importer importer;
+    const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/FBX/cubes_with_names.fbx", aiProcess_ValidateDataStructure);
+    ASSERT_TRUE(scene);
+
+    ASSERT_TRUE(scene->mRootNode);
+    const auto root = scene->mRootNode;
+    ASSERT_STREQ(root->mName.C_Str(), "RootNode");
+    ASSERT_TRUE(root->mChildren);
+    ASSERT_EQ(root->mNumChildren, 2);
+
+    const auto child0 = root->mChildren[0];
+    ASSERT_TRUE(child0);
+    ASSERT_STREQ(child0->mName.C_Str(), "Cube2");
+    ASSERT_TRUE(child0->mChildren);
+    ASSERT_EQ(child0->mNumChildren, 1);
+
+    const auto child00 = child0->mChildren[0];
+    ASSERT_TRUE(child00);
+    ASSERT_STREQ(child00->mName.C_Str(), "\xd0\x9a\xd1\x83\xd0\xb1\x31");
+
+    const auto child1 = root->mChildren[1];
+    ASSERT_TRUE(child1);
+    ASSERT_STREQ(child1->mName.C_Str(), "Cube3");
+    ASSERT_TRUE(child1->mChildren);
+    ASSERT_EQ(child1->mNumChildren, 1);
+
+    const auto child10 = child1->mChildren[0];
+    ASSERT_TRUE(child10);
+    ASSERT_STREQ(child10->mName.C_Str(), "\xd0\x9a\xd1\x83\xd0\xb1\x31""001");
+}
+
+TEST_F(utFBXImporterExporter, importCubesComplexTransform) {
+    Assimp::Importer importer;
+    const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/FBX/cubes_with_mirroring_and_pivot.fbx", aiProcess_ValidateDataStructure);
+    ASSERT_TRUE(scene);
+
+    ASSERT_TRUE(scene->mRootNode);
+    const auto root = scene->mRootNode;
+    ASSERT_STREQ(root->mName.C_Str(), "RootNode");
+    ASSERT_TRUE(root->mChildren);
+    ASSERT_EQ(root->mNumChildren, 2);
+
+    const auto child0 = root->mChildren[0];
+    ASSERT_TRUE(child0);
+    ASSERT_STREQ(child0->mName.C_Str(), "Cube2");
+    ASSERT_TRUE(child0->mChildren);
+    ASSERT_EQ(child0->mNumChildren, 1);
+
+    const auto child00 = child0->mChildren[0];
+    ASSERT_TRUE(child00);
+    ASSERT_STREQ(child00->mName.C_Str(), "Cube1");
+
+    const auto child1 = root->mChildren[1];
+    ASSERT_TRUE(child1);
+    ASSERT_STREQ(child1->mName.C_Str(), "Cube3");
+
+    auto parent = child1;
+    const size_t chain_length = 8u;
+    const char* chainStr[chain_length] = {
+        "Cube1001_$AssimpFbx$_Translation",
+        "Cube1001_$AssimpFbx$_RotationPivot",
+        "Cube1001_$AssimpFbx$_RotationPivotInverse",
+        "Cube1001_$AssimpFbx$_ScalingOffset",
+        "Cube1001_$AssimpFbx$_ScalingPivot",
+        "Cube1001_$AssimpFbx$_Scaling",
+        "Cube1001_$AssimpFbx$_ScalingPivotInverse",
+        "Cube1001"
+    };
+    for (size_t i = 0; i < chain_length; ++i)
+    {
+        ASSERT_TRUE(parent->mChildren);
+        ASSERT_EQ(parent->mNumChildren, 1);
+        auto node = parent->mChildren[0];
+        ASSERT_TRUE(node);
+        ASSERT_STREQ(node->mName.C_Str(), chainStr[i]);
+        parent = node;
+    }
+    ASSERT_EQ(0, parent->mNumChildren) << "Leaf node";
+}
+
 TEST_F( utFBXImporterExporter, importPhongMaterial ) {
     Assimp::Importer importer;
     const aiScene *scene = importer.ReadFile( ASSIMP_TEST_MODELS_DIR "/FBX/phong_cube.fbx", aiProcess_ValidateDataStructure );