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] 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 << "<node ";
     if (is_skeleton_root) {
-        mOutput << "id=\"" << node_id << "\" " << (is_joint ? "sid=\"" + node_id + "\" " : ""); // For now, only support one skeleton in a scene.
-        mFoundSkeletonRootNodeID = node_id;
-    } else {
-        mOutput << "id=\"" << node_id << "\" " << (is_joint ? "sid=\"" + node_id + "\" " : "");
+        mFoundSkeletonRootNodeID = node_id; // For now, only support one skeleton in a scene.
     }
-
+    mOutput << "id=\"" << node_id << "\" " << (is_joint ? "sid=\"" + node_id + "\" " : "");
     mOutput << "name=\"" << node_name
             << "\" type=\"" << node_type
             << "\">" << 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 <assimp/ColladaMetaData.h>
+#include <assimp/SceneCombiner.h>
 #include <assimp/commonMetaData.h>
 #include <assimp/postprocess.h>
 #include <assimp/scene.h>
@@ -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;