Merge pull request #4449 from AndrewMeadows/thermonuclear

ShapeManager can release shape by pointer

libraries/physics/src/ShapeManager.cpp

@@ -21,7 +21,7 @@ ShapeManager::~ShapeManager() {
     int numShapes = _shapeMap.size();
     for (int i = 0; i < numShapes; ++i) {
         ShapeReference* shapeRef = _shapeMap.getAtIndex(i);
-        delete shapeRef->_shape;
+        delete shapeRef->shape;
     }
     _shapeMap.clear();
 }
@@ -40,26 +40,27 @@ btCollisionShape* ShapeManager::getShape(const ShapeInfo& info) {
     DoubleHashKey key = info.getHash();
     ShapeReference* shapeRef = _shapeMap.find(key);
     if (shapeRef) {
-        shapeRef->_refCount++;
-        return shapeRef->_shape;
+        shapeRef->refCount++;
+        return shapeRef->shape;
     }
     btCollisionShape* shape = ShapeInfoUtil::createShapeFromInfo(info);
     if (shape) {
         ShapeReference newRef;
-        newRef._refCount = 1;
-        newRef._shape = shape;
+        newRef.refCount = 1;
+        newRef.shape = shape;
+        newRef.key = key;
         _shapeMap.insert(key, newRef);
     }
     return shape;
 }
 
-bool ShapeManager::releaseShape(const ShapeInfo& info) {
-    DoubleHashKey key = info.getHash();
+// private helper method
+bool ShapeManager::releaseShape(const DoubleHashKey& key) {
     ShapeReference* shapeRef = _shapeMap.find(key);
     if (shapeRef) {
-        if (shapeRef->_refCount > 0) {
-            shapeRef->_refCount--;
-            if (shapeRef->_refCount == 0) {
+        if (shapeRef->refCount > 0) {
+            shapeRef->refCount--;
+            if (shapeRef->refCount == 0) {
                 _pendingGarbage.push_back(key);
                 const int MAX_GARBAGE_CAPACITY = 127;
                 if (_pendingGarbage.size() > MAX_GARBAGE_CAPACITY) {
@@ -78,10 +79,19 @@ bool ShapeManager::releaseShape(const ShapeInfo& info) {
     return false;
 }
 
+bool ShapeManager::releaseShape(const ShapeInfo& info) {
+    return releaseShape(info.getHash());
+}
+
 bool ShapeManager::releaseShape(const btCollisionShape* shape) {
-    ShapeInfo info;
-    ShapeInfoUtil::collectInfoFromShape(shape, info);
-    return releaseShape(info);
+    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 false;
 }
 
 void ShapeManager::collectGarbage() {
@@ -89,8 +99,8 @@ void ShapeManager::collectGarbage() {
     for (int i = 0; i < numShapes; ++i) {
         DoubleHashKey& key = _pendingGarbage[i];
         ShapeReference* shapeRef = _shapeMap.find(key);
-        if (shapeRef && shapeRef->_refCount == 0) {
-            delete shapeRef->_shape;
+        if (shapeRef && shapeRef->refCount == 0) {
+            delete shapeRef->shape;
             _shapeMap.remove(key);
         }
     }
@@ -101,7 +111,29 @@ int ShapeManager::getNumReferences(const ShapeInfo& info) const {
     DoubleHashKey key = info.getHash();
     const ShapeReference* shapeRef = _shapeMap.find(key);
     if (shapeRef) {
-        return shapeRef->_refCount;
+        return shapeRef->refCount;
     }
-    return -1;
+    return 0;
+}
+
+int ShapeManager::getNumReferences(const btCollisionShape* shape) const {
+    int numShapes = _shapeMap.size();
+    for (int i = 0; i < numShapes; ++i) {
+        const ShapeReference* shapeRef = _shapeMap.getAtIndex(i);
+        if (shape == shapeRef->shape) {
+            return shapeRef->refCount;
+        }
+    }
+    return 0;
+}
+
+bool ShapeManager::hasShape(const btCollisionShape* shape) const {
+    int numShapes = _shapeMap.size();
+    for (int i = 0; i < numShapes; ++i) {
+        const ShapeReference* shapeRef = _shapeMap.getAtIndex(i);
+        if (shape == shapeRef->shape) {
+            return true;
+        }
+    }
+    return false;
 }

libraries/physics/src/ShapeManager.h

@@ -38,12 +38,17 @@ public:
     // validation methods
     int getNumShapes() const { return _shapeMap.size(); }
     int getNumReferences(const ShapeInfo& info) const;
+    int getNumReferences(const btCollisionShape* shape) const;
+    bool hasShape(const btCollisionShape* shape) const; 
 
 private:
+    bool releaseShape(const DoubleHashKey& key);
+
     struct ShapeReference {
-        int _refCount;
-        btCollisionShape* _shape;
-        ShapeReference() : _refCount(0), _shape(NULL) {}
+        int refCount;
+        btCollisionShape* shape;
+        DoubleHashKey key;
+        ShapeReference() : refCount(0), shape(NULL) {}
     };
 
     btHashMap<DoubleHashKey, ShapeReference> _shapeMap;

tests/physics/src/ShapeManagerTests.cpp

@@ -21,10 +21,8 @@ void ShapeManagerTests::testShapeAccounting() {
     ShapeInfo info;
     info.setBox(glm::vec3(1.0f, 1.0f, 1.0f));
     
-    // NOTE: ShapeManager returns -1 as refcount when the shape is unknown, 
-    // which is distinct from "known but with zero references"
     int numReferences = shapeManager.getNumReferences(info);
-    if (numReferences != -1) {
+    if (numReferences != 0) {
         std::cout << __FILE__ << ":" << __LINE__
             << " ERROR: expected ignorant ShapeManager after initialization" << std::endl;
     }
@@ -104,8 +102,7 @@ void ShapeManagerTests::testShapeAccounting() {
     if (shapeManager.getNumShapes() != 0) {
         std::cout << __FILE__ << ":" << __LINE__ << " ERROR: expected zero shapes after release" << std::endl;
     }
-    numReferences = shapeManager.getNumReferences(info);
-    if (numReferences != -1) {
+    if (shapeManager.hasShape(shape)) {
         std::cout << __FILE__ << ":" << __LINE__
             << " ERROR: expected ignorant ShapeManager after garbage collection" << std::endl;
     }
@@ -122,33 +119,64 @@ void ShapeManagerTests::testShapeAccounting() {
 void ShapeManagerTests::addManyShapes() {
     ShapeManager shapeManager;
 
+    QVector<btCollisionShape*> shapes;
+
     int numSizes = 100;
     float startSize = 1.0f;
     float endSize = 99.0f;
     float deltaSize = (endSize - startSize) / (float)numSizes;
     ShapeInfo info;
     for (int i = 0; i < numSizes; ++i) {
+        // make a sphere
         float s = startSize + (float)i * deltaSize;
         glm::vec3 scale(s, 1.23f + s, s - 0.573f);
         info.setBox(0.5f * scale);
         btCollisionShape* shape = shapeManager.getShape(info);
+        shapes.push_back(shape);
         if (!shape) {
             std::cout << __FILE__ << ":" << __LINE__
                 << " ERROR: i = " << i << " null box shape for scale = " << scale << std::endl;
         }
+
+        // make a box
         float radius = 0.5f * s;
         info.setSphere(radius);
         shape = shapeManager.getShape(info);
+        shapes.push_back(shape);
         if (!shape) {
             std::cout << __FILE__ << ":" << __LINE__
                 << " ERROR: i = " << i << " null sphere shape for radius = " << radius << std::endl;
         }
     }
+
+    // verify shape count
     int numShapes = shapeManager.getNumShapes();
     if (numShapes != 2 * numSizes) {
         std::cout << __FILE__ << ":" << __LINE__
             << " ERROR: expected numShapes = " << numSizes << " but found numShapes = " << numShapes << std::endl;
     }
+
+    // release each shape by pointer
+    for (int i = 0; i < numShapes; ++i) {
+        btCollisionShape* shape = shapes[i];
+        bool success = shapeManager.releaseShape(shape);
+        if (!success) {
+            std::cout << __FILE__ << ":" << __LINE__
+                << " ERROR: failed to release shape index " << i << std::endl;
+            break;
+        }
+    }
+
+    // verify zero references
+    for (int i = 0; i < numShapes; ++i) {
+        btCollisionShape* shape = shapes[i];
+        int numReferences = shapeManager.getNumReferences(shape);
+        if (numReferences != 0) {
+            std::cout << __FILE__ << ":" << __LINE__
+                << " ERROR: expected zero references for shape " << i 
+                << " but refCount = " << numReferences << std::endl;
+        }
+    }
 }
 
 void ShapeManagerTests::addBoxShape() {