From 2c1943cd10c84d1a1b1ebf1982ddc35756f463cd Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Mon, 8 Aug 2022 13:35:58 +0300 Subject: [PATCH 01/17] Improve BlenderDNA error message --- code/AssetLib/Blender/BlenderDNA.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/Blender/BlenderDNA.h b/code/AssetLib/Blender/BlenderDNA.h index b2158f283..dc5a36c2a 100644 --- a/code/AssetLib/Blender/BlenderDNA.h +++ b/code/AssetLib/Blender/BlenderDNA.h @@ -416,10 +416,10 @@ template <> struct Structure::_defaultInitializer { template - void operator()(T & /*out*/, const char * = "") { + void operator()(T & /*out*/, const char *message = "") { // obviously, it is crucial that _DefaultInitializer is used // only from within a catch clause. - throw DeadlyImportError("Constructing BlenderDNA Structure encountered an error"); + throw DeadlyImportError("Constructing BlenderDNA Structure encountered an error: ", message); } }; From e43e3e11b93ea8cd6bce757c4a62d59eda20e8f5 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Mon, 8 Aug 2022 13:53:38 +0300 Subject: [PATCH 02/17] Move SharedModifierData definition to BlenderScene.h --- code/AssetLib/Blender/BlenderModifier.cpp | 4 ---- code/AssetLib/Blender/BlenderScene.h | 7 +++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/code/AssetLib/Blender/BlenderModifier.cpp b/code/AssetLib/Blender/BlenderModifier.cpp index d2b393dc4..09bad0cfa 100644 --- a/code/AssetLib/Blender/BlenderModifier.cpp +++ b/code/AssetLib/Blender/BlenderModifier.cpp @@ -71,10 +71,6 @@ static const fpCreateModifier creators[] = { nullptr // sentinel }; -// ------------------------------------------------------------------------------------------------ -struct SharedModifierData : ElemBase { - ModifierData modifier; -}; // ------------------------------------------------------------------------------------------------ void BlenderModifierShowcase::ApplyModifiers(aiNode &out, ConversionData &conv_data, const Scene &in, const Object &orig_object) { diff --git a/code/AssetLib/Blender/BlenderScene.h b/code/AssetLib/Blender/BlenderScene.h index c153d3c22..cef40b660 100644 --- a/code/AssetLib/Blender/BlenderScene.h +++ b/code/AssetLib/Blender/BlenderScene.h @@ -648,6 +648,13 @@ struct ModifierData : ElemBase { char name[32]; }; + +// ------------------------------------------------------------------------------------------------ +struct SharedModifierData : ElemBase { + ModifierData modifier; +}; + + // ------------------------------------------------------------------------------- struct SubsurfModifierData : ElemBase { From 2e64880abf50b17864913b7db3279f5862c19d7e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Mon, 8 Aug 2022 13:56:20 +0300 Subject: [PATCH 03/17] Inherit SubsurfModifierData and MirrorModifierData from SharedModifierData This makes the cast in BlenderModifierShowcase::ApplyModifiers work correctly and not invoke UB. --- code/AssetLib/Blender/BlenderScene.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/code/AssetLib/Blender/BlenderScene.h b/code/AssetLib/Blender/BlenderScene.h index cef40b660..d44a7c470 100644 --- a/code/AssetLib/Blender/BlenderScene.h +++ b/code/AssetLib/Blender/BlenderScene.h @@ -656,7 +656,7 @@ struct SharedModifierData : ElemBase { // ------------------------------------------------------------------------------- -struct SubsurfModifierData : ElemBase { +struct SubsurfModifierData : SharedModifierData { enum Type { @@ -669,7 +669,6 @@ struct SubsurfModifierData : ElemBase { FLAGS_SubsurfUV = 1 << 3 }; - ModifierData modifier FAIL; short subdivType WARN; short levels FAIL; short renderLevels; @@ -677,7 +676,7 @@ struct SubsurfModifierData : ElemBase { }; // ------------------------------------------------------------------------------- -struct MirrorModifierData : ElemBase { +struct MirrorModifierData : SharedModifierData { enum Flags { Flags_CLIPPING = 1 << 0, @@ -689,8 +688,6 @@ struct MirrorModifierData : ElemBase { Flags_VGROUP = 1 << 6 }; - ModifierData modifier FAIL; - short axis, flag; float tolerance; std::shared_ptr mirror_ob; From 90d14644053b2704b0839f0ec61de678b1ee7608 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 10:32:01 +0300 Subject: [PATCH 04/17] Remove set but unused variables --- code/AssetLib/LWO/LWOMaterial.cpp | 2 -- code/AssetLib/LWS/LWSLoader.cpp | 6 +----- code/AssetLib/Obj/ObjFileParser.cpp | 2 -- code/AssetLib/X3D/X3DImporter.cpp | 3 --- code/Common/ScenePreprocessor.cpp | 2 -- contrib/openddlparser/code/OpenDDLParser.cpp | 3 --- 6 files changed, 1 insertion(+), 17 deletions(-) diff --git a/code/AssetLib/LWO/LWOMaterial.cpp b/code/AssetLib/LWO/LWOMaterial.cpp index 4728706db..50bac884b 100644 --- a/code/AssetLib/LWO/LWOMaterial.cpp +++ b/code/AssetLib/LWO/LWOMaterial.cpp @@ -707,12 +707,10 @@ void LWOImporter::LoadNodalBlocks(unsigned int size) { if (mFileBuffer + head.length > end) { throw DeadlyImportError("LWO3: cannot read length; LoadNodalBlocks"); } - int node_idx = 0; uint8_t *const next = mFileBuffer + head.length; mFileBuffer += bufOffset; switch (head.type) { case AI_LWO_NNDS: - node_idx++; LoadNodes(head.length); break; } diff --git a/code/AssetLib/LWS/LWSLoader.cpp b/code/AssetLib/LWS/LWSLoader.cpp index abaaaa305..0e29bb38f 100644 --- a/code/AssetLib/LWS/LWSLoader.cpp +++ b/code/AssetLib/LWS/LWSLoader.cpp @@ -516,7 +516,7 @@ void LWSImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy std::list nodes; unsigned int cur_light = 0, cur_camera = 0, cur_object = 0; - unsigned int num_light = 0, num_camera = 0, num_object = 0; + unsigned int num_light = 0, num_camera = 0; // check magic identifier, 'LWSC' bool motion_file = false; @@ -586,7 +586,6 @@ void LWSImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy d.id = batch.AddLoadRequest(path, 0, &props); nodes.push_back(d); - ++num_object; } else if ((*it).tokens[0] == "LoadObject") { // 'LoadObject': load a LWO file into the scene-graph // add node to list @@ -604,7 +603,6 @@ void LWSImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy d.path = path; nodes.push_back(d); - ++num_object; } else if ((*it).tokens[0] == "AddNullObject") { // 'AddNullObject': add a dummy node to the hierarchy // add node to list @@ -618,8 +616,6 @@ void LWSImporter::InternReadFile(const std::string &pFile, aiScene *pScene, IOSy } d.name = c; nodes.push_back(d); - - num_object++; } // 'NumChannels': Number of envelope channels assigned to last layer else if ((*it).tokens[0] == "NumChannels") { diff --git a/code/AssetLib/Obj/ObjFileParser.cpp b/code/AssetLib/Obj/ObjFileParser.cpp index 93f37bc6b..167cab544 100644 --- a/code/AssetLib/Obj/ObjFileParser.cpp +++ b/code/AssetLib/Obj/ObjFileParser.cpp @@ -112,7 +112,6 @@ ObjFile::Model *ObjFileParser::GetModel() const { void ObjFileParser::parseFile(IOStreamBuffer &streamBuffer) { // only update every 100KB or it'll be too slow //const unsigned int updateProgressEveryBytes = 100 * 1024; - unsigned int progressCounter = 0; const unsigned int bytesToProcess = static_cast(streamBuffer.size()); const unsigned int progressTotal = bytesToProcess; unsigned int processed = 0; @@ -129,7 +128,6 @@ void ObjFileParser::parseFile(IOStreamBuffer &streamBuffer) { if (lastFilePos < filePos) { processed = static_cast(filePos); lastFilePos = filePos; - progressCounter++; m_progress->UpdateFileRead(processed, progressTotal); } diff --git a/code/AssetLib/X3D/X3DImporter.cpp b/code/AssetLib/X3D/X3DImporter.cpp index 01c97eb4c..e28c9e5f3 100644 --- a/code/AssetLib/X3D/X3DImporter.cpp +++ b/code/AssetLib/X3D/X3DImporter.cpp @@ -477,9 +477,6 @@ void X3DImporter::ParseHelper_Node_Exit() { // check if we can walk up. if (mNodeElementCur != nullptr) { mNodeElementCur = mNodeElementCur->Parent; - } else { - int i = 0; - ++i; } } diff --git a/code/Common/ScenePreprocessor.cpp b/code/Common/ScenePreprocessor.cpp index 60133f651..c769ec30c 100644 --- a/code/Common/ScenePreprocessor.cpp +++ b/code/Common/ScenePreprocessor.cpp @@ -118,10 +118,8 @@ void ScenePreprocessor::ProcessMesh(aiMesh *mesh) { // as if they were 2D channels .. just in case an application doesn't handle // this case if (2 == mesh->mNumUVComponents[i]) { - size_t num = 0; for (; p != end; ++p) { p->z = 0.f; - num++; } } else if (1 == mesh->mNumUVComponents[i]) { for (; p != end; ++p) { diff --git a/contrib/openddlparser/code/OpenDDLParser.cpp b/contrib/openddlparser/code/OpenDDLParser.cpp index e2bef97a7..fe9d23ab5 100644 --- a/contrib/openddlparser/code/OpenDDLParser.cpp +++ b/contrib/openddlparser/code/OpenDDLParser.cpp @@ -647,12 +647,9 @@ char *OpenDDLParser::parseBooleanLiteral(char *in, char *end, Value **boolean) { in = lookForNextToken(in, end); char *start(in); - size_t len(0); while (!isSeparator(*in) && in != end) { ++in; - ++len; } - ++len; int res = ::strncmp(Grammar::BoolTrue, start, strlen(Grammar::BoolTrue)); if (0 != res) { res = ::strncmp(Grammar::BoolFalse, start, strlen(Grammar::BoolFalse)); From 56b644691340c6040670a9f8fe5f221e68cb4eeb Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 11:12:53 +0300 Subject: [PATCH 05/17] Use weak pointer for MirrorModifierData mirror_ob to avoid memory leak --- code/AssetLib/Blender/BlenderModifier.cpp | 5 +++-- code/AssetLib/Blender/BlenderScene.cpp | 4 +++- code/AssetLib/Blender/BlenderScene.h | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/code/AssetLib/Blender/BlenderModifier.cpp b/code/AssetLib/Blender/BlenderModifier.cpp index 09bad0cfa..6cc11ec8e 100644 --- a/code/AssetLib/Blender/BlenderModifier.cpp +++ b/code/AssetLib/Blender/BlenderModifier.cpp @@ -153,6 +153,7 @@ void BlenderModifier_Mirror ::DoIt(aiNode &out, ConversionData &conv_data, const // hijacking the ABI, see the big note in BlenderModifierShowcase::ApplyModifiers() const MirrorModifierData &mir = static_cast(orig_modifier); ai_assert(mir.modifier.type == ModifierData::eModifierType_Mirror); + std::shared_ptr mirror_ob = mir.mirror_ob.lock(); conv_data.meshes->reserve(conv_data.meshes->size() + out.mNumMeshes); @@ -167,8 +168,8 @@ void BlenderModifier_Mirror ::DoIt(aiNode &out, ConversionData &conv_data, const const float ys = mir.flag & MirrorModifierData::Flags_AXIS_Y ? -1.f : 1.f; const float zs = mir.flag & MirrorModifierData::Flags_AXIS_Z ? -1.f : 1.f; - if (mir.mirror_ob) { - const aiVector3D center(mir.mirror_ob->obmat[3][0], mir.mirror_ob->obmat[3][1], mir.mirror_ob->obmat[3][2]); + if (mirror_ob) { + const aiVector3D center(mirror_ob->obmat[3][0], mirror_ob->obmat[3][1], mirror_ob->obmat[3][2]); for (unsigned int j = 0; j < mesh->mNumVertices; ++j) { aiVector3D &v = mesh->mVertices[j]; diff --git a/code/AssetLib/Blender/BlenderScene.cpp b/code/AssetLib/Blender/BlenderScene.cpp index 9ad086fe6..d458ede2e 100644 --- a/code/AssetLib/Blender/BlenderScene.cpp +++ b/code/AssetLib/Blender/BlenderScene.cpp @@ -772,7 +772,9 @@ void Structure ::Convert( ReadField(dest.axis, "axis", db); ReadField(dest.flag, "flag", db); ReadField(dest.tolerance, "tolerance", db); - ReadFieldPtr(dest.mirror_ob, "*mirror_ob", db); + std::shared_ptr mirror_ob; + ReadFieldPtr(mirror_ob, "*mirror_ob", db); + dest.mirror_ob = mirror_ob; db.reader->IncPtr(size); } diff --git a/code/AssetLib/Blender/BlenderScene.h b/code/AssetLib/Blender/BlenderScene.h index d44a7c470..479eae018 100644 --- a/code/AssetLib/Blender/BlenderScene.h +++ b/code/AssetLib/Blender/BlenderScene.h @@ -690,7 +690,7 @@ struct MirrorModifierData : SharedModifierData { short axis, flag; float tolerance; - std::shared_ptr mirror_ob; + std::weak_ptr mirror_ob; }; // ------------------------------------------------------------------------------- From 219dbbd4afc0eed61521369c5e4a5316c78e591e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 12:39:27 +0300 Subject: [PATCH 06/17] Fix crash of "assimp info" with no parameters --- tools/assimp_cmd/Info.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/assimp_cmd/Info.cpp b/tools/assimp_cmd/Info.cpp index 4703713c8..ef6554619 100644 --- a/tools/assimp_cmd/Info.cpp +++ b/tools/assimp_cmd/Info.cpp @@ -286,12 +286,6 @@ void PrintHierarchy( // ----------------------------------------------------------------------------------- // Implementation of the assimp info utility to print basic file info int Assimp_Info(const char *const *params, unsigned int num) { - // --help - if (!strcmp(params[0], "-h") || !strcmp(params[0], "--help") || !strcmp(params[0], "-?")) { - printf("%s", AICMD_MSG_INFO_HELP_E); - return AssimpCmdError::Success; - } - // asssimp info [-r] if (num < 1) { printf("assimp info: Invalid number of arguments. " @@ -299,6 +293,12 @@ int Assimp_Info(const char *const *params, unsigned int num) { return AssimpCmdError::InvalidNumberOfArguments; } + // --help + if (!strcmp(params[0], "-h") || !strcmp(params[0], "--help") || !strcmp(params[0], "-?")) { + printf("%s", AICMD_MSG_INFO_HELP_E); + return AssimpCmdError::Success; + } + const std::string in = std::string(params[0]); // get -r and -v arguments From 30555282fea98e334606f61ef0f28ef355580d63 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 12:43:50 +0300 Subject: [PATCH 07/17] Use weak pointer for ElemBase last to avoid memory leak --- code/AssetLib/Blender/BlenderScene.cpp | 4 +++- code/AssetLib/Blender/BlenderScene.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/Blender/BlenderScene.cpp b/code/AssetLib/Blender/BlenderScene.cpp index d458ede2e..76582db76 100644 --- a/code/AssetLib/Blender/BlenderScene.cpp +++ b/code/AssetLib/Blender/BlenderScene.cpp @@ -624,7 +624,9 @@ void Structure ::Convert( const FileDatabase &db) const { ReadFieldPtr(dest.first, "*first", db); - ReadFieldPtr(dest.last, "*last", db); + std::shared_ptr last; + ReadFieldPtr(last, "*last", db); + dest.last = last; db.reader->IncPtr(size); } diff --git a/code/AssetLib/Blender/BlenderScene.h b/code/AssetLib/Blender/BlenderScene.h index 479eae018..c592db57f 100644 --- a/code/AssetLib/Blender/BlenderScene.h +++ b/code/AssetLib/Blender/BlenderScene.h @@ -124,7 +124,7 @@ struct ID : ElemBase { // ------------------------------------------------------------------------------- struct ListBase : ElemBase { std::shared_ptr first; - std::shared_ptr last; + std::weak_ptr last; }; // ------------------------------------------------------------------------------- From d2dc0401c3ebdf5b39c380855e4e9a1d40691f90 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 12:47:11 +0300 Subject: [PATCH 08/17] Use weak pointer for ModifierData prev to avoid memory leak --- code/AssetLib/Blender/BlenderScene.cpp | 4 +++- code/AssetLib/Blender/BlenderScene.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/code/AssetLib/Blender/BlenderScene.cpp b/code/AssetLib/Blender/BlenderScene.cpp index 76582db76..b503cb8ff 100644 --- a/code/AssetLib/Blender/BlenderScene.cpp +++ b/code/AssetLib/Blender/BlenderScene.cpp @@ -650,7 +650,9 @@ void Structure ::Convert( const FileDatabase &db) const { ReadFieldPtr(dest.next, "*next", db); - ReadFieldPtr(dest.prev, "*prev", db); + std::shared_ptr prev; + ReadFieldPtr(prev, "*prev", db); + dest.prev = prev; ReadField(dest.type, "type", db); ReadField(dest.mode, "mode", db); ReadFieldArray(dest.name, "name", db); diff --git a/code/AssetLib/Blender/BlenderScene.h b/code/AssetLib/Blender/BlenderScene.h index c592db57f..436e47061 100644 --- a/code/AssetLib/Blender/BlenderScene.h +++ b/code/AssetLib/Blender/BlenderScene.h @@ -642,7 +642,7 @@ struct ModifierData : ElemBase { }; std::shared_ptr next WARN; - std::shared_ptr prev WARN; + std::weak_ptr prev WARN; int type, mode; char name[32]; From 1c59de42fcb62207b5659e96c9c4b11c2da24df4 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 9 Aug 2022 12:41:46 +0300 Subject: [PATCH 09/17] Make some blender fields warn if missing instead of error --- code/AssetLib/Blender/BlenderScene.cpp | 6 +-- test/unit/utBlenderImportExport.cpp | 51 +++++++++----------------- 2 files changed, 20 insertions(+), 37 deletions(-) diff --git a/code/AssetLib/Blender/BlenderScene.cpp b/code/AssetLib/Blender/BlenderScene.cpp index b503cb8ff..3a9a02fd0 100644 --- a/code/AssetLib/Blender/BlenderScene.cpp +++ b/code/AssetLib/Blender/BlenderScene.cpp @@ -839,9 +839,9 @@ void Structure::Convert( ReadField(dest.flag, "flag", db); ReadField(dest.active, "active", db); ReadField(dest.active_rnd, "active_rnd", db); - ReadField(dest.active_clone, "active_clone", db); - ReadField(dest.active_mask, "active_mask", db); - ReadField(dest.uid, "uid", db); + ReadField(dest.active_clone, "active_clone", db); + ReadField(dest.active_mask, "active_mask", db); + ReadField(dest.uid, "uid", db); ReadFieldArray(dest.name, "name", db); ReadCustomDataPtr(dest.data, dest.type, "*data", db); diff --git a/test/unit/utBlenderImportExport.cpp b/test/unit/utBlenderImportExport.cpp index 297098672..c9cce72b4 100644 --- a/test/unit/utBlenderImportExport.cpp +++ b/test/unit/utBlenderImportExport.cpp @@ -64,8 +64,7 @@ TEST_F(utBlenderImporterExporter, importBlenFromFileTest) { TEST(utBlenderImporter, import4cubes) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/4Cubes4Mats_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, import269_regress1) { @@ -77,22 +76,19 @@ TEST(utBlenderImporter, import269_regress1) { TEST(utBlenderImporter, importBlenderDefault248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/BlenderDefault_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importBlenderDefault250) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/BlenderDefault_250.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importBlenderDefault250Compressed) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/BlenderDefault_250_Compressed.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importBlenderDefault262) { @@ -123,92 +119,79 @@ TEST(utBlenderImporter, importBlenderDefault293) { TEST(utBlenderImporter, importCubeHierarchy_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/CubeHierarchy_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importHuman) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/HUMAN.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importMirroredCube_252) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/MirroredCube_252.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importNoisyTexturedCube_VoronoiGlob_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/NoisyTexturedCube_VoronoiGlob_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importSmoothVsSolidCube_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/SmoothVsSolidCube_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importSuzanne_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/Suzanne_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importSuzanneSubdiv_252) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/SuzanneSubdiv_252.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importTexturedCube_ImageGlob_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/TexturedCube_ImageGlob_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importTexturedPlane_ImageUv_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/TexturedPlane_ImageUv_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importTexturedPlane_ImageUvPacked_248) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/TexturedPlane_ImageUvPacked_248.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importTorusLightsCams_250_compressed) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/TorusLightsCams_250_compressed.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, import_yxa_1) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/BLEND/yxa_1.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importBob) { Assimp::Importer importer; const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_NONBSD_DIR "/BLEND/Bob.blend", aiProcess_ValidateDataStructure); - // FIXME: this is probably not right, loading this should succeed - ASSERT_EQ(nullptr, scene); + ASSERT_NE(nullptr, scene); } TEST(utBlenderImporter, importFleurOptonl) { From 2013ae114a089ce67a73d2108fbb0dc0317a4084 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Thu, 18 Aug 2022 17:41:07 +0200 Subject: [PATCH 10/17] [WIP] Use ai_Real to write correct accuracy - closes https://github.com/assimp/assimp/issues/4676 --- code/AssetLib/Assbin/AssbinFileWriter.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/code/AssetLib/Assbin/AssbinFileWriter.cpp b/code/AssetLib/Assbin/AssbinFileWriter.cpp index 1d16f179e..d1388cde6 100644 --- a/code/AssetLib/Assbin/AssbinFileWriter.cpp +++ b/code/AssetLib/Assbin/AssbinFileWriter.cpp @@ -130,7 +130,7 @@ inline size_t Write(IOStream *stream, const double &f) { // Serialize a vec3 template <> inline size_t Write(IOStream *stream, const aiVector3D &v) { - size_t t = Write(stream, v.x); + size_t t = Write(stream, v.x); t += Write(stream, v.y); t += Write(stream, v.z); @@ -141,7 +141,7 @@ inline size_t Write(IOStream *stream, const aiVector3D &v) { // Serialize a color value template <> inline size_t Write(IOStream *stream, const aiColor3D &v) { - size_t t = Write(stream, v.r); + size_t t = Write(stream, v.r); t += Write(stream, v.g); t += Write(stream, v.b); @@ -152,7 +152,7 @@ inline size_t Write(IOStream *stream, const aiColor3D &v) { // Serialize a color value template <> inline size_t Write(IOStream *stream, const aiColor4D &v) { - size_t t = Write(stream, v.r); + size_t t = Write(stream, v.r); t += Write(stream, v.g); t += Write(stream, v.b); t += Write(stream, v.a); @@ -164,7 +164,7 @@ inline size_t Write(IOStream *stream, const aiColor4D &v) { // Serialize a quaternion template <> inline size_t Write(IOStream *stream, const aiQuaternion &v) { - size_t t = Write(stream, v.w); + size_t t = Write(stream, v.w); t += Write(stream, v.x); t += Write(stream, v.y); t += Write(stream, v.z); @@ -190,7 +190,7 @@ template <> inline size_t Write(IOStream *stream, const aiMatrix4x4 &m) { for (unsigned int i = 0; i < 4; ++i) { for (unsigned int i2 = 0; i2 < 4; ++i2) { - Write(stream, m[i][i2]); + Write(stream, m[i][i2]); } } From 5f28c51c03dd07ce94d6c41427a61c503ef2567d Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 13:59:42 +0300 Subject: [PATCH 11/17] Apply clang-tidy modernize-loop-convert transformation --- code/AssetLib/SMD/SMDLoader.cpp | 42 +++++++++++++-------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 46fd968c8..f8b760f74 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -147,10 +147,8 @@ void SMDImporter::InternReadFile( const std::string& pFile, aiScene* scene, IOSy if (!asBones.empty()) { // Check whether all bones have been initialized - for (std::vector::const_iterator - i = asBones.begin(); - i != asBones.end();++i) { - if (!(*i).mName.length()) { + for (const auto &asBone : asBones) { + if (!asBone.mName.length()) { ASSIMP_LOG_WARN("SMD: Not all bones have been initialized"); break; } @@ -210,12 +208,10 @@ void SMDImporter::LogWarning(const char* msg) { void SMDImporter::FixTimeValues() { double dDelta = (double)iSmallestFrame; double dMax = 0.0f; - for (std::vector::iterator - iBone = asBones.begin(); - iBone != asBones.end();++iBone) { + for (auto &asBone : asBones) { for (std::vector::iterator - iKey = (*iBone).sAnim.asKeys.begin(); - iKey != (*iBone).sAnim.asKeys.end();++iKey) { + iKey = asBone.sAnim.asKeys.begin(); + iKey != asBone.sAnim.asKeys.end();++iKey) { (*iKey).dTime -= dDelta; dMax = std::max(dMax, (*iKey).dTime); } @@ -351,8 +347,7 @@ void SMDImporter::CreateOutputMeshes() { if (fSum) { fSum = 1 / fSum; - for (unsigned int iBone = 0;iBone < face.avVertices[iVert].aiBoneLinks.size();++iBone) { - TempWeightListEntry& pairval = face.avVertices[iVert].aiBoneLinks[iBone]; + for (auto &pairval : face.avVertices[iVert].aiBoneLinks) { if (pairval.first >= asBones.size()) { continue; } @@ -411,8 +406,7 @@ void SMDImporter::AddBoneChildren(aiNode* pcNode, uint32_t iParent) { ai_assert( nullptr == pcNode->mChildren); // first count ... - for (unsigned int i = 0; i < asBones.size();++i) { - SMD::Bone& bone = asBones[i]; + for (auto &bone : asBones) { if (bone.iParent == iParent) { ++pcNode->mNumChildren; } @@ -516,21 +510,21 @@ void SMDImporter::CreateOutputAnimation(int index, const std::string &name) { // now build valid keys unsigned int a = 0; - for (std::vector::const_iterator i = asBones.begin(); i != asBones.end(); ++i) { + for (const auto &asBone : asBones) { aiNodeAnim* p = pp[a] = new aiNodeAnim(); // copy the name of the bone - p->mNodeName.Set(i->mName); + p->mNodeName.Set(asBone.mName); - p->mNumRotationKeys = (unsigned int)(*i).sAnim.asKeys.size(); + p->mNumRotationKeys = (unsigned int)asBone.sAnim.asKeys.size(); if (p->mNumRotationKeys){ p->mNumPositionKeys = p->mNumRotationKeys; aiVectorKey* pVecKeys = p->mPositionKeys = new aiVectorKey[p->mNumRotationKeys]; aiQuatKey* pRotKeys = p->mRotationKeys = new aiQuatKey[p->mNumRotationKeys]; for (std::vector::const_iterator - qq = (*i).sAnim.asKeys.begin(); - qq != (*i).sAnim.asKeys.end(); ++qq) { + qq = asBone.sAnim.asKeys.begin(); + qq != asBone.sAnim.asKeys.end(); ++qq) { pRotKeys->mTime = pVecKeys->mTime = (*qq).dTime; // compute the rotation quaternion from the euler angles @@ -982,8 +976,8 @@ void SMDImporter::ParseTriangle(const char* szCurrent, const char** szCurrentOut SkipSpacesAndLineEnd(szCurrent,&szCurrent); // load three vertices - for (unsigned int iVert = 0; iVert < 3;++iVert) { - ParseVertex(szCurrent,&szCurrent, face.avVertices[iVert]); + for (auto &avVertex : face.avVertices) { + ParseVertex(szCurrent,&szCurrent, avVertex); } *szCurrentOut = szCurrent; } @@ -1080,13 +1074,11 @@ void SMDImporter::ParseVertex(const char* szCurrent, } vertex.aiBoneLinks.resize(iSize,std::pair(0,0.0f)); - for (std::vector >::iterator - i = vertex.aiBoneLinks.begin(); - i != vertex.aiBoneLinks.end();++i) { - if(!ParseUnsignedInt(szCurrent,&szCurrent,(*i).first)) { + for (auto &aiBoneLink : vertex.aiBoneLinks) { + if(!ParseUnsignedInt(szCurrent,&szCurrent,aiBoneLink.first)) { SMDI_PARSE_RETURN; } - if(!ParseFloat(szCurrent,&szCurrent,(*i).second)) { + if(!ParseFloat(szCurrent,&szCurrent,aiBoneLink.second)) { SMDI_PARSE_RETURN; } } From 712671e81a51e4f778c42b23362700a6fb4fb85e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:02:36 +0300 Subject: [PATCH 12/17] Apply modernize-loop-convert again --- code/AssetLib/SMD/SMDLoader.cpp | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index f8b760f74..97637b20a 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -209,11 +209,9 @@ void SMDImporter::FixTimeValues() { double dDelta = (double)iSmallestFrame; double dMax = 0.0f; for (auto &asBone : asBones) { - for (std::vector::iterator - iKey = asBone.sAnim.asKeys.begin(); - iKey != asBone.sAnim.asKeys.end();++iKey) { - (*iKey).dTime -= dDelta; - dMax = std::max(dMax, (*iKey).dTime); + for (auto &asKey : asBone.sAnim.asKeys) { + asKey.dTime -= dDelta; + dMax = std::max(dMax, asKey.dTime); } } dLengthOfAnim = dMax; @@ -522,15 +520,13 @@ void SMDImporter::CreateOutputAnimation(int index, const std::string &name) { aiVectorKey* pVecKeys = p->mPositionKeys = new aiVectorKey[p->mNumRotationKeys]; aiQuatKey* pRotKeys = p->mRotationKeys = new aiQuatKey[p->mNumRotationKeys]; - for (std::vector::const_iterator - qq = asBone.sAnim.asKeys.begin(); - qq != asBone.sAnim.asKeys.end(); ++qq) { - pRotKeys->mTime = pVecKeys->mTime = (*qq).dTime; + for (const auto &asKey : asBone.sAnim.asKeys) { + pRotKeys->mTime = pVecKeys->mTime = asKey.dTime; // compute the rotation quaternion from the euler angles // aiQuaternion: The order of the parameters is yzx? - pRotKeys->mValue = aiQuaternion((*qq).vRot.y, (*qq).vRot.z, (*qq).vRot.x); - pVecKeys->mValue = (*qq).vPos; + pRotKeys->mValue = aiQuaternion(asKey.vRot.y, asKey.vRot.z, asKey.vRot.x); + pVecKeys->mValue = asKey.vPos; ++pVecKeys; ++pRotKeys; } From 1ca44acebcdc7fae599eb1ae9e038ba4264b1707 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:03:48 +0300 Subject: [PATCH 13/17] Simplify a for loop --- code/AssetLib/SMD/SMDLoader.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 97637b20a..4ff6234c2 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -244,7 +244,7 @@ void SMDImporter::CreateOutputMeshes() { iNum = 0; for (std::vector::const_iterator iFace = asTriangles.begin(); - iFace != asTriangles.end();++iFace,++iNum) { + iFace != asTriangles.end(); ++iFace) { if (UINT_MAX == (*iFace).iTexture) { aaiFaces[(*iFace).iTexture].push_back( 0 ); } else if ((*iFace).iTexture >= aszTextures.size()) { @@ -253,6 +253,7 @@ void SMDImporter::CreateOutputMeshes() { } else { aaiFaces[(*iFace).iTexture].push_back(iNum); } + ++iNum; } // now create the output meshes From 795c0abcc865f139daf5e44cb89e54bf8b110673 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:10:00 +0300 Subject: [PATCH 14/17] Apply modernize-loop-convert to the simplified loop --- code/AssetLib/SMD/SMDLoader.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 4ff6234c2..1a860568a 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -242,16 +242,14 @@ void SMDImporter::CreateOutputMeshes() { // collect all faces iNum = 0; - for (std::vector::const_iterator - iFace = asTriangles.begin(); - iFace != asTriangles.end(); ++iFace) { - if (UINT_MAX == (*iFace).iTexture) { - aaiFaces[(*iFace).iTexture].push_back( 0 ); - } else if ((*iFace).iTexture >= aszTextures.size()) { + for (const auto &asTriangle : asTriangles) { + if (UINT_MAX == asTriangle.iTexture) { + aaiFaces[asTriangle.iTexture].push_back( 0 ); + } else if (asTriangle.iTexture >= aszTextures.size()) { ASSIMP_LOG_INFO("[SMD/VTA] Material index overflow in face"); - aaiFaces[(*iFace).iTexture].push_back((unsigned int)aszTextures.size()-1); + aaiFaces[asTriangle.iTexture].push_back((unsigned int)aszTextures.size()-1); } else { - aaiFaces[(*iFace).iTexture].push_back(iNum); + aaiFaces[asTriangle.iTexture].push_back(iNum); } ++iNum; } From 03397d42e2dc5fb29d4fc374086daebfe38be21a Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:15:06 +0300 Subject: [PATCH 15/17] Use unique_ptr for aaiFaces instead of explicit delete[] --- code/AssetLib/SMD/SMDLoader.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 1a860568a..2a43aab09 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -231,7 +231,7 @@ void SMDImporter::CreateOutputMeshes() { pScene->mMeshes = new aiMesh*[pScene->mNumMeshes]; typedef std::vector FaceList; - FaceList* aaiFaces = new FaceList[pScene->mNumMeshes]; + std::unique_ptr aaiFaces(new FaceList[pScene->mNumMeshes]); // approximate the space that will be required unsigned int iNum = (unsigned int)asTriangles.size() / pScene->mNumMeshes; @@ -392,7 +392,6 @@ void SMDImporter::CreateOutputMeshes() { } delete[] aaiBones; } - delete[] aaiFaces; } // ------------------------------------------------------------------------------------------------ From 68bc6a06b93ee2881ed4b62dbe7f29294743d33e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:17:56 +0300 Subject: [PATCH 16/17] Use unique_ptr for aaiBones instead of explicit delete[] --- code/AssetLib/SMD/SMDLoader.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 2a43aab09..78ff397b7 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -268,7 +268,7 @@ void SMDImporter::CreateOutputMeshes() { typedef std::pair TempWeightListEntry; typedef std::vector< TempWeightListEntry > TempBoneWeightList; - TempBoneWeightList* aaiBones = new TempBoneWeightList[asBones.size()](); + std::unique_ptr aaiBones(new TempBoneWeightList[asBones.size()]()); // try to reserve enough memory without wasting too much for (unsigned int iBone = 0; iBone < asBones.size();++iBone) { @@ -390,7 +390,6 @@ void SMDImporter::CreateOutputMeshes() { ++iNum; } } - delete[] aaiBones; } } From f890bc791fc055d25788f9fdeda5fc3e0097924a Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 23 Aug 2022 14:38:27 +0300 Subject: [PATCH 17/17] Fix out of bounds write --- code/AssetLib/SMD/SMDLoader.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/code/AssetLib/SMD/SMDLoader.cpp b/code/AssetLib/SMD/SMDLoader.cpp index 78ff397b7..686851c92 100644 --- a/code/AssetLib/SMD/SMDLoader.cpp +++ b/code/AssetLib/SMD/SMDLoader.cpp @@ -243,9 +243,7 @@ void SMDImporter::CreateOutputMeshes() { // collect all faces iNum = 0; for (const auto &asTriangle : asTriangles) { - if (UINT_MAX == asTriangle.iTexture) { - aaiFaces[asTriangle.iTexture].push_back( 0 ); - } else if (asTriangle.iTexture >= aszTextures.size()) { + if (asTriangle.iTexture >= aszTextures.size()) { ASSIMP_LOG_INFO("[SMD/VTA] Material index overflow in face"); aaiFaces[asTriangle.iTexture].push_back((unsigned int)aszTextures.size()-1); } else {