simplify add/removal to EntitySimulation

libraries/entities/src/EntitySimulation.cpp

@@ -47,7 +47,7 @@ void EntitySimulation::takeDeadEntities(SetOfEntities& entitiesToDelete) {
     _deadEntities.clear();
 }
 
-void EntitySimulation::removeEntityInternal(EntityItemPointer entity) {
+void EntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
     // remove from all internal lists except _deadEntities
     _mortalEntities.remove(entity);
     _entitiesToUpdate.remove(entity);
@@ -62,7 +62,7 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
     assert(entity->isDead());
     if (entity->isSimulated()) {
         QMutexLocker lock(&_mutex);
-        removeEntityInternal(entity);
+        removeEntityFromInternalLists(entity);
         if (entity->getElement()) {
             _deadEntities.insert(entity);
             _entityTree->cleanupCloneIDs(entity->getEntityItemID());
@@ -148,9 +148,7 @@ void EntitySimulation::sortEntitiesThatMoved() {
     _entitiesToSort.clear();
 }
 
-void EntitySimulation::addEntity(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
-    assert(entity);
+void EntitySimulation::addEntityToInternalLists(EntityItemPointer entity) {
     if (entity->isMortal()) {
         _mortalEntities.insert(entity);
         uint64_t expiry = entity->getExpiry();
@@ -161,10 +159,14 @@ void EntitySimulation::addEntity(EntityItemPointer entity) {
     if (entity->needsToCallUpdate()) {
         _entitiesToUpdate.insert(entity);
     }
-    addEntityInternal(entity);
-
     _allEntities.insert(entity);
     entity->setSimulated(true);
+}
+
+void EntitySimulation::addEntity(EntityItemPointer entity) {
+    QMutexLocker lock(&_mutex);
+    assert(entity);
+    addEntityToInternalLists(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.

libraries/entities/src/EntitySimulation.h

@@ -88,8 +88,8 @@ protected:
     // These pure virtual methods are protected because they are not to be called will-nilly. The base class
     // calls them in the right places.
     virtual void updateEntitiesInternal(uint64_t now) = 0;
-    virtual void addEntityInternal(EntityItemPointer entity) = 0;
-    virtual void removeEntityInternal(EntityItemPointer entity);
+    virtual void addEntityToInternalLists(EntityItemPointer entity);
+    virtual void removeEntityFromInternalLists(EntityItemPointer entity);
     virtual void processChangedEntity(const EntityItemPointer& entity);
     virtual void clearEntitiesInternal() = 0;
 

libraries/entities/src/SimpleEntitySimulation.cpp

@@ -52,9 +52,9 @@ void SimpleEntitySimulation::updateEntitiesInternal(uint64_t now) {
     stopOwnerlessEntities(now);
 }
 
-void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
+void SimpleEntitySimulation::addEntityToInternalLists(EntityItemPointer entity) {
+    EntitySimulation::addEntityToInternalLists(entity);
     if (entity->getSimulatorID().isNull()) {
-        QMutexLocker lock(&_mutex);
         if (entity->getDynamic()) {
             // we don't allow dynamic objects to move without an owner so nothing to do here
         } else if (entity->isMovingRelativeToParent()) {
@@ -65,7 +65,6 @@ void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
             }
         }
     } else {
-        QMutexLocker lock(&_mutex);
         _entitiesWithSimulationOwner.insert(entity);
         _nextStaleOwnershipExpiry = glm::min(_nextStaleOwnershipExpiry, entity->getSimulationOwnershipExpiry());
 
@@ -79,10 +78,10 @@ void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
     }
 }
 
-void SimpleEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
-    EntitySimulation::removeEntityInternal(entity);
+void SimpleEntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
     _entitiesWithSimulationOwner.remove(entity);
     _entitiesThatNeedSimulationOwner.remove(entity);
+    EntitySimulation::removeEntityFromInternalLists(entity);
 }
 
 void SimpleEntitySimulation::processChangedEntity(const EntityItemPointer& entity) {

libraries/entities/src/SimpleEntitySimulation.h

@@ -29,8 +29,8 @@ public:
 
 protected:
     void updateEntitiesInternal(uint64_t now) override;
-    void addEntityInternal(EntityItemPointer entity) override;
-    void removeEntityInternal(EntityItemPointer entity) override;
+    void addEntityToInternalLists(EntityItemPointer entity) override;
+    void removeEntityFromInternalLists(EntityItemPointer entity) override;
     void processChangedEntity(const EntityItemPointer& entity) override;
     void clearEntitiesInternal() override;
 

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -44,11 +44,9 @@ void PhysicalEntitySimulation::updateEntitiesInternal(uint64_t now) {
     // Do nothing here because the "internal" update the PhysicsEngine::stepSimulation() which is done elsewhere.
 }
 
-void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
-    assert(entity);
-    assert(!entity->isDead());
-    entity->deserializeActions();
+void PhysicalEntitySimulation::addEntityToInternalLists(EntityItemPointer entity) {
+    EntitySimulation::addEntityToInternalLists(entity);
+    entity->deserializeActions(); // TODO: do this elsewhere
     uint8_t region = _space->getRegion(entity->getSpaceIndex());
     bool maybeShouldBePhysical = (region < workload::Region::R3 || region == workload::Region::UNKNOWN) && entity->shouldBePhysical();
     bool canBeKinematic = region <= workload::Region::R3;
@@ -67,23 +65,19 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
     }
 }
 
-void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
-    if (entity->isSimulated()) {
-        EntitySimulation::removeEntityInternal(entity);
-        _entitiesToAddToPhysics.remove(entity);
-
-        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
-        if (motionState) {
-            removeOwnershipData(motionState);
-            _entitiesToRemoveFromPhysics.insert(entity);
-        }
-        if (entity->isDead() && entity->getElement()) {
-            _deadEntities.insert(entity);
-        }
+void PhysicalEntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
+    _entitiesToAddToPhysics.remove(entity);
+    EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
+    if (motionState) {
+        removeOwnershipData(motionState);
+        _entitiesToRemoveFromPhysics.insert(entity);
+    } else if (entity->isDead() && entity->getElement()) {
+        _deadEntities.insert(entity);
     }
     if (entity->isAvatarEntity()) {
         _deadAvatarEntities.insert(entity);
     }
+    EntitySimulation::removeEntityFromInternalLists(entity);
 }
 
 void PhysicalEntitySimulation::removeOwnershipData(EntityMotionState* motionState) {
@@ -233,7 +227,9 @@ void PhysicalEntitySimulation::removeDeadEntities() {
     QMutexLocker lock(&_mutex);
     for (auto& entity : _entitiesToDeleteLater) {
         entity->clearActions(getThisPointer());
-        removeEntityInternal(entity);
+        if (entity->isSimulated()) {
+            removeEntityFromInternalLists(entity);
+        }
     }
     _entitiesToDeleteLater.clear();
 }

libraries/physics/src/PhysicalEntitySimulation.h

@@ -72,8 +72,8 @@ signals:
 protected: // only called by EntitySimulation
     // overrides for EntitySimulation
     virtual void updateEntitiesInternal(uint64_t now) override;
-    virtual void addEntityInternal(EntityItemPointer entity) override;
-    virtual void removeEntityInternal(EntityItemPointer entity) override;
+    virtual void addEntityToInternalLists(EntityItemPointer entity) override;
+    virtual void removeEntityFromInternalLists(EntityItemPointer entity) override;
     void processChangedEntity(const EntityItemPointer& entity) override;
     virtual void clearEntitiesInternal() override;