more cleanup for deleting entities

libraries/entities/src/EntityItem.cpp

@@ -2962,13 +2962,6 @@ void EntityItem::retrieveMarketplacePublicKey() {
 }
 
 void EntityItem::preDelete() {
-    // clear out any left-over actions
-    EntityTreeElementPointer element = _element; // use local copy of _element for logic below
-    EntityTreePointer entityTree = element ? element->getTree() : nullptr;
-    EntitySimulationPointer simulation = entityTree ? entityTree->getSimulation() : nullptr;
-    if (simulation) {
-        clearActions(simulation);
-    }
 }
 
 void EntityItem::addMaterial(graphics::MaterialLayer material, const std::string& parentMaterialName) {

libraries/entities/src/EntitySimulation.cpp

@@ -40,18 +40,17 @@ void EntitySimulation::updateEntities() {
     sortEntitiesThatMoved();
 }
 
-void EntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) {
+void EntitySimulation::takeDeadEntities(VectorOfEntities& entitiesToDelete) {
     QMutexLocker lock(&_mutex);
-    for (auto entity : _entitiesToDelete) {
+    for (auto entity : _deadEntities) {
         // push this entity onto the external list
         entitiesToDelete.push_back(entity);
     }
-    _entitiesToDelete.clear();
+    _deadEntities.clear();
 }
 
 void EntitySimulation::removeEntityInternal(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
+    // remove from all internal lists except _deadEntities
-    // remove from all internal lists except _entitiesToDelete
     _mortalEntities.remove(entity);
     _entitiesToUpdate.remove(entity);
     _entitiesToSort.remove(entity);
@@ -67,7 +66,9 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
         QMutexLocker lock(&_mutex);
         entity->clearActions(getThisPointer());
         removeEntityInternal(entity);
-        _entitiesToDelete.insert(entity);
+        if (entity->getElement()) {
+            _deadEntities.insert(entity);
+        }
     }
 }
 
@@ -229,12 +230,8 @@ void EntitySimulation::clearEntities() {
 
     clearEntitiesInternal();
 
-    for (auto entity : _allEntities) {
-        entity->setSimulated(false);
-        entity->die();
-    }
     _allEntities.clear();
-    _entitiesToDelete.clear();
+    _deadEntities.clear();
 }
 
 void EntitySimulation::moveSimpleKinematics(const quint64& now) {

libraries/entities/src/EntitySimulation.h

@@ -75,7 +75,7 @@ public:
 
     EntityTreePointer getEntityTree() { return _entityTree; }
 
-    virtual void takeEntitiesToDelete(VectorOfEntities& entitiesToDelete);
+    virtual void takeDeadEntities(VectorOfEntities& entitiesToDelete);
 
     /// \param entity pointer to EntityItem that needs to be put on the entitiesToDelete list and removed from others.
     virtual void prepareEntityForDelete(EntityItemPointer entity);
@@ -102,7 +102,7 @@ protected:
     QMutex _dynamicsMutex { QMutex::Recursive };
 
 protected:
-    SetOfEntities _entitiesToDelete; // entities simulation decided needed to be deleted (EntityTree will actually delete)
+    SetOfEntities _deadEntities;
 
 private:
     void moveSimpleKinematics();

libraries/entities/src/EntityTree.cpp

@@ -94,7 +94,6 @@ OctreeElementPointer EntityTree::createNewElement(unsigned char* octalCode) {
 void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
     emit clearingEntities();
 
-    // this would be a good place to clean up our entities...
     if (_simulation) {
         _simulation->clearEntities();
     }
@@ -552,8 +551,6 @@ void EntityTree::setSimulation(EntitySimulationPointer simulation) {
             assert(simulation->getEntityTree().get() == this);
         }
         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->clearEntities();
         }
         _simulation = simulation;
@@ -1803,13 +1800,13 @@ void EntityTree::update(bool simulate) {
             _simulation->updateEntities();
             {
                 PROFILE_RANGE(simulation_physics, "Deletes");
-                VectorOfEntities pendingDeletes;
+                VectorOfEntities deadEntities;
-                _simulation->takeEntitiesToDelete(pendingDeletes);
+                _simulation->takeDeadEntities(deadEntities);
-                if (pendingDeletes.size() > 0) {
+                if (deadEntities.size() > 0) {
                     // translate into list of ID's
                     QSet<EntityItemID> idsToDelete;
 
-                    for (auto entity : pendingDeletes) {
+                    for (auto entity : deadEntities) {
                         idsToDelete.insert(entity->getEntityItemID());
                     }
 

libraries/entities/src/MaterialEntityItem.cpp

@@ -267,8 +267,11 @@ void MaterialEntityItem::setOwningAvatarID(const QUuid& owningAvatarID) {
 
 void MaterialEntityItem::removeMaterial() {
     graphics::MaterialPointer material = getMaterial();
+    if (!material) {
+        return;
+    }
     QUuid parentID = getClientOnly() ? getOwningAvatarID() : getParentID();
-    if (!material || parentID.isNull()) {
+    if (parentID.isNull()) {
         return;
     }
 
@@ -336,4 +339,4 @@ void MaterialEntityItem::update(const quint64& now) {
     }
 
     EntityItem::update(now);
-}
+}

libraries/entities/src/SimpleEntitySimulation.cpp

@@ -105,7 +105,6 @@ void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
 
 void SimpleEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
     EntitySimulation::removeEntityInternal(entity);
-    QMutexLocker lock(&_mutex);
     _entitiesWithSimulationOwner.remove(entity);
     _entitiesThatNeedSimulationOwner.remove(entity);
 }

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -61,31 +61,30 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
 void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
     if (entity->isSimulated()) {
         EntitySimulation::removeEntityInternal(entity);
-        QMutexLocker lock(&_mutex);
         _entitiesToAddToPhysics.remove(entity);
 
         EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
         if (motionState) {
             _outgoingChanges.remove(motionState);
             _entitiesToRemoveFromPhysics.insert(entity);
-        } else {
+        } else if (entity->isDead() && entity->getElement()) {
-            _entitiesToDelete.insert(entity);
+            _deadEntities.insert(entity);
         }
     }
 }
 
-void PhysicalEntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) {
+void PhysicalEntitySimulation::takeDeadEntities(VectorOfEntities& deadEntities) {
     QMutexLocker lock(&_mutex);
-    for (auto entity : _entitiesToDelete) {
+    for (auto entity : _deadEntities) {
         // this entity is still in its tree, so we insert into the external list
-        entitiesToDelete.push_back(entity);
+        deadEntities.push_back(entity);
 
         EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
         if (motionState) {
             _entitiesToRemoveFromPhysics.insert(entity);
         }
     }
-    _entitiesToDelete.clear();
+    _deadEntities.clear();
 }
 
 void PhysicalEntitySimulation::changeEntityInternal(EntityItemPointer entity) {
@@ -123,32 +122,24 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
     // 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...
 
-    // copy everything into _entitiesToDelete
+    // remove the objects (aka MotionStates) from physics
-    for (auto stateItr : _physicalObjects) {
-        EntityMotionState* motionState = static_cast<EntityMotionState*>(&(*stateItr));
-        _entitiesToDelete.insert(motionState->getEntity());
-    }
-
-    // then remove the objects (aka MotionStates) from physics
     _physicsEngine->removeSetOfObjects(_physicalObjects);
 
     // delete the MotionStates
-    // TODO: after we invert the entities/physics lib dependencies we will let EntityItem delete
+    for (auto stateItr : _physicalObjects) {
-    // its own PhysicsInfo rather than do it here
+        EntityMotionState* motionState = static_cast<EntityMotionState*>(&(*stateItr));
-    for (auto entity : _entitiesToDelete) {
+        assert(motionState);
-        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
+        EntityItemPointer entity = motionState->getEntity();
-        if (motionState) {
+        entity->setPhysicsInfo(nullptr);
-            entity->setPhysicsInfo(nullptr);
+        // TODO: someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo
-            // someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo
+        // until then we must do it here
-            // until then we must do it here
+        delete motionState;
-            delete motionState;
-        }
     }
-
-    // finally clear all lists maintained by this class
     _physicalObjects.clear();
+
+    // clear all other lists specific to this derived class
     _entitiesToRemoveFromPhysics.clear();
-    _entitiesToRelease.clear();
+    _entitiesToForget.clear();
     _entitiesToAddToPhysics.clear();
     _pendingChanges.clear();
     _outgoingChanges.clear();
@@ -158,6 +149,7 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
 void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
     assert(entity);
     assert(entity->isDead());
+    QMutexLocker lock(&_mutex);
     entity->clearActions(getThisPointer());
     removeEntityInternal(entity);
 }
@@ -176,11 +168,9 @@ void PhysicalEntitySimulation::getObjectsToRemoveFromPhysics(VectorOfMotionState
             _outgoingChanges.remove(motionState);
             _physicalObjects.remove(motionState);
             result.push_back(motionState);
-            _entitiesToRelease.insert(entity);
+            _entitiesToForget.insert(entity);
-        }
+        } else if (entity->isDead() && entity->getElement()) {
-
+            _deadEntities.insert(entity);
-        if (entity->isDead()) {
-            _entitiesToDelete.insert(entity);
         }
     }
     _entitiesToRemoveFromPhysics.clear();
@@ -188,7 +178,7 @@ void PhysicalEntitySimulation::getObjectsToRemoveFromPhysics(VectorOfMotionState
 
 void PhysicalEntitySimulation::deleteObjectsRemovedFromPhysics() {
     QMutexLocker lock(&_mutex);
-    for (auto entity: _entitiesToRelease) {
+    for (auto entity: _entitiesToForget) {
         EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
         assert(motionState);
         entity->setPhysicsInfo(nullptr);
@@ -196,11 +186,11 @@ void PhysicalEntitySimulation::deleteObjectsRemovedFromPhysics() {
         // until then we must do it here
         delete motionState;
 
-        if (entity->isDead()) {
+        if (entity->isDead() && entity->getElement()) {
-            _entitiesToDelete.insert(entity);
+            _deadEntities.insert(entity);
         }
     }
-    _entitiesToRelease.clear();
+    _entitiesToForget.clear();
 }
 
 void PhysicalEntitySimulation::getObjectsToAddToPhysics(VectorOfMotionStates& result) {

libraries/physics/src/PhysicalEntitySimulation.h

@@ -38,7 +38,7 @@ public:
     virtual void addDynamic(EntityDynamicPointer dynamic) override;
     virtual void applyDynamicChanges() override;
 
-    virtual void takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) override;
+    virtual void takeDeadEntities(VectorOfEntities& deadEntities) override;
 
 signals:
     void entityCollisionWithEntity(const EntityItemID& idA, const EntityItemID& idB, const Collision& collision);
@@ -68,7 +68,7 @@ public:
 
 private:
     SetOfEntities _entitiesToRemoveFromPhysics;
-    SetOfEntities _entitiesToRelease;
+    SetOfEntities _entitiesToForget;
     SetOfEntities _entitiesToAddToPhysics;
 
     SetOfEntityMotionStates _pendingChanges; // EntityMotionStates already in PhysicsEngine that need their physics changed