From c61146f52e0555fe803ded2bd6ca64e44d7a817c Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 21 Nov 2016 23:54:39 +0100 Subject: [PATCH 1/4] Add unittest fixure for aiMetadata. --- code/Subdivision.cpp | 15 +++---- include/assimp/metadata.h | 24 +++++------ test/CMakeLists.txt | 1 + test/unit/utMatrix4x4.cpp | 6 +-- test/unit/utMetadata.cpp | 88 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 25 deletions(-) create mode 100644 test/unit/utMetadata.cpp diff --git a/code/Subdivision.cpp b/code/Subdivision.cpp index 012c70047..0a67ad520 100644 --- a/code/Subdivision.cpp +++ b/code/Subdivision.cpp @@ -43,6 +43,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "SpatialSort.h" #include "ProcessHelper.h" #include "Vertex.h" +#include #include using namespace Assimp; @@ -55,9 +56,7 @@ void mydummy() {} // ------------------------------------------------------------------------------------------------ class CatmullClarkSubdivider : public Subdivider { - public: - void Subdivide (aiMesh* mesh, aiMesh*& out, unsigned int num, bool discard_input); void Subdivide (aiMesh** smesh, size_t nmesh, aiMesh** out, unsigned int num, bool discard_input); @@ -74,8 +73,6 @@ public: unsigned int ref; }; - - typedef std::vector UIntVector; typedef std::map EdgeMap; @@ -99,7 +96,6 @@ public: unsigned int eh_tmp0__, eh_tmp1__; private: - void InternSubdivide (const aiMesh* const * smesh, size_t nmesh,aiMesh** out, unsigned int num); }; @@ -128,7 +124,8 @@ void CatmullClarkSubdivider::Subdivide ( bool discard_input ) { - assert(mesh != out); + ai_assert(mesh != out); + Subdivide(&mesh,1,&out,num,discard_input); } @@ -142,12 +139,12 @@ void CatmullClarkSubdivider::Subdivide ( bool discard_input ) { - ai_assert(NULL != smesh && NULL != out); + ai_assert( NULL != smesh ); + ai_assert( NULL != out ); // course, both regions may not overlap - assert(smeshout+nmesh); + ai_assert(smeshout+nmesh); if (!num) { - // No subdivision at all. Need to copy all the meshes .. argh. if (discard_input) { for (size_t s = 0; s < nmesh; ++s) { diff --git a/include/assimp/metadata.h b/include/assimp/metadata.h index da9d4578e..8d6912f32 100644 --- a/include/assimp/metadata.h +++ b/include/assimp/metadata.h @@ -46,8 +46,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #ifndef AI_METADATA_H_INC #define AI_METADATA_H_INC -#include - #if defined(_MSC_VER) && (_MSC_VER <= 1500) #include "Compiler/pstdint.h" #else @@ -55,8 +53,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #endif - - // ------------------------------------------------------------------------------- /** * Enum used to distinguish data types @@ -77,8 +73,6 @@ typedef enum aiMetadataType #endif } aiMetadataType; - - // ------------------------------------------------------------------------------- /** * Metadata entry @@ -179,7 +173,6 @@ struct aiMetadata case FORCE_32BIT: #endif default: - assert(false); break; } } @@ -190,13 +183,13 @@ struct aiMetadata } } - - template - inline void Set( unsigned index, const std::string& key, const T& value ) + inline bool Set( unsigned index, const std::string& key, const T& value ) { // In range assertion - assert(index < mNumProperties); + if ( index >= mNumProperties ) { + return false; + } // Set metadata key mKeys[index] = key; @@ -205,13 +198,17 @@ struct aiMetadata mValues[index].mType = GetAiType(value); // Copy the given value to the dynamic storage mValues[index].mData = new T(value); + + return true; } template inline bool Get( unsigned index, T& value ) { // In range assertion - assert(index < mNumProperties); + if ( index >= mNumProperties ) { + return false; + } // Return false if the output data type does // not match the found value's data type @@ -226,7 +223,8 @@ struct aiMetadata } template - inline bool Get( const aiString& key, T& value ) + inline + bool Get( const aiString& key, T& value ) { // Search for the given key for (unsigned i=0; i + +using namespace ::Assimp; + +class utMetadata: public ::testing::Test { +protected: + aiMetadata *m_data; + + virtual void SetUp() { + m_data = new aiMetadata(); + } + + virtual void TearDown() { + delete m_data; + } + +}; + +TEST_F( utMetadata, creationTest ) { + bool ok( true ); + try { + aiMetadata data; + } catch ( ... ) { + ok = false; + } + EXPECT_TRUE( ok ); +} + +TEST_F( utMetadata, get_set_Test ) { + m_data->mNumProperties = 1; + m_data->mKeys = new aiString[ m_data->mNumProperties ](); + m_data->mValues = new aiMetadataEntry[ m_data->mNumProperties ](); + unsigned int index( 0 ); + bool success( false ); + const std::string key = "test"; + success = m_data->Set( index, key, aiString( std::string( "test" ) ) ); + EXPECT_TRUE( success ); + + aiString result; + success = m_data->Get( key, result ); + EXPECT_TRUE( success ); + + success = m_data->Get( "bla", result ); + EXPECT_FALSE( success ); +} From c5e3058ab35e64e0e2ded688b43b09efffc5cbb8 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 22 Nov 2016 10:22:15 +0100 Subject: [PATCH 2/4] Fix coverity findings. --- code/BaseImporter.cpp | 36 ++++++++++++++++--------------- code/X3DImporter_Node.hpp | 14 ++++++------ code/glTFAsset.h | 13 +++++++++-- code/glTFAsset.inl | 22 ++++++++++++++----- code/glTFExporter.cpp | 38 +++++++++++++++++---------------- test/unit/utObjImportExport.cpp | 2 ++ 6 files changed, 75 insertions(+), 50 deletions(-) diff --git a/code/BaseImporter.cpp b/code/BaseImporter.cpp index ca5264d3d..20321d0ad 100644 --- a/code/BaseImporter.cpp +++ b/code/BaseImporter.cpp @@ -56,7 +56,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include - using namespace Assimp; // ------------------------------------------------------------------------------------------------ @@ -461,29 +460,32 @@ void BaseImporter::TextFileToBuffer(IOStream* stream, } // ------------------------------------------------------------------------------------------------ -namespace Assimp -{ +namespace Assimp { // Represents an import request - struct LoadRequest - { + struct LoadRequest { LoadRequest(const std::string& _file, unsigned int _flags,const BatchLoader::PropertyMap* _map, unsigned int _id) - : file(_file), flags(_flags), refCnt(1),scene(NULL), loaded(false), id(_id) - { - if (_map) + : file(_file) + , flags(_flags) + , refCnt(1) + , scene(NULL) + , loaded(false) + , id(_id) { + if ( _map ) { map = *_map; + } } - const std::string file; - unsigned int flags; - unsigned int refCnt; - aiScene* scene; - bool loaded; - BatchLoader::PropertyMap map; - unsigned int id; - - bool operator== (const std::string& f) const { + bool operator== ( const std::string& f ) const { return file == f; } + + const std::string file; + unsigned int flags; + unsigned int refCnt; + aiScene *scene; + bool loaded; + BatchLoader::PropertyMap map; + unsigned int id; }; } diff --git a/code/X3DImporter_Node.hpp b/code/X3DImporter_Node.hpp index 87ee73866..f1f4bc57e 100644 --- a/code/X3DImporter_Node.hpp +++ b/code/X3DImporter_Node.hpp @@ -130,37 +130,35 @@ public: public: std::string ID;///< ID of the element. Can be empty. In X3D synonym for "ID" attribute. - CX3DImporter_NodeElement* Parent;///< Parrent element. If nullptr then this node is root. + CX3DImporter_NodeElement* Parent;///< Parent element. If nullptr then this node is root. std::list Child;///< Child elements. /***********************************************/ /****************** Functions ******************/ /***********************************************/ -private: + /// @brief The destructor, virtual. + virtual ~CX3DImporter_NodeElement() { + // empty + } - /// \fn CX3DImporter_NodeElement(const CX3DImporter_NodeElement& pNodeElement) +private: /// Disabled copy constructor. CX3DImporter_NodeElement(const CX3DImporter_NodeElement& pNodeElement); - /// \fn CX3DImporter_NodeElement& operator=(const CX3DImporter_NodeElement& pNodeElement) /// Disabled assign operator. CX3DImporter_NodeElement& operator=(const CX3DImporter_NodeElement& pNodeElement); - /// \fn CX3DImporter_NodeElement() /// Disabled default constructor. CX3DImporter_NodeElement(); protected: - - /// \fn CX3DImporter_NodeElement(const EType pType, CX3DImporter_NodeElement* pParent) /// In constructor inheritor must set element type. /// \param [in] pType - element type. /// \param [in] pParent - parent element. CX3DImporter_NodeElement(const EType pType, CX3DImporter_NodeElement* pParent) : Type(pType), Parent(pParent) {} - };// class IX3DImporter_NodeElement /// \class CX3DImporter_NodeElement_Group diff --git a/code/glTFAsset.h b/code/glTFAsset.h index 2e49b0d8f..b0f1b99f0 100644 --- a/code/glTFAsset.h +++ b/code/glTFAsset.h @@ -744,6 +744,10 @@ namespace glTF SExtension(const EType pType) : Type(pType) {} + + virtual ~SExtension() { + // empty + } }; #ifdef ASSIMP_IMPORTER_GLTF_USE_OPEN3DGC @@ -765,8 +769,13 @@ namespace glTF /// \fn SCompression_Open3DGC /// Constructor. SCompression_Open3DGC() - : SExtension(Compression_Open3DGC) - {} + : SExtension(Compression_Open3DGC) { + // empty + } + + virtual ~SCompression_Open3DGC() { + // empty + } }; #endif diff --git a/code/glTFAsset.inl b/code/glTFAsset.inl index a7afb3424..cf419e0d7 100644 --- a/code/glTFAsset.inl +++ b/code/glTFAsset.inl @@ -1005,6 +1005,12 @@ Ref buf = pAsset_Root.buffers.Get(pCompression_Open3DGC.Buffer); size_t tval = ifs.GetNIntAttribute(idx); switch( ifs.GetIntAttributeType( idx ) ) { + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_UNKOWN: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_INDEX: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_JOINT_ID: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_INDEX_BUFFER_ID: + break; + default: throw DeadlyImportError("GLTF: Open3DGC. Unsupported type of int attribute: " + to_string(ifs.GetIntAttributeType(idx))); } @@ -1049,10 +1055,14 @@ Ref buf = pAsset_Root.buffers.Get(pCompression_Open3DGC.Buffer); } } - for(size_t idx = 0, idx_end = size_intattr.size(); idx < idx_end; idx++) - { - switch(ifs.GetIntAttributeType(idx)) - { + for(size_t idx = 0, idx_end = size_intattr.size(); idx < idx_end; idx++) { + switch(ifs.GetIntAttributeType(idx)) { + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_UNKOWN: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_INDEX: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_JOINT_ID: + case o3dgc::O3DGC_IFS_INT_ATTRIBUTE_TYPE_INDEX_BUFFER_ID: + break; + // ifs.SetIntAttribute(idx, (long* const)(decoded_data + get_buf_offset(primitives[0].attributes.joint))); default: throw DeadlyImportError("GLTF: Open3DGC. Unsupported type of int attribute: " + to_string(ifs.GetIntAttributeType(idx))); @@ -1062,7 +1072,9 @@ Ref buf = pAsset_Root.buffers.Get(pCompression_Open3DGC.Buffer); // // Decode data // - if(decoder.DecodePayload(ifs, bstream) != o3dgc::O3DGC_OK) throw DeadlyImportError("GLTF: can not decode Open3DGC data."); + if ( decoder.DecodePayload( ifs, bstream ) != o3dgc::O3DGC_OK ) { + throw DeadlyImportError( "GLTF: can not decode Open3DGC data." ); + } // Set encoded region for "buffer". buf->EncodedRegion_Mark(pCompression_Open3DGC.Offset, pCompression_Open3DGC.Count, decoded_data, decoded_data_size, id); diff --git a/code/glTFExporter.cpp b/code/glTFExporter.cpp index f2203929c..4f02f4bb1 100644 --- a/code/glTFExporter.cpp +++ b/code/glTFExporter.cpp @@ -37,9 +37,6 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. ---------------------------------------------------------------------- */ - - - #ifndef ASSIMP_BUILD_NO_EXPORT #ifndef ASSIMP_BUILD_NO_GLTF_EXPORTER @@ -94,8 +91,6 @@ namespace Assimp { } // end of namespace Assimp - - glTFExporter::glTFExporter(const char* filename, IOSystem* pIOSystem, const aiScene* pScene, const ExportProperties* pProperties, bool isBinary) : mFilename(filename) @@ -419,17 +414,18 @@ Ref FindSkeletonRootJoint(Ref& skinRef) return parentNodeRef; } -void ExportSkin(Asset& mAsset, const aiMesh* aim, Ref& meshRef, Ref& bufferRef, Ref& skinRef, std::vector& inverseBindMatricesData) +void ExportSkin(Asset& mAsset, const aiMesh* aimesh, Ref& meshRef, Ref& bufferRef, Ref& skinRef, std::vector& inverseBindMatricesData) { - if (aim->mNumBones < 1) { + if (aimesh->mNumBones < 1) { return; } // Store the vertex joint and weight data. - vec4* vertexJointData = new vec4[aim->mNumVertices]; - vec4* vertexWeightData = new vec4[aim->mNumVertices]; - int* jointsPerVertex = new int[aim->mNumVertices]; - for (size_t i = 0; i < aim->mNumVertices; ++i) { + const size_t NumVerts( aimesh->mNumVertices ); + vec4* vertexJointData = new vec4[ NumVerts ]; + vec4* vertexWeightData = new vec4[ NumVerts ]; + int* jointsPerVertex = new int[ NumVerts ]; + for (size_t i = 0; i < NumVerts; ++i) { jointsPerVertex[i] = 0; for (size_t j = 0; j < 4; ++j) { vertexJointData[i][j] = 0; @@ -437,8 +433,8 @@ void ExportSkin(Asset& mAsset, const aiMesh* aim, Ref& meshRef, RefmNumBones; ++idx_bone) { - const aiBone* aib = aim->mBones[idx_bone]; + for (unsigned int idx_bone = 0; idx_bone < aimesh->mNumBones; ++idx_bone) { + const aiBone* aib = aimesh->mBones[idx_bone]; // aib->mName =====> skinRef->jointNames // Find the node with id = mName. @@ -470,7 +466,9 @@ void ExportSkin(Asset& mAsset, const aiMesh* aim, Ref& meshRef, RefmWeights[idx_weights].mWeight; // A vertex can only have at most four joint weights. Ignore all others. - if (jointsPerVertex[vertexId] > 3) { continue; } + if (jointsPerVertex[vertexId] > 3) { + continue; + } vertexJointData[vertexId][jointsPerVertex[vertexId]] = jointNamesIndex; vertexWeightData[vertexId][jointsPerVertex[vertexId]] = vertWeight; @@ -481,11 +479,15 @@ void ExportSkin(Asset& mAsset, const aiMesh* aim, Ref& meshRef, Refprimitives.back(); - Ref vertexJointAccessor = ExportData(mAsset, skinRef->id, bufferRef, aim->mNumVertices, vertexJointData, AttribType::VEC4, AttribType::VEC4, ComponentType_FLOAT); - if (vertexJointAccessor) p.attributes.joint.push_back(vertexJointAccessor); + Ref vertexJointAccessor = ExportData(mAsset, skinRef->id, bufferRef, aimesh->mNumVertices, vertexJointData, AttribType::VEC4, AttribType::VEC4, ComponentType_FLOAT); + if ( vertexJointAccessor ) { + p.attributes.joint.push_back( vertexJointAccessor ); + } - Ref vertexWeightAccessor = ExportData(mAsset, skinRef->id, bufferRef, aim->mNumVertices, vertexWeightData, AttribType::VEC4, AttribType::VEC4, ComponentType_FLOAT); - if (vertexWeightAccessor) p.attributes.weight.push_back(vertexWeightAccessor); + Ref vertexWeightAccessor = ExportData(mAsset, skinRef->id, bufferRef, aimesh->mNumVertices, vertexWeightData, AttribType::VEC4, AttribType::VEC4, ComponentType_FLOAT); + if ( vertexWeightAccessor ) { + p.attributes.weight.push_back( vertexWeightAccessor ); + } } void glTFExporter::ExportMeshes() diff --git a/test/unit/utObjImportExport.cpp b/test/unit/utObjImportExport.cpp index 44b0a6e11..70c4310e5 100644 --- a/test/unit/utObjImportExport.cpp +++ b/test/unit/utObjImportExport.cpp @@ -186,4 +186,6 @@ TEST_F( utObjImportExport, obj_import_test ) { SceneDiffer differ; EXPECT_TRUE( differ.isEqual( expected, scene ) ); differ.showReport(); + + m_im->FreeScene(); } From a446d75250992ca4bc478d7f349b77c75ec8738f Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 22 Nov 2016 21:06:14 +0100 Subject: [PATCH 3/4] Ue new alloc semantic when using aiMetadata + increase test coverage. --- code/AMFImporter_Postprocess.cpp | 20 +++--- code/FBXConverter.cpp | 23 +++---- code/IFCLoader.cpp | 14 ++-- code/SIBImporter.cpp | 7 +- code/SceneCombiner.cpp | 27 ++++---- code/X3DImporter_Postprocess.cpp | 15 ++-- include/assimp/metadata.h | 113 ++++++++++++++++++------------- test/unit/utMetadata.cpp | 101 +++++++++++++++++++++++++-- 8 files changed, 209 insertions(+), 111 deletions(-) diff --git a/code/AMFImporter_Postprocess.cpp b/code/AMFImporter_Postprocess.cpp index e88c2c08b..3cd6d77a4 100644 --- a/code/AMFImporter_Postprocess.cpp +++ b/code/AMFImporter_Postprocess.cpp @@ -336,25 +336,21 @@ auto texmap_is_equal = [](const CAMFImporter_NodeElement_TexMap* pTexMap1, const } while(pInputList.size() > 0); } -void AMFImporter::Postprocess_AddMetadata(const std::list& pMetadataList, aiNode& pSceneNode) const +void AMFImporter::Postprocess_AddMetadata(const std::list& metadataList, aiNode& sceneNode) const { - if(pMetadataList.size() > 0) + if ( !metadataList.empty() ) { - if(pSceneNode.mMetaData != nullptr) throw DeadlyImportError("Postprocess. MetaData member in node are not nullptr. Something went wrong."); + if(sceneNode.mMetaData != nullptr) throw DeadlyImportError("Postprocess. MetaData member in node are not nullptr. Something went wrong."); // copy collected metadata to output node. - pSceneNode.mMetaData = new aiMetadata(); - pSceneNode.mMetaData->mNumProperties = pMetadataList.size(); - pSceneNode.mMetaData->mKeys = new aiString[pSceneNode.mMetaData->mNumProperties]; - pSceneNode.mMetaData->mValues = new aiMetadataEntry[pSceneNode.mMetaData->mNumProperties]; + sceneNode.mMetaData = aiMetadata::Alloc( metadataList.size() ); + size_t meta_idx( 0 ); - size_t meta_idx = 0; - - for(const CAMFImporter_NodeElement_Metadata& metadata: pMetadataList) + for(const CAMFImporter_NodeElement_Metadata& metadata: metadataList) { - pSceneNode.mMetaData->Set(meta_idx++, metadata.Type, aiString(metadata.Value)); + sceneNode.mMetaData->Set(meta_idx++, metadata.Type, aiString(metadata.Value)); } - }// if(pMetadataList.size() > 0) + }// if(!metadataList.empty()) } void AMFImporter::Postprocess_BuildNodeAndObject(const CAMFImporter_NodeElement_Object& pNodeElement, std::list& pMeshList, aiNode** pSceneNode) diff --git a/code/FBXConverter.cpp b/code/FBXConverter.cpp index d266a6037..86e9b9df8 100644 --- a/code/FBXConverter.cpp +++ b/code/FBXConverter.cpp @@ -1075,10 +1075,7 @@ void Converter::SetupNodeMetadata( const Model& model, aiNode& nd ) // create metadata on node std::size_t numStaticMetaData = 2; - aiMetadata* data = new aiMetadata(); - data->mNumProperties = unparsedProperties.size() + numStaticMetaData; - data->mKeys = new aiString[ data->mNumProperties ](); - data->mValues = new aiMetadataEntry[ data->mNumProperties ](); + aiMetadata* data = aiMetadata::Alloc( unparsedProperties.size() + numStaticMetaData ); nd.mMetaData = data; int index = 0; @@ -1089,22 +1086,22 @@ void Converter::SetupNodeMetadata( const Model& model, aiNode& nd ) // add unparsed properties to the node's metadata for( const DirectPropertyMap::value_type& prop : unparsedProperties ) { - // Interpret the property as a concrete type - if ( const TypedProperty* interpreted = prop.second->As >() ) + if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, interpreted->Value() ); - else if ( const TypedProperty* interpreted = prop.second->As >() ) + } else if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, interpreted->Value() ); - else if ( const TypedProperty* interpreted = prop.second->As >() ) + } else if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, interpreted->Value() ); - else if ( const TypedProperty* interpreted = prop.second->As >() ) + } else if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, interpreted->Value() ); - else if ( const TypedProperty* interpreted = prop.second->As >() ) + } else if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, aiString( interpreted->Value() ) ); - else if ( const TypedProperty* interpreted = prop.second->As >() ) + } else if ( const TypedProperty* interpreted = prop.second->As >() ) { data->Set( index++, prop.first, interpreted->Value() ); - else - assert( false ); + } else { + ai_assert( false ); + } } } diff --git a/code/IFCLoader.cpp b/code/IFCLoader.cpp index 34977f5b0..d6a165f44 100644 --- a/code/IFCLoader.cpp +++ b/code/IFCLoader.cpp @@ -707,15 +707,11 @@ aiNode* ProcessSpatialStructure(aiNode* parent, const IfcProduct& el, Conversion } if (!properties.empty()) { - aiMetadata* data = new aiMetadata(); - data->mNumProperties = properties.size(); - data->mKeys = new aiString[data->mNumProperties](); - data->mValues = new aiMetadataEntry[data->mNumProperties](); - - unsigned int index = 0; - for(const Metadata::value_type& kv : properties) - data->Set(index++, kv.first, aiString(kv.second)); - + aiMetadata* data = aiMetadata::Alloc( properties.size() ); + unsigned int index( 0 ); + for ( const Metadata::value_type& kv : properties ) { + data->Set( index++, kv.first, aiString( kv.second ) ); + } nd->mMetaData = data; } } diff --git a/code/SIBImporter.cpp b/code/SIBImporter.cpp index 05afb5198..8e91ce7c3 100644 --- a/code/SIBImporter.cpp +++ b/code/SIBImporter.cpp @@ -901,11 +901,8 @@ void SIBImporter::InternReadFile(const std::string& pFile, // Mark instanced objects as being so. if (n >= firstInst) { - node->mMetaData = new aiMetadata; - node->mMetaData->mNumProperties = 1; - node->mMetaData->mKeys = new aiString[1]; - node->mMetaData->mValues = new aiMetadataEntry[1]; - node->mMetaData->Set(0, "IsInstance", true); + node->mMetaData = aiMetadata::Alloc( 1 ); + node->mMetaData->Set( 0, "IsInstance", true ); } } diff --git a/code/SceneCombiner.cpp b/code/SceneCombiner.cpp index e19741a9d..67497e0b8 100644 --- a/code/SceneCombiner.cpp +++ b/code/SceneCombiner.cpp @@ -43,11 +43,12 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. // possible as new fields are added to assimp structures. // ---------------------------------------------------------------------------- -/** @file Implements Assimp::SceneCombiner. This is a smart utility - * class that combines multiple scenes, meshes, ... into one. Currently - * these utilities are used by the IRR and LWS loaders and the - * OptimizeGraph step. - */ +/** + * @file Implements Assimp::SceneCombiner. This is a smart utility + * class that combines multiple scenes, meshes, ... into one. Currently + * these utilities are used by the IRR and LWS loaders and the + * OptimizeGraph step. + */ // ---------------------------------------------------------------------------- #include "SceneCombiner.h" #include "StringUtils.h" @@ -59,7 +60,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include #include "ScenePrivate.h" -namespace Assimp { +namespace Assimp { // ------------------------------------------------------------------------------------------------ // Add a prefix to a string @@ -198,8 +199,9 @@ void SceneCombiner::MergeScenes(aiScene** _dest,std::vector& src, void SceneCombiner::AttachToGraph (aiNode* attach, std::vector& srcList) { unsigned int cnt; - for (cnt = 0; cnt < attach->mNumChildren;++cnt) - AttachToGraph(attach->mChildren[cnt],srcList); + for ( cnt = 0; cnt < attach->mNumChildren; ++cnt ) { + AttachToGraph( attach->mChildren[ cnt ], srcList ); + } cnt = 0; for (std::vector::iterator it = srcList.begin(); @@ -1219,13 +1221,12 @@ void SceneCombiner::Copy (aiNode** _dest, const aiNode* src) } // ------------------------------------------------------------------------------------------------ -void SceneCombiner::Copy (aiMetadata** _dest, const aiMetadata* src) +void SceneCombiner::Copy(aiMetadata** _dest, const aiMetadata* src) { - ai_assert(NULL != _dest && NULL != src); + ai_assert( NULL != _dest ); + ai_assert( NULL != src); - aiMetadata* dest = *_dest = new aiMetadata(); - dest->mNumProperties = src->mNumProperties; - dest->mKeys = new aiString[src->mNumProperties]; + aiMetadata* dest = *_dest = aiMetadata::Alloc( src->mNumProperties ); std::copy(src->mKeys, src->mKeys + src->mNumProperties, dest->mKeys); dest->mValues = new aiMetadataEntry[src->mNumProperties]; diff --git a/code/X3DImporter_Postprocess.cpp b/code/X3DImporter_Postprocess.cpp index c3ac102dd..676816be9 100644 --- a/code/X3DImporter_Postprocess.cpp +++ b/code/X3DImporter_Postprocess.cpp @@ -758,15 +758,14 @@ void X3DImporter::Postprocess_CollectMetadata(const CX3DImporter_NodeElement& pN size_t meta_idx; PostprocessHelper_CollectMetadata(pNodeElement, meta_list);// find metadata in current node element. - if(meta_list.size() > 0) + if ( !meta_list.empty() ) { - if(pSceneNode.mMetaData != nullptr) throw DeadlyImportError("Postprocess. MetaData member in node are not nullptr. Something went wrong."); + if ( pSceneNode.mMetaData != nullptr ) { + throw DeadlyImportError( "Postprocess. MetaData member in node are not nullptr. Something went wrong." ); + } - // copy collected metadata to output node. - pSceneNode.mMetaData = new aiMetadata(); - pSceneNode.mMetaData->mNumProperties = meta_list.size(); - pSceneNode.mMetaData->mKeys = new aiString[pSceneNode.mMetaData->mNumProperties]; - pSceneNode.mMetaData->mValues = new aiMetadataEntry[pSceneNode.mMetaData->mNumProperties]; + // copy collected metadata to output node. + pSceneNode.mMetaData = aiMetadata::Alloc( meta_list.size() ); meta_idx = 0; for(std::list::const_iterator it = meta_list.begin(); it != meta_list.end(); it++, meta_idx++) { @@ -808,7 +807,7 @@ void X3DImporter::Postprocess_CollectMetadata(const CX3DImporter_NodeElement& pN throw DeadlyImportError("Postprocess. Unknown metadata type."); }// if((*it)->Type == CX3DImporter_NodeElement::ENET_Meta*) else }// for(std::list::const_iterator it = meta_list.begin(); it != meta_list.end(); it++) - }// if(meta_list.size() > 0) + }// if( !meta_list.empty() ) } }// namespace Assimp diff --git a/include/assimp/metadata.h b/include/assimp/metadata.h index 803426b3f..78b214581 100644 --- a/include/assimp/metadata.h +++ b/include/assimp/metadata.h @@ -47,10 +47,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #define AI_METADATA_H_INC #if defined(_MSC_VER) && (_MSC_VER <= 1500) -#include "Compiler/pstdint.h" +# include "Compiler/pstdint.h" #else -#include -#include +# include #endif // ------------------------------------------------------------------------------- @@ -58,8 +57,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * Enum used to distinguish data types */ // ------------------------------------------------------------------------------- -typedef enum aiMetadataType -{ +typedef enum aiMetadataType { AI_BOOL = 0, AI_INT32 = 1, AI_UINT64 = 2, @@ -80,8 +78,7 @@ typedef enum aiMetadataType * The type field uniquely identifies the underlying type of the data field */ // ------------------------------------------------------------------------------- -struct aiMetadataEntry -{ +struct aiMetadataEntry { aiMetadataType mType; void* mData; }; @@ -96,12 +93,12 @@ struct aiMetadataEntry */ // ------------------------------------------------------------------------------- -inline aiMetadataType GetAiType( bool ) { return AI_BOOL; } -inline aiMetadataType GetAiType( int32_t ) { return AI_INT32; } -inline aiMetadataType GetAiType( uint64_t ) { return AI_UINT64; } -inline aiMetadataType GetAiType( float ) { return AI_FLOAT; } -inline aiMetadataType GetAiType( double ) { return AI_DOUBLE; } -inline aiMetadataType GetAiType( aiString ) { return AI_AISTRING; } +inline aiMetadataType GetAiType( bool ) { return AI_BOOL; } +inline aiMetadataType GetAiType( int32_t ) { return AI_INT32; } +inline aiMetadataType GetAiType( uint64_t ) { return AI_UINT64; } +inline aiMetadataType GetAiType( float ) { return AI_FLOAT; } +inline aiMetadataType GetAiType( double ) { return AI_DOUBLE; } +inline aiMetadataType GetAiType( aiString ) { return AI_AISTRING; } inline aiMetadataType GetAiType( aiVector3D ) { return AI_AIVECTOR3D; } #endif // __cplusplus @@ -113,8 +110,7 @@ inline aiMetadataType GetAiType( aiVector3D ) { return AI_AIVECTOR3D; } * Metadata is a key-value store using string keys and values. */ // ------------------------------------------------------------------------------- -struct aiMetadata -{ +struct aiMetadata { /** Length of the mKeys and mValues arrays, respectively */ unsigned int mNumProperties; @@ -127,28 +123,28 @@ struct aiMetadata #ifdef __cplusplus - /** Constructor */ + /** + * @brief The default constructor, set all members to zero by default. + */ aiMetadata() - // set all members to zero by default - : mNumProperties(0) - , mKeys(NULL) - , mValues(NULL) - {} + : mNumProperties(0) + , mKeys(NULL) + , mValues(NULL) { + // empty + } - /** Destructor */ - ~aiMetadata() - { - delete[] mKeys; + /** + * @brief The destructor. + */ + ~aiMetadata() { + delete [] mKeys; mKeys = NULL; - if (mValues) - { + if (mValues) { // Delete each metadata entry - for (unsigned i=0; i(data); break; @@ -184,14 +180,37 @@ struct aiMetadata } } + /** + * @brief Allocates property fields + keys. + * @param numProperties Number of requested properties. + */ + static inline + aiMetadata *Alloc( unsigned int numProperties ) { + if ( 0 == numProperties ) { + return nullptr; + } + + aiMetadata *data = new aiMetadata; + data->mNumProperties = numProperties; + data->mKeys = new aiString[ data->mNumProperties ](); + data->mValues = new aiMetadataEntry[ data->mNumProperties ](); + + return data; + } + template - inline bool Set( unsigned index, const std::string& key, const T& value ) - { + inline + bool Set( unsigned index, const std::string& key, const T& value ) { // In range assertion if ( index >= mNumProperties ) { return false; } + // Ensure that we have a valid key. + if ( key.empty() ) { + return false; + } + // Set metadata key mKeys[index] = key; @@ -204,8 +223,8 @@ struct aiMetadata } template - inline bool Get( unsigned index, T& value ) - { + inline + bool Get( unsigned index, T& value ) { // In range assertion if ( index >= mNumProperties ) { return false; @@ -220,17 +239,19 @@ struct aiMetadata // Otherwise, output the found value and // return true value = *static_cast(mValues[index].mData); + return true; } template inline - bool Get( const aiString& key, T& value ) - { + bool Get( const aiString& key, T& value ) { // Search for the given key - for (unsigned i=0; i= mNumProperties) return false; + inline bool Get(size_t index, const aiString*& key, const aiMetadataEntry*& entry) { + if ( index >= mNumProperties ) { + return false; + } - pKey = &mKeys[pIndex]; - pEntry = &mValues[pIndex]; + key = &mKeys[index]; + entry = &mValues[index]; return true; } diff --git a/test/unit/utMetadata.cpp b/test/unit/utMetadata.cpp index e64dd4336..61bff3897 100644 --- a/test/unit/utMetadata.cpp +++ b/test/unit/utMetadata.cpp @@ -50,7 +50,7 @@ protected: aiMetadata *m_data; virtual void SetUp() { - m_data = new aiMetadata(); + m_data = nullptr; } virtual void TearDown() { @@ -69,20 +69,111 @@ TEST_F( utMetadata, creationTest ) { EXPECT_TRUE( ok ); } -TEST_F( utMetadata, get_set_Test ) { - m_data->mNumProperties = 1; - m_data->mKeys = new aiString[ m_data->mNumProperties ](); - m_data->mValues = new aiMetadataEntry[ m_data->mNumProperties ](); +TEST_F( utMetadata, allocTest ) { + aiMetadata *data = aiMetadata::Alloc( 0 ); + EXPECT_EQ( nullptr, data ); + + data = aiMetadata::Alloc( 1 ); + EXPECT_NE( nullptr, data ); + EXPECT_EQ( 1, data->mNumProperties ); + EXPECT_NE( nullptr, data->mKeys ); + EXPECT_NE( nullptr, data->mValues ); +} + +TEST_F( utMetadata, get_set_pod_Test ) { + m_data = aiMetadata::Alloc( 5 ); + + // int, 32 bit + unsigned int index( 0 ); + bool success( false ); + const std::string key_int = "test_int"; + success = m_data->Set( index, key_int, 1 ); + EXPECT_TRUE( success ); + success = m_data->Set( index + 10, key_int, 1 ); + EXPECT_FALSE( success ); + + // unsigned int, 64 bit + index++; + const std::string key_uint = "test_uint"; + success = m_data->Set( index, key_uint, 1UL ); + EXPECT_TRUE( success ); + uint64_t result_uint( 0 ); + success = m_data->Get( key_uint, result_uint ); + EXPECT_TRUE( success ); + EXPECT_EQ( 1UL, result_uint ); + + // bool + index++; + const std::string key_bool = "test_bool"; + success = m_data->Set( index, key_bool, true ); + EXPECT_TRUE( success ); + bool result_bool( false ); + success = m_data->Get( key_bool, result_bool ); + EXPECT_TRUE( success ); + EXPECT_EQ( true, result_bool ); + + // float + index++; + const std::string key_float = "test_float"; + float fVal = 2.0f; + success = m_data->Set( index, key_float, fVal ); + EXPECT_TRUE( success ); + float result_float( 0.0f ); + success = m_data->Get( key_float, result_float ); + EXPECT_TRUE( success ); + EXPECT_FLOAT_EQ( 2.0f, result_float ); + + // double + index++; + const std::string key_double = "test_double"; + double dVal = 3.0; + success = m_data->Set( index, key_double, dVal ); + EXPECT_TRUE( success ); + double result_double( 0.0 ); + success = m_data->Get( key_double, result_double ); + EXPECT_TRUE( success ); + EXPECT_DOUBLE_EQ( 3.0, result_double ); + + // error + int result; + success = m_data->Get( "bla", result ); + EXPECT_FALSE( success ); +} + +TEST_F( utMetadata, get_set_string_Test ) { + m_data = aiMetadata::Alloc( 1 ); + unsigned int index( 0 ); bool success( false ); const std::string key = "test"; success = m_data->Set( index, key, aiString( std::string( "test" ) ) ); EXPECT_TRUE( success ); + success = m_data->Set( index+10, key, aiString( std::string( "test" ) ) ); + EXPECT_FALSE( success ); + aiString result; success = m_data->Get( key, result ); + EXPECT_EQ( aiString( std::string( "test" ) ), result ); EXPECT_TRUE( success ); success = m_data->Get( "bla", result ); EXPECT_FALSE( success ); } + +TEST_F( utMetadata, get_set_aiVector3D_Test ) { + m_data = aiMetadata::Alloc( 1 ); + + unsigned int index( 0 ); + bool success( false ); + const std::string key = "test"; + aiVector3D vec( 1, 2, 3 ); + + success = m_data->Set( index, key, vec ); + EXPECT_TRUE( success ); + + aiVector3D result( 0, 0, 0 ); + success = m_data->Get( key, result ); + EXPECT_EQ( vec, result ); + EXPECT_TRUE( success ); +} From ba2f377b52252d2f3aa7b1e4692b4adaeca7bedb Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 22 Nov 2016 22:03:31 +0100 Subject: [PATCH 4/4] Fix coverity findings. --- code/ObjFileParser.cpp | 1 + code/X3DImporter.cpp | 10 ++++++++-- code/glTFExporter.cpp | 18 +++++++++++++++--- include/assimp/types.h | 2 +- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/code/ObjFileParser.cpp b/code/ObjFileParser.cpp index 27e4b27c4..7371b2d81 100644 --- a/code/ObjFileParser.cpp +++ b/code/ObjFileParser.cpp @@ -451,6 +451,7 @@ void ObjFileParser::getFace( aiPrimitiveType type ) { DefaultLogger::get()->error("Obj: Ignoring empty face"); // skip line and clean up m_DataIt = skipLine( m_DataIt, m_DataItEnd, m_uiLine ); + delete face; return; } diff --git a/code/X3DImporter.cpp b/code/X3DImporter.cpp index ad5e43d83..0e4eae0e3 100644 --- a/code/X3DImporter.cpp +++ b/code/X3DImporter.cpp @@ -1480,10 +1480,11 @@ void X3DImporter::ParseNode_Head() XML_CheckNode_MustBeEmpty(); // adding metadata from as MetaString from - CX3DImporter_NodeElement_MetaString* ms = new CX3DImporter_NodeElement_MetaString(NodeElement_Cur); + bool added( false ); + CX3DImporter_NodeElement_MetaString* ms = new CX3DImporter_NodeElement_MetaString(NodeElement_Cur); ms->Name = mReader->getAttributeValueSafe("name"); - // name can not be empty + // name must not be empty if(!ms->Name.empty()) { ms->Value.push_back(mReader->getAttributeValueSafe("content")); @@ -1491,8 +1492,13 @@ void X3DImporter::ParseNode_Head() if ( NodeElement_Cur != nullptr ) { NodeElement_Cur->Child.push_back( ms ); + added = true; } } + // if an error has occurred, release instance + if ( !added ) { + delete ms; + } }// if(XML_CheckNode_NameEqual("meta")) }// if(mReader->getNodeType() == irr::io::EXN_ELEMENT) else if(mReader->getNodeType() == irr::io::EXN_ELEMENT_END) diff --git a/code/glTFExporter.cpp b/code/glTFExporter.cpp index 4f02f4bb1..14d99c3e4 100644 --- a/code/glTFExporter.cpp +++ b/code/glTFExporter.cpp @@ -488,6 +488,9 @@ void ExportSkin(Asset& mAsset, const aiMesh* aimesh, Ref& meshRef, Ref tranAccessor = ExportData(mAsset, animId, buffer, nodeChannel->mNumPositionKeys, translationData, AttribType::VEC3, AttribType::VEC3, ComponentType_FLOAT); - if (tranAccessor) animRef->Parameters.translation = tranAccessor; + if ( tranAccessor ) { + animRef->Parameters.translation = tranAccessor; + } + delete[] translationData; } //------------------------------------------------------- @@ -879,7 +885,10 @@ inline void ExtractAnimationData(Asset& mAsset, std::string& animId, Ref scaleAccessor = ExportData(mAsset, animId, buffer, nodeChannel->mNumScalingKeys, scaleData, AttribType::VEC3, AttribType::VEC3, ComponentType_FLOAT); - if (scaleAccessor) animRef->Parameters.scale = scaleAccessor; + if ( scaleAccessor ) { + animRef->Parameters.scale = scaleAccessor; + } + delete[] scaleData; } //------------------------------------------------------- @@ -894,7 +903,10 @@ inline void ExtractAnimationData(Asset& mAsset, std::string& animId, Ref rotAccessor = ExportData(mAsset, animId, buffer, nodeChannel->mNumRotationKeys, rotationData, AttribType::VEC4, AttribType::VEC4, ComponentType_FLOAT); - if (rotAccessor) animRef->Parameters.rotation = rotAccessor; + if ( rotAccessor ) { + animRef->Parameters.rotation = rotAccessor; + } + delete[] rotationData; } } diff --git a/include/assimp/types.h b/include/assimp/types.h index d28a93b52..4a2e133e4 100644 --- a/include/assimp/types.h +++ b/include/assimp/types.h @@ -109,7 +109,7 @@ extern "C" { /** Maximum dimension for strings, ASSIMP strings are zero terminated. */ #ifdef __cplusplus -const size_t MAXLEN = 1024; +static const size_t MAXLEN = 1024; #else # define MAXLEN 1024 #endif