Merge pull request #7445 from AndrewMeadows/shape-management

fix memory leak in ShapeManager

libraries/physics/src/ShapeFactory.cpp

@@ -120,3 +120,21 @@ btCollisionShape* ShapeFactory::createShapeFromInfo(const ShapeInfo& info) {
     }
     return shape;
 }
+
+void ShapeFactory::deleteShape(btCollisionShape* shape) {
+    assert(shape);
+    if (shape->getShapeType() == (int)COMPOUND_SHAPE_PROXYTYPE) {
+        btCompoundShape* compoundShape = static_cast<btCompoundShape*>(shape);
+        const int numChildShapes = compoundShape->getNumChildShapes();
+        for (int i = 0; i < numChildShapes; i ++) {
+            btCollisionShape* childShape = compoundShape->getChildShape(i);
+            if (childShape->getShapeType() == (int)COMPOUND_SHAPE_PROXYTYPE) {
+                // recurse
+                ShapeFactory::deleteShape(childShape);
+            } else {
+                delete childShape;
+            }
+        }
+    }
+    delete shape;
+}

libraries/physics/src/ShapeFactory.h

@@ -22,6 +22,7 @@
 namespace ShapeFactory {
     btConvexHullShape* createConvexHull(const QVector<glm::vec3>& points);
     btCollisionShape* createShapeFromInfo(const ShapeInfo& info);
+    void deleteShape(btCollisionShape* shape);
 };
 
 #endif // hifi_ShapeFactory_h

libraries/physics/src/ShapeManager.cpp

@@ -23,7 +23,7 @@ ShapeManager::~ShapeManager() {
     int numShapes = _shapeMap.size();
     for (int i = 0; i < numShapes; ++i) {
         ShapeReference* shapeRef = _shapeMap.getAtIndex(i);
-        delete shapeRef->shape;
+        ShapeFactory::deleteShape(shapeRef->shape);
     }
     _shapeMap.clear();
 }
@@ -32,13 +32,14 @@ btCollisionShape* ShapeManager::getShape(const ShapeInfo& info) {
     if (info.getType() == SHAPE_TYPE_NONE) {
         return NULL;
     }
-    // Very small or large objects are not supported.
+    if (info.getType() != SHAPE_TYPE_COMPOUND) {
-    float diagonal = 4.0f * glm::length2(info.getHalfExtents());
+        // Very small or large non-compound objects are not supported.
-    const float MIN_SHAPE_DIAGONAL_SQUARED = 3.0e-4f; // 1 cm cube
+        float diagonal = 4.0f * glm::length2(info.getHalfExtents());
-    //const float MAX_SHAPE_DIAGONAL_SQUARED = 3.0e6f;  // 1000 m cube
+        const float MIN_SHAPE_DIAGONAL_SQUARED = 3.0e-4f; // 1 cm cube
-    if (diagonal < MIN_SHAPE_DIAGONAL_SQUARED /* || diagonal > MAX_SHAPE_DIAGONAL_SQUARED*/ ) {
+        if (diagonal < MIN_SHAPE_DIAGONAL_SQUARED) {
-        // qCDebug(physics) << "ShapeManager::getShape -- not making shape due to size" << diagonal;
+            // qCDebug(physics) << "ShapeManager::getShape -- not making shape due to size" << diagonal;
-        return NULL;
+            return NULL;
+        }
     }
     DoubleHashKey key = info.getHash();
     ShapeReference* shapeRef = _shapeMap.find(key);
@@ -58,14 +59,14 @@ btCollisionShape* ShapeManager::getShape(const ShapeInfo& info) {
 }
 
 // private helper method
-bool ShapeManager::releaseShape(const DoubleHashKey& key) {
+bool ShapeManager::releaseShapeByKey(const DoubleHashKey& key) {
     ShapeReference* shapeRef = _shapeMap.find(key);
     if (shapeRef) {
         if (shapeRef->refCount > 0) {
             shapeRef->refCount--;
             if (shapeRef->refCount == 0) {
                 _pendingGarbage.push_back(key);
-                const int MAX_GARBAGE_CAPACITY = 127;
+                const int MAX_GARBAGE_CAPACITY = 255;
                 if (_pendingGarbage.size() > MAX_GARBAGE_CAPACITY) {
                     collectGarbage();
                 }
@@ -82,16 +83,12 @@ bool ShapeManager::releaseShape(const DoubleHashKey& key) {
     return false;
 }
 
-bool ShapeManager::releaseShape(const ShapeInfo& info) {
-    return releaseShape(info.getHash());
-}
-
 bool ShapeManager::releaseShape(const btCollisionShape* shape) {
     int numShapes = _shapeMap.size();
     for (int i = 0; i < numShapes; ++i) {
         ShapeReference* shapeRef = _shapeMap.getAtIndex(i);
         if (shape == shapeRef->shape) {
-            return releaseShape(shapeRef->key);
+            return releaseShapeByKey(shapeRef->key);
         }
     }
     return false;
@@ -103,17 +100,7 @@ void ShapeManager::collectGarbage() {
         DoubleHashKey& key = _pendingGarbage[i];
         ShapeReference* shapeRef = _shapeMap.find(key);
         if (shapeRef && shapeRef->refCount == 0) {
-            // if the shape we're about to delete is compound, delete the children first.
+            ShapeFactory::deleteShape(shapeRef->shape);
-            if (shapeRef->shape->getShapeType() == COMPOUND_SHAPE_PROXYTYPE) {
-                const btCompoundShape* compoundShape = static_cast<const btCompoundShape*>(shapeRef->shape);
-                const int numChildShapes = compoundShape->getNumChildShapes();
-                for (int i = 0; i < numChildShapes; i ++) {
-                    const btCollisionShape* childShape = compoundShape->getChildShape(i);
-                    delete childShape;
-                }
-            }
-
-            delete shapeRef->shape;
             _shapeMap.remove(key);
         }
     }

libraries/physics/src/ShapeManager.h

@@ -29,7 +29,6 @@ public:
     btCollisionShape* getShape(const ShapeInfo& info);
 
     /// \return true if shape was found and released
-    bool releaseShape(const ShapeInfo& info);
     bool releaseShape(const btCollisionShape* shape);
 
     /// delete shapes that have zero references
@@ -39,10 +38,10 @@ public:
     int getNumShapes() const { return _shapeMap.size(); }
     int getNumReferences(const ShapeInfo& info) const;
     int getNumReferences(const btCollisionShape* shape) const;
-    bool hasShape(const btCollisionShape* shape) const; 
+    bool hasShape(const btCollisionShape* shape) const;
 
 private:
-    bool releaseShape(const DoubleHashKey& key);
+    bool releaseShapeByKey(const DoubleHashKey& key);
 
     struct ShapeReference {
         int refCount;

libraries/shared/src/ShapeInfo.h

@@ -63,7 +63,7 @@ public:
     void appendToPoints (const QVector<glm::vec3>& newPoints) { _points << newPoints; }
 
     float computeVolume() const;
-    
+
     /// Returns whether point is inside the shape
     /// For compound shapes it will only return whether it is inside the bounding box
     bool contains(const glm::vec3& point) const;

tests/physics/src/BulletTestUtils.h

@@ -14,6 +14,8 @@
 
 #include <BulletUtil.h>
 
+#include <functional>
+
 // Implements functionality in QTestExtensions.h for glm types
 // There are 3 functions in here (which need to be defined for all types that use them):
 //

tests/physics/src/MeshMassPropertiesTests.cpp

@@ -33,7 +33,7 @@ void MeshMassPropertiesTests::testParallelAxisTheorem() {
     // verity we can compute the inertia tensor of a box in two different ways:
     // (a) as one box
     // (b) as a combination of two partial boxes.
-    
+
     btScalar bigBoxX = 7.0f;
     btScalar bigBoxY = 9.0f;
     btScalar bigBoxZ = 11.0f;
@@ -62,9 +62,9 @@ void MeshMassPropertiesTests::testParallelAxisTheorem() {
 }
 
 void MeshMassPropertiesTests::testTetrahedron(){
-    // given the four vertices of a tetrahedron verify the analytic formula for inertia 
+    // given the four vertices of a tetrahedron verify the analytic formula for inertia
     // agrees with expected results
-    
+
     // these numbers from the Tonon paper:
     btVector3 points[4];
     points[0] = btVector3(8.33220f, -11.86875f, 0.93355f);
@@ -102,14 +102,14 @@ void MeshMassPropertiesTests::testTetrahedron(){
     }
     btMatrix3x3 inertia;
     computeTetrahedronInertia(volume, points, inertia);
-    
+
     QCOMPARE_WITH_ABS_ERROR(volume, expectedVolume, acceptableRelativeError * volume);
-    
+
     QCOMPARE_WITH_RELATIVE_ERROR(inertia, expectedInertia, acceptableRelativeError);
 }
 
 void MeshMassPropertiesTests::testOpenTetrahedonMesh() {
-    // given the simplest possible mesh (open, with one triangle) 
+    // given the simplest possible mesh (open, with one triangle)
     // verify MeshMassProperties computes the right nubers
 
     // these numbers from the Tonon paper:
@@ -155,7 +155,7 @@ void MeshMassPropertiesTests::testOpenTetrahedonMesh() {
 void MeshMassPropertiesTests::testClosedTetrahedronMesh() {
     // given a tetrahedron as a closed mesh of four tiangles
     // verify MeshMassProperties computes the right nubers
-    
+
     // these numbers from the Tonon paper:
     VectorOfPoints points;
     points.push_back(btVector3(8.33220f, -11.86875f, 0.93355f));
@@ -186,7 +186,7 @@ void MeshMassPropertiesTests::testClosedTetrahedronMesh() {
 
     // compute mass properties
     MeshMassProperties mesh(points, triangles);
-    
+
     // verify
     QCOMPARE_WITH_ABS_ERROR(mesh._volume, expectedVolume, acceptableRelativeError * expectedVolume);
     QCOMPARE_WITH_ABS_ERROR(mesh._centerOfMass, expectedCenterOfMass, acceptableAbsoluteError);
@@ -210,7 +210,7 @@ void MeshMassPropertiesTests::testClosedTetrahedronMesh() {
 
 void MeshMassPropertiesTests::testBoxAsMesh() {
     // verify that a mesh box produces the same mass properties as the analytic box.
-    
+
     // build a box:
     //                            /
     //                           y
@@ -265,7 +265,7 @@ void MeshMassPropertiesTests::testBoxAsMesh() {
     MeshMassProperties mesh(points, triangles);
 
     // verify
-    
+
     QCOMPARE_WITH_ABS_ERROR(mesh._volume, expectedVolume, acceptableRelativeError * expectedVolume);
     QCOMPARE_WITH_ABS_ERROR(mesh._centerOfMass, expectedCenterOfMass, acceptableAbsoluteError);
 

tests/physics/src/ShapeManagerTests.cpp

@@ -21,7 +21,7 @@ void ShapeManagerTests::testShapeAccounting() {
     ShapeManager shapeManager;
     ShapeInfo info;
     info.setBox(glm::vec3(1.0f, 1.0f, 1.0f));
-    
+
     int numReferences = shapeManager.getNumReferences(info);
     QCOMPARE(numReferences, 0);
 
@@ -42,10 +42,10 @@ void ShapeManagerTests::testShapeAccounting() {
     QCOMPARE(numReferences, expectedNumReferences);
 
     // release all references
-    bool released = shapeManager.releaseShape(info);
+    bool released = shapeManager.releaseShape(shape);
     numReferences--;
     while (numReferences > 0) {
-        released = shapeManager.releaseShape(info) && released;
+        released = shapeManager.releaseShape(shape) && released;
         numReferences--;
     }
     QCOMPARE(released, true);
@@ -69,7 +69,7 @@ void ShapeManagerTests::testShapeAccounting() {
     QCOMPARE(numReferences, 1);
 
     // release reference and verify that it is collected as garbage
-    released = shapeManager.releaseShape(info);
+    released = shapeManager.releaseShape(shape);
     shapeManager.collectGarbage();
     QCOMPARE(shapeManager.getNumShapes(), 0);
     QCOMPARE(shapeManager.hasShape(shape), false);
@@ -183,3 +183,58 @@ void ShapeManagerTests::addCapsuleShape() {
     QCOMPARE(shape, otherShape);
     */
 }
+
+void ShapeManagerTests::addCompoundShape() {
+    // initialize some points for generating tetrahedral convex hulls
+    QVector<glm::vec3> tetrahedron;
+    tetrahedron.push_back(glm::vec3(1.0f, 1.0f, 1.0f));
+    tetrahedron.push_back(glm::vec3(1.0f, -1.0f, -1.0f));
+    tetrahedron.push_back(glm::vec3(-1.0f, 1.0f, -1.0f));
+    tetrahedron.push_back(glm::vec3(-1.0f, -1.0f, 1.0f));
+    int numHullPoints = tetrahedron.size();
+
+    // compute the points of the hulls
+    QVector< QVector<glm::vec3> > hulls;
+    int numHulls = 5;
+    glm::vec3 offsetNormal(1.0f, 0.0f, 0.0f);
+    for (int i = 0; i < numHulls; ++i) {
+        glm::vec3 offset = (float)(i - numHulls/2) * offsetNormal;
+        QVector<glm::vec3> hull;
+        float radius = (float)(i + 1);
+        for (int j = 0; j < numHullPoints; ++j) {
+            glm::vec3 point = radius * tetrahedron[j] + offset;
+            hull.push_back(point);
+        }
+        hulls.push_back(hull);
+    }
+
+    // create the ShapeInfo
+    ShapeInfo info;
+    info.setConvexHulls(hulls);
+
+    // create the shape
+    ShapeManager shapeManager;
+    btCollisionShape* shape = shapeManager.getShape(info);
+    QVERIFY(shape != nullptr);
+
+    // verify the shape is correct type
+    QCOMPARE(shape->getShapeType(), (int)COMPOUND_SHAPE_PROXYTYPE);
+
+    // verify the shape has correct number of children
+    btCompoundShape* compoundShape = static_cast<btCompoundShape*>(shape);
+    QCOMPARE(compoundShape->getNumChildShapes(), numHulls);
+
+    // verify manager has only one shape
+    QCOMPARE(shapeManager.getNumShapes(), 1);
+    QCOMPARE(shapeManager.getNumReferences(info), 1);
+
+    // release the shape
+    shapeManager.releaseShape(shape);
+    QCOMPARE(shapeManager.getNumShapes(), 1);
+    QCOMPARE(shapeManager.getNumReferences(info), 0);
+
+    // collect garbage
+    shapeManager.collectGarbage();
+    QCOMPARE(shapeManager.getNumShapes(), 0);
+    QCOMPARE(shapeManager.getNumReferences(info), 0);
+}

tests/physics/src/ShapeManagerTests.h

@@ -16,7 +16,7 @@
 
 class ShapeManagerTests : public QObject {
     Q_OBJECT
-    
+
 private slots:
     void testShapeAccounting();
     void addManyShapes();
@@ -24,6 +24,7 @@ private slots:
     void addSphereShape();
     void addCylinderShape();
     void addCapsuleShape();
+    void addCompoundShape();
 };
 
 #endif // hifi_ShapeManagerTests_h