From fdd01bda8343b65981498d3c96d8d253290e4b99 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Fri, 11 Nov 2016 12:49:05 +0100 Subject: [PATCH] BatchImporter: make validation configurable and add unittest for class. --- code/BaseImporter.cpp | 98 +++++++++++++++++++++---------------- code/BaseImporter.h | 15 +++--- code/Importer.h | 39 +++++++++------ test/CMakeLists.txt | 2 + test/unit/TestIOSystem.h | 29 +++++++++++ test/unit/utBatchLoader.cpp | 78 +++++++++++++++++++++++++++++ test/unit/utIOSystem.cpp | 31 +++--------- test/unit/utImporter.cpp | 2 + 8 files changed, 207 insertions(+), 87 deletions(-) create mode 100644 test/unit/TestIOSystem.h create mode 100644 test/unit/utBatchLoader.cpp diff --git a/code/BaseImporter.cpp b/code/BaseImporter.cpp index aa615e592..ca5264d3d 100644 --- a/code/BaseImporter.cpp +++ b/code/BaseImporter.cpp @@ -481,7 +481,7 @@ namespace Assimp BatchLoader::PropertyMap map; unsigned int id; - bool operator== (const std::string& f) { + bool operator== (const std::string& f) const { return file == f; } }; @@ -489,13 +489,22 @@ namespace Assimp // ------------------------------------------------------------------------------------------------ // BatchLoader::pimpl data structure -struct Assimp::BatchData -{ - BatchData() - : pIOSystem() - , pImporter() - , next_id(0xffff) - {} +struct Assimp::BatchData { + BatchData( IOSystem* pIO, bool validate ) + : pIOSystem( pIO ) + , pImporter( nullptr ) + , next_id(0xffff) + , validate( validate ) { + ai_assert( NULL != pIO ); + + pImporter = new Importer(); + pImporter->SetIOHandler( pIO ); + } + + ~BatchData() { + pImporter->SetIOHandler( NULL ); /* get pointer back into our possession */ + delete pImporter; + } // IO system to be used for all imports IOSystem* pIOSystem; @@ -511,53 +520,59 @@ struct Assimp::BatchData // Id for next item unsigned int next_id; + + // Validation enabled state + bool validate; }; +typedef std::list::iterator LoadReqIt; + // ------------------------------------------------------------------------------------------------ -BatchLoader::BatchLoader(IOSystem* pIO) +BatchLoader::BatchLoader(IOSystem* pIO, bool validate ) { ai_assert(NULL != pIO); - data = new BatchData(); - data->pIOSystem = pIO; - - data->pImporter = new Importer(); - data->pImporter->SetIOHandler(data->pIOSystem); + m_data = new BatchData( pIO, validate ); } // ------------------------------------------------------------------------------------------------ BatchLoader::~BatchLoader() { - // delete all scenes wthat have not been polled by the user - for (std::list::iterator it = data->requests.begin();it != data->requests.end(); ++it) { - + // delete all scenes what have not been polled by the user + for ( LoadReqIt it = m_data->requests.begin();it != m_data->requests.end(); ++it) { delete (*it).scene; } - data->pImporter->SetIOHandler(NULL); /* get pointer back into our possession */ - delete data->pImporter; - delete data; + delete m_data; } +// ------------------------------------------------------------------------------------------------ +void BatchLoader::setValidation( bool enabled ) { + m_data->validate = enabled; +} // ------------------------------------------------------------------------------------------------ -unsigned int BatchLoader::AddLoadRequest (const std::string& file, +bool BatchLoader::getValidation() const { + return m_data->validate; +} + +// ------------------------------------------------------------------------------------------------ +unsigned int BatchLoader::AddLoadRequest(const std::string& file, unsigned int steps /*= 0*/, const PropertyMap* map /*= NULL*/) { ai_assert(!file.empty()); // check whether we have this loading request already - std::list::iterator it; - for (it = data->requests.begin();it != data->requests.end(); ++it) { - + for ( LoadReqIt it = m_data->requests.begin();it != m_data->requests.end(); ++it) { // Call IOSystem's path comparison function here - if (data->pIOSystem->ComparePaths((*it).file,file)) { - + if ( m_data->pIOSystem->ComparePaths((*it).file,file)) { if (map) { - if (!((*it).map == *map)) + if ( !( ( *it ).map == *map ) ) { continue; + } } - else if (!(*it).map.empty()) + else if ( !( *it ).map.empty() ) { continue; + } (*it).refCnt++; return (*it).id; @@ -565,20 +580,18 @@ unsigned int BatchLoader::AddLoadRequest (const std::string& file, } // no, we don't have it. So add it to the queue ... - data->requests.push_back(LoadRequest(file,steps,map,data->next_id)); - return data->next_id++; + m_data->requests.push_back(LoadRequest(file,steps,map, m_data->next_id)); + return m_data->next_id++; } // ------------------------------------------------------------------------------------------------ -aiScene* BatchLoader::GetImport (unsigned int which) +aiScene* BatchLoader::GetImport( unsigned int which ) { - for (std::list::iterator it = data->requests.begin();it != data->requests.end(); ++it) { - + for ( LoadReqIt it = m_data->requests.begin();it != m_data->requests.end(); ++it) { if ((*it).id == which && (*it).loaded) { - aiScene* sc = (*it).scene; if (!(--(*it).refCnt)) { - data->requests.erase(it); + m_data->requests.erase(it); } return sc; } @@ -590,14 +603,15 @@ aiScene* BatchLoader::GetImport (unsigned int which) void BatchLoader::LoadAll() { // no threaded implementation for the moment - for (std::list::iterator it = data->requests.begin();it != data->requests.end(); ++it) { + for ( LoadReqIt it = m_data->requests.begin();it != m_data->requests.end(); ++it) { // force validation in debug builds unsigned int pp = (*it).flags; -#ifdef ASSIMP_BUILD_DEBUG - pp |= aiProcess_ValidateDataStructure; -#endif + if ( m_data->validate ) { + pp |= aiProcess_ValidateDataStructure; + } + // setup config properties if necessary - ImporterPimpl* pimpl = data->pImporter->Pimpl(); + ImporterPimpl* pimpl = m_data->pImporter->Pimpl(); pimpl->mFloatProperties = (*it).map.floats; pimpl->mIntProperties = (*it).map.ints; pimpl->mStringProperties = (*it).map.strings; @@ -608,8 +622,8 @@ void BatchLoader::LoadAll() DefaultLogger::get()->info("%%% BEGIN EXTERNAL FILE %%%"); DefaultLogger::get()->info("File: " + (*it).file); } - data->pImporter->ReadFile((*it).file,pp); - (*it).scene = data->pImporter->GetOrphanedScene(); + m_data->pImporter->ReadFile((*it).file,pp); + (*it).scene = m_data->pImporter->GetOrphanedScene(); (*it).loaded = true; DefaultLogger::get()->info("%%% END EXTERNAL FILE %%%"); diff --git a/code/BaseImporter.h b/code/BaseImporter.h index 5c9ddfa5e..7c8932a3f 100644 --- a/code/BaseImporter.h +++ b/code/BaseImporter.h @@ -347,7 +347,12 @@ public: // static utilities static void ConvertUTF8toISO8859_1( std::string& data); - enum TextFileMode { ALLOW_EMPTY, FORBID_EMPTY }; + // ------------------------------------------------------------------- + /// @brief Enum to define, if empty files are ok or not. + enum TextFileMode { + ALLOW_EMPTY, + FORBID_EMPTY + }; // ------------------------------------------------------------------- /** Utility for text file loaders which copies the contents of the @@ -382,14 +387,10 @@ public: // static utilities } } - - protected: - - /** Error description in case there was one. */ + /// Error description in case there was one. std::string m_ErrorText; - - /** Currently set progress handler */ + /// Currently set progress handler. ProgressHandler* m_progress; }; diff --git a/code/Importer.h b/code/Importer.h index a35fb80d9..a0df1a6c1 100644 --- a/code/Importer.h +++ b/code/Importer.h @@ -39,6 +39,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ /** @file Importer.h mostly internal stuff for use by #Assimp::Importer */ +#pragma once #ifndef INCLUDED_AI_IMPORTER_H #define INCLUDED_AI_IMPORTER_H @@ -133,12 +134,11 @@ struct BatchData; * could, this has not yet been implemented at the moment). * * @note The class may not be used by more than one thread*/ -class BatchLoader +class ASSIMP_API BatchLoader { // friend of Importer public: - //! @cond never // ------------------------------------------------------------------- /** Wraps a full list of configuration properties for an importer. @@ -162,15 +162,29 @@ public: //! @endcond public: - - // ------------------------------------------------------------------- /** Construct a batch loader from a given IO system to be used - * to access external files */ - explicit BatchLoader(IOSystem* pIO); + * to access external files + */ + explicit BatchLoader(IOSystem* pIO, bool validate = false ); + + // ------------------------------------------------------------------- + /** The class destructor. + */ ~BatchLoader(); - + // ------------------------------------------------------------------- + /** Sets the validation step. True for enable validation during postprocess. + * @param enable True for validation. + */ + void setValidation( bool enabled ); + + // ------------------------------------------------------------------- + /** Returns the current validation step. + * @return The current validation step. + */ + bool getValidation() const; + // ------------------------------------------------------------------- /** Add a new file to the list of files to be loaded. * @param file File to be loaded @@ -185,7 +199,6 @@ public: const PropertyMap* map = NULL ); - // ------------------------------------------------------------------- /** Get an imported scene. * This polls the import from the internal request list. @@ -199,20 +212,16 @@ public: unsigned int which ); - // ------------------------------------------------------------------- /** Waits until all scenes have been loaded. This returns * immediately if no scenes are queued.*/ void LoadAll(); private: - // No need to have that in the public API ... - BatchData* data; + BatchData *m_data; }; -} +} // Namespace Assimp - - -#endif +#endif // INCLUDED_AI_IMPORTER_H diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f2584753d..b1428ce2d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -55,7 +55,9 @@ SOURCE_GROUP( unit FILES ) SET( TEST_SRCS + unit/TestIOSystem.h unit/AssimpAPITest.cpp + unit/utBatchLoader.cpp unit/utBlenderIntermediate.cpp unit/utBlendImportAreaLight.cpp unit/utBlendImportMaterials.cpp diff --git a/test/unit/TestIOSystem.h b/test/unit/TestIOSystem.h new file mode 100644 index 000000000..cf7962f98 --- /dev/null +++ b/test/unit/TestIOSystem.h @@ -0,0 +1,29 @@ +#pragma once + +#include "UnitTestPCH.h" + +#include + +using namespace std; +using namespace Assimp; + +static const string Sep = "/"; +class TestIOSystem : public IOSystem { +public: + TestIOSystem() : IOSystem() {} + virtual ~TestIOSystem() {} + virtual bool Exists( const char* ) const { + return true; + } + virtual char getOsSeparator() const { + return Sep[ 0 ]; + } + + virtual IOStream* Open( const char* pFile, const char* pMode = "rb" ) { + return NULL; + } + + virtual void Close( IOStream* pFile ) { + // empty + } +}; diff --git a/test/unit/utBatchLoader.cpp b/test/unit/utBatchLoader.cpp new file mode 100644 index 000000000..b3d74a0aa --- /dev/null +++ b/test/unit/utBatchLoader.cpp @@ -0,0 +1,78 @@ +/* +--------------------------------------------------------------------------- +Open Asset Import Library (assimp) +--------------------------------------------------------------------------- + +Copyright (c) 2006-2016, assimp team + +All rights reserved. + +Redistribution and use of this software in source and binary forms, +with or without modification, are permitted provided that the following +conditions are met: + +* Redistributions of source code must retain the above +copyright notice, this list of conditions and the +following disclaimer. + +* Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the +following disclaimer in the documentation and/or other +materials provided with the distribution. + +* Neither the name of the assimp team, nor the names of its +contributors may be used to endorse or promote products +derived from this software without specific prior +written permission of the assimp team. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +--------------------------------------------------------------------------- +*/ +#include "UnitTestPCH.h" +#include "Importer.h" +#include "TestIOSystem.h" + +using namespace ::Assimp; + +class BatchLoaderTest : public ::testing::Test { +public: + virtual void SetUp() { + m_io = new TestIOSystem(); + } + + virtual void TearDown() { + delete m_io; + } + +protected: + TestIOSystem* m_io; +}; + +TEST_F( BatchLoaderTest, createTest ) { + bool ok( true ); + try { + BatchLoader loader( m_io ); + } catch ( ... ) { + ok = false; + } +} + +TEST_F( BatchLoaderTest, validateAccessTest ) { + BatchLoader loader1( m_io ); + EXPECT_FALSE( loader1.getValidation() ); + loader1.setValidation( true ); + EXPECT_TRUE( loader1.getValidation() ); + + BatchLoader loader2( m_io, true ); + EXPECT_TRUE( loader2.getValidation() ); +} diff --git a/test/unit/utIOSystem.cpp b/test/unit/utIOSystem.cpp index b091e8c36..dfae55320 100644 --- a/test/unit/utIOSystem.cpp +++ b/test/unit/utIOSystem.cpp @@ -39,37 +39,22 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. --------------------------------------------------------------------------- */ #include "UnitTestPCH.h" +#include "TestIOSystem.h" #include using namespace std; using namespace Assimp; -static const string Sep = "/"; -class TestIOSystem : public IOSystem { -public: - TestIOSystem() : IOSystem() {} - virtual ~TestIOSystem() {} - virtual bool Exists( const char* ) const { - return true; - } - virtual char getOsSeparator() const { - return Sep[ 0 ]; - } - - virtual IOStream* Open(const char* pFile, const char* pMode = "rb") { - return NULL; - } - - virtual void Close( IOStream* pFile) { - // empty - } -}; - class IOSystemTest : public ::testing::Test { public: - virtual void SetUp() { pImp = new TestIOSystem(); } - virtual void TearDown() { delete pImp; } + virtual void SetUp() { + pImp = new TestIOSystem(); + } + + virtual void TearDown() { + delete pImp; + } protected: TestIOSystem* pImp; diff --git a/test/unit/utImporter.cpp b/test/unit/utImporter.cpp index db71f8206..6e2d03a63 100644 --- a/test/unit/utImporter.cpp +++ b/test/unit/utImporter.cpp @@ -270,3 +270,5 @@ TEST_F(ImporterTest, testMultipleReads) EXPECT_TRUE(pImp->ReadFile(ASSIMP_TEST_MODELS_DIR "/X/BCN_Epileptic.X",flags)); //EXPECT_TRUE(pImp->ReadFile(ASSIMP_TEST_MODELS_DIR "/X/dwarf.x",flags)); # is in nonbsd } + +// ------------------------------------------------------------------------------------------------