EntitySimulation is lockable

also DeleteEntityOperator just removes the entities
and EntityTree does the actual delete
(after properly locking its _simulation)

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 = details.entity;
-                EntityItem* theEntity = entityTreeElement->getEntityWithEntityItemID(entityItemID); // find the actual entity
+                assert(entityTreeElement->removeEntityItem(theEntity)); // remove it from the element
-                assert(theEntity);
+                _tree->setContainingElement(details.entity->getEntityItemID(), NULL); // update or id to element lookup
-                _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!
                 _foundCount++;
             }
         }

libraries/entities/src/DeleteEntityOperator.h

@@ -14,11 +14,13 @@
 
 class EntityToDeleteDetails {
 public:
-    const EntityItem* entity;
+    EntityItem* entity;
     AACube cube;
     EntityTreeElement* containingElement;
 };
 
+typedef QSet<EntityToDeleteDetails> 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<EntityToDeleteDetails> _entitiesToDelete;
+    RemovedEntities _entitiesToDelete;
     quint64 _changeTime;
     int _foundCount;
     int _lookingCount;

libraries/entities/src/EntitySimulation.cpp

@@ -38,6 +38,7 @@ void EntitySimulation::updateEntities(QSet<EntityItem*>& 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<EntityItem*>::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.

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;
 

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<EntityItemID> 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<EntityItem*> entitiesToDelete;
+        _simulation->lock();
         _simulation->updateEntities(entitiesToDelete);
+        _simulation->unlock();
         if (entitiesToDelete.size() > 0) {
             // translate into list of ID's
             QSet<EntityItemID> idsToDelete;

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);

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:

libraries/physics/src/ThreadSafeDynamicsWorld.cpp

@@ -15,8 +15,6 @@
  * Copied and modified from btDiscreteDynamicsWorld.cpp by AndrewMeadows on 2014.11.12.
  * */
 
-#include <EntityTree.h>
-
 #include "ThreadSafeDynamicsWorld.h"
 
 #ifdef USE_BULLET_PHYSICS
@@ -24,17 +22,8 @@ ThreadSafeDynamicsWorld::ThreadSafeDynamicsWorld(
         btDispatcher* dispatcher,
         btBroadphaseInterface* pairCache,
         btConstraintSolver* constraintSolver,
-        btCollisionConfiguration* collisionConfiguration,
+        btCollisionConfiguration* collisionConfiguration)
-        EntityTree* entities)
     :   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.  
+    // NOTE: We do NOT call synchronizeMotionState() after each substep (to avoid multiple locks on the
-    // This is to avoid placing multiple, repeated thread locks on _entities.
+    // object data outside of the physics engine).  A consequence of this is that the transforms of the
-    synchronizeMotionStates();
+    // 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

libraries/physics/src/ThreadSafeDynamicsWorld.h

@@ -21,8 +21,6 @@
 #ifdef USE_BULLET_PHYSICS
 #include <BulletDynamics/Dynamics/btDiscreteDynamicsWorld.h>
 
-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,
+            btCollisionConfiguration* collisionConfiguration);
-            EntityTree* entities);
 
     // 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