From ecfe198e35fad5b07d09eba40ede017b7f1eb65c Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Mar 2016 14:37:01 -0700 Subject: [PATCH] patch btCompoundShape memory leak --- libraries/physics/src/ShapeFactory.cpp | 18 ++++++++ libraries/physics/src/ShapeFactory.h | 1 + libraries/physics/src/ShapeManager.cpp | 31 +++++--------- tests/physics/src/ShapeManagerTests.cpp | 55 +++++++++++++++++++++++++ tests/physics/src/ShapeManagerTests.h | 1 + 5 files changed, 86 insertions(+), 20 deletions(-) diff --git a/libraries/physics/src/ShapeFactory.cpp b/libraries/physics/src/ShapeFactory.cpp index f956e562cd..de3e9cc794 100644 --- a/libraries/physics/src/ShapeFactory.cpp +++ b/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(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; +} diff --git a/libraries/physics/src/ShapeFactory.h b/libraries/physics/src/ShapeFactory.h index f6a6dfb3e6..1ba2bdb619 100644 --- a/libraries/physics/src/ShapeFactory.h +++ b/libraries/physics/src/ShapeFactory.h @@ -22,6 +22,7 @@ namespace ShapeFactory { btConvexHullShape* createConvexHull(const QVector& points); btCollisionShape* createShapeFromInfo(const ShapeInfo& info); + void deleteShape(btCollisionShape* shape); }; #endif // hifi_ShapeFactory_h diff --git a/libraries/physics/src/ShapeManager.cpp b/libraries/physics/src/ShapeManager.cpp index e9430b36db..4231d1eb60 100644 --- a/libraries/physics/src/ShapeManager.cpp +++ b/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. - float diagonal = 4.0f * glm::length2(info.getHalfExtents()); - const float MIN_SHAPE_DIAGONAL_SQUARED = 3.0e-4f; // 1 cm cube - //const float MAX_SHAPE_DIAGONAL_SQUARED = 3.0e6f; // 1000 m cube - if (diagonal < MIN_SHAPE_DIAGONAL_SQUARED /* || diagonal > MAX_SHAPE_DIAGONAL_SQUARED*/ ) { - // qCDebug(physics) << "ShapeManager::getShape -- not making shape due to size" << diagonal; - return NULL; + if (info.getType() != SHAPE_TYPE_COMPOUND) { + // Very small or large non-compound objects are not supported. + float diagonal = 4.0f * glm::length2(info.getHalfExtents()); + const float MIN_SHAPE_DIAGONAL_SQUARED = 3.0e-4f; // 1 cm cube + if (diagonal < MIN_SHAPE_DIAGONAL_SQUARED) { + // qCDebug(physics) << "ShapeManager::getShape -- not making shape due to size" << diagonal; + return NULL; + } } DoubleHashKey key = info.getHash(); ShapeReference* shapeRef = _shapeMap.find(key); @@ -65,7 +66,7 @@ bool ShapeManager::releaseShapeByKey(const DoubleHashKey& key) { 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(); } @@ -99,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. - if (shapeRef->shape->getShapeType() == COMPOUND_SHAPE_PROXYTYPE) { - const btCompoundShape* compoundShape = static_cast(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; + ShapeFactory::deleteShape(shapeRef->shape); _shapeMap.remove(key); } } diff --git a/tests/physics/src/ShapeManagerTests.cpp b/tests/physics/src/ShapeManagerTests.cpp index 75ad8b3d3a..66ac9d0c4a 100644 --- a/tests/physics/src/ShapeManagerTests.cpp +++ b/tests/physics/src/ShapeManagerTests.cpp @@ -183,3 +183,58 @@ void ShapeManagerTests::addCapsuleShape() { QCOMPARE(shape, otherShape); */ } + +void ShapeManagerTests::addCompoundShape() { + // initialize some points for generating tetrahedral convex hulls + QVector 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 > 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 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(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); +} diff --git a/tests/physics/src/ShapeManagerTests.h b/tests/physics/src/ShapeManagerTests.h index 1dac307413..cff02dcfa4 100644 --- a/tests/physics/src/ShapeManagerTests.h +++ b/tests/physics/src/ShapeManagerTests.h @@ -24,6 +24,7 @@ private slots: void addSphereShape(); void addCylinderShape(); void addCapsuleShape(); + void addCompoundShape(); }; #endif // hifi_ShapeManagerTests_h