From e1d2a5e5f3477020956d77a663562a8a1574b70e Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 12 Mar 2018 15:26:11 -0700 Subject: [PATCH] more cleanup for deleting entities --- libraries/entities/src/EntityItem.cpp | 7 --- libraries/entities/src/EntitySimulation.cpp | 19 +++--- libraries/entities/src/EntitySimulation.h | 4 +- libraries/entities/src/EntityTree.cpp | 11 ++-- libraries/entities/src/MaterialEntityItem.cpp | 7 ++- .../entities/src/SimpleEntitySimulation.cpp | 1 - .../physics/src/PhysicalEntitySimulation.cpp | 62 ++++++++----------- .../physics/src/PhysicalEntitySimulation.h | 4 +- 8 files changed, 47 insertions(+), 68 deletions(-) diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 5d056e17d8..901d7cf424 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/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) { diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 24e139da61..0edb7492d5 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/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 _entitiesToDelete + // remove from all internal lists except _deadEntities _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) { diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index bdd205c11a..92a3e3207b 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/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(); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 1171b79bbc..7a3ed91d51 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/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; - _simulation->takeEntitiesToDelete(pendingDeletes); - if (pendingDeletes.size() > 0) { + VectorOfEntities deadEntities; + _simulation->takeDeadEntities(deadEntities); + if (deadEntities.size() > 0) { // translate into list of ID's QSet idsToDelete; - for (auto entity : pendingDeletes) { + for (auto entity : deadEntities) { idsToDelete.insert(entity->getEntityItemID()); } diff --git a/libraries/entities/src/MaterialEntityItem.cpp b/libraries/entities/src/MaterialEntityItem.cpp index 137d6ef396..8b595bf69d 100644 --- a/libraries/entities/src/MaterialEntityItem.cpp +++ b/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); -} \ No newline at end of file +} diff --git a/libraries/entities/src/SimpleEntitySimulation.cpp b/libraries/entities/src/SimpleEntitySimulation.cpp index c7353fb53a..583a310b41 100644 --- a/libraries/entities/src/SimpleEntitySimulation.cpp +++ b/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); } diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 8ca514fea5..fb7123b7b6 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/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(entity->getPhysicsInfo()); if (motionState) { _outgoingChanges.remove(motionState); _entitiesToRemoveFromPhysics.insert(entity); - } else { - _entitiesToDelete.insert(entity); + } else if (entity->isDead() && entity->getElement()) { + _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(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 - for (auto stateItr : _physicalObjects) { - EntityMotionState* motionState = static_cast(&(*stateItr)); - _entitiesToDelete.insert(motionState->getEntity()); - } - - // then remove the objects (aka MotionStates) from physics + // 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 - // its own PhysicsInfo rather than do it here - for (auto entity : _entitiesToDelete) { - EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); - if (motionState) { - entity->setPhysicsInfo(nullptr); - // someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo - // until then we must do it here - delete motionState; - } + for (auto stateItr : _physicalObjects) { + EntityMotionState* motionState = static_cast(&(*stateItr)); + assert(motionState); + EntityItemPointer entity = motionState->getEntity(); + entity->setPhysicsInfo(nullptr); + // TODO: someday when we invert the entities/physics lib dependencies we can let EntityItem delete its own PhysicsInfo + // until then we must do it here + 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); - } - - if (entity->isDead()) { - _entitiesToDelete.insert(entity); + _entitiesToForget.insert(entity); + } else if (entity->isDead() && entity->getElement()) { + _deadEntities.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(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()) { - _entitiesToDelete.insert(entity); + if (entity->isDead() && entity->getElement()) { + _deadEntities.insert(entity); } } - _entitiesToRelease.clear(); + _entitiesToForget.clear(); } void PhysicalEntitySimulation::getObjectsToAddToPhysics(VectorOfMotionStates& result) { diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 24d97cf509..0823198e2d 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/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