Improvements in Assimp importer and adding test to MaterialHighlighter (

#42) * add test to MaterialHighlighter * Simplify MeshProcessor and improves BoneProcessor * suggestions from cppCheck
fernandotonon · Aug 30, 2023 · 19deac6 · 19deac6
1 parent 2e7de27
commit 19deac6
Show file tree

Hide file tree

Showing 12 changed files with 150 additions and 131 deletions.
diff --git a/src/Assimp/AnimationProcessor.cpp b/src/Assimp/AnimationProcessor.cpp
@@ -19,8 +19,9 @@ void AnimationProcessor::processAnimation(aiAnimation* animation, const aiScene*
     }
 }
 
-
 void AnimationProcessor::processAnimationChannel(aiNodeAnim* nodeAnim, Ogre::Animation* animation, const aiScene* scene, unsigned int channelIndex) {
+    if(!skeleton->hasBone(nodeAnim->mNodeName.C_Str())) return;
+
     // Create the animation track
     Ogre::Bone* bone = skeleton->getBone(nodeAnim->mNodeName.C_Str());
     Ogre::NodeAnimationTrack* track = animation->createNodeTrack(bone->getHandle(), bone);

diff --git a/src/Assimp/BoneProcessor.cpp b/src/Assimp/BoneProcessor.cpp
@@ -1,52 +1,54 @@
 #include "BoneProcessor.h"
 
-void BoneProcessor::processBoneHierarchy(aiBone* bone) {
-    // Process the bone node
-    processBoneNode(bone);
-
-    // Recursively process children bones
-    for(auto i = 0u; i < bone->mNode->mNumChildren; i++) {
-        aiNode* childNode = bone->mNode->mChildren[i];
-        if(aiBonesMap.find(childNode->mName.C_Str()) != aiBonesMap.end()) {
-            processBoneHierarchy(aiBonesMap[childNode->mName.C_Str()]);
-        }
-    }
-}
-
 void BoneProcessor::processBones(Ogre::SkeletonPtr skeleton, const aiScene *scene) {
     this->skeleton = skeleton;
+
+    // First, create a map of bone names to aiBones for easier look-up
     for(auto i = 0u; i < scene->mNumMeshes; i++) {
         aiMesh* mesh = scene->mMeshes[i];
         for(auto j = 0u; j < mesh->mNumBones; j++) {
             aiBone* bone = mesh->mBones[j];
-            // Check if the bone already exists
-            if(!skeleton->hasBone(bone->mName.C_Str())) {
-                aiBonesMap[bone->mName.C_Str()] = bone;
-                createBone(bone->mName.C_Str());
-            }
+            aiBonesMap[bone->mName.C_Str()] = bone;
         }
     }
 
-    // Some models have bone animations but not all the bones are related to the meshes
-    for(auto i = 0u; i < scene->mNumAnimations; i++) {
-        aiAnimation* anim = scene->mAnimations[i];
-        for(auto j = 0u; j < anim->mNumChannels; j++) {
-            aiNodeAnim* nodeAnim = anim->mChannels[j];
-            createBone(nodeAnim->mNodeName.C_Str());
-        }
-    }
-
-    // Process the root bones first
+    // Create the root bones
     for(auto i = 0u; i < scene->mNumMeshes; i++) {
         aiMesh* mesh = scene->mMeshes[i];
         for(auto j = 0u; j < mesh->mNumBones; j++) {
             aiBone* bone = mesh->mBones[j];
             if(bone->mNode && bone->mNode->mParent && !skeleton->hasBone(bone->mNode->mParent->mName.C_Str())) {
                 createBone(bone->mNode->mParent->mName.C_Str());
-                processBoneHierarchy(bone);
+                Ogre::Matrix4 rootBoneGlobalTransformation = convertToOgreMatrix4(bone->mNode->mParent->mTransformation).inverse();
+                applyTransformation(bone->mNode->mParent->mName.C_Str(), rootBoneGlobalTransformation);
             }
         }
     }
+
+    // Start from the root node and process the hierarchy
+    processBoneHierarchy(scene->mRootNode);
+}
+
+void BoneProcessor::processBoneHierarchy(aiNode* node) {
+    // Check if this node corresponds to a bone
+    if(aiBonesMap.find(node->mName.C_Str()) != aiBonesMap.end()) {
+        aiBone* bone = aiBonesMap[node->mName.C_Str()];
+        createBone(bone->mName.C_Str());
+        processBoneNode(bone);
+
+        // Recursively process children bones
+        for(auto i = 0u; i < node->mNumChildren; i++) {
+            aiNode* childNode = node->mChildren[i];
+            processBoneHierarchy(childNode);
+        }
+    }
+    else {
+        // If this node isn't a bone, still process its children
+        for(auto i = 0u; i < node->mNumChildren; i++) {
+            aiNode* childNode = node->mChildren[i];
+            processBoneHierarchy(childNode);
+        }
+    }
 }
 
 void BoneProcessor::createBone(const std::string& boneName) {
@@ -57,47 +59,57 @@ void BoneProcessor::createBone(const std::string& boneName) {
     }
 }
 
-void BoneProcessor::processBoneNode(aiBone* bone) {
-    // Convert the aiBone's offset matrix to an Ogre::Matrix4
-    Ogre::Matrix4 offsetMatrix(
-        bone->mOffsetMatrix.a1, bone->mOffsetMatrix.a2, bone->mOffsetMatrix.a3, bone->mOffsetMatrix.a4,
-        bone->mOffsetMatrix.b1, bone->mOffsetMatrix.b2, bone->mOffsetMatrix.b3, bone->mOffsetMatrix.b4,
-        bone->mOffsetMatrix.c1, bone->mOffsetMatrix.c2, bone->mOffsetMatrix.c3, bone->mOffsetMatrix.c4,
-        bone->mOffsetMatrix.d1, bone->mOffsetMatrix.d2, bone->mOffsetMatrix.d3, bone->mOffsetMatrix.d4
+Ogre::Matrix4 BoneProcessor::convertToOgreMatrix4(const aiMatrix4x4& aiMat) {
+    return Ogre::Matrix4(
+        aiMat.a1, aiMat.a2, aiMat.a3, aiMat.a4,
+        aiMat.b1, aiMat.b2, aiMat.b3, aiMat.b4,
+        aiMat.c1, aiMat.c2, aiMat.c3, aiMat.c4,
+        aiMat.d1, aiMat.d2, aiMat.d3, aiMat.d4
         );
+}
 
-    // Invert the offset matrix to get the global transformation of the bone
-    Ogre::Matrix4 globalTransform = offsetMatrix.inverse();
-
-    // If the bone has a parent, multiply the global transformation of the bone with the inverse of the global transformation of the parent to get the local transformation
-    if(bone->mNode->mParent && bone->mNode->mParent->mName.length) {
-        Ogre::Bone* parentBone = skeleton->getBone(bone->mNode->mParent->mName.C_Str());
-        if(parentBone) {
-            Ogre::Matrix4 parentGlobalTransform = parentBone->_getFullTransform().inverse();
-            globalTransform = parentGlobalTransform * globalTransform;
-        }
-    }
-
+void BoneProcessor::applyTransformation(const std::string &boneName, const Ogre::Matrix4 &transform)
+{
     // Convert the Ogre::Matrix4 to an Ogre::Affine3
-    Ogre::Affine3 affine(globalTransform);
+    Ogre::Affine3 affine(transform);
 
     // Decompose the offset matrix into position, scale, and orientation
     Ogre::Vector3 position, scale;
     Ogre::Quaternion orientation;
     affine.decomposition(position, scale, orientation);
 
     // Retrieve the bone (it should already exist)
-    Ogre::Bone* ogreBone = skeleton->getBone(bone->mName.C_Str());
+    Ogre::Bone* ogreBone = skeleton->getBone(boneName);
 
     // Set the bone's position, orientation, and scale
     ogreBone->setPosition(position);
     ogreBone->setOrientation(orientation);
     ogreBone->setScale(scale);
+}
+
+void BoneProcessor::processBoneNode(aiBone* bone) {
+    // Convert the aiBone's offset matrix to an Ogre::Matrix4
+    Ogre::Matrix4 offsetMatrix = convertToOgreMatrix4(bone->mOffsetMatrix);
+
+    // Invert the offset matrix to get the global transformation of the bone
+    Ogre::Matrix4 globalTransform = offsetMatrix.inverse();
+
+    // If the bone has a parent, multiply the global transformation of the bone with the inverse of the global transformation of the parent to get the local transformation
+    if(bone->mNode->mParent && bone->mNode->mParent->mName.length) {
+        if(skeleton->hasBone(bone->mNode->mParent->mName.C_Str())){
+            Ogre::Bone* parentBone = skeleton->getBone(bone->mNode->mParent->mName.C_Str());
+            Ogre::Matrix4 parentGlobalTransform = parentBone->_getFullTransform().inverse();
+            globalTransform = parentGlobalTransform * globalTransform;
+        }
+    }
+
+    applyTransformation(bone->mName.C_Str(), globalTransform);
 
     // Add the bone to the parent bone, if it exists
     if(bone->mNode->mParent && bone->mNode->mParent->mName.length) {
-        Ogre::Bone* parentBone = skeleton->getBone(bone->mNode->mParent->mName.C_Str());
-        if(parentBone) {
+        if(skeleton->hasBone(bone->mNode->mParent->mName.C_Str())){
+            Ogre::Bone* parentBone = skeleton->getBone(bone->mNode->mParent->mName.C_Str());
+            Ogre::Bone* ogreBone = skeleton->getBone(bone->mName.C_Str());
             // Check if ogreBone is already a child of parentBone
             if (!std::any_of(parentBone->getChildren().begin(), parentBone->getChildren().end(),
                              [&ogreBone](const auto& childNode) {

diff --git a/src/Assimp/BoneProcessor.h b/src/Assimp/BoneProcessor.h
@@ -9,8 +9,10 @@ class BoneProcessor {
 
     private:
         void createBone(const std::string& boneName);
-        void processBoneHierarchy(aiBone* bone);
+        void processBoneHierarchy(aiNode* node);
         void processBoneNode(aiBone *bone);
+        Ogre::Matrix4 convertToOgreMatrix4(const aiMatrix4x4& aiMat);
+        void applyTransformation(const std::string& boneName, const Ogre::Matrix4 &transform);
 
         Ogre::SkeletonPtr skeleton;    
         std::map<std::string, aiBone*> aiBonesMap;

diff --git a/src/Assimp/BoneProcessor_test.cpp b/src/Assimp/BoneProcessor_test.cpp
@@ -34,6 +34,19 @@ TEST_F(BoneProcessorTest, CreateBones) {
     mockScene.mMeshes[0]->mBones[1]->mName = aiString("Bone2");
     mockScene.mMeshes[0]->mBones[0]->mOffsetMatrix = aiMatrix4x4();
     mockScene.mMeshes[0]->mBones[1]->mOffsetMatrix = aiMatrix4x4();
+    mockScene.mRootNode = new aiNode();
+    mockScene.mRootNode->mName = aiString("Root");
+    mockScene.mRootNode->mTransformation = aiMatrix4x4();
+    mockScene.mRootNode->mNumChildren = 2;
+    mockScene.mRootNode->mChildren = new aiNode*[2];
+    mockScene.mRootNode->mChildren[0] = new aiNode();
+    mockScene.mRootNode->mChildren[1] = new aiNode();
+    mockScene.mRootNode->mChildren[0]->mName = aiString("Bone1");
+    mockScene.mRootNode->mChildren[1]->mName = aiString("Bone2");
+    mockScene.mRootNode->mChildren[0]->mTransformation = aiMatrix4x4();
+    mockScene.mRootNode->mChildren[1]->mTransformation = aiMatrix4x4();
+    mockScene.mMeshes[0]->mBones[0]->mNode = mockScene.mRootNode->mChildren[0];
+    mockScene.mMeshes[0]->mBones[1]->mNode = mockScene.mRootNode->mChildren[1];
 
     processor.processBones(mockSkeleton, &mockScene);
 
@@ -88,6 +101,9 @@ TEST_F(BoneProcessorTest, BoneHierarchy) {
 
     mockScene.mMeshes[0]->mBones[0] = childBone;
 
+    mockScene.mRootNode = parentNode;
+    mockScene.mRootNode->mTransformation = aiMatrix4x4();
+
     processor.processBones(mockSkeleton, &mockScene);
 
     EXPECT_EQ(mockSkeleton->getBone("ChildBone")->getParent(), mockSkeleton->getBone("RootBone"));
@@ -100,6 +116,8 @@ TEST_F(BoneProcessorTest, BoneTransformation) {
     aiBone* mockBone = new aiBone();
     mockBone->mName = aiString("TestBone");
     aiNode* mockNode = new aiNode();
+
+    mockNode->mName = aiString("TestBone");
     mockBone->mNode = mockNode;
     mockNode->mParent = rootNode;
 
@@ -119,6 +137,12 @@ TEST_F(BoneProcessorTest, BoneTransformation) {
     mockScene.mMeshes[0]->mBones = new aiBone*[1];
     mockScene.mMeshes[0]->mBones[0] = mockBone;
 
+    mockScene.mRootNode = rootNode;
+    mockScene.mRootNode->mTransformation = aiMatrix4x4();
+    mockScene.mRootNode->mNumChildren = 1;
+    mockScene.mRootNode->mChildren = new aiNode*[1];
+    mockScene.mRootNode->mChildren[0] = mockNode;
+
     processor.processBones(mockSkeleton, &mockScene);
 
     Ogre::Bone* testBone = mockSkeleton->getBone("TestBone");
@@ -131,22 +155,3 @@ TEST_F(BoneProcessorTest, BoneTransformation) {
     EXPECT_EQ(testBone->getOrientation(), expectedOrientation);
     EXPECT_EQ(testBone->getScale(), expectedScale);
 }
-
-TEST_F(BoneProcessorTest, AnimationBones) {
-    aiAnimation* mockAnimation = new aiAnimation();
-    aiNodeAnim* mockNodeAnim = new aiNodeAnim();
-    mockNodeAnim->mNodeName = aiString("AnimBone");
-
-    mockAnimation->mNumChannels = 1;
-    mockAnimation->mChannels = new aiNodeAnim*[1];
-    mockAnimation->mChannels[0] = mockNodeAnim;
-
-    mockScene.mNumAnimations = 1;
-    mockScene.mAnimations = new aiAnimation*[1];
-    mockScene.mAnimations[0] = mockAnimation;
-
-    processor.processBones(mockSkeleton, &mockScene);
-
-    // Check if animation bone "AnimBone" is correctly created
-    EXPECT_TRUE(mockSkeleton->hasBone("AnimBone"));
-}
diff --git a/src/Assimp/Importer.cpp b/src/Assimp/Importer.cpp
@@ -45,7 +45,6 @@ Ogre::MeshPtr AssimpToOgreImporter::loadModel(const std::string& path) {
                                                  aiProcess_ValidateDataStructure |
                                                  aiProcess_OptimizeGraph |
                                                  aiProcess_LimitBoneWeights |
-                                                 aiProcess_FindInvalidData |
                                                  aiProcess_SortByPType |
                                                  aiProcess_ImproveCacheLocality |
                                                  aiProcess_FixInfacingNormals |

diff --git a/src/Assimp/MeshProcessor.cpp b/src/Assimp/MeshProcessor.cpp
@@ -20,9 +20,6 @@ SubMeshData* MeshProcessor::processMesh(aiMesh* mesh, const aiScene* scene) {
 
     // Initialize blend indices and blend weights
     for(auto i = 0u; i < mesh->mNumVertices; i++) {
-        subMeshData->blendIndices.push_back(Ogre::Vector4(0.0f, 0.0f, 0.0f, 0.0f));
-        subMeshData->blendWeights.push_back(Ogre::Vector4(0.0f, 0.0f, 0.0f, 0.0f));
-
         // Process vertices
         subMeshData->vertices.push_back(Ogre::Vector3(mesh->mVertices[i].x, mesh->mVertices[i].y, mesh->mVertices[i].z));
 
@@ -44,34 +41,6 @@ SubMeshData* MeshProcessor::processMesh(aiMesh* mesh, const aiScene* scene) {
         Ogre::Bone* ogreBone = skeleton->getBone(bone->mName.C_Str());
         for(auto j = 0u; j < bone->mNumWeights; j++) {
             aiVertexWeight weight = bone->mWeights[j];
-            int index = -1;
-            int freeIndex = -1;
-            for(int k = 0; k < 4; k++) {
-                if(subMeshData->blendWeights[weight.mVertexId][k] == 0.0f) {
-                    freeIndex = k;
-                    break;
-                }
-            }
-
-            if(freeIndex != -1) {
-                index = freeIndex;
-            } else {
-                // Find the smallest weight, replace it if the current weight is larger
-                int smallest = 0;
-                for(int k = 1; k < 4; k++) {
-                    if(subMeshData->blendWeights[weight.mVertexId][k] < subMeshData->blendWeights[weight.mVertexId][smallest]) {
-                        smallest = k;
-                    }
-                }
-                if(subMeshData->blendWeights[weight.mVertexId][smallest] < weight.mWeight) {
-                    index = smallest;
-                }
-            }
-
-            if(index != -1) {
-                subMeshData->blendIndices[weight.mVertexId][index] = i;
-                subMeshData->blendWeights[weight.mVertexId][index] = weight.mWeight;
-            }
 
             Ogre::VertexBoneAssignment vba;
             vba.vertexIndex = weight.mVertexId;
@@ -82,19 +51,6 @@ SubMeshData* MeshProcessor::processMesh(aiMesh* mesh, const aiScene* scene) {
         }
     }
 
-    // Normalize the blend weights so they sum to 1
-    for(auto i = 0u; i < mesh->mNumVertices; i++) {
-        float sum = 0.0f;
-        for(int j = 0; j < 4; j++) {
-            sum += subMeshData->blendWeights[i][j];
-        }
-        if(sum > 0.0f) {
-            for(int j = 0; j < 4; j++) {
-                subMeshData->blendWeights[i][j] /= sum;
-            }
-        }
-    }
-
     // Process indices
     for(auto i = 0u; i < mesh->mNumFaces; i++) {
         aiFace face = mesh->mFaces[i];

diff --git a/src/Assimp/MeshProcessor.h b/src/Assimp/MeshProcessor.h
@@ -12,8 +12,6 @@ struct SubMeshData {
     std::vector<Ogre::Vector3> bitangents;
     std::vector<Ogre::ColourValue> colors;
     std::vector<unsigned long> indices;
-    std::vector<Ogre::Vector4> blendIndices;
-    std::vector<Ogre::Vector4> blendWeights;
     std::vector<Ogre::VertexBoneAssignment> boneAssignments;
     unsigned int materialIndex;
 };

diff --git a/src/Assimp/MeshProcessor_test.cpp b/src/Assimp/MeshProcessor_test.cpp
@@ -97,11 +97,5 @@ TEST_F(MeshProcessorTest, MeshDataProcessingTest) {
     EXPECT_EQ(resultData->vertices[0], Ogre::Vector3(0, 0, 0));
     EXPECT_EQ(resultData->vertices[1], Ogre::Vector3(1, 1, 1));
     EXPECT_EQ(resultData->vertices[2], Ogre::Vector3(2, 2, 2));
-
-    // Verify blend indices and blend weights for each vertex
-    for (unsigned int i = 0; i < mockMesh->mNumVertices; i++) {
-        EXPECT_EQ(resultData->blendIndices[i][0], 0);  // Assuming the bone only one bone
-        EXPECT_FLOAT_EQ(resultData->blendWeights[i][0], 1.0f);  // the values get normalized (0.5+1+1.5)/3=1
-    }
 }
 // TODO: test the other functions
diff --git a/src/MaterialHighlighter.cpp b/src/MaterialHighlighter.cpp
@@ -42,7 +42,7 @@ MaterialHighlighter::~MaterialHighlighter()
 
 void MaterialHighlighter::highlightBlock(const QString &text)
 {
-    mParent->blockSignals(true);
+    if(mParent) mParent->blockSignals(true);
 
     QColor palette = QApplication::palette().color(QPalette::Text);
     bool dark = qGray(palette.red(),palette.green(),palette.blue())>100;
@@ -73,7 +73,7 @@ void MaterialHighlighter::highlightBlock(const QString &text)
     pattern = "\\b\\d+|(\\d+\\.)|(\\d+\\.\\d+)\\b";
     applyHighlight(format,pattern, text);
 
-    mParent->blockSignals(false);
+    if(mParent) mParent->blockSignals(false);
 }
 
 void MaterialHighlighter::applyHighlight(const QTextCharFormat &format, const QString &pattern, const QString &text)

diff --git a/src/MaterialHighlighter.h b/src/MaterialHighlighter.h
@@ -36,11 +36,14 @@ class MaterialHighlighter : public QSyntaxHighlighter
 public:
     explicit MaterialHighlighter(QObject *parent = 0);
     ~MaterialHighlighter();
-
+    
     void highlightBlock(const QString &text);
+
+protected:
+    virtual void applyHighlight(const QTextCharFormat &format, const QString &pattern, const QString &text);
+
 private:
     QObject *mParent;
-    void applyHighlight(const QTextCharFormat &format, const QString &pattern, const QString &text);
 };
 
 #endif // MATERIALHIGHLIGHTER_H