Page 1 of 1

[BUG] Manually created resources leak

Posted: Tue Mar 08, 2011 7:38 am
I discovered that manually created resources leak their memory when they are destroyed unless you manually call load() before hand. An example of creating a manual object is:

Code: Select all

Ogre::MeshPtr mesh = pMeshManager->createManual( pTin->m_Name + "_12da_mesh" , "12DConversionGroup" );
Calling load() sets mLoadingState to LOADSTATE_LOADED which prevents Resource::unload() from simply returning. If it doesn't return then unloadImpl() will get called and will free the memory correctly.

I have simply made my code call load() as a work around which solved my memory leak problem, however I suggest that a fix is needed to prevent this work around being necessary.

Off the top of my head I see two possible solutions. One is to set mLoadingState to LOADSTATE_LOADED by default when a reasource is created manually.

The second is to modify the quick return in Resource::unload() to check mIsManual first:

Code: Select all

LoadingState old = mLoadingState.get();
if (old!=LOADSTATE_LOADED && old!=LOADSTATE_PREPARED && !mIsManual) return

Both appear as though they will solve the problem, however I am not sure if there is something I am missing here.


Re: [BUG] Manually created resources leak

Posted: Tue Mar 08, 2011 4:31 pm
by dark_sylinc
Wait, the purpose of the early out is that there's nothing to unload, therefore there shouldn't be any leaks.

So, this means there's a deeper bug in which something that shouldn't have been allocated was allocated, or there is something that must be allocated, therefore the right thing to do is to flag the particular resource as loaded.

What is exactly leaking?

Re: [BUG] Manually created resources leak

Posted: Wed Mar 09, 2011 2:11 am
Wait, the purpose of the early out is that there's nothing to unload, therefore there shouldn't be any leaks.
Thats how its meant to work however the way it does work is that it bails if nothing has been loaded, even if there is stuff to unload. This only applies to manually created Resources whose contents have been created and assigned by you.

Here is some of my code for an offline mesh generation tool. It creates a manual mesh and populates it will manually generated data. Notice that at the end I call load so that the mesh will then unload the vertex and index buffers that I allocated and assigned to it, when it is deleted.

Code: Select all

	// create our mesh
	Ogre::MeshPtr mesh = pMeshManager->createManual( pTin->m_Name + "_a_mesh" , "ConversionGroup" );

	mesh->sharedVertexData = new Ogre::VertexData;
	mesh->sharedVertexData->vertexStart = 0;
	mesh->sharedVertexData->vertexCount = pVertexList.size();

	static const unsigned short source = 0;
	size_t offset = 0, vertColourStart = 0;
	offset += mesh->sharedVertexData->vertexDeclaration->addElement( source, offset, Ogre::VET_FLOAT3, Ogre::VES_POSITION ).getSize();
	offset += mesh->sharedVertexData->vertexDeclaration->addElement( source, offset, Ogre::VET_FLOAT3, Ogre::VES_NORMAL ).getSize();
	offset += mesh->sharedVertexData->vertexDeclaration->addElement( source, offset, Ogre::VET_FLOAT2, Ogre::VES_TEXTURE_COORDINATES ).getSize();
	vertColourStart = offset;
	offset += mesh->sharedVertexData->vertexDeclaration->addElement( source, offset, Ogre::VET_COLOUR_ARGB, Ogre::VES_DIFFUSE ).getSize();

	// create and populate the vertex buffer
	Ogre::HardwareVertexBufferSharedPtr vb = pBufferManager->createVertexBuffer( mesh->sharedVertexData->vertexDeclaration->getVertexSize( source ), mesh->sharedVertexData->vertexCount, Ogre::HardwareBuffer::HBU_DYNAMIC );
	generateVertexBuffer( pVertexList, vb, pTin, mesh, Ogre::ColourValue::White );

	std::vector< Ogre::ColourValue > colours;
	unsigned int coloursSize = pTin->m_Colours.size();
	colours.resize( coloursSize );

	// set the correct vert colours
	for ( unsigned int i = 0; i < coloursSize; ++i )
		Ogre::ColourValue colour( Ogre::ColourValue::White );
		pTypeColours._getColour( pTin->m_Colours[i], colour );
		colours[ i ] = colour;

	offset /= sizeof( float );
	vertColourStart /= sizeof( float );

	float* vdata = static_cast< float* >( vb->lock( Ogre::HardwareBuffer::HBL_NORMAL ) );

	for ( unsigned int i = 0; i < pTin->m_TriColours.size(); ++i )
		Ogre::ColourValue& colour = colours[ pTin->m_TriColours[ i ] ]; 
		const Triangles::Triangle& triangle = pTin->m_Triangles->m_Triangles[ i ];
		float* p1 = vdata + ( triangle.m_Point1 * offset ) + vertColourStart;
		float* p2 = vdata + ( triangle.m_Point2 * offset ) + vertColourStart;
		float* p3 = vdata + ( triangle.m_Point3 * offset ) + vertColourStart;
		unsigned int colourInt = colour.getAsARGB();
		*p1 = *(float*)(&colourInt);
		*p2 = *(float*)(&colourInt);
		*p3 = *(float*)(&colourInt);


	mesh->sharedVertexData->vertexBufferBinding->setBinding( source, vb );
	for ( unsigned int j = 0; j < coloursSize; ++j )
		Ogre::ColourValue colour;
		bool hasColour = pTypeColours._getColour( pTin->m_Colours[ j ], colour );

		// set up the submesh
		Ogre::SubMesh* subMesh = mesh->createSubMesh();
		subMesh->setMaterialName( hasColour ? "base/vertcolour" : pTypeColours.getMaterialName( pTin->m_Colours[ j ] ) );

		subMesh->useSharedVertices = true;
		subMesh->indexData->indexStart = 0;

		size_t indexCount = 0;

		unsigned int triColoursSize = pTin->m_TriColours.size();
		for ( unsigned int k = 0; k < triColoursSize; ++k )
			if ( pTin->m_TriColours[ k ] == j )

		subMesh->indexData->indexCount = indexCount * 3; // points per triangle

		Ogre::HardwareIndexBuffer::IndexType indexType;

		if ( pVertexList.size() > 0xFFFF )
			indexType = Ogre::HardwareIndexBuffer::IT_32BIT;
			indexType = Ogre::HardwareIndexBuffer::IT_16BIT;

		subMesh->indexData->indexBuffer = pBufferManager->createIndexBuffer( indexType, subMesh->indexData->indexCount, Ogre::HardwareBuffer::HBU_DYNAMIC );

		if ( indexType == Ogre::HardwareIndexBuffer::IT_32BIT )
			setIndexBuffer<Ogre::uint32>( subMesh, pTin, j );
			setIndexBuffer<Ogre::uint16>( subMesh, pTin, j );


	Ogre::MeshSerializer meshSerialiser;

	std::string meshFileName = pTempDir + pTin->m_Name + ".mesh";
	meshSerialiser.exportMesh( mesh.getPointer(), meshFileName );
	pFilesToAdd.push_back( meshFileName );

	mesh->load(); // Change the state to loaded so that the buffers are deleted
	mesh->unload(); // make the mesh unload now (can be removed after testing as this will happen when the SharedPtr deletes the final copy)
	pMeshManager->remove( pTin->m_Name + "_a_mesh" );
In that code I create and assign two buffers to the manual mesh. So even though it hasn't been loaded from a mesh file it has to be unloaded.

On the generating a mesh tutorial, the final line in the code example is

Code: Select all

    /// Notify -Mesh object that it has been loaded
So maybe its not really a leak and is by design, however it seems like its an easy one to shoot your self in the foot with especially if you, like me, didn't follow a tutorial to generate your mesh.

Re: [BUG] Manually created resources leak

Posted: Wed Mar 09, 2011 4:16 am
by dark_sylinc
I don't like the idea of massively assuming manual resources are loaded by default. But like you've said, in it's current state it's very easy to shoot yourself in the foot.

First, this should be explained in createManual's comments (which is by the way, API's documentation through doxygen)

Second, a debug assert could be placed during unload time, to check if we're manual and there's allocated stuff, warning the user.

Re: [BUG] Manually created resources leak

Posted: Tue Jun 03, 2014 4:28 am
by loath
thanks for pointing this out 0xC0DEFACE.

do we still need to do this in 1.8+?

i added load() calls just to be safe but this should all be handled automatically like you said.