From 8e73984a1105f6b713942fd8125e5bf9cd3e2f6e Mon Sep 17 00:00:00 2001 From: RichardTea <31507749+RichardTea@users.noreply.github.com> Date: Mon, 4 May 2020 17:47:09 +0100 Subject: [PATCH 1/3] Collada Root Nodes aren't allowed to have meshes Create a null parent node instead --- code/AssetLib/Collada/ColladaExporter.cpp | 12 +++-- code/AssetLib/Collada/ColladaParser.cpp | 2 +- test/unit/utColladaImportExport.cpp | 55 +++++++++++++++++++++++ 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/code/AssetLib/Collada/ColladaExporter.cpp b/code/AssetLib/Collada/ColladaExporter.cpp index 5e4cdda2d..e15e81985 100644 --- a/code/AssetLib/Collada/ColladaExporter.cpp +++ b/code/AssetLib/Collada/ColladaExporter.cpp @@ -233,12 +233,13 @@ void ColladaExporter::WriteHeader() { add_root_node = true; } - if (mScene->mRootNode->mNumChildren == 0) { + // Assimp root nodes can have meshes, Collada Scenes cannot + if (mScene->mRootNode->mNumChildren == 0 || mScene->mRootNode->mMeshes != 0) { add_root_node = true; } if (add_root_node) { - aiScene *scene; + aiScene *scene = nullptr; SceneCombiner::CopyScene(&scene, mScene); aiNode *root = new aiNode("Scene"); @@ -1493,12 +1494,9 @@ void ColladaExporter::WriteNode(const aiNode *pNode) { const std::string node_name = GetNodeName(pNode); mOutput << startstr << "" << endstr; diff --git a/code/AssetLib/Collada/ColladaParser.cpp b/code/AssetLib/Collada/ColladaParser.cpp index d83980929..04e6eb7d4 100644 --- a/code/AssetLib/Collada/ColladaParser.cpp +++ b/code/AssetLib/Collada/ColladaParser.cpp @@ -2479,7 +2479,7 @@ void ColladaParser::ReadSceneLibrary() { // read name if given. int indexName = TestAttribute("name"); - const char *attrName = "unnamed"; + const char *attrName = "Scene"; if (indexName > -1) attrName = mReader->getAttributeValue(indexName); diff --git a/test/unit/utColladaImportExport.cpp b/test/unit/utColladaImportExport.cpp index 876f60c54..3d6e0db23 100644 --- a/test/unit/utColladaImportExport.cpp +++ b/test/unit/utColladaImportExport.cpp @@ -42,6 +42,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "UnitTestPCH.h" #include +#include #include #include #include @@ -211,6 +212,60 @@ TEST_F(utColladaImportExport, importDaeFromFileTest) { EXPECT_TRUE(importerTest()); } +unsigned int GetMeshUseCount(const aiNode *rootNode) { + unsigned int result = rootNode->mNumMeshes; + for (unsigned int i = 0; i < rootNode->mNumChildren; ++i) { + result += GetMeshUseCount(rootNode->mChildren[i]); + } + return result; +} + +TEST_F(utColladaImportExport, exportRootNodeMeshTest) { + Assimp::Importer importer; + Assimp::Exporter exporter; + const char *outFile = "exportRootNodeMeshTest_out.dae"; + + const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/Collada/duck.dae", aiProcess_ValidateDataStructure); + ASSERT_TRUE(scene != nullptr) << "Fatal: could not import duck.dae!"; + + ASSERT_EQ(0u, scene->mRootNode->mNumMeshes) << "Collada import should not give the root node a mesh"; + + { + // Clone the scene and give the root node a mesh and a transform + aiScene *rootMeshScene = nullptr; + SceneCombiner::CopyScene(&rootMeshScene, scene); + ASSERT_TRUE(rootMeshScene != nullptr) << "Fatal: could not copy scene!"; + // Do this by moving the meshes from the first child that has some + aiNode *rootNode = rootMeshScene->mRootNode; + ASSERT_TRUE(rootNode->mNumChildren > 0) << "Fatal: root has no children"; + aiNode *meshNode = rootNode->mChildren[0]; + ASSERT_EQ(1u, meshNode->mNumMeshes) << "Fatal: First child node has no duck mesh"; + + // Move the meshes to the parent + rootNode->mNumMeshes = meshNode->mNumMeshes; + rootNode->mMeshes = new unsigned int[rootNode->mNumMeshes]; + for (unsigned int i = 0; i < rootNode->mNumMeshes; ++i) { + rootNode->mMeshes[i] = meshNode->mMeshes[i]; + } + + meshNode->mNumMeshes = 0; + delete[] meshNode->mMeshes; + + ASSERT_EQ(AI_SUCCESS, exporter.Export(rootMeshScene, "collada", outFile)) << "Fatal: Could not export file"; + } + + // Reimport and look for meshes + scene = importer.ReadFile(outFile, aiProcess_ValidateDataStructure); + ASSERT_TRUE(scene != nullptr) << "Fatal: could not reimport!"; + + // A Collada root node is not allowed to have a mesh + ASSERT_EQ(0u, scene->mRootNode->mNumMeshes) << "Collada reimport should not give the root node a mesh"; + + // Walk nodes and counts used meshes + // Should be exactly one + EXPECT_EQ(1u, GetMeshUseCount(scene->mRootNode)) << "Nodes had unexpected number of meshes in use"; +} + TEST_F(utColladaImportExport, exporterUniqueIdsTest) { Assimp::Importer importer; Assimp::Exporter exporter; From 56a4e615336ea7410b25fe612b216340217adc0f Mon Sep 17 00:00:00 2001 From: RichardTea <31507749+RichardTea@users.noreply.github.com> Date: Tue, 5 May 2020 10:16:13 +0100 Subject: [PATCH 2/3] Collada: Don't copy the scene when exporting This was purely to add a virtual top-level node Use a flag instead. Also add more const --- code/AssetLib/Collada/ColladaExporter.cpp | 105 ++++++++++------------ code/AssetLib/Collada/ColladaExporter.h | 7 +- 2 files changed, 53 insertions(+), 59 deletions(-) diff --git a/code/AssetLib/Collada/ColladaExporter.cpp b/code/AssetLib/Collada/ColladaExporter.cpp index 480d6790b..567f7c8e7 100644 --- a/code/AssetLib/Collada/ColladaExporter.cpp +++ b/code/AssetLib/Collada/ColladaExporter.cpp @@ -117,22 +117,37 @@ static const std::string XMLIDEncode(const std::string &name) { return idEncoded.str(); } +// ------------------------------------------------------------------------------------------------ +// Helper functions to create unique ids +inline bool IsUniqueId(const std::unordered_set &idSet, const std::string &idStr) { + return (idSet.find(idStr) == idSet.end()); +} + +inline std::string MakeUniqueId(const std::unordered_set &idSet, const std::string &idPrefix, const std::string &postfix) { + std::string result(idPrefix + postfix); + if (!IsUniqueId(idSet, result)) { + // Select a number to append + size_t idnum = 1; + do { + result = idPrefix + '_' + to_string(idnum) + postfix; + ++idnum; + } while (!IsUniqueId(idSet, result)); + } + return result; +} + // ------------------------------------------------------------------------------------------------ // Constructor for a specific scene to export ColladaExporter::ColladaExporter(const aiScene *pScene, IOSystem *pIOSystem, const std::string &path, const std::string &file) : mIOSystem(pIOSystem), mPath(path), - mFile(file) { + mFile(file), + mScene(pScene), + endstr("\n") { // make sure that all formatting happens using the standard, C locale and not the user's current locale mOutput.imbue(std::locale("C")); mOutput.precision(ASSIMP_AI_REAL_TEXT_PRECISION); - mScene = pScene; - mSceneOwned = false; - - // set up strings - endstr = "\n"; - // start writing the file WriteFile(); } @@ -140,9 +155,6 @@ ColladaExporter::ColladaExporter(const aiScene *pScene, IOSystem *pIOSystem, con // ------------------------------------------------------------------------------------------------ // Destructor ColladaExporter::~ColladaExporter() { - if (mSceneOwned) { - delete mScene; - } } // ------------------------------------------------------------------------------------------------ @@ -171,10 +183,11 @@ void ColladaExporter::WriteFile() { // customized, Writes the animation library WriteAnimationsLibrary(); - // useless Collada fu at the end, just in case we haven't had enough indirections, yet. + // instantiate the scene(s) + // For Assimp there will only ever be one mOutput << startstr << "" << endstr; PushTag(); - mOutput << startstr << "mRootNode) + "\" />" << endstr; + mOutput << startstr << "" << endstr; PopTag(); mOutput << startstr << "" << endstr; PopTag(); @@ -209,13 +222,13 @@ void ColladaExporter::WriteHeader() { mScene->mRootNode->mTransformation.Decompose(scaling, rotation, position); rotation.Normalize(); - bool add_root_node = false; + mAdd_root_node = false; ai_real scale = 1.0; if (std::abs(scaling.x - scaling.y) <= epsilon && std::abs(scaling.x - scaling.z) <= epsilon && std::abs(scaling.y - scaling.z) <= epsilon) { scale = (ai_real)((((double)scaling.x) + ((double)scaling.y) + ((double)scaling.z)) / 3.0); } else { - add_root_node = true; + mAdd_root_node = true; } std::string up_axis = "Y_UP"; @@ -226,34 +239,19 @@ void ColladaExporter::WriteHeader() { } else if (rotation.Equal(z_rot, epsilon)) { up_axis = "Z_UP"; } else { - add_root_node = true; + mAdd_root_node = true; } if (!position.Equal(aiVector3D(0, 0, 0))) { - add_root_node = true; + mAdd_root_node = true; } // Assimp root nodes can have meshes, Collada Scenes cannot if (mScene->mRootNode->mNumChildren == 0 || mScene->mRootNode->mMeshes != 0) { - add_root_node = true; + mAdd_root_node = true; } - if (add_root_node) { - aiScene *scene = nullptr; - SceneCombiner::CopyScene(&scene, mScene); - - aiNode *root = new aiNode("Scene"); - - root->mNumChildren = 1; - root->mChildren = new aiNode *[root->mNumChildren]; - - root->mChildren[0] = scene->mRootNode; - scene->mRootNode->mParent = root; - scene->mRootNode = root; - - mScene = scene; - mSceneOwned = true; - + if (mAdd_root_node) { up_axis = "Y_UP"; scale = 1.0; } @@ -1227,17 +1225,29 @@ void ColladaExporter::WriteFloatArray(const std::string &pIdString, FloatDataTyp // ------------------------------------------------------------------------------------------------ // Writes the scene library void ColladaExporter::WriteSceneLibrary() { - const std::string sceneId = GetNodeUniqueId(mScene->mRootNode); - const std::string sceneName = GetNodeName(mScene->mRootNode); + // Determine if we are using the aiScene root or our own + std::string sceneName("Scene"); + if (mAdd_root_node) { + mSceneId = MakeUniqueId(mUniqueIds, sceneName, std::string()); + mUniqueIds.insert(mSceneId); + } else { + mSceneId = GetNodeUniqueId(mScene->mRootNode); + sceneName = GetNodeName(mScene->mRootNode); + } mOutput << startstr << "" << endstr; PushTag(); - mOutput << startstr << "" << endstr; + mOutput << startstr << "" << endstr; PushTag(); - // start recursive write at the root node - for (size_t a = 0; a < mScene->mRootNode->mNumChildren; ++a) - WriteNode(mScene->mRootNode->mChildren[a]); + if (mAdd_root_node) { + // Export the root node + WriteNode(mScene->mRootNode); + } else { + // Have already exported the root node + for (size_t a = 0; a < mScene->mRootNode->mNumChildren; ++a) + WriteNode(mScene->mRootNode->mChildren[a]); + } PopTag(); mOutput << startstr << "" << endstr; @@ -1610,23 +1620,6 @@ void ColladaExporter::WriteNode(const aiNode *pNode) { mOutput << startstr << "" << endstr; } -inline bool IsUniqueId(const std::unordered_set &idSet, const std::string &idStr) { - return (idSet.find(idStr) == idSet.end()); -} - -inline std::string MakeUniqueId(const std::unordered_set &idSet, const std::string &idPrefix, const std::string &postfix) { - std::string result(idPrefix + postfix); - if (!IsUniqueId(idSet, result)) { - // Select a number to append - size_t idnum = 1; - do { - result = idPrefix + '_' + to_string(idnum) + postfix; - ++idnum; - } while (!IsUniqueId(idSet, result)); - } - return result; -} - void ColladaExporter::CreateNodeIds(const aiNode *node) { GetNodeUniqueId(node); for (size_t a = 0; a < node->mNumChildren; ++a) diff --git a/code/AssetLib/Collada/ColladaExporter.h b/code/AssetLib/Collada/ColladaExporter.h index bea65bafc..e9a3530ae 100644 --- a/code/AssetLib/Collada/ColladaExporter.h +++ b/code/AssetLib/Collada/ColladaExporter.h @@ -196,13 +196,14 @@ public: const std::string mFile; /// The scene to be written - const aiScene *mScene; - bool mSceneOwned; + const aiScene *const mScene; + std::string mSceneId; + bool mAdd_root_node = false; /// current line start string, contains the current indentation for simple stream insertion std::string startstr; /// current line end string for simple stream insertion - std::string endstr; + const std::string endstr; // pair of color and texture - texture precedences color struct Surface { From dc8550290ef2ce2b9d26e98c4172cbb289b56f83 Mon Sep 17 00:00:00 2001 From: RichardTea <31507749+RichardTea@users.noreply.github.com> Date: Tue, 5 May 2020 10:53:26 +0100 Subject: [PATCH 3/3] Ensure to delete the scene copy after the test --- test/unit/utColladaImportExport.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/test/unit/utColladaImportExport.cpp b/test/unit/utColladaImportExport.cpp index 3d6e0db23..451c8e235 100644 --- a/test/unit/utColladaImportExport.cpp +++ b/test/unit/utColladaImportExport.cpp @@ -53,6 +53,20 @@ using namespace Assimp; class utColladaImportExport : public AbstractImportExportBase { public: + // Clones the scene in an exception-safe way + struct SceneCloner { + SceneCloner(const aiScene *scene) { + sceneCopy = nullptr; + SceneCombiner::CopyScene(&sceneCopy, scene); + } + + ~SceneCloner() { + delete sceneCopy; + sceneCopy = nullptr; + } + aiScene *sceneCopy; + }; + virtual bool importerTest() final { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/Collada/duck.dae", aiProcess_ValidateDataStructure); @@ -231,12 +245,10 @@ TEST_F(utColladaImportExport, exportRootNodeMeshTest) { ASSERT_EQ(0u, scene->mRootNode->mNumMeshes) << "Collada import should not give the root node a mesh"; { - // Clone the scene and give the root node a mesh and a transform - aiScene *rootMeshScene = nullptr; - SceneCombiner::CopyScene(&rootMeshScene, scene); - ASSERT_TRUE(rootMeshScene != nullptr) << "Fatal: could not copy scene!"; + SceneCloner clone(scene); + ASSERT_TRUE(clone.sceneCopy != nullptr) << "Fatal: could not copy scene!"; // Do this by moving the meshes from the first child that has some - aiNode *rootNode = rootMeshScene->mRootNode; + aiNode *rootNode = clone.sceneCopy->mRootNode; ASSERT_TRUE(rootNode->mNumChildren > 0) << "Fatal: root has no children"; aiNode *meshNode = rootNode->mChildren[0]; ASSERT_EQ(1u, meshNode->mNumMeshes) << "Fatal: First child node has no duck mesh"; @@ -248,10 +260,12 @@ TEST_F(utColladaImportExport, exportRootNodeMeshTest) { rootNode->mMeshes[i] = meshNode->mMeshes[i]; } + // Remove the meshes from the original node meshNode->mNumMeshes = 0; delete[] meshNode->mMeshes; + meshNode->mMeshes = nullptr; - ASSERT_EQ(AI_SUCCESS, exporter.Export(rootMeshScene, "collada", outFile)) << "Fatal: Could not export file"; + ASSERT_EQ(AI_SUCCESS, exporter.Export(clone.sceneCopy, "collada", outFile)) << "Fatal: Could not export file"; } // Reimport and look for meshes