From d284d107e73cd3ae80733e3351c213025365d74c Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 18:43:51 +0200 Subject: [PATCH 01/10] XGLLoader: Fix a memory leak --- code/XGLLoader.cpp | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/code/XGLLoader.cpp b/code/XGLLoader.cpp index 2d7a40362..a9fd6a5ab 100644 --- a/code/XGLLoader.cpp +++ b/code/XGLLoader.cpp @@ -72,17 +72,6 @@ using namespace irr::io; #endif -// scopeguard for a malloc'ed buffer -struct free_it -{ - free_it(void* free) : free(free) {} - ~free_it() { - ::free(this->free); - } - - void* free; -}; - namespace Assimp { // this has to be in here because LogFunctions is in ::Assimp template<> const char* LogFunctions::Prefix() { @@ -155,8 +144,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { #ifndef ASSIMP_BUILD_NO_COMPRESSED_XGL - Bytef* dest = NULL; - free_it free_it_really(dest); + std::vector uncompressed; #endif m_scene = pScene; @@ -192,6 +180,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, size_t total = 0l; + // TODO: be smarter about this, decompress directly into heap buffer // and decompress the data .... do 1k chunks in the hope that we won't kill the stack #define MYBLOCK 1024 Bytef block[MYBLOCK]; @@ -206,8 +195,8 @@ void XGLImporter::InternReadFile( const std::string& pFile, } const size_t have = MYBLOCK - zstream.avail_out; total += have; - dest = reinterpret_cast( realloc(dest,total) ); - memcpy(dest + total - have,block,have); + uncompressed.resize(total); + memcpy(uncompressed.data() + total - have,block,have); } while (ret != Z_STREAM_END); @@ -215,7 +204,7 @@ void XGLImporter::InternReadFile( const std::string& pFile, inflateEnd(&zstream); // replace the input stream with a memory stream - stream.reset(new MemoryIOStream(reinterpret_cast(dest),total)); + stream.reset(new MemoryIOStream(reinterpret_cast(uncompressed.data()),total)); #endif } From c42dd9104cb7a4faea288a634cd2f0c170366308 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 18:52:23 +0200 Subject: [PATCH 02/10] BlenderLoader: Fix memory leak --- code/BlenderLoader.cpp | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/code/BlenderLoader.cpp b/code/BlenderLoader.cpp index 482f9ae5c..d4d6473c5 100644 --- a/code/BlenderLoader.cpp +++ b/code/BlenderLoader.cpp @@ -154,14 +154,6 @@ void BlenderImporter::SetupProperties(const Importer* /*pImp*/) // nothing to be done for the moment } -struct free_it { - free_it(void* free) : free(free) {} - ~free_it() { - ::free(this->free); - } - - void* free; -}; // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. @@ -169,8 +161,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) { #ifndef ASSIMP_BUILD_NO_COMPRESSED_BLEND - Bytef* dest = NULL; - free_it free_it_really(dest); + std::vector uncompressed; #endif @@ -218,6 +209,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, size_t total = 0l; + // TODO: be smarter about this, decompress directly into heap buffer // and decompress the data .... do 1k chunks in the hope that we won't kill the stack #define MYBLOCK 1024 Bytef block[MYBLOCK]; @@ -232,8 +224,8 @@ void BlenderImporter::InternReadFile( const std::string& pFile, } const size_t have = MYBLOCK - zstream.avail_out; total += have; - dest = reinterpret_cast( realloc(dest,total) ); - memcpy(dest + total - have,block,have); + uncompressed.resize(total); + memcpy(uncompressed.data() + total - have,block,have); } while (ret != Z_STREAM_END); @@ -241,7 +233,7 @@ void BlenderImporter::InternReadFile( const std::string& pFile, inflateEnd(&zstream); // replace the input stream with a memory stream - stream.reset(new MemoryIOStream(reinterpret_cast(dest),total)); + stream.reset(new MemoryIOStream(reinterpret_cast(uncompressed.data()),total)); // .. and retry stream->Read(magic,7,1); From 880be5403f466541335cce1b6fcef26b18cc9f15 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:03:47 +0200 Subject: [PATCH 03/10] OpenGEX: Replace raw pointer with vector to fix a memory leak --- code/OpenGEXImporter.cpp | 12 ++++-------- code/OpenGEXImporter.h | 3 +-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index 37c765f4c..a2844ccdf 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -221,9 +221,7 @@ static void propId2StdString( Property *prop, std::string &name, std::string &ke //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::VertexContainer() -: m_numVerts( 0 ) -, m_vertices( nullptr ) -, m_numColors( 0 ) +: m_numColors( 0 ) , m_colors( nullptr ) , m_numNormals( 0 ) , m_normals( nullptr ) @@ -234,7 +232,6 @@ OpenGEXImporter::VertexContainer::VertexContainer() //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::~VertexContainer() { - delete[] m_vertices; delete[] m_colors; delete[] m_normals; @@ -857,9 +854,8 @@ void OpenGEXImporter::handleVertexArrayNode( ODDLParser::DDLNode *node, aiScene const size_t numItems( countDataArrayListItems( vaList ) ); if( Position == attribType ) { - m_currentVertices.m_numVerts = numItems; - m_currentVertices.m_vertices = new aiVector3D[ numItems ]; - copyVectorArray( numItems, vaList, m_currentVertices.m_vertices ); + m_currentVertices.m_vertices.resize( numItems ); + copyVectorArray( numItems, vaList, m_currentVertices.m_vertices.data() ); } else if ( Color == attribType ) { m_currentVertices.m_numColors = numItems; m_currentVertices.m_colors = new aiColor4D[ numItems ]; @@ -922,7 +918,7 @@ void OpenGEXImporter::handleIndexArrayNode( ODDLParser::DDLNode *node, aiScene * Value *next( vaList->m_dataList ); for( size_t indices = 0; indices < current.mNumIndices; indices++ ) { const int idx( next->getUnsignedInt32() ); - ai_assert( static_cast( idx ) <= m_currentVertices.m_numVerts ); + ai_assert( static_cast( idx ) <= m_currentVertices.m_vertices.size() ); ai_assert( index < m_currentMesh->mNumVertices ); aiVector3D &pos = ( m_currentVertices.m_vertices[ idx ] ); m_currentMesh->mVertices[ index ].Set( pos.x, pos.y, pos.z ); diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index bbbe3678d..eedb84375 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -144,8 +144,7 @@ protected: private: struct VertexContainer { - size_t m_numVerts; - aiVector3D *m_vertices; + std::vector m_vertices; size_t m_numColors; aiColor4D *m_colors; size_t m_numNormals; From 1aed63afb73a83fbfd2bb246f54f2110471aa724 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:13:54 +0200 Subject: [PATCH 04/10] OpenGEX: Replace another raw pointer with vector to fix a memory leak --- code/OpenGEXImporter.cpp | 10 +++------- code/OpenGEXImporter.h | 3 +-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index a2844ccdf..dac8299c5 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -223,8 +223,6 @@ static void propId2StdString( Property *prop, std::string &name, std::string &ke OpenGEXImporter::VertexContainer::VertexContainer() : m_numColors( 0 ) , m_colors( nullptr ) -, m_numNormals( 0 ) -, m_normals( nullptr ) , m_numUVComps() , m_textureCoords() { // empty @@ -233,7 +231,6 @@ OpenGEXImporter::VertexContainer::VertexContainer() //------------------------------------------------------------------------------------------------ OpenGEXImporter::VertexContainer::~VertexContainer() { delete[] m_colors; - delete[] m_normals; for(auto &texcoords : m_textureCoords) { delete [] texcoords; @@ -861,9 +858,8 @@ void OpenGEXImporter::handleVertexArrayNode( ODDLParser::DDLNode *node, aiScene m_currentVertices.m_colors = new aiColor4D[ numItems ]; copyColor4DArray( numItems, vaList, m_currentVertices.m_colors ); } else if( Normal == attribType ) { - m_currentVertices.m_numNormals = numItems; - m_currentVertices.m_normals = new aiVector3D[ numItems ]; - copyVectorArray( numItems, vaList, m_currentVertices.m_normals ); + m_currentVertices.m_normals.resize( numItems ); + copyVectorArray( numItems, vaList, m_currentVertices.m_normals.data() ); } else if( TexCoord == attribType ) { m_currentVertices.m_numUVComps[ 0 ] = numItems; m_currentVertices.m_textureCoords[ 0 ] = new aiVector3D[ numItems ]; @@ -900,7 +896,7 @@ void OpenGEXImporter::handleIndexArrayNode( ODDLParser::DDLNode *node, aiScene * hasColors = true; } bool hasNormalCoords( false ); - if ( m_currentVertices.m_numNormals > 0 ) { + if ( !m_currentVertices.m_normals.empty() ) { m_currentMesh->mNormals = new aiVector3D[ m_currentMesh->mNumVertices ]; hasNormalCoords = true; } diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index eedb84375..ad331d8e6 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -147,8 +147,7 @@ private: std::vector m_vertices; size_t m_numColors; aiColor4D *m_colors; - size_t m_numNormals; - aiVector3D *m_normals; + std::vector m_normals; size_t m_numUVComps[ AI_MAX_NUMBER_OF_TEXTURECOORDS ]; aiVector3D *m_textureCoords[ AI_MAX_NUMBER_OF_TEXTURECOORDS ]; From 9344074a04357fb0e8fd2537557f148dbc15ce8f Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:22:32 +0200 Subject: [PATCH 05/10] MDLLoader: Replace raw pointer with vector to fix a memory leak --- code/MDLFileData.h | 4 ++-- code/MDLLoader.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/code/MDLFileData.h b/code/MDLFileData.h index 56f686280..ba732add0 100644 --- a/code/MDLFileData.h +++ b/code/MDLFileData.h @@ -844,11 +844,11 @@ struct IntGroupInfo_MDL7 struct IntGroupData_MDL7 { IntGroupData_MDL7() - : pcFaces(NULL), bNeed2UV(false) + : bNeed2UV(false) {} //! Array of faces that belong to the group - MDL::IntFace_MDL7* pcFaces; + std::vector pcFaces; //! Array of vertex positions std::vector vPositions; diff --git a/code/MDLLoader.cpp b/code/MDLLoader.cpp index 5dd471cf5..3f2bb084b 100644 --- a/code/MDLLoader.cpp +++ b/code/MDLLoader.cpp @@ -1502,7 +1502,7 @@ void MDLImporter::InternReadFile_3DGS_MDL7( ) groupData.bNeed2UV = true; } } - groupData.pcFaces = new MDL::IntFace_MDL7[groupInfo.pcGroup->numtris]; + groupData.pcFaces.resize(groupInfo.pcGroup->numtris); // read all faces into the preallocated arrays ReadFaces_3DGS_MDL7(groupInfo, groupData); From 3b68ffe363401cafd8da701a4a757bc88845e164 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:32:34 +0200 Subject: [PATCH 06/10] LWO: Use C++11 auto for easier refactoring --- code/LWOLoader.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/code/LWOLoader.cpp b/code/LWOLoader.cpp index 38e330b8f..10cfd67bd 100644 --- a/code/LWOLoader.cpp +++ b/code/LWOLoader.cpp @@ -584,7 +584,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Set parent of all children, inserting pivots //std::cout << "Set parent of all children" << std::endl; std::map mapPivot; - for (std::map::iterator itapcNodes = apcNodes.begin(); itapcNodes != apcNodes.end(); ++itapcNodes) { + for (auto itapcNodes = apcNodes.begin(); itapcNodes != apcNodes.end(); ++itapcNodes) { //Get the parent index LWO::Layer* nodeLayer = (LWO::Layer*)(itapcNodes->second->mParent); @@ -615,14 +615,14 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Merge pivot map into node map //std::cout << "Merge pivot map into node map" << std::endl; - for (std::map::iterator itMapPivot = mapPivot.begin(); itMapPivot != mapPivot.end(); ++itMapPivot) { + for (auto itMapPivot = mapPivot.begin(); itMapPivot != mapPivot.end(); ++itMapPivot) { apcNodes[itMapPivot->first] = itMapPivot->second; } //Set children of all parents apcNodes[-1] = root; - for (std::map::iterator itMapParentNodes = apcNodes.begin(); itMapParentNodes != apcNodes.end(); ++itMapParentNodes) { - for (std::map::iterator itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { + for (auto itMapParentNodes = apcNodes.begin(); itMapParentNodes != apcNodes.end(); ++itMapParentNodes) { + for (auto itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { if ((itMapParentNodes->first != itMapChildNodes->first) && (itMapParentNodes->second == itMapChildNodes->second->mParent)) { ++(itMapParentNodes->second->mNumChildren); } @@ -630,7 +630,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) if (itMapParentNodes->second->mNumChildren) { itMapParentNodes->second->mChildren = new aiNode* [ itMapParentNodes->second->mNumChildren ]; uint16_t p = 0; - for (std::map::iterator itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { + for (auto itMapChildNodes = apcNodes.begin(); itMapChildNodes != apcNodes.end(); ++itMapChildNodes) { if ((itMapParentNodes->first != itMapChildNodes->first) && (itMapParentNodes->second == itMapChildNodes->second->mParent)) { itMapParentNodes->second->mChildren[p++] = itMapChildNodes->second; } From ef891fb850af5edb86869a1e175ce0e07e13ac02 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 19:36:27 +0200 Subject: [PATCH 07/10] LWO: Move some assignments to make it clearer when the thing should be moved --- code/LWOLoader.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/code/LWOLoader.cpp b/code/LWOLoader.cpp index 10cfd67bd..e908ea178 100644 --- a/code/LWOLoader.cpp +++ b/code/LWOLoader.cpp @@ -432,7 +432,6 @@ void LWOImporter::InternReadFile( const std::string& pFile, unsigned int num = static_cast(apcMeshes.size() - meshStart); if (layer.mName != "" || num > 0) { aiNode* pcNode = new aiNode(); - apcNodes[layer.mIndex] = pcNode; pcNode->mName.Set(layer.mName); pcNode->mParent = (aiNode*)&layer; pcNode->mNumMeshes = num; @@ -442,6 +441,7 @@ void LWOImporter::InternReadFile( const std::string& pFile, for (unsigned int p = 0; p < pcNode->mNumMeshes;++p) pcNode->mMeshes[p] = p + meshStart; } + apcNodes[layer.mIndex] = pcNode; } } @@ -593,7 +593,6 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) //Create pivot node, store it into the pivot map, and set the parent as the pivot aiNode* pivotNode = new aiNode(); pivotNode->mName.Set("Pivot-"+std::string(itapcNodes->second->mName.data)); - mapPivot[-(itapcNodes->first+2)] = pivotNode; itapcNodes->second->mParent = pivotNode; //Look for the parent node to attach the pivot to @@ -611,6 +610,7 @@ void LWOImporter::GenerateNodeGraph(std::map& apcNodes) pivotNode->mTransformation.a4 = nodeLayer->mPivot.x; pivotNode->mTransformation.b4 = nodeLayer->mPivot.y; pivotNode->mTransformation.c4 = nodeLayer->mPivot.z; + mapPivot[-(itapcNodes->first+2)] = pivotNode; } //Merge pivot map into node map From aa434b956648af35917c9b18933b066eb16171dd Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:05:02 +0200 Subject: [PATCH 08/10] OpenGEX: Add comment about pointer ownership --- code/OpenGEXImporter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index ad331d8e6..5b87d5ce5 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -192,7 +192,7 @@ private: MetricInfo m_metrics[ MetricInfo::Max ]; aiNode *m_currentNode; VertexContainer m_currentVertices; - aiMesh *m_currentMesh; + aiMesh *m_currentMesh; // not owned, target is owned by m_meshCache aiMaterial *m_currentMaterial; aiLight *m_currentLight; aiCamera *m_currentCamera; From 5ce9ece0ccc110bf858bd21a19b10986236bf63e Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:08:49 +0200 Subject: [PATCH 09/10] OpenGEX: Replace std::copy with explicit loop --- code/OpenGEXImporter.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index dac8299c5..a0bef75a9 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -1137,7 +1137,9 @@ void OpenGEXImporter::copyMeshes( aiScene *pScene ) { pScene->mNumMeshes = static_cast(m_meshCache.size()); pScene->mMeshes = new aiMesh*[ pScene->mNumMeshes ]; - std::copy( m_meshCache.begin(), m_meshCache.end(), pScene->mMeshes ); + for (unsigned int i = 0; i < pScene->mNumMeshes; i++) { + pScene->mMeshes[i] = m_meshCache[i]; + } } //------------------------------------------------------------------------------------------------ From 17b26c91e254a17091f96ab861569f10b408a9c8 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Tue, 6 Feb 2018 20:20:16 +0200 Subject: [PATCH 10/10] OpenGEX: Use std::unique_ptr to fix some memory leaks --- code/OpenGEXImporter.cpp | 5 +++-- code/OpenGEXImporter.h | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/code/OpenGEXImporter.cpp b/code/OpenGEXImporter.cpp index a0bef75a9..43c92c4f5 100644 --- a/code/OpenGEXImporter.cpp +++ b/code/OpenGEXImporter.cpp @@ -691,7 +691,8 @@ void OpenGEXImporter::handleTransformNode( ODDLParser::DDLNode *node, aiScene * void OpenGEXImporter::handleMeshNode( ODDLParser::DDLNode *node, aiScene *pScene ) { m_currentMesh = new aiMesh; const size_t meshidx( m_meshCache.size() ); - m_meshCache.push_back( m_currentMesh ); + // ownership is transfered but a reference remains in m_currentMesh + m_meshCache.emplace_back( m_currentMesh ); Property *prop = node->getProperties(); if( nullptr != prop ) { @@ -1138,7 +1139,7 @@ void OpenGEXImporter::copyMeshes( aiScene *pScene ) { pScene->mNumMeshes = static_cast(m_meshCache.size()); pScene->mMeshes = new aiMesh*[ pScene->mNumMeshes ]; for (unsigned int i = 0; i < pScene->mNumMeshes; i++) { - pScene->mMeshes[i] = m_meshCache[i]; + pScene->mMeshes[i] = m_meshCache[i].release(); } } diff --git a/code/OpenGEXImporter.h b/code/OpenGEXImporter.h index 5b87d5ce5..8e86a4aa8 100644 --- a/code/OpenGEXImporter.h +++ b/code/OpenGEXImporter.h @@ -183,7 +183,7 @@ private: typedef std::map > NodeChildMap; NodeChildMap m_nodeChildMap; - std::vector m_meshCache; + std::vector > m_meshCache; typedef std::map ReferenceMap; std::map m_mesh2refMap; std::map m_material2refMap;