From a446d75250992ca4bc478d7f349b77c75ec8738f Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 22 Nov 2016 21:06:14 +0100 Subject: [PATCH] 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 ); +}