From ddf7c0ad8f6ede4742ab65de9c0cfba86262dd02 Mon Sep 17 00:00:00 2001 From: Yingying Wang Date: Tue, 5 Nov 2019 17:34:32 -0800 Subject: [PATCH 1/7] avoid weighting vertex repeatedly when joining identical vertices --- code/Common/Exporter.cpp | 3 +-- code/FBX/FBXExporter.cpp | 5 ++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/code/Common/Exporter.cpp b/code/Common/Exporter.cpp index 4ce1a2bd8..8b224369d 100644 --- a/code/Common/Exporter.cpp +++ b/code/Common/Exporter.cpp @@ -452,8 +452,7 @@ aiReturn Exporter::Export( const aiScene* pScene, const char* pFormatId, const c ExportProperties emptyProperties; // Never pass NULL ExportProperties so Exporters don't have to worry. ExportProperties* pProp = pProperties ? (ExportProperties*)pProperties : &emptyProperties; - pProp->SetPropertyBool("bJoinIdenticalVertices", must_join_again); - exp.mExportFunction(pPath,pimpl->mIOSystem.get(),scenecopy.get(), pProp); + pProp->SetPropertyBool("bJoinIdenticalVertices", pp & aiProcess_JoinIdenticalVertices); exp.mExportFunction(pPath,pimpl->mIOSystem.get(),scenecopy.get(), pProp); pimpl->mProgressHandler->UpdateFileWrite(4, 4); diff --git a/code/FBX/FBXExporter.cpp b/code/FBX/FBXExporter.cpp index 9767f9a0a..413d1d6c8 100644 --- a/code/FBX/FBXExporter.cpp +++ b/code/FBX/FBXExporter.cpp @@ -1860,6 +1860,7 @@ void FBXExporter::WriteObjects () sdnode.AddChild("Version", int32_t(100)); sdnode.AddChild("UserData", "", ""); + std::set setWeightedVertex; // add indices and weights, if any if (b) { std::vector subdef_indices; @@ -1867,7 +1868,8 @@ void FBXExporter::WriteObjects () int32_t last_index = -1; for (size_t wi = 0; wi < b->mNumWeights; ++wi) { int32_t vi = vertex_indices[b->mWeights[wi].mVertexId]; - if (vi == last_index) { + bool bIsWeightedAlready = (setWeightedVertex.find(vi) != setWeightedVertex.end()); + if (vi == last_index || bIsWeightedAlready) { // only for vertices we exported to fbx // TODO, FIXME: this assumes identically-located vertices // will always deform in the same way. @@ -1877,6 +1879,7 @@ void FBXExporter::WriteObjects () // identical vertex. continue; } + setWeightedVertex.insert(vi); subdef_indices.push_back(vi); subdef_weights.push_back(b->mWeights[wi].mWeight); last_index = vi; From 80f5283b2fd3fac7dc612e2163b38786ebe1125e Mon Sep 17 00:00:00 2001 From: Mike Samsonov Date: Mon, 18 Nov 2019 16:57:01 +0000 Subject: [PATCH 2/7] Error string of Importer should contain a message in case of an exception --- code/Common/Importer.cpp | 4 +- include/assimp/Exceptional.h | 10 + test/models/glTF2/MissingBin/BoxTextured.gltf | 181 ++++++++++++++++++ test/unit/utglTF2ImportExport.cpp | 9 + 4 files changed, 202 insertions(+), 2 deletions(-) create mode 100644 test/models/glTF2/MissingBin/BoxTextured.gltf diff --git a/code/Common/Importer.cpp b/code/Common/Importer.cpp index 91b50859a..880b8e83d 100644 --- a/code/Common/Importer.cpp +++ b/code/Common/Importer.cpp @@ -493,7 +493,7 @@ const aiScene* Importer::ReadFileFromMemory( const void* pBuffer, ReadFile(fbuff,pFlags); SetIOHandler(io); - ASSIMP_END_EXCEPTION_REGION(const aiScene*); + ASSIMP_END_EXCEPTION_REGION_WITH_ERROR_STRING(const aiScene*, pimpl->mErrorString); return pimpl->mScene; } @@ -710,7 +710,7 @@ const aiScene* Importer::ReadFile( const char* _pFile, unsigned int pFlags) #endif // ! ASSIMP_CATCH_GLOBAL_EXCEPTIONS // either successful or failure - the pointer expresses it anyways - ASSIMP_END_EXCEPTION_REGION(const aiScene*); + ASSIMP_END_EXCEPTION_REGION_WITH_ERROR_STRING(const aiScene*, pimpl->mErrorString); return pimpl->mScene; } diff --git a/include/assimp/Exceptional.h b/include/assimp/Exceptional.h index 6bb6ce1e3..f0d23d0f3 100644 --- a/include/assimp/Exceptional.h +++ b/include/assimp/Exceptional.h @@ -119,6 +119,16 @@ struct ExceptionSwallower { {\ try { +#define ASSIMP_END_EXCEPTION_REGION_WITH_ERROR_STRING(type, ASSIMP_END_EXCEPTION_REGION_errorString)\ + } catch(const DeadlyImportError& e) {\ + ASSIMP_END_EXCEPTION_REGION_errorString = e.what();\ + return ExceptionSwallower()();\ + } catch(...) {\ + ASSIMP_END_EXCEPTION_REGION_errorString = "Unknown exception";\ + return ExceptionSwallower()();\ + }\ +} + #define ASSIMP_END_EXCEPTION_REGION(type)\ } catch(...) {\ return ExceptionSwallower()();\ diff --git a/test/models/glTF2/MissingBin/BoxTextured.gltf b/test/models/glTF2/MissingBin/BoxTextured.gltf new file mode 100644 index 000000000..88d65391e --- /dev/null +++ b/test/models/glTF2/MissingBin/BoxTextured.gltf @@ -0,0 +1,181 @@ +{ + "asset": { + "generator": "COLLADA2GLTF", + "version": "2.0" + }, + "scene": 0, + "scenes": [ + { + "nodes": [ + 0 + ] + } + ], + "nodes": [ + { + "children": [ + 1 + ], + "matrix": [ + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + -1.0, + 0.0, + 0.0, + 1.0, + 0.0, + 0.0, + 0.0, + 0.0, + 0.0, + 1.0 + ] + }, + { + "mesh": 0 + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "NORMAL": 1, + "POSITION": 2, + "TEXCOORD_0": 3 + }, + "indices": 0, + "mode": 4, + "material": 0 + } + ], + "name": "Mesh" + } + ], + "accessors": [ + { + "bufferView": 0, + "byteOffset": 0, + "componentType": 5123, + "count": 36, + "max": [ + 23 + ], + "min": [ + 0 + ], + "type": "SCALAR" + }, + { + "bufferView": 1, + "byteOffset": 0, + "componentType": 5126, + "count": 24, + "max": [ + 1.0, + 1.0, + 1.0 + ], + "min": [ + -1.0, + -1.0, + -1.0 + ], + "type": "VEC3" + }, + { + "bufferView": 1, + "byteOffset": 288, + "componentType": 5126, + "count": 24, + "max": [ + 0.5, + 0.5, + 0.5 + ], + "min": [ + -0.5, + -0.5, + -0.5 + ], + "type": "VEC3" + }, + { + "bufferView": 2, + "byteOffset": 0, + "componentType": 5126, + "count": 24, + "max": [ + 6.0, + 1.0 + ], + "min": [ + 0.0, + 0.0 + ], + "type": "VEC2" + } + ], + "materials": [ + { + "pbrMetallicRoughness": { + "baseColorTexture": { + "index": 0 + }, + "metallicFactor": 0.0 + }, + "name": "Texture" + } + ], + "textures": [ + { + "sampler": 0, + "source": 0 + } + ], + "images": [ + { + "uri": "CesiumLogoFlat.png" + } + ], + "samplers": [ + { + "magFilter": 9729, + "minFilter": 9986, + "wrapS": 33648, + "wrapT": 33071 + } + ], + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 768, + "byteLength": 72, + "target": 34963 + }, + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 576, + "byteStride": 12, + "target": 34962 + }, + { + "buffer": 0, + "byteOffset": 576, + "byteLength": 192, + "byteStride": 8, + "target": 34962 + } + ], + "buffers": [ + { + "byteLength": 840, + "uri": "BoxTextured0.bin" + } + ] +} diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 6925098b9..3c1f6fea6 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -419,4 +419,13 @@ TEST_F( utglTF2ImportExport, crash_in_anim_mesh_destructor ) { ASSERT_EQ(aiReturn_SUCCESS, exporter.Export(scene, "glb2", ASSIMP_TEST_MODELS_DIR "/glTF2/glTF-Sample-Models/AnimatedMorphCube-glTF/AnimatedMorphCube_out.glTF")); } +TEST_F(utglTF2ImportExport, error_string_preserved) { + Assimp::Importer importer; + const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/MissingBin/BoxTextured.gltf", + aiProcess_ValidateDataStructure); + ASSERT_EQ(nullptr, scene); + std::string error = importer.GetErrorString(); + ASSERT_NE(error.find("BoxTextured0.bin"), std::string::npos) << "Error string should contain an error about missing .bin file"; +} + #endif // ASSIMP_BUILD_NO_EXPORT From b93c360b877b6cd73f10cc0b1bf5f670dd92a4b7 Mon Sep 17 00:00:00 2001 From: Mike Samsonov Date: Tue, 19 Nov 2019 16:10:34 +0000 Subject: [PATCH 3/7] memory leak fix --- code/glTF2/glTF2Asset.inl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/code/glTF2/glTF2Asset.inl b/code/glTF2/glTF2Asset.inl index 6b47b1607..3c98a8150 100644 --- a/code/glTF2/glTF2Asset.inl +++ b/code/glTF2/glTF2Asset.inl @@ -270,13 +270,14 @@ Ref LazyDict::Retrieve(unsigned int i) throw DeadlyImportError("GLTF: Object at index \"" + to_string(i) + "\" is not a JSON object"); } - T* inst = new T(); + // In case Read method throws an exception this will not leak + auto inst = std::make_unique(); inst->id = std::string(mDictId) + "_" + to_string(i); inst->oIndex = i; ReadMember(obj, "name", inst->name); inst->Read(obj, mAsset); - return Add(inst); + return Add(inst.release()); } template From 6f7cb6af06a92e9d8404d4cf41f31e5bf67799af Mon Sep 17 00:00:00 2001 From: Mike Samsonov Date: Tue, 19 Nov 2019 16:58:48 +0000 Subject: [PATCH 4/7] revert memory leak fix --- code/glTF2/glTF2Asset.inl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/code/glTF2/glTF2Asset.inl b/code/glTF2/glTF2Asset.inl index 3c98a8150..6b47b1607 100644 --- a/code/glTF2/glTF2Asset.inl +++ b/code/glTF2/glTF2Asset.inl @@ -270,14 +270,13 @@ Ref LazyDict::Retrieve(unsigned int i) throw DeadlyImportError("GLTF: Object at index \"" + to_string(i) + "\" is not a JSON object"); } - // In case Read method throws an exception this will not leak - auto inst = std::make_unique(); + T* inst = new T(); inst->id = std::string(mDictId) + "_" + to_string(i); inst->oIndex = i; ReadMember(obj, "name", inst->name); inst->Read(obj, mAsset); - return Add(inst.release()); + return Add(inst); } template From 12f184867e514e393c9e4eaa78d8cf421759476e Mon Sep 17 00:00:00 2001 From: Mike Samsonov Date: Tue, 19 Nov 2019 17:05:24 +0000 Subject: [PATCH 5/7] Fix for memory leak in glTF2 Importer if an exception has been thrown --- code/glTF2/glTF2Asset.inl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/code/glTF2/glTF2Asset.inl b/code/glTF2/glTF2Asset.inl index 6b47b1607..0d4de11f2 100644 --- a/code/glTF2/glTF2Asset.inl +++ b/code/glTF2/glTF2Asset.inl @@ -270,13 +270,14 @@ Ref LazyDict::Retrieve(unsigned int i) throw DeadlyImportError("GLTF: Object at index \"" + to_string(i) + "\" is not a JSON object"); } - T* inst = new T(); + // Unique ptr prevents memory leak in case of Read throws an exception + auto inst = std::unique_ptr(); inst->id = std::string(mDictId) + "_" + to_string(i); inst->oIndex = i; ReadMember(obj, "name", inst->name); inst->Read(obj, mAsset); - return Add(inst); + return Add(inst.release()); } template From 91af4b7476fb9ecd86cdf1c7ae0829aa4e219dc1 Mon Sep 17 00:00:00 2001 From: Mike Samsonov Date: Tue, 19 Nov 2019 17:43:31 +0000 Subject: [PATCH 6/7] fix the crash --- code/glTF2/glTF2Asset.inl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/glTF2/glTF2Asset.inl b/code/glTF2/glTF2Asset.inl index 0d4de11f2..4cd0f65c0 100644 --- a/code/glTF2/glTF2Asset.inl +++ b/code/glTF2/glTF2Asset.inl @@ -271,7 +271,7 @@ Ref LazyDict::Retrieve(unsigned int i) } // Unique ptr prevents memory leak in case of Read throws an exception - auto inst = std::unique_ptr(); + auto inst = std::unique_ptr(new T()); inst->id = std::string(mDictId) + "_" + to_string(i); inst->oIndex = i; ReadMember(obj, "name", inst->name); From 4071fcd398e4a7cca0fbf5586c680f75dffb2006 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 1 Dec 2019 22:46:48 +0100 Subject: [PATCH 7/7] Update Exporter.cpp Fix format. --- code/Common/Exporter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/Common/Exporter.cpp b/code/Common/Exporter.cpp index 07f95fbb4..25a78114f 100644 --- a/code/Common/Exporter.cpp +++ b/code/Common/Exporter.cpp @@ -445,7 +445,7 @@ aiReturn Exporter::Export( const aiScene* pScene, const char* pFormatId, const c ExportProperties emptyProperties; // Never pass NULL ExportProperties so Exporters don't have to worry. ExportProperties* pProp = pProperties ? (ExportProperties*)pProperties : &emptyProperties; - pProp->SetPropertyBool("bJoinIdenticalVertices", pp & aiProcess_JoinIdenticalVertices); + pProp->SetPropertyBool("bJoinIdenticalVertices", pp & aiProcess_JoinIdenticalVertices); exp.mExportFunction(pPath,pimpl->mIOSystem.get(),scenecopy.get(), pProp); pimpl->mProgressHandler->UpdateFileWrite(4, 4);