From c37c1515ba43c2e0d3abda1499504cee1938b22d Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 8 Jan 2015 10:22:25 -0800 Subject: [PATCH 1/3] EntitySimulation is lockable also DeleteEntityOperator just removes the entities and EntityTree does the actual delete (after properly locking its _simulation) --- .../entities/src/DeleteEntityOperator.cpp | 10 ++-- libraries/entities/src/DeleteEntityOperator.h | 8 ++- libraries/entities/src/EntitySimulation.cpp | 3 ++ libraries/entities/src/EntitySimulation.h | 7 ++- libraries/entities/src/EntityTree.cpp | 52 +++++++++++++------ libraries/entities/src/EntityTree.h | 2 - libraries/physics/src/PhysicsEngine.cpp | 22 ++++++-- .../physics/src/ThreadSafeDynamicsWorld.cpp | 26 ++++------ .../physics/src/ThreadSafeDynamicsWorld.h | 9 +--- 9 files changed, 85 insertions(+), 54 deletions(-) diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index 12441e5427..890c301682 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/libraries/entities/src/DeleteEntityOperator.cpp @@ -91,13 +91,9 @@ bool DeleteEntityOperator::preRecursion(OctreeElement* element) { // If this is the element we're looking for, then ask it to remove the old entity // and we can stop searching. if (entityTreeElement == details.containingElement) { - EntityItemID entityItemID = details.entity->getEntityItemID(); - EntityItem* theEntity = entityTreeElement->getEntityWithEntityItemID(entityItemID); // find the actual entity - assert(theEntity); - _tree->trackDeletedEntity(theEntity); - entityTreeElement->removeEntityItem(theEntity); // remove it from the element - _tree->setContainingElement(entityItemID, NULL); // update or id to element lookup - delete theEntity; // now actually delete the entity! + EntityItem* theEntity = details.entity; + assert(entityTreeElement->removeEntityItem(theEntity)); // remove it from the element + _tree->setContainingElement(details.entity->getEntityItemID(), NULL); // update or id to element lookup _foundCount++; } } diff --git a/libraries/entities/src/DeleteEntityOperator.h b/libraries/entities/src/DeleteEntityOperator.h index a3119da0b9..b6e6f9e2ff 100644 --- a/libraries/entities/src/DeleteEntityOperator.h +++ b/libraries/entities/src/DeleteEntityOperator.h @@ -14,11 +14,13 @@ class EntityToDeleteDetails { public: - const EntityItem* entity; + EntityItem* entity; AACube cube; EntityTreeElement* containingElement; }; +typedef QSet RemovedEntities; + inline uint qHash(const EntityToDeleteDetails& a, uint seed) { return qHash(a.entity->getEntityItemID(), seed); } @@ -36,9 +38,11 @@ public: void addEntityIDToDeleteList(const EntityItemID& searchEntityID); virtual bool preRecursion(OctreeElement* element); virtual bool postRecursion(OctreeElement* element); + + const RemovedEntities& getEntities() const { return _entitiesToDelete; } private: EntityTree* _tree; - QSet _entitiesToDelete; + RemovedEntities _entitiesToDelete; quint64 _changeTime; int _foundCount; int _lookingCount; diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 09f75c9ebf..f7d6c55803 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -38,6 +38,7 @@ void EntitySimulation::updateEntities(QSet& entitiesToDelete) { _entitiesToDelete.clear(); } +// private void EntitySimulation::expireMortalEntities(const quint64& now) { if (now > _nextExpiry) { // only search for expired entities if we expect to find one @@ -63,6 +64,7 @@ void EntitySimulation::expireMortalEntities(const quint64& now) { } } +// private void EntitySimulation::callUpdateOnEntitiesThatNeedIt(const quint64& now) { PerformanceTimer perfTimer("updatingEntities"); QSet::iterator itemItr = _updateableEntities.begin(); @@ -79,6 +81,7 @@ void EntitySimulation::callUpdateOnEntitiesThatNeedIt(const quint64& now) { } } +// private void EntitySimulation::sortEntitiesThatMoved() { // NOTE: this is only for entities that have been moved by THIS EntitySimulation. // External changes to entity position/shape are expected to be sorted outside of the EntitySimulation. diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index a32a1226c7..dee6b1c43a 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -33,9 +33,12 @@ const int DIRTY_SIMULATION_FLAGS = class EntitySimulation { public: - EntitySimulation() : _entityTree(NULL) { } + EntitySimulation() : _mutex(QMutex::Recursive), _entityTree(NULL) { } virtual ~EntitySimulation() { setEntityTree(NULL); } + void lock() { _mutex.lock(); } + void unlock() { _mutex.unlock(); } + /// \param tree pointer to EntityTree which is stored internally void setEntityTree(EntityTree* tree); @@ -80,6 +83,8 @@ protected: void callUpdateOnEntitiesThatNeedIt(const quint64& now); void sortEntitiesThatMoved(); + QMutex _mutex; + // back pointer to EntityTree structure EntityTree* _entityTree; diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 73d944b084..62e5c866a7 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -40,7 +40,9 @@ EntityTreeElement* EntityTree::createNewElement(unsigned char * octalCode) { void EntityTree::eraseAllOctreeElements(bool createNewRoot) { // this would be a good place to clean up our entities... if (_simulation) { + _simulation->lock(); _simulation->clearEntities(); + _simulation->unlock(); } foreach (EntityTreeElement* element, _entityToElementMap) { element->cleanupEntities(); @@ -84,7 +86,9 @@ void EntityTree::postAddEntity(EntityItem* entity) { assert(entity); // check to see if we need to simulate this entity.. if (_simulation) { + _simulation->lock(); _simulation->addEntity(entity); + _simulation->unlock(); } _isDirty = true; emit addingEntity(entity->getEntityItemID()); @@ -141,7 +145,9 @@ bool EntityTree::updateEntityWithElement(EntityItem* entity, const EntityItemPro if (newFlags) { if (_simulation) { if (newFlags & DIRTY_SIMULATION_FLAGS) { + _simulation->lock(); _simulation->entityChanged(entity); + _simulation->unlock(); } } else { // normally the _simulation clears ALL updateFlags, but since there is none we do it explicitly @@ -204,21 +210,6 @@ EntityItem* EntityTree::addEntity(const EntityItemID& entityID, const EntityItem return result; } - -void EntityTree::trackDeletedEntity(EntityItem* entity) { - if (_simulation) { - _simulation->removeEntity(entity); - } - // this is only needed on the server to send delete messages for recently deleted entities to the viewers - if (getIsServer()) { - // set up the deleted entities ID - quint64 deletedAt = usecTimestampNow(); - _recentlyDeletedEntitiesLock.lockForWrite(); - _recentlyDeletedEntityItemIDs.insert(deletedAt, entity->getEntityItemID().id); - _recentlyDeletedEntitiesLock.unlock(); - } -} - void EntityTree::emitEntityScriptChanging(const EntityItemID& entityItemID) { emit entityScriptChanging(entityItemID); } @@ -231,7 +222,9 @@ void EntityTree::setSimulation(EntitySimulation* simulation) { if (_simulation && _simulation != simulation) { // It's important to clearEntities() on the simulation since taht will update each // EntityItem::_simulationState correctly so as to not confuse the next _simulation. + _simulation->lock(); _simulation->clearEntities(); + _simulation->unlock(); } _simulation = simulation; } @@ -255,6 +248,31 @@ void EntityTree::deleteEntities(QSet entityIDs) { } recurseTreeWithOperator(&theOperator); + + const RemovedEntities& entities = theOperator.getEntities(); + if (_simulation) { + _simulation->lock(); + } + foreach(const EntityToDeleteDetails& details, entities) { + EntityItem* theEntity = details.entity; + + if (getIsServer()) { + // set up the deleted entities ID + quint64 deletedAt = usecTimestampNow(); + _recentlyDeletedEntitiesLock.lockForWrite(); + _recentlyDeletedEntityItemIDs.insert(deletedAt, theEntity->getEntityItemID().id); + _recentlyDeletedEntitiesLock.unlock(); + } + + if (_simulation) { + _simulation->removeEntity(theEntity); + } + delete theEntity; // now actually delete the entity! + } + if (_simulation) { + _simulation->unlock(); + } + _isDirty = true; } @@ -592,7 +610,9 @@ void EntityTree::releaseSceneEncodeData(OctreeElementExtraEncodeData* extraEncod void EntityTree::entityChanged(EntityItem* entity) { if (_simulation) { + _simulation->lock(); _simulation->entityChanged(entity); + _simulation->unlock(); } } @@ -600,7 +620,9 @@ void EntityTree::update() { if (_simulation) { lockForWrite(); QSet entitiesToDelete; + _simulation->lock(); _simulation->updateEntities(entitiesToDelete); + _simulation->unlock(); if (entitiesToDelete.size() > 0) { // translate into list of ID's QSet idsToDelete; diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index cfd08c3b5c..b2757683ba 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -146,8 +146,6 @@ public: void entityChanged(EntityItem* entity); - void trackDeletedEntity(EntityItem* entity); - void emitEntityScriptChanging(const EntityItemID& entityItemID); void setSimulation(EntitySimulation* simulation); diff --git a/libraries/physics/src/PhysicsEngine.cpp b/libraries/physics/src/PhysicsEngine.cpp index edff3f8291..4129f1c7b0 100644 --- a/libraries/physics/src/PhysicsEngine.cpp +++ b/libraries/physics/src/PhysicsEngine.cpp @@ -169,7 +169,7 @@ void PhysicsEngine::init(EntityEditPacketSender* packetSender) { _collisionDispatcher = new btCollisionDispatcher(_collisionConfig); _broadphaseFilter = new btDbvtBroadphase(); _constraintSolver = new btSequentialImpulseConstraintSolver; - _dynamicsWorld = new ThreadSafeDynamicsWorld(_collisionDispatcher, _broadphaseFilter, _constraintSolver, _collisionConfig, _entityTree); + _dynamicsWorld = new ThreadSafeDynamicsWorld(_collisionDispatcher, _broadphaseFilter, _constraintSolver, _collisionConfig); // default gravity of the world is zero, so each object must specify its own gravity // TODO: set up gravity zones @@ -200,13 +200,14 @@ void PhysicsEngine::init(EntityEditPacketSender* packetSender) { const float FIXED_SUBSTEP = 1.0f / 60.0f; void PhysicsEngine::stepSimulation() { + lock(); // NOTE: the grand order of operations is: // (1) relay incoming changes // (2) step simulation // (3) synchronize outgoing motion states // (4) send outgoing packets - // this is step (1) + // This is step (1). relayIncomingChangesToSimulation(); const int MAX_NUM_SUBSTEPS = 4; @@ -215,9 +216,24 @@ void PhysicsEngine::stepSimulation() { _clock.reset(); float timeStep = btMin(dt, MAX_TIMESTEP); - // steps (2) and (3) are performed by ThreadSafeDynamicsWorld::stepSimulation()) + // This is step (2). int numSubSteps = _dynamicsWorld->stepSimulation(timeStep, MAX_NUM_SUBSTEPS, FIXED_SUBSTEP); _frameCount += (uint32_t)numSubSteps; + unlock(); + + // This is step (3) which is done outside of stepSimulation() so we can lock _entityTree. + // + // Unfortunately we have to unlock the simulation (above) before we try to lock the _entityTree + // to avoid deadlock -- the _entityTree may try to lock its EntitySimulation (from which this + // PhysicsEngine derives) when updating/adding/deleting entities so we need to wait for our own + // lock on the tree before we re-lock ourselves. + // + // TODO: untangle these lock sequences. + _entityTree->lockForWrite(); + lock(); + _dynamicsWorld->synchronizeMotionStates(); + unlock(); + _entityTree->unlock(); } // Bullet collision flags are as follows: diff --git a/libraries/physics/src/ThreadSafeDynamicsWorld.cpp b/libraries/physics/src/ThreadSafeDynamicsWorld.cpp index e70bdc7210..e204ae9e7f 100644 --- a/libraries/physics/src/ThreadSafeDynamicsWorld.cpp +++ b/libraries/physics/src/ThreadSafeDynamicsWorld.cpp @@ -15,8 +15,6 @@ * Copied and modified from btDiscreteDynamicsWorld.cpp by AndrewMeadows on 2014.11.12. * */ -#include - #include "ThreadSafeDynamicsWorld.h" #ifdef USE_BULLET_PHYSICS @@ -24,17 +22,8 @@ ThreadSafeDynamicsWorld::ThreadSafeDynamicsWorld( btDispatcher* dispatcher, btBroadphaseInterface* pairCache, btConstraintSolver* constraintSolver, - btCollisionConfiguration* collisionConfiguration, - EntityTree* entities) + btCollisionConfiguration* collisionConfiguration) : btDiscreteDynamicsWorld(dispatcher, pairCache, constraintSolver, collisionConfiguration) { - assert(entities); - _entities = entities; -} - -void ThreadSafeDynamicsWorld::synchronizeMotionStates() { - _entities->lockForWrite(); - btDiscreteDynamicsWorld::synchronizeMotionStates(); - _entities->unlock(); } int ThreadSafeDynamicsWorld::stepSimulation( btScalar timeStep, int maxSubSteps, btScalar fixedTimeStep) { @@ -82,10 +71,15 @@ int ThreadSafeDynamicsWorld::stepSimulation( btScalar timeStep, int maxSubSteps, } } - // We only sync motion states once at the end of all substeps. - // This is to avoid placing multiple, repeated thread locks on _entities. - synchronizeMotionStates(); + // NOTE: We do NOT call synchronizeMotionState() after each substep (to avoid multiple locks on the + // object data outside of the physics engine). A consequence of this is that the transforms of the + // external objects only ever update at the end of the full step. + + // NOTE: We do NOT call synchronizeMotionStates() here. Instead it is called by an external class + // that knows how to lock threads correctly. + clearForces(); - return subSteps; + + return subSteps; } #endif // USE_BULLET_PHYSICS diff --git a/libraries/physics/src/ThreadSafeDynamicsWorld.h b/libraries/physics/src/ThreadSafeDynamicsWorld.h index 3f3bc2517b..64d5413c56 100644 --- a/libraries/physics/src/ThreadSafeDynamicsWorld.h +++ b/libraries/physics/src/ThreadSafeDynamicsWorld.h @@ -21,8 +21,6 @@ #ifdef USE_BULLET_PHYSICS #include -class EntityTree; - ATTRIBUTE_ALIGNED16(class) ThreadSafeDynamicsWorld : public btDiscreteDynamicsWorld { public: BT_DECLARE_ALIGNED_ALLOCATOR(); @@ -31,20 +29,15 @@ public: btDispatcher* dispatcher, btBroadphaseInterface* pairCache, btConstraintSolver* constraintSolver, - btCollisionConfiguration* collisionConfiguration, - EntityTree* entities); + btCollisionConfiguration* collisionConfiguration); // virtual overrides from btDiscreteDynamicsWorld int stepSimulation( btScalar timeStep, int maxSubSteps=1, btScalar fixedTimeStep=btScalar(1.)/btScalar(60.)); - void synchronizeMotionStates(); // btDiscreteDynamicsWorld::m_localTime is the portion of real-time that has not yet been simulated // but is used for MotionState::setWorldTransform() extrapolation (a feature that Bullet uses to provide // smoother rendering of objects when the physics simulation loop is ansynchronous to the render loop). float getLocalTimeAccumulation() const { return m_localTime; } - -private: - EntityTree* _entities; }; #else // USE_BULLET_PHYSICS From b0b2eb5d388d036220ddf212f0aa5b21564c96d6 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 8 Jan 2015 11:09:32 -0800 Subject: [PATCH 2/3] use exact equality for filtering dimension changes --- libraries/entities/src/EntityItem.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 3a2b8c64b1..a744d39f05 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1019,7 +1019,6 @@ void EntityItem::recalculateCollisionShape() { } const float MIN_POSITION_DELTA = 0.0001f; -const float MIN_DIMENSION_DELTA = 0.0001f; const float MIN_ALIGNMENT_DOT = 0.9999f; const float MIN_MASS_DELTA = 0.001f; const float MIN_VELOCITY_DELTA = 0.025f; @@ -1045,17 +1044,17 @@ void EntityItem::updatePositionInMeters(const glm::vec3& value) { } void EntityItem::updateDimensions(const glm::vec3& value) { - if (glm::distance(_dimensions, value) * (float)TREE_SCALE > MIN_DIMENSION_DELTA) { - _dimensions = value; + if (_dimensions != value) { + _dimensions = glm::abs(value); recalculateCollisionShape(); _dirtyFlags |= (EntityItem::DIRTY_SHAPE | EntityItem::DIRTY_MASS); } } void EntityItem::updateDimensionsInMeters(const glm::vec3& value) { - glm::vec3 dimensions = value / (float) TREE_SCALE; - if (glm::distance(_dimensions, dimensions) * (float)TREE_SCALE > MIN_DIMENSION_DELTA) { - _dimensions = dimensions; + glm::vec3 dimensions = glm::abs(value) / (float) TREE_SCALE; + if (_dimensions != dimensions) { + _dimensions = dimensions; recalculateCollisionShape(); _dirtyFlags |= (EntityItem::DIRTY_SHAPE | EntityItem::DIRTY_MASS); } From f8c512b6902ffea9346a2a41f5990593fed9a8e0 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 8 Jan 2015 12:42:16 -0800 Subject: [PATCH 3/3] fix deleteEntity() to actually delete result --- libraries/entities/src/EntityTree.cpp | 8 +++++--- libraries/entities/src/EntityTree.h | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 62e5c866a7..580fed8790 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -15,7 +15,6 @@ #include "EntitySimulation.h" #include "AddEntityOperator.h" -#include "DeleteEntityOperator.h" #include "MovingEntitiesOperator.h" #include "UpdateEntityOperator.h" @@ -235,6 +234,7 @@ void EntityTree::deleteEntity(const EntityItemID& entityID) { // NOTE: callers must lock the tree before using this method DeleteEntityOperator theOperator(this, entityID); recurseTreeWithOperator(&theOperator); + processRemovedEntities(theOperator); _isDirty = true; } @@ -248,7 +248,11 @@ void EntityTree::deleteEntities(QSet entityIDs) { } recurseTreeWithOperator(&theOperator); + processRemovedEntities(theOperator); + _isDirty = true; +} +void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) { const RemovedEntities& entities = theOperator.getEntities(); if (_simulation) { _simulation->lock(); @@ -272,8 +276,6 @@ void EntityTree::deleteEntities(QSet entityIDs) { if (_simulation) { _simulation->unlock(); } - - _isDirty = true; } /// This method is used to find and fix entity IDs that are shifting from creator token based to known ID based entity IDs. diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index b2757683ba..e8c8a93165 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -16,6 +16,7 @@ #include #include "EntityTreeElement.h" +#include "DeleteEntityOperator.h" class Model; @@ -158,6 +159,7 @@ signals: private: + void processRemovedEntities(const DeleteEntityOperator& theOperator); bool updateEntityWithElement(EntityItem* entity, const EntityItemProperties& properties, EntityTreeElement* containingElement); static bool findNearPointOperation(OctreeElement* element, void* extraData);