OgreBullet Memoryleak Prevention

Hi Everyone,

I started with OB not long ago and most of the time like it but not the memory handling. So, deciced to make some changes/do some functions to be able to destroy objects/shapes in a more convenient way.
Please review this method and reveal my mistakes if you find some. I'm ready to commit the code to the SVN is you agree, and will try to refactor/rethink if somebody have better ideas.
This code is tested with the basics: created some boxes and spheres dinamically inside a demo app and tried also with trimesh. It's not leaving memory leaks as far as I can tell. So, if there is something I forgot (for example some more testing with something), please comment.

The problem: as soon as OgreBullet gets the shape/object, OgreBullet is responsible for it. Until this time OgreBullet didn't care about these shapes/objects and if you didn't create separate deques for the shapes/objects memory leaks happened.

The idea: I created some functions which collects/destroys the shapes/objects so from now on you only have to call one function and after that just delete the main collisionWorld (dynamicWorld) variable. No more memory leaks!

The changes/addons:
The OgreBullet revision is: 2914

OgreBulletCollisionsWorld.h

Most of the addons happened here. I added one include:
#include "OgreBulletCollisionsShape.h"

And several functions:
std::set<CollisionShape *> getOgreBulletShapes(); // get all the OgreBullet CollisionShapes in a list
std::deque<Object *> getOgreBulletObjects();// get all the CollisionObjects what were created

void removeBulletObjectDependencies(); // removes the objects from Bullet -> no dependencies
void destroyOgreBulletShapes(); // destroys OgreBullet Shapes (call after removeBulletObjectDependencies)
void destroyOgreBulletObjects(); /* destroys OgreBullet Objects. Does NOT care for the
Shapes! (call after removeBulletObjectDependencies) */
void destroyOgreBulletObjectsAndShapes(); // destroys both Shapes and Objects. Simply calls the two other function

The diff file:

--- /OgreBulletCollisionsWorld.h-revBASE.svn000.tmp.h Szo jún. 5 14:12:38 2010
+++ /OgreBulletCollisionsWorld.h Szo jún. 5 10:58:17 2010
@@ -28,8 +28,8 @@
#define _OGREBULLETCOLLISIONS_CollisionWorld_H

#include "OgreBulletCollisionsPreRequisites.h"
+#include "OgreBulletCollisionsShape.h"

-
namespace OgreBulletCollisions
{
// -------------------------------------------------------------------------
@@ -73,7 +73,18 @@

void launchRay (CollisionRayResultCallback &ray, short int collisionFilterMask = -1);

- protected:
+
+ std::set<CollisionShape *> getOgreBulletShapes(); // get all the OgreBullet CollisionShapes in a list
+ std::deque<Object *> getOgreBulletObjects();// get all the CollisionObjects what were created
+
+ void removeBulletObjectDependencies(); // removes the objects from Bullet -> no dependencies
+ void destroyOgreBulletShapes(); // destroys OgreBullet Shapes (call after removeBulletObjectDependencies)
+ void destroyOgreBulletObjects(); /* destroys OgreBullet Objects. Does NOT care for the
+ Shapes! (call after removeBulletObjectDependencies) */
+ void destroyOgreBulletObjectsAndShapes(); // destroys both Shapes and Objects. Simply calls the two other function
+
+ protected:
+
btCollisionWorld* mWorld;
btCollisionDispatcher* mDispatcher;


OgreBulletCollisionsWorld.cpp

Here is the implementation for the new functions and a little add-on to ~CollisionsWorld().
I just paste the diff file:


---/OgreBulletCollisionsWorld.cp-revBASE.svn001.tmp.cpp Szo jún. 5 19:53:31 2010
+++/OgreBulletCollisionsWorld.cpp Szo jún. 5 19:48:08 2010
@@ -90,11 +90,69 @@
// -------------------------------------------------------------------------
CollisionsWorld::~CollisionsWorld()
{
+
+ delete mWorld->getDebugDrawer();
+ mWorld->setDebugDrawer(0);
+
delete mWorld;
delete mBroadphase;
delete mDispatcher;
}
+// -------------------------------------------------------------------------
+std::deque<Object *> CollisionsWorld::getOgreBulletObjects()
+{
+ // simply returns the mObjects deque where OgreBullet stores the Objects
+ return mObjects;
+}
+// -------------------------------------------------------------------------
+void CollisionsWorld::removeBulletObjectDependencies()
+{
+ // remove object from Bullet so we won't get memory access errors
+ // when destroying Objects and Shapes (hopefully :))
+ if (mObjects.size() > 0) // (mObjects.size()-1) don't work properly if mObject.size() == 0
+ for (int i=0; i < (mObjects.size()-1); i++)
+ mWorld->removeCollisionObject(mObjects[i]->getBulletObject());
+}
// -------------------------------------------------------------------------
+void CollisionsWorld::destroyOgreBulletShapes()
+{
+ // get the used shapes and delete them. We can't be sure that
+ // the amount of shapes equals with the amount of Objects
+ // because several Object can have the same Shape
+ std::set<CollisionShape *> myShapes = getOgreBulletShapes();
+ while (myShapes.size()) {
+ delete *myShapes.begin();
+ myShapes.erase(myShapes.begin());
+ }
+ myShapes.clear();
+}
+// -------------------------------------------------------------------------
+void CollisionsWorld::destroyOgreBulletObjects()
+{
+ while (mObjects.size())
+ delete mObjects[0];
+ mObjects.clear(); // don't think this is really needed
+ // but I leave it here just to be sure
+}
+// -------------------------------------------------------------------------
+std::set<CollisionShape *> CollisionsWorld::getOgreBulletShapes()
+{
+ // set is used because several Objects can share the same Shape
+ std::set<CollisionShape *> list;
+
+ for (std::deque<Object *>::const_iterator itObj = mObjects.begin();
+ itObj != mObjects.end(); ++itObj) {
+ list.insert(itObj[0]->getShape());
+ }
+ return list;
+};
+// -------------------------------------------------------------------------
+void CollisionsWorld::destroyOgreBulletObjectsAndShapes()
+{
+ removeBulletObjectDependencies();
+ destroyOgreBulletShapes();
+ destroyOgreBulletObjects();
+}
+// -------------------------------------------------------------------------
void CollisionsWorld::setShowDebugContactPoints(bool show)
{
if (show && !mShowDebugContactPoints)
@@ -291,4 +349,3 @@
);
}
}
-



That's it, at least Tortoise SVN didn't show more differences.. I hope that I didn't forgot anything.

So, in a nutshell:
As you can see there are some new functions. If you call destroyOgreBulletObjectsAndShapes() it will set free the shapes and objects used by OgreBullet.
The deletion of the OgreBullet 'system' would look like this:


~destructor_of_your_frameListener_or_something()
{
mWorld->destroyOgreBulletObjectsAndShapes();
delete mWorld;
}


The other new functions are there for better customization (if you like to delete your stuff in more steps or idontknow ).
I could put the destroy function into the CollisionWorld destructor but that would kill the customization options and this way the original OgreBullet demos are working.

So, what are your opinions and suggests?

Thanks for the patch and the work you have put into this. However, Bullet uses the design philosophy of "whoever allocates, also deletes". OgreBullet follows that same design philosophy in order to be consistent with Bullet. So OB does not take responsibility for the object, instead it is simply using the object your application provides to it.

But you are of course free to use OB how you wish and add methods that work best for your particular application. That's one of the beauties of Open Source.

- Fish

Thanks for the patch and the work you have put into this. However, Bullet uses the design philosophy of "whoever allocates, also deletes". OgreBullet follows that same design philosophy in order to be consistent with Bullet. So OB does not take responsibility for the object, instead it is simply using the object your application provides to it.

But you are of course free to use OB how you wish and add methods that work best for your particular application. That's one of the beauties of Open Source.

- Fish

I understand your opinion and will use these functions . Thanks!