Merge pull request #3484 from MalcolmTyrrell/findDegeneratesOptimization

Optimize FindDegenerates so it doesn't explode

code/PostProcessing/FindDegenerates.cpp

@@ -5,8 +5,6 @@ Open Asset Import Library (assimp)
 
 Copyright (c) 2006-2020, assimp team
 
-
-
 All rights reserved.
 
 Redistribution and use of this software in source and binary forms,
@@ -45,25 +43,23 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *  @brief Implementation of the FindDegenerates post-process step.
 */
 
-
-
-// internal headers
 #include "ProcessHelper.h"
 #include "FindDegenerates.h"
+
 #include <assimp/Exceptional.h>
 
+#include <unordered_map>
+
 using namespace Assimp;
 
-//remove mesh at position 'index' from the scene
+// Correct node indices to meshes and remove references to deleted mesh
-static void removeMesh(aiScene* pScene, unsigned const index);
+static void updateSceneGraph(aiNode* pNode, const std::unordered_map<unsigned int, unsigned int>& meshMap);
-//correct node indices to meshes and remove references to deleted mesh
-static void updateSceneGraph(aiNode* pNode, unsigned const index);
 
 // ------------------------------------------------------------------------------------------------
 // Constructor to be privately used by Importer
-FindDegeneratesProcess::FindDegeneratesProcess()
+FindDegeneratesProcess::FindDegeneratesProcess() :
-: mConfigRemoveDegenerates( false )
+        mConfigRemoveDegenerates( false ),
-, mConfigCheckAreaOfTriangle( false ){
+        mConfigCheckAreaOfTriangle( false ){
     // empty
 }
 
@@ -91,50 +87,50 @@ void FindDegeneratesProcess::SetupProperties(const Importer* pImp) {
 // Executes the post processing step on the given imported data.
 void FindDegeneratesProcess::Execute( aiScene* pScene) {
     ASSIMP_LOG_DEBUG("FindDegeneratesProcess begin");
-    for (unsigned int i = 0; i < pScene->mNumMeshes;++i)
+    if ( nullptr == pScene) {
-    {
+        return;
-        //Do not process point cloud, ExecuteOnMesh works only with faces data
+    }
+    
+    std::unordered_map<unsigned int, unsigned int> meshMap;
+    meshMap.reserve(pScene->mNumMeshes);
+
+    const unsigned int originalNumMeshes = pScene->mNumMeshes;
+    unsigned int targetIndex = 0;
+    for (unsigned int i = 0; i < pScene->mNumMeshes; ++i) {
+        // Do not process point cloud, ExecuteOnMesh works only with faces data
         if ((pScene->mMeshes[i]->mPrimitiveTypes != aiPrimitiveType::aiPrimitiveType_POINT) && ExecuteOnMesh(pScene->mMeshes[i])) {
-            removeMesh(pScene, i);
+            delete pScene->mMeshes[i];
-            --i; //the current i is removed, do not skip the next one
+            // Not strictly required, but clean:
+            pScene->mMeshes[i] = nullptr;
+        } else {
+            meshMap[i] = targetIndex;
+            pScene->mMeshes[targetIndex] = pScene->mMeshes[i];
+            ++targetIndex;
         }
     }
+    pScene->mNumMeshes = targetIndex;
+
+    if (meshMap.size() < originalNumMeshes) {
+        updateSceneGraph(pScene->mRootNode, meshMap);
+    }
+
     ASSIMP_LOG_DEBUG("FindDegeneratesProcess finished");
 }
 
-static void removeMesh(aiScene* pScene, unsigned const index) {
+static void updateSceneGraph(aiNode* pNode, const std::unordered_map<unsigned int, unsigned int>& meshMap) {
-    //we start at index and copy the pointers one position forward
+    unsigned int targetIndex = 0;
-    //save the mesh pointer to delete it later
-    auto delete_me = pScene->mMeshes[index];
-    for (unsigned i = index; i < pScene->mNumMeshes - 1; ++i) {
-        pScene->mMeshes[i] = pScene->mMeshes[i+1];
-    }
-    pScene->mMeshes[pScene->mNumMeshes - 1] = nullptr;
-    --(pScene->mNumMeshes);
-    delete delete_me;
-
-    //removing a mesh also requires updating all references to it in the scene graph
-    updateSceneGraph(pScene->mRootNode, index);
-}
-
-static void updateSceneGraph(aiNode* pNode, unsigned const index) {
     for (unsigned i = 0; i < pNode->mNumMeshes; ++i) {
-        if (pNode->mMeshes[i] > index) {
+        const unsigned int sourceMeshIndex = pNode->mMeshes[i];
-            --(pNode->mMeshes[i]);
+        auto it = meshMap.find(sourceMeshIndex);
-            continue;
+        if (it != meshMap.end()) {
-        }
+            pNode->mMeshes[targetIndex] = it->second;
-        if (pNode->mMeshes[i] == index) {
+            ++targetIndex;
-            for (unsigned j = i; j < pNode->mNumMeshes -1; ++j) {
-                pNode->mMeshes[j] = pNode->mMeshes[j+1];
-            }
-            --(pNode->mNumMeshes);
-            --i;
-            continue;
         }
     }
+    pNode->mNumMeshes = targetIndex;
     //recurse to all children
     for (unsigned i = 0; i < pNode->mNumChildren; ++i) {
-        updateSceneGraph(pNode->mChildren[i], index);
+        updateSceneGraph(pNode->mChildren[i], meshMap);
     }
 }
 

test/unit/utFindDegenerates.cpp

@@ -40,8 +40,11 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
 #include "UnitTestPCH.h"
 
+#include "../../include/assimp/scene.h"
 #include "PostProcessing/FindDegenerates.h"
 
+#include <memory>
+
 using namespace std;
 using namespace Assimp;
 
@@ -147,3 +150,59 @@ TEST_F(FindDegeneratesProcessTest, testDegeneratesRemovalWithAreaCheck) {
 
     EXPECT_EQ(mMesh->mNumUVComponents[1] - 100, mMesh->mNumFaces);
 }
+
+namespace
+{
+    std::unique_ptr<aiMesh> getDegenerateMesh()
+    {
+        std::unique_ptr<aiMesh> mesh(new aiMesh);
+        mesh->mNumVertices = 2;
+        mesh->mVertices = new aiVector3D[2];
+        mesh->mVertices[0] = aiVector3D{ 0.0f, 0.0f, 0.0f };
+        mesh->mVertices[1] = aiVector3D{ 1.0f, 0.0f, 0.0f };
+        mesh->mNumFaces = 1;
+        mesh->mFaces = new aiFace[1];
+        mesh->mFaces[0].mNumIndices = 3;
+        mesh->mFaces[0].mIndices = new unsigned int[3];
+        mesh->mFaces[0].mIndices[0] = 0;
+        mesh->mFaces[0].mIndices[1] = 1;
+        mesh->mFaces[0].mIndices[2] = 0;
+        return mesh;
+    }
+}
+
+TEST_F(FindDegeneratesProcessTest, meshRemoval) {
+    mProcess->EnableAreaCheck(true);
+    mProcess->EnableInstantRemoval(true);
+    mProcess->ExecuteOnMesh(mMesh);
+
+    std::unique_ptr<aiScene> scene(new aiScene);
+    scene->mNumMeshes = 5;
+    scene->mMeshes = new aiMesh*[5];
+
+    /// Use the mesh which doesn't get completely stripped of faces from the main test.
+    aiMesh* meshWhichSurvives = mMesh;
+    mMesh = nullptr;
+
+    scene->mMeshes[0] = getDegenerateMesh().release();
+    scene->mMeshes[1] = getDegenerateMesh().release();
+    scene->mMeshes[2] = meshWhichSurvives;
+    scene->mMeshes[3] = getDegenerateMesh().release();
+    scene->mMeshes[4] = getDegenerateMesh().release();
+
+    scene->mRootNode = new aiNode;
+    scene->mRootNode->mNumMeshes = 5;
+    scene->mRootNode->mMeshes = new unsigned int[5];
+    scene->mRootNode->mMeshes[0] = 0;
+    scene->mRootNode->mMeshes[1] = 1;
+    scene->mRootNode->mMeshes[2] = 2;
+    scene->mRootNode->mMeshes[3] = 3;
+    scene->mRootNode->mMeshes[4] = 4;
+
+    mProcess->Execute(scene.get());    
+
+    EXPECT_EQ(scene->mNumMeshes, 1);
+    EXPECT_EQ(scene->mMeshes[0], meshWhichSurvives);
+    EXPECT_EQ(scene->mRootNode->mNumMeshes, 1);
+    EXPECT_EQ(scene->mRootNode->mMeshes[0], 0);    
+}