Memory leak and other issues

[shatcher]

In the process of integrating Bullet 2.64 into our engine most things have worked very smoothly, but I came across a few issues that look like bugs, so I thought I'd pass them on.

One of the "new"s was not converted to a placement new during the 2.64 upgrade which should cause a mem leak for anyone using the MultiSap broadphase. In btMultiSapBroadphase.cpp line 83 the line currently reads:

Code: Select all

btChildProxy* childProxyRef = new btChildProxy();

should probably be changed to:

Code: Select all

btChildProxy* childProxyRef = new (mem) btChildProxy();

Next, I found that the hash used for overlapping pairs only uses one of the proxy ids rather than both. This seems odd and contradicts the comments immediately before the function. At btOverlappingPairCache.h line 180:

Code: Select all

int key = ((unsigned int)proxyId1) | (((unsigned int)proxyId1) <<16);

likely should be:

Code: Select all

int key = ((unsigned int)proxyId1) | (((unsigned int)proxyId2) <<16);

I encountered an crash when using btBvhTriangleMeshShape as the btBvhTriangleMeshShape::processAllTriangles calls back through the btStridingMeshInterface->getLockedReadOnlyVertexIndexBase() method, which lets you specify the format of your triangle indices as a PHY_ScalarType. It then access the data as an integer causing very bad things to happen if you provided an array of shorts. At a minimum this should assert, preferably of course it should support all formats listed in the enum.

While not necessarily a bug, there are a few includes that use the system path for Bullet files. This is inconsistent with the majority of the code base and is annoying if you are using a development environment that doesn't search the user include path for system includes by default.
btDiscreteDynamicsWorld.cpp line 24 : #include <LinearMath/btTransformUtil.h>
btSimpleBroadphase.cpp line 17: #include <BulletCollision/BroadphaseCollision/btDispatcher.h>
btSimpleBroadphase.cpp line 18: #include <BulletCollision/BroadphaseCollision/btCollisionAlgorithm.h>
btPolyhedralConvexShape.cpp line 16 : #include <BulletCollision/CollisionShapes/btPolyhedralConvexShape.h>
btGjkEpa.cpp line 29 : #include <LinearMath/btStackAlloc.h>

I hope the above are helpful. Overall I've been quite please with Bullet and expect to continue using it. Thanks for all the good work.

Best regards,
Stephen

[Erwin Coumans]

One of the "new"s was not converted to a placement new during the 2.64 upgrade which should cause a mem leak for anyone using the MultiSap broadphase.
[...]
Next, I found that the hash used for overlapping pairs only uses one of the proxy ids rather than both.

THanks a lot for reporting both bugs, they have been fixed.
Note that the btMultiSapBroadphase is work in progress, some assert should be in place to let users know. Please use the btAxisSweep3 or bt32BitAxisSweep3 for now.

specify the format of your triangle indices as a PHY_ScalarType. It then access the data as an integer causing very bad things to happen if you provided an array of shorts.

This was fixed recently, support for 16bit indices has been added and will be available in Bullet 2.65 (or using Subversion).

While not necessarily a bug, there are a few includes that use the system path for Bullet files. This is inconsistent with the majority of the code base and is annoying if you are using a development environment that doesn't search the user include path for system includes by default.
btDiscreteDynamicsWorld.cpp line 24 : #include <LinearMath/btTransformUtil.h>
btSimpleBroadphase.cpp line 17: #include <BulletCollision/BroadphaseCollision/btDispatcher.h>
btSimpleBroadphase.cpp line 18: #include <BulletCollision/BroadphaseCollision/btCollisionAlgorithm.h>
btPolyhedralConvexShape.cpp line 16 : #include <BulletCollision/CollisionShapes/btPolyhedralConvexShape.h>
btGjkEpa.cpp line 29 : #include <LinearMath/btStackAlloc.h>

Has been fixed in Subversion, ready for 2.65.
Thanks a lot for all the feedback Stephen, that is very welcome!
Erwin