From a52ac3cc506ead268e372f5bdd3f29c9bfe40bc4 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Tue, 22 Mar 2016 11:41:32 -0700 Subject: [PATCH 1/3] remove excess whitespace --- libraries/shared/src/ShapeInfo.h | 2 +- tests/physics/src/MeshMassPropertiesTests.cpp | 20 +++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/libraries/shared/src/ShapeInfo.h b/libraries/shared/src/ShapeInfo.h index a3fbe55f36..79390b6680 100644 --- a/libraries/shared/src/ShapeInfo.h +++ b/libraries/shared/src/ShapeInfo.h @@ -63,7 +63,7 @@ public: void appendToPoints (const QVector& 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; diff --git a/tests/physics/src/MeshMassPropertiesTests.cpp b/tests/physics/src/MeshMassPropertiesTests.cpp index 825721641f..0b22ea1223 100644 --- a/tests/physics/src/MeshMassPropertiesTests.cpp +++ b/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); From 119ef24d5def7f688611af1a7ea3bd2240c63110 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Tue, 22 Mar 2016 11:41:45 -0700 Subject: [PATCH 2/3] cleanup ShapeManager API and fix bitrot in tests --- libraries/physics/src/ShapeManager.cpp | 8 ++------ libraries/physics/src/ShapeManager.h | 5 ++--- tests/physics/src/BulletTestUtils.h | 2 ++ tests/physics/src/ShapeManagerTests.cpp | 8 ++++---- tests/physics/src/ShapeManagerTests.h | 2 +- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/libraries/physics/src/ShapeManager.cpp b/libraries/physics/src/ShapeManager.cpp index 7a3065dfff..e9430b36db 100644 --- a/libraries/physics/src/ShapeManager.cpp +++ b/libraries/physics/src/ShapeManager.cpp @@ -58,7 +58,7 @@ 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) { @@ -82,16 +82,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; diff --git a/libraries/physics/src/ShapeManager.h b/libraries/physics/src/ShapeManager.h index e04fa1280c..0c411f5f62 100644 --- a/libraries/physics/src/ShapeManager.h +++ b/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; diff --git a/tests/physics/src/BulletTestUtils.h b/tests/physics/src/BulletTestUtils.h index 9166f80ba1..c047a2c1ce 100644 --- a/tests/physics/src/BulletTestUtils.h +++ b/tests/physics/src/BulletTestUtils.h @@ -14,6 +14,8 @@ #include +#include + // 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): // diff --git a/tests/physics/src/ShapeManagerTests.cpp b/tests/physics/src/ShapeManagerTests.cpp index 1ee7cd561e..75ad8b3d3a 100644 --- a/tests/physics/src/ShapeManagerTests.cpp +++ b/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); diff --git a/tests/physics/src/ShapeManagerTests.h b/tests/physics/src/ShapeManagerTests.h index a4b7fbecd1..1dac307413 100644 --- a/tests/physics/src/ShapeManagerTests.h +++ b/tests/physics/src/ShapeManagerTests.h @@ -16,7 +16,7 @@ class ShapeManagerTests : public QObject { Q_OBJECT - + private slots: void testShapeAccounting(); void addManyShapes(); From ecfe198e35fad5b07d09eba40ede017b7f1eb65c Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 23 Mar 2016 14:37:01 -0700 Subject: [PATCH 3/3] 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