From 1dabb1a0946168332f601bb46bfdfa7743c64d18 Mon Sep 17 00:00:00 2001 From: RichardTea <31507749+RichardTea@users.noreply.github.com> Date: Fri, 1 May 2020 14:59:09 +0100 Subject: [PATCH] Collada: Fix crash with AI_CONFIG_IMPORT_COLLADA_USE_COLLADA_NAMES Add unit test for this --- code/Collada/ColladaLoader.cpp | 13 ++++- test/unit/utColladaImportExport.cpp | 76 +++++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/code/Collada/ColladaLoader.cpp b/code/Collada/ColladaLoader.cpp index c76954fdf..8ca0b130e 100644 --- a/code/Collada/ColladaLoader.cpp +++ b/code/Collada/ColladaLoader.cpp @@ -258,6 +258,15 @@ void ColladaLoader::InternReadFile(const std::string &pFile, aiScene *pScene, IO } } +// Add an item of metadata to a node +// Assumes the key is not already in the list +template +inline void AddNodeMetaData(aiNode *node, const std::string &key, const T &value) { + if (nullptr == node->mMetaData) + node->mMetaData = new aiMetadata(); + node->mMetaData->Add(key, value); +} + // ------------------------------------------------------------------------------------------------ // Recursively constructs a scene node for the given parser node and returns it. aiNode *ColladaLoader::BuildHierarchy(const ColladaParser &pParser, const Collada::Node *pNode) { @@ -269,9 +278,9 @@ aiNode *ColladaLoader::BuildHierarchy(const ColladaParser &pParser, const Collad // if we're not using the unique IDs, hold onto them for reference and export if (useColladaName) { if (!pNode->mID.empty()) - node->mMetaData->Add(AI_METADATA_COLLADA_ID, aiString(pNode->mID)); + AddNodeMetaData(node, AI_METADATA_COLLADA_ID, aiString(pNode->mID)); if (!pNode->mSID.empty()) - node->mMetaData->Add(AI_METADATA_COLLADA_SID, aiString(pNode->mSID)); + AddNodeMetaData(node, AI_METADATA_COLLADA_SID, aiString(pNode->mSID)); } // calculate the transformation matrix for it diff --git a/test/unit/utColladaImportExport.cpp b/test/unit/utColladaImportExport.cpp index 5b0c9f880..876f60c54 100644 --- a/test/unit/utColladaImportExport.cpp +++ b/test/unit/utColladaImportExport.cpp @@ -41,6 +41,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "AbstractImportExportBase.h" #include "UnitTestPCH.h" +#include #include #include #include @@ -85,6 +86,18 @@ public: typedef std::pair IdNameString; typedef std::map IdNameMap; + template + static inline IdNameString GetColladaIdName(const T *item, size_t index) { + std::ostringstream stream; + stream << typeid(T).name() << "@" << index; + if (item->mMetaData) { + aiString aiStr; + if (item->mMetaData->Get(AI_METADATA_COLLADA_ID, aiStr)) + return std::make_pair(std::string(aiStr.C_Str()), stream.str()); + } + return std::make_pair(std::string(), stream.str()); + } + template static inline IdNameString GetItemIdName(const T *item, size_t index) { std::ostringstream stream; @@ -121,11 +134,23 @@ public: static inline void CheckUniqueIds(IdNameMap &itemIdMap, const aiNode *parent, size_t index) { IdNameString namePair = GetItemIdName(parent, index); ReportDuplicate(itemIdMap, namePair, typeid(aiNode).name()); + for (size_t idx = 0; idx < parent->mNumChildren; ++idx) { CheckUniqueIds(itemIdMap, parent->mChildren[idx], idx); } } + static inline void CheckNodeIdNames(IdNameMap &nodeIdMap, IdNameMap &nodeNameMap, const aiNode *parent, size_t index) { + IdNameString namePair = GetItemIdName(parent, index); + const auto result = nodeNameMap.insert(namePair); + IdNameString idPair = GetColladaIdName(parent, index); + ReportDuplicate(nodeIdMap, idPair, typeid(aiNode).name()); + + for (size_t idx = 0; idx < parent->mNumChildren; ++idx) { + CheckNodeIdNames(nodeIdMap, nodeNameMap, parent->mChildren[idx], idx); + } + } + static inline void SetAllNodeNames(const aiString &newName, aiNode *node) { node->mName = newName; for (size_t idx = 0; idx < node->mNumChildren; ++idx) { @@ -133,12 +158,12 @@ public: } } - void ImportAndCheckIds(const char *file, size_t meshCount) { - // Import the Collada using the 'default' where aiMesh names are the Collada ids + void ImportAndCheckIds(const char *file, const aiScene *origScene) { + // Import the Collada using the 'default' where aiNode and aiMesh names are the Collada ids Assimp::Importer importer; const aiScene *scene = importer.ReadFile(file, aiProcess_ValidateDataStructure); ASSERT_TRUE(scene != nullptr) << "Fatal: could not re-import " << file; - EXPECT_EQ(meshCount, scene->mNumMeshes) << "in " << file; + EXPECT_EQ(origScene->mNumMeshes, scene->mNumMeshes) << "in " << file; // Check the ids are unique IdNameMap itemIdMap; @@ -146,11 +171,39 @@ public: CheckUniqueIds(itemIdMap, scene->mRootNode, 0); // Check the lists CheckUniqueIds(itemIdMap, scene->mNumMeshes, scene->mMeshes); - CheckUniqueIds(itemIdMap, scene->mNumAnimations, scene->mAnimations); - CheckUniqueIds(itemIdMap, scene->mNumMaterials, scene->mMaterials); - CheckUniqueIds(itemIdMap, scene->mNumTextures, scene->mTextures); - CheckUniqueIds(itemIdMap, scene->mNumLights, scene->mLights); - CheckUniqueIds(itemIdMap, scene->mNumCameras, scene->mCameras); + // The remaining will come in using the name, which may not be unique + // Check we have the right number + EXPECT_EQ(origScene->mNumAnimations, scene->mNumAnimations); + EXPECT_EQ(origScene->mNumMaterials, scene->mNumMaterials); + EXPECT_EQ(origScene->mNumTextures, scene->mNumTextures); + EXPECT_EQ(origScene->mNumLights, scene->mNumLights); + EXPECT_EQ(origScene->mNumCameras, scene->mNumCameras); + } + + void ImportAsNames(const char *file, const aiScene *origScene) { + // Import the Collada but using the user-visible names for aiNode and aiMesh + // Note that this mode may not support bones or animations + Assimp::Importer importer; + importer.SetPropertyInteger(AI_CONFIG_IMPORT_COLLADA_USE_COLLADA_NAMES, 1); + + const aiScene *scene = importer.ReadFile(file, aiProcess_ValidateDataStructure); + ASSERT_TRUE(scene != nullptr) << "Fatal: could not re-import " << file; + EXPECT_EQ(origScene->mNumMeshes, scene->mNumMeshes) << "in " << file; + + // Check the node ids are unique but the node names are not + IdNameMap nodeIdMap; + IdNameMap nodeNameMap; + // Recurse the Nodes + CheckNodeIdNames(nodeIdMap, nodeNameMap, scene->mRootNode, 0); + + // nodeNameMap should have fewer than nodeIdMap + EXPECT_LT(nodeNameMap.size(), nodeIdMap.size()) << "Some nodes should have the same names"; + // Check the counts haven't changed + EXPECT_EQ(origScene->mNumAnimations, scene->mNumAnimations); + EXPECT_EQ(origScene->mNumMaterials, scene->mNumMaterials); + EXPECT_EQ(origScene->mNumTextures, scene->mNumTextures); + EXPECT_EQ(origScene->mNumLights, scene->mNumLights); + EXPECT_EQ(origScene->mNumCameras, scene->mNumCameras); } }; @@ -192,7 +245,7 @@ TEST_F(utColladaImportExport, exporterUniqueIdsTest) { ASSERT_EQ(AI_SUCCESS, exporter.Export(scene, "collada", outFileEmpty)) << "Fatal: Could not export un-named meshes file"; - ImportAndCheckIds(outFileEmpty, 3); + ImportAndCheckIds(outFileEmpty, scene); // Force everything to have the same non-empty name aiString testName("test_name"); @@ -213,9 +266,12 @@ TEST_F(utColladaImportExport, exporterUniqueIdsTest) { scene->mCameras[idx]->mName = testName; } + SetAllNodeNames(testName, scene->mRootNode); + ASSERT_EQ(AI_SUCCESS, exporter.Export(scene, "collada", outFileNamed)) << "Fatal: Could not export named meshes file"; - ImportAndCheckIds(outFileNamed, 3); + ImportAndCheckIds(outFileNamed, scene); + ImportAsNames(outFileNamed, scene); } class utColladaZaeImportExport : public AbstractImportExportBase {