more PhysicalEntitySimulation implementation

with changes to API's accordingly (does not compile yet)

interface/src/Application.cpp

@@ -2356,8 +2356,7 @@ void Application::update(float deltaTime) {
 
         _physicsEngine.removeObjects(_entitySimulation.getObjectsToRemove());
         _physicsEngine.addObjects(_entitySimulation.getObjectsToAdd());
-        _physicsEngine.updateObjects(_entitySimulation.getObjectsToUpdate());
-        _entitySimulation.clearIncomingChanges();
+        _physicsEngine.updateObjects(_entitySimulation.getObjectsToChange());
 
         _physicsEngine.stepSimulation();
 

libraries/entities/src/EntityItem.cpp

@@ -63,9 +63,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID) :
     _simulatorID(ENTITY_ITEM_DEFAULT_SIMULATOR_ID),
     _simulatorIDChangedTime(0),
     _marketplaceID(ENTITY_ITEM_DEFAULT_MARKETPLACE_ID),
-    _physicsInfo(NULL),
-    _dirtyFlags(0),
-    _element(NULL)
+    _dirtyFlags(0)
 {
     quint64 now = usecTimestampNow();
     _lastSimulated = now;
@@ -78,9 +76,11 @@ EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemPropert
 }
 
 EntityItem::~EntityItem() {
-    // be sure to clean up _physicsInfo before calling this dtor
-    assert(_physicsInfo == NULL);
-    assert(_element == NULL);
+    // these pointers MUST be NULL at delete, else we probably have a dangling backpointer 
+    // to this EntityItem in the corresponding data structure.
+    assert(!_element);
+    assert(!_simulation);
+    assert(!_physicsInfo);
 }
 
 EntityPropertyFlags EntityItem::getEntityProperties(EncodeBitstreamParams& params) const {

libraries/entities/src/EntityItem.h

@@ -28,7 +28,7 @@
 #include "EntityItemPropertiesDefaults.h" 
 #include "EntityTypes.h"
 
-class EntityTree;
+class EntitySimulation;
 class EntityTreeElement;
 class EntityTreeElementExtraEncodeData;
 
@@ -44,7 +44,12 @@ class EntityTreeElementExtraEncodeData;
 /// to all other entity types. In particular: postion, size, rotation, age, lifetime, velocity, gravity. You can not instantiate
 /// one directly, instead you must only construct one of it's derived classes with additional features.
 class EntityItem {
+    // These two classes manage lists of EntityItem pointers and must be able to cleanup pointers when an EntityItem is deleted.
+    // To make the cleanup robust each EntityItem has backpointers to its manager classes (which are only ever set/cleared by 
+    // the managers themselves, hence they are fiends) whose NULL status can be used to determine which managers still need to
+    // do cleanup.
     friend class EntityTreeElement;
+    friend class EntitySimulation;
 public:
     enum EntityDirtyFlags {
         DIRTY_POSITION = 0x0001,
@@ -293,9 +298,9 @@ public:
     bool isMoving() const;
 
     void* getPhysicsInfo() const { return _physicsInfo; }
-    void setPhysicsInfo(void* data) { _physicsInfo = data; }
     
     EntityTreeElement* getElement() const { return _element; }
+    EntitySimulation* getSimulation() const { return _simulation; }
 
     static void setSendPhysicsUpdates(bool value) { _sendPhysicsUpdates = value; }
     static bool getSendPhysicsUpdates() { return _sendPhysicsUpdates; }
@@ -306,6 +311,7 @@ public:
     glm::vec3 entityToWorld(const glm::vec3& point) const;
     
 protected:
+    void setPhysicsInfo(void* data) { _physicsInfo = data; }
 
     static bool _sendPhysicsUpdates;
     EntityTypes::EntityType _type;
@@ -365,14 +371,13 @@ protected:
     /// set radius in domain scale units (0.0 - 1.0) this will also reset dimensions to be equal for each axis
     void setRadius(float value); 
 
-    // _physicsInfo is a hook reserved for use by the EntitySimulation, which is guaranteed to set _physicsInfo 
-    // to a non-NULL value when the EntityItem has a representation in the physics engine.
-    void* _physicsInfo = NULL; // only set by EntitySimulation
-
     // DirtyFlags are set whenever a property changes that the EntitySimulation needs to know about.
     uint32_t _dirtyFlags;   // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation
 
-    EntityTreeElement* _element;    // back pointer to containing Element
+    // these backpointers are only ever set/cleared by friends:
+    EntityTreeElement* _element = nullptr; // set by EntityTreeElement
+    EntitySimulation* _simulation = nullptr; // set by EntitySimulation
+    void* _physicsInfo = nullptr; // set by EntitySimulation
 };
 
 

libraries/entities/src/EntitySimulation.cpp

@@ -19,13 +19,13 @@ void EntitySimulation::setEntityTree(EntityTree* tree) {
     if (_entityTree && _entityTree != tree) {
         _mortalEntities.clear();
         _nextExpiry = quint64(-1);
-        _updateableEntities.clear();
-        _entitiesToBeSorted.clear();
+        _entitiesToUpdate.clear();
+        _entitiesToSort.clear();
     }
     _entityTree = tree;
 }
 
-void EntitySimulation::updateEntities(QSet<EntityItem*>& entitiesToDelete) {
+void EntitySimulation::updateEntities() {
     quint64 now = usecTimestampNow();
 
     // these methods may accumulate entries in _entitiesToBeDeleted
@@ -33,10 +33,27 @@ void EntitySimulation::updateEntities(QSet<EntityItem*>& entitiesToDelete) {
     callUpdateOnEntitiesThatNeedIt(now);
     updateEntitiesInternal(now);
     sortEntitiesThatMoved();
+}
 
-    // at this point we harvest _entitiesToBeDeleted
-    entitiesToDelete.unite(_entitiesToDelete);
-    _entitiesToDelete.clear();
+void EntitySimulation::getEntitiesToDelete(SetOfEntities& entitiesToDelete) {
+    SetOfEntities::iterator entityItr = _entitiesToDelete.begin();
+    while (entityItr != _entitiesToDelete.end()) {
+        EntityItem* entity = *entityItr;
+        if (entity->_simulation != this) {
+            if (entity->_element) {
+                // this entity is still in its tree, so we insert into the external list
+                entitiesToDelete.insert(entity);
+            } else {
+                // no more backpointers, so it should be safe to delete
+                delete entity;
+            }
+            // we're done with the entity in this context, so remove it from our internal list
+            entityItr = _entitiesToDelete.erase(entityItr);
+        } else {
+            // internal cleanup will happen later (probably in updateEntitiesInternal())
+            ++entityItr;
+        }
+    }
 }
 
 // private
@@ -44,16 +61,16 @@ void EntitySimulation::expireMortalEntities(const quint64& now) {
     if (now > _nextExpiry) {
         // only search for expired entities if we expect to find one
         _nextExpiry = quint64(-1);
-        QSet<EntityItem*>::iterator itemItr = _mortalEntities.begin();
+        SetOfEntities::iterator itemItr = _mortalEntities.begin();
         while (itemItr != _mortalEntities.end()) {
             EntityItem* entity = *itemItr;
             quint64 expiry = entity->getExpiry();
             if (expiry < now) {
                 _entitiesToDelete.insert(entity);
                 itemItr = _mortalEntities.erase(itemItr);
-                _updateableEntities.remove(entity);
-                _entitiesToBeSorted.remove(entity);
-                removeEntityInternal(entity);
+                _entitiesToUpdate.remove(entity);
+                _entitiesToSort.remove(entity);
+                deleteEntityInternal(entity);
             } else {
                 if (expiry < _nextExpiry) {
                     // remeber the smallest _nextExpiry so we know when to start the next search
@@ -68,13 +85,13 @@ void EntitySimulation::expireMortalEntities(const quint64& now) {
 // private
 void EntitySimulation::callUpdateOnEntitiesThatNeedIt(const quint64& now) {
     PerformanceTimer perfTimer("updatingEntities");
-    QSet<EntityItem*>::iterator itemItr = _updateableEntities.begin();
-    while (itemItr != _updateableEntities.end()) {
+    SetOfEntities::iterator itemItr = _entitiesToUpdate.begin();
+    while (itemItr != _entitiesToUpdate.end()) {
         EntityItem* entity = *itemItr;
         // TODO: catch transition from needing update to not as a "change" 
         // so we don't have to scan for it here.
         if (!entity->needsToCallUpdate()) {
-            itemItr = _updateableEntities.erase(itemItr);
+            itemItr = _entitiesToUpdate.erase(itemItr);
         } else {
             entity->update(now);
             ++itemItr;
@@ -89,8 +106,8 @@ void EntitySimulation::sortEntitiesThatMoved() {
     PerformanceTimer perfTimer("sortingEntities");
     MovingEntitiesOperator moveOperator(_entityTree);
     AACube domainBounds(glm::vec3(0.0f,0.0f,0.0f), (float)TREE_SCALE);
-    QSet<EntityItem*>::iterator itemItr = _entitiesToBeSorted.begin();
-    while (itemItr != _entitiesToBeSorted.end()) {
+    SetOfEntities::iterator itemItr = _entitiesToSort.begin();
+    while (itemItr != _entitiesToSort.end()) {
         EntityItem* entity = *itemItr;
         // check to see if this movement has sent the entity outside of the domain.
         AACube newCube = entity->getMaximumAACube();
@@ -98,9 +115,9 @@ void EntitySimulation::sortEntitiesThatMoved() {
             qCDebug(entities) << "Entity " << entity->getEntityItemID() << " moved out of domain bounds.";
             _entitiesToDelete.insert(entity);
             _mortalEntities.remove(entity);
-            _updateableEntities.remove(entity);
-            removeEntityInternal(entity);
-            itemItr = _entitiesToBeSorted.erase(itemItr);
+            _entitiesToUpdate.remove(entity);
+            deleteEntityInternal(entity);
+            itemItr = _entitiesToSort.erase(itemItr);
         } else {
             moveOperator.addEntityToMoveList(entity, newCube);
             ++itemItr;
@@ -112,83 +129,120 @@ void EntitySimulation::sortEntitiesThatMoved() {
     }
 
     sortEntitiesThatMovedInternal();
-    _entitiesToBeSorted.clear();
+    _entitiesToSort.clear();
 }
 
 void EntitySimulation::addEntity(EntityItem* entity) {
     assert(entity);
-    if (entity->isMortal()) {
-        _mortalEntities.insert(entity);
-        quint64 expiry = entity->getExpiry();
-        if (expiry < _nextExpiry) {
-            _nextExpiry = expiry;
+    if (!entity->_simulation) {
+        entity->_simulation = this;
+        if (entity->isMortal()) {
+            _mortalEntities.insert(entity);
+            quint64 expiry = entity->getExpiry();
+            if (expiry < _nextExpiry) {
+                _nextExpiry = expiry;
+            }
         }
+        if (entity->needsToCallUpdate()) {
+            _entitiesToUpdate.insert(entity);
+        }
+        addEntityInternal(entity);
+    
+        // DirtyFlags are used to signal changes to entities that have already been added, 
+        // so we can clear them for this entity which has just been added.
+        entity->clearDirtyFlags();
     }
-    if (entity->needsToCallUpdate()) {
-        _updateableEntities.insert(entity);
-    }
-    addEntityInternal(entity);
-
-    // DirtyFlags are used to signal changes to entities that have already been added, 
-    // so we can clear them for this entity which has just been added.
-    entity->clearDirtyFlags();
 }
 
 void EntitySimulation::removeEntity(EntityItem* entity) {
     assert(entity);
-    _updateableEntities.remove(entity);
-    _mortalEntities.remove(entity);
-    _entitiesToBeSorted.remove(entity);
-    _entitiesToDelete.remove(entity);
-    removeEntityInternal(entity);
+    if (entity->_simulation == this) {
+        _entitiesToUpdate.remove(entity);
+        _mortalEntities.remove(entity);
+        _entitiesToSort.remove(entity);
+        if (entity->_tree) {
+            // the tree still references this entity, but it's being removed from this simulation
+            _entitiesToDelete.remove(entity);
+            removeEntityInternal(entity);
+        } else {
+            // we're the last to reference this entity, so we really need to delete it
+            deleteEntityInternal(entity);
+        }
+    } else if (!entity->_tree) {
+        // nothing else is referencing this entity, so we delete it now
+        delete entity;
+    }
 }
 
-void EntitySimulation::entityChanged(EntityItem* entity) {
+void EntitySimulation::deleteEntity(EntityItem* entity) {
     assert(entity);
-
-    // Although it is not the responsibility of the EntitySimulation to sort the tree for EXTERNAL changes
-    // it IS responsibile for triggering deletes for entities that leave the bounds of the domain, hence 
-    // we must check for that case here, however we rely on the change event to have set DIRTY_POSITION flag.
-    bool wasRemoved = false;
-    uint32_t dirtyFlags = entity->getDirtyFlags();
-    if (dirtyFlags & EntityItem::DIRTY_POSITION) {
-        AACube domainBounds(glm::vec3(0.0f,0.0f,0.0f), (float)TREE_SCALE);
-        AACube newCube = entity->getMaximumAACube();
-        if (!domainBounds.touches(newCube)) {
-            qCDebug(entities) << "Entity " << entity->getEntityItemID() << " moved out of domain bounds.";
+    if (entity->_simulation == this) {
+        _entitiesToUpdate.remove(entity);
+        _mortalEntities.remove(entity);
+        _entitiesToSort.remove(entity);
+        deleteEntityInternal(entity);
+    } else {
+        if (entity->_tree) {
+            // the tree still references this entity, so we put it on the list 
+            // which will be harvested by the tree later
             _entitiesToDelete.insert(entity);
-            _mortalEntities.remove(entity);
-            _updateableEntities.remove(entity);
-            removeEntityInternal(entity);
-            wasRemoved = true;
+        } else {
+            // nothing else is referencing this entity, so we delete it now
+            delete entity;
         }
     }
-    if (!wasRemoved) {
-        if (dirtyFlags & EntityItem::DIRTY_LIFETIME) {
-            if (entity->isMortal()) {
-                _mortalEntities.insert(entity);
-                quint64 expiry = entity->getExpiry();
-                if (expiry < _nextExpiry) {
-                    _nextExpiry = expiry;
-                }
-            } else {
+}
+
+void EntitySimulation::changeEntity(EntityItem* entity) {
+    assert(entity);
+    if (entity->_simulation == this) {
+        // Although it is not the responsibility of the EntitySimulation to sort the tree for EXTERNAL changes
+        // it IS responsibile for triggering deletes for entities that leave the bounds of the domain, hence 
+        // we must check for that case here, however we rely on the change event to have set DIRTY_POSITION flag.
+        bool wasRemoved = false;
+        uint32_t dirtyFlags = entity->getDirtyFlags();
+        if (dirtyFlags & EntityItem::DIRTY_POSITION) {
+            AACube domainBounds(glm::vec3(0.0f,0.0f,0.0f), (float)TREE_SCALE);
+            AACube newCube = entity->getMaximumAACube();
+            if (!domainBounds.touches(newCube)) {
+                qCDebug(entities) << "Entity " << entity->getEntityItemID() << " moved out of domain bounds.";
+                _entitiesToDelete.insert(entity);
                 _mortalEntities.remove(entity);
+                _entitiesToUpdate.remove(entity);
+                deleteEntityInternal(entity);
+                wasRemoved = true;
             }
-            entity->clearDirtyFlags(EntityItem::DIRTY_LIFETIME);
         }
-        if (entity->needsToCallUpdate()) {
-            _updateableEntities.insert(entity);
-        } else {
-            _updateableEntities.remove(entity);
+        if (!wasRemoved) {
+            if (dirtyFlags & EntityItem::DIRTY_LIFETIME) {
+                if (entity->isMortal()) {
+                    _mortalEntities.insert(entity);
+                    quint64 expiry = entity->getExpiry();
+                    if (expiry < _nextExpiry) {
+                        _nextExpiry = expiry;
+                    }
+                } else {
+                    _mortalEntities.remove(entity);
+                }
+                entity->clearDirtyFlags(EntityItem::DIRTY_LIFETIME);
+            }
+            if (entity->needsToCallUpdate()) {
+                _entitiesToUpdate.insert(entity);
+            } else {
+                _entitiesToUpdate.remove(entity);
+            }
+            changeEntityInternal(entity);
         }
-        entityChangedInternal(entity);
+    } else {
+        // this entity is not yet in this simulation but something (the tree) assumes that it is --> try to add it
+        addEntity(entity);
     }
 }
 
 void EntitySimulation::clearEntities() {
     _mortalEntities.clear();
     _nextExpiry = quint64(-1);
-    _updateableEntities.clear();
-    _entitiesToBeSorted.clear();
+    _entitiesToUpdate.clear();
+    _entitiesToSort.clear();
     clearEntitiesInternal();
 }

libraries/entities/src/EntitySimulation.h

@@ -20,6 +20,8 @@
 #include "EntityItem.h"
 #include "EntityTree.h"
 
+typedef QSet<EntityItem*> SetOfEntities;
+
 // the EntitySimulation needs to know when these things change on an entity, 
 // so it can sort EntityItem or relay its state to the PhysicsEngine.
 const int DIRTY_SIMULATION_FLAGS = 
@@ -44,20 +46,25 @@ public:
     /// \param tree pointer to EntityTree which is stored internally
     void setEntityTree(EntityTree* tree);
 
-    /// \param[out] entitiesToDelete list of entities removed from simulation and should be deleted.
-    void updateEntities(QSet<EntityItem*>& entitiesToDelete);
+    void updateEntities();
 
-    /// \param entity pointer to EntityItem to add to the simulation
-    /// \sideeffect the EntityItem::_simulationState member may be updated to indicate membership to internal list
+    /// \param entity pointer to EntityItem to be added
+    /// \sideeffect sets relevant backpointers in entity, but maybe later when appropriate data structures are locked
     void addEntity(EntityItem* entity);
 
-    /// \param entity pointer to EntityItem to removed from the simulation
-    /// \sideeffect the EntityItem::_simulationState member may be updated to indicate non-membership to internal list
+    /// \param entity pointer to EntityItem to be removed
+    /// \brief the actual removal may happen later when appropriate data structures are locked
+    /// \sideeffect nulls relevant backpointers in entity
     void removeEntity(EntityItem* entity);
 
+    /// \param pointer to EntityItem to be removed from simulation, and deleted if possible
+    /// \brief actual removal/delete may happen later when appropriate data structures are locked
+    /// \sideeffect nulls relevant backpointers in entity
+    void deleteEntity(EntityItem* entity);
+
     /// \param entity pointer to EntityItem to that may have changed in a way that would affect its simulation
     /// call this whenever an entity was changed from some EXTERNAL event (NOT by the EntitySimulation itself)
-    void entityChanged(EntityItem* entity);
+    void changeEntity(EntityItem* entity);
 
     void clearEntities();
 
@@ -78,7 +85,9 @@ protected:
 
     virtual void removeEntityInternal(EntityItem* entity) = 0;
 
-    virtual void entityChangedInternal(EntityItem* entity) = 0;
+    virtual void deleteEntityInternal(EntityItem* entity) = 0;
+
+    virtual void changeEntityInternal(EntityItem* entity) = 0;
 
     virtual void sortEntitiesThatMovedInternal() {}
 
@@ -95,11 +104,11 @@ protected:
 
     // We maintain multiple lists, each for its distinct purpose.
     // An entity may be in more than one list.
-    QSet<EntityItem*> _mortalEntities; // entities that have an expiry
+    SetOfEntities _mortalEntities; // entities that have an expiry
     quint64 _nextExpiry;
-    QSet<EntityItem*> _updateableEntities; // entities that need update() called
-    QSet<EntityItem*> _entitiesToBeSorted; // entities that were moved by THIS simulation and might need to be resorted in the tree
-    QSet<EntityItem*> _entitiesToDelete;
+    SetOfEntities _entitiesToUpdate; // entities that need update() called
+    SetOfEntities _entitiesToSort; // entities that were moved by THIS simulation and might need to be resorted in the tree
+    SetOfEntities _entitiesToDelete;
 };
 
 #endif // hifi_EntitySimulation_h

libraries/entities/src/EntityTree.cpp

@@ -163,7 +163,7 @@ bool EntityTree::updateEntityWithElement(EntityItem* entity, const EntityItemPro
             if (_simulation) { 
                 if (newFlags & DIRTY_SIMULATION_FLAGS) {
                     _simulation->lock();
-                    _simulation->entityChanged(entity);
+                    _simulation->changeEntity(entity);
                     _simulation->unlock();
                 }
             } else {
@@ -349,10 +349,12 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator)
             _recentlyDeletedEntitiesLock.unlock();
         }
 
-        if (_simulation) {
-            _simulation->removeEntity(theEntity);
+        if (_simulation && theEntity->getSimulation()) {
+            // delegate entity destruction to the simulation so it can clean up its own pointers
+            _simulation->deleteEntity(theEntity);
+        } else {
+            delete theEntity; // we can delete the entity directly now
         }
-        delete theEntity; // now actually delete the entity!
     }
     if (_simulation) {
         _simulation->unlock();
@@ -742,7 +744,7 @@ void EntityTree::releaseSceneEncodeData(OctreeElementExtraEncodeData* extraEncod
 void EntityTree::entityChanged(EntityItem* entity) {
     if (_simulation) {
         _simulation->lock();
-        _simulation->entityChanged(entity);
+        _simulation->changeEntity(entity);
         _simulation->unlock();
     }
 }
@@ -750,10 +752,12 @@ void EntityTree::entityChanged(EntityItem* entity) {
 void EntityTree::update() {
     if (_simulation) {
         lockForWrite();
-        QSet<EntityItem*> entitiesToDelete;
         _simulation->lock();
-        _simulation->updateEntities(entitiesToDelete);
+        _simulation->updateEntities();
+        SetOfEntities entitiesToDelete;
+        _simulation->getEntitiesToDelete(entitiesToDelete);
         _simulation->unlock();
+
         if (entitiesToDelete.size() > 0) {
             // translate into list of ID's
             QSet<EntityItemID> idsToDelete;

libraries/physics/src/EntityMotionState.cpp

@@ -47,7 +47,6 @@ EntityMotionState::EntityMotionState(EntityItem* entity) :
 
 EntityMotionState::~EntityMotionState() {
     assert(_entity);
-    _entity->setPhysicsInfo(NULL);
     _entity = NULL;
 }
 

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -74,28 +74,26 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItem* entity) {
     }
 }
 
-void PhysicalEntitySimulation::get {
-        if (entity->isReadyToComputeShape()) {
-            ShapeInfo shapeInfo;
-            entity->computeShapeInfo(shapeInfo);
-            btCollisionShape* shape = _physicsEngine->getShape(shapeInfo);
-            if (shape) {
-                EntityMotionState* motionState = new EntityMotionState(entity);
-                entity->setPhysicsInfo(static_cast<void*>(motionState));
-                _physicalEntities.insert(motionState);
-                _physicsEngine->addObject(shapeInfo, shape, motionState);
-                motionState->setPhysical(true);
-            } 
-        }
-    }
-}
-
 void PhysicalEntitySimulation::removeEntityInternal(EntityItem* entity) {
     assert(entity);
     void* physicsInfo = entity->getPhysicsInfo();
     if (physicsInfo) {
         ObjectMotionState* motionState = static_cast<ObjectMotionState*>(physicsInfo);
         _pendingRemoves.insert(motionState);
+    } else {
+        entity->_simulation = nullptr;
+    }
+}
+
+void PhysicalEntitySimulation::deleteEntityInternal(EntityItem* entity) {
+    assert(entity);
+    void* physicsInfo = entity->getPhysicsInfo();
+    if (physicsInfo) {
+        ObjectMotionState* motionState = static_cast<ObjectMotionState*>(physicsInfo);
+        _pendingRemoves.insert(motionState);
+    } else {
+        entity->_simulation = nullptr;
+        delete entity;
     }
 }
 
@@ -105,7 +103,7 @@ void PhysicalEntitySimulation::entityChangedInternal(EntityItem* entity) {
     void* physicsInfo = entity->getPhysicsInfo();
     if (physicsInfo) {
         ObjectMotionState* motionState = static_cast<ObjectMotionState*>(physicsInfo);
-        _pendingUpdates.insert(motionState);
+        _pendingChanges.insert(motionState);
     } else { 
         if (!entity->ignoreForCollisions()) {
             // The intent is for this object to be in the PhysicsEngine.  
@@ -119,49 +117,52 @@ void PhysicalEntitySimulation::sortEntitiesThatMovedInternal() {
     // entities that have been simulated forward (hence in the _entitiesToBeSorted list) 
     // also need to be put in the outgoingPackets list
     QSet<EntityItem*>::iterator entityItr = _entitiesToBeSorted.begin();
-    while (entityItr != _entitiesToBeSorted.end()) {
+    for (auto entityItr : _entitiesToBeSorted) {
         EntityItem* entity = *entityItr;
         void* physicsInfo = entity->getPhysicsInfo();
         assert(physicsInfo);
-        ObjectMotionState* motionState = static_cast<ObjectMotionState*>(physicsInfo);
-        _outgoingPackets.insert(motionState);
-        ++entityItr;
+        // BOOKMARK XXX -- Andrew to fix this next
+        _outgoingPackets.insert(static_cast<ObjectMotionState*>(physicsInfo));
     }
 }
 
 void PhysicalEntitySimulation::clearEntitiesInternal() {
+    // TODO: we should probably wait to lock the _physicsEngine so we don't mess up data structures
+    // while it is in the middle of a simulation step.  As it is, we're probably in shutdown mode
+    // anyway, so maybe the simulation was already properly shutdown?  Cross our fingers...
     SetOfMotionStates::const_iterator stateItr = _physicalEntities.begin();
-    for (stateItr = _physicalEntities.begin(); stateItr != _physicalEntities.end(); ++stateItr) {
-        removeObjectFromBullet(*stateItr);
-        delete (*stateItr);
+    for (auto stateItr : _physicalEntities) {
+        EntityMotionState motionState = static_cast<EntityMotionState*>(*stateItr);
+        _physicsEngine->removeObjectFromBullet(motionState);
+        EntityItem* entity = motionState->_entity;
+        _entity->setPhysicsInfo(nullptr);
+        delete motionState;
     }
     _physicalEntities.clear();
     _pendingRemoves.clear();
     _pendingAdds.clear();
-    _pendingUpdates.clear();
+    _pendingChanges.clear();
 }
 // end EntitySimulation overrides
 
 
-SetOfMotionState& PhysicalEntitySimulation::getObjectsToRemove() {
+VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToRemove() {
     _tempSet.clear();
     for (auto entityItr : _pendingRemoves) {
         EntityItem* entity = *entityItr;
+        _physicalEntities.remove(entity);
         _pendingAdds.remove(entity);
-        _pendingUpdates.remove(entity);
+        _pendingChanges.remove(entity);
         ObjectMotionState* motionState = static_cast<ObjectMotionState*>(entity->getPhysicsInfo());
         if (motionState) {
             _tempSet.push_back(motionState);
         }
     }
-    // DO NOT clear _pendingRemoves -- we still need to remove from _physicalEntities 
-    // and then communicate to the EntityTree that they can really can be deleted.
-    // TODO: build the pipeline for this.
-    //_pendingRemoves.clear();
+    _pendingRemoves.clear();
     return _tempSet;
 }
 
-SetOfMotionState& PhysicalEntitySimulation::getObjectsToAdd() {
+VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToAdd() {
     _tempSet.clear();
     SetOfEntities::iterator entityItr = _pendingAdds.begin();
     while (entityItr != _pendingAdds.end()) {
@@ -189,25 +190,19 @@ SetOfMotionState& PhysicalEntitySimulation::getObjectsToAdd() {
     return _tempSet;
 }
 
-SetOfMotionState& PhysicalEntitySimulation::getObjectsToUpdate() {
+VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToChange() {
     _tempSet.clear();
-    for (auto entityItr : _pendingUpdates) {
+    for (auto entityItr : _pendingChanges) {
         EntityItem* entity = *entityItr;
         ObjectMotionState* motionState = static_cast<ObjectMotionState*>(entity->getPhysicsInfo());
         if (motionState) {
             _tempSet.push_back(motionState);
         }
     }
-    _pendingUpdates.clear();
+    _pendingChanges.clear();
     return _tempSet;
 }
 
-void PhysicalEntitySimulation::clearIncomingChanges() {
-    // TODO: finalize deletes in the EntityTree?  
-    // or should we allow EntityMotionState::_entity to be NULL during normal operation?
-    // (In order to do this _pendingDeletes would have to be list of MotionStates, or something)
-}
-
 void PhysicalEntitySimulation::bump(EntityItem* bumpEntity) {
 }
 

libraries/physics/src/PhysicalEntitySimulation.h

@@ -15,6 +15,7 @@
 #include <stdint.h>
 
 #include <QSet>
+#include <QVector>
 #include <btBulletDynamicsCommon.h>
 #include <BulletCollision/CollisionDispatch/btGhostObject.h>
 
@@ -23,7 +24,8 @@
 
 #include "PhysicsEngine.h"
 
-typedef QSet<EntityItem*> SetOfEntities;
+typedef QSet<ObjectMotionState*> SetOfMotionStates;
+typedef QVector<ObjectMotionState*> VectorOfMotionStates;
 
 class PhysicalEntitySimulation :public EntitySimulation {
 public:
@@ -36,22 +38,21 @@ public:
     void updateEntitiesInternal(const quint64& now);
     void addEntityInternal(EntityItem* entity);
     void removeEntityInternal(EntityItem* entity);
+    void deleteEntityInternal(EntityItem* entity);
     void entityChangedInternal(EntityItem* entity);
     void sortEntitiesThatMovedInternal();
     void clearEntitiesInternal();
 
-    SetOfMotionState& getObjectsToRemove();
-    SetOfMotionState& getObjectsToAdd();
-    SetOfMotionState& getObjectsToUpdate();
-
-    void clearIncomingChanges();
+    VectorOfMotionStates& getObjectsToRemove();
+    VectorOfMotionStates& getObjectsToAdd();
+    VectorOfMotionStates& getObjectsToChange();
 
 private:
     void bump(EntityItem* bumpEntity);
 
     SetOfEntities _pendingRemoves; // entities to be removed from simulation
     SetOfEntities _pendingAdds; // entities to be be added to simulation
-    SetOfEntities _pendingUpdates; // entities to be updated in simulation
+    SetOfEntities _pendingChanges; // entities already in simulation that need to be changed
 
     SetOfMotionStates _physicalEntities; // MotionStates of entities in PhysicsEngine
     VectorOfMotionStates _tempSet; // temporary list valid immediately after call to getObjectsToRemove/Add/Update()