[BUG] Manually created resources leak

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
Post Reply
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

[BUG] Manually created resources leak

Post by 0xC0DEFACE »

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.

Thanks!
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: [BUG] Manually created resources leak

Post 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?
User avatar
0xC0DEFACE
OGRE Expert User
OGRE Expert User
Posts: 84
Joined: Thu May 21, 2009 4:55 am
x 7

Re: [BUG] Manually created resources leak

Post by 0xC0DEFACE »

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);
	}

	vb->unlock();

	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 )
				++indexCount;

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

		Ogre::HardwareIndexBuffer::IndexType indexType;

		if ( pVertexList.size() > 0xFFFF )
			indexType = Ogre::HardwareIndexBuffer::IT_32BIT;
		else
			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 );
		else
			setIndexBuffer<Ogre::uint16>( subMesh, pTin, j );
	}

	mesh->buildTangentVectors();

	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
    msh->load();
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.
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5296
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: [BUG] Manually created resources leak

Post 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.
loath
Platinum Sponsor
Platinum Sponsor
Posts: 290
Joined: Tue Jan 17, 2012 5:18 am
x 67

Re: [BUG] Manually created resources leak

Post 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.
Post Reply