From d8fb61e820628b29c2c676e832c3bc1d789762f7 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 21 Jan 2016 17:30:04 -0800 Subject: [PATCH] fix crash when deleting entities quickly --- interface/src/Application.cpp | 5 ++- libraries/entities/src/EntitySimulation.cpp | 7 +++ .../entities/src/SimpleEntitySimulation.cpp | 4 ++ .../physics/src/PhysicalEntitySimulation.cpp | 45 +++++++++++++------ .../physics/src/PhysicalEntitySimulation.h | 1 + 5 files changed, 47 insertions(+), 15 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 4a4c9c6bc7..2266463962 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -3089,13 +3089,14 @@ void Application::update(float deltaTime) { static VectorOfMotionStates motionStates; _entitySimulation.getObjectsToRemoveFromPhysics(motionStates); _physicsEngine->removeObjects(motionStates); + _entitySimulation.deleteObjectsRemovedFromPhysics(); - getEntities()->getTree()->withWriteLock([&] { + getEntities()->getTree()->withReadLock([&] { _entitySimulation.getObjectsToAddToPhysics(motionStates); _physicsEngine->addObjects(motionStates); }); - getEntities()->getTree()->withWriteLock([&] { + getEntities()->getTree()->withReadLock([&] { _entitySimulation.getObjectsToChange(motionStates); VectorOfMotionStates stillNeedChange = _physicsEngine->changeObjects(motionStates); _entitySimulation.setObjectsToChange(stillNeedChange); diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index ab29cfb2a4..23e5b99337 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -48,6 +48,7 @@ void EntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesToDelete) } void EntitySimulation::removeEntityInternal(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); // remove from all internal lists except _entitiesToDelete _mortalEntities.remove(entity); _entitiesToUpdate.remove(entity); @@ -61,6 +62,7 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { assert(entity); assert(entity->isDead()); if (entity->isSimulated()) { + QMutexLocker lock(&_mutex); entity->clearActions(this); removeEntityInternal(entity); _entitiesToDelete.insert(entity); @@ -69,11 +71,13 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { void EntitySimulation::addEntityInternal(EntityItemPointer entity) { if (entity->isMoving() && !entity->getPhysicsInfo()) { + QMutexLocker lock(&_mutex); _simpleKinematicEntities.insert(entity); } } void EntitySimulation::changeEntityInternal(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); if (entity->isMoving() && !entity->getPhysicsInfo()) { _simpleKinematicEntities.insert(entity); } else { @@ -86,6 +90,7 @@ void EntitySimulation::expireMortalEntities(const quint64& now) { if (now > _nextExpiry) { // only search for expired entities if we expect to find one _nextExpiry = quint64(-1); + QMutexLocker lock(&_mutex); SetOfEntities::iterator itemItr = _mortalEntities.begin(); while (itemItr != _mortalEntities.end()) { EntityItemPointer entity = *itemItr; @@ -108,6 +113,7 @@ void EntitySimulation::expireMortalEntities(const quint64& now) { // protected void EntitySimulation::callUpdateOnEntitiesThatNeedIt(const quint64& now) { PerformanceTimer perfTimer("updatingEntities"); + QMutexLocker lock(&_mutex); SetOfEntities::iterator itemItr = _entitiesToUpdate.begin(); while (itemItr != _entitiesToUpdate.end()) { EntityItemPointer entity = *itemItr; @@ -124,6 +130,7 @@ void EntitySimulation::callUpdateOnEntitiesThatNeedIt(const quint64& now) { // protected void EntitySimulation::sortEntitiesThatMoved() { + QMutexLocker lock(&_mutex); // 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. PerformanceTimer perfTimer("sortingEntities"); diff --git a/libraries/entities/src/SimpleEntitySimulation.cpp b/libraries/entities/src/SimpleEntitySimulation.cpp index 3b2523bc92..120b536161 100644 --- a/libraries/entities/src/SimpleEntitySimulation.cpp +++ b/libraries/entities/src/SimpleEntitySimulation.cpp @@ -23,6 +23,7 @@ void SimpleEntitySimulation::updateEntitiesInternal(const quint64& now) { // has finished simulating it. auto nodeList = DependencyManager::get(); + QMutexLocker lock(&_mutex); SetOfEntities::iterator itemItr = _entitiesWithSimulator.begin(); while (itemItr != _entitiesWithSimulator.end()) { EntityItemPointer entity = *itemItr; @@ -48,18 +49,21 @@ void SimpleEntitySimulation::updateEntitiesInternal(const quint64& now) { void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) { EntitySimulation::addEntityInternal(entity); if (!entity->getSimulatorID().isNull()) { + QMutexLocker lock(&_mutex); _entitiesWithSimulator.insert(entity); } } void SimpleEntitySimulation::removeEntityInternal(EntityItemPointer entity) { EntitySimulation::removeEntityInternal(entity); + QMutexLocker lock(&_mutex); _entitiesWithSimulator.remove(entity); } void SimpleEntitySimulation::changeEntityInternal(EntityItemPointer entity) { EntitySimulation::changeEntityInternal(entity); if (!entity->getSimulatorID().isNull()) { + QMutexLocker lock(&_mutex); _entitiesWithSimulator.insert(entity); } entity->clearDirtyFlags(); diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index a677cfae4d..d2c91f29dd 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -43,6 +43,7 @@ void PhysicalEntitySimulation::updateEntitiesInternal(const quint64& now) { } void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); assert(entity); assert(!entity->isDead()); if (entity->shouldBePhysical()) { @@ -57,6 +58,7 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) { void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) { EntitySimulation::removeEntityInternal(entity); + QMutexLocker lock(&_mutex); _entitiesToAddToPhysics.remove(entity); EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); @@ -78,8 +80,7 @@ void PhysicalEntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesTo // rather than do it here EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); if (motionState) { - delete motionState; - entity->setPhysicsInfo(nullptr); + _entitiesToRemoveFromPhysics.insert(entity); } } _entitiesToDelete.clear(); @@ -87,6 +88,7 @@ void PhysicalEntitySimulation::takeEntitiesToDelete(VectorOfEntities& entitiesTo void PhysicalEntitySimulation::changeEntityInternal(EntityItemPointer entity) { // queue incoming changes: from external sources (script, EntityServer, etc) to physics engine + QMutexLocker lock(&_mutex); assert(entity); EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); if (motionState) { @@ -119,7 +121,7 @@ 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... - // first disconnect each MotionStates from its Entity + // copy everything into _entitiesToDelete for (auto stateItr : _physicalObjects) { EntityMotionState* motionState = static_cast(&(*stateItr)); _entitiesToDelete.insert(motionState->getEntity()); @@ -134,8 +136,8 @@ void PhysicalEntitySimulation::clearEntitiesInternal() { for (auto entity : _entitiesToDelete) { EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); if (motionState) { - delete motionState; entity->setPhysicsInfo(nullptr); + delete motionState; } } @@ -158,22 +160,39 @@ void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) // end EntitySimulation overrides void PhysicalEntitySimulation::getObjectsToRemoveFromPhysics(VectorOfMotionStates& result) { - _entitiesToRelease.clear(); result.clear(); QMutexLocker lock(&_mutex); for (auto entity: _entitiesToRemoveFromPhysics) { - EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); - if (motionState) { - _pendingChanges.remove(motionState); - _physicalObjects.remove(motionState); - result.push_back(motionState); - } + // make sure it isn't on any side lists _entitiesToAddToPhysics.remove(entity); - if (entity->isDead()) { + + EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); + assert(motionState); + _pendingChanges.remove(motionState); + _physicalObjects.remove(motionState); + result.push_back(motionState); + _entitiesToRelease.insert(entity); + + if (entity->isSimulated() && entity->isDead()) { _entitiesToDelete.insert(entity); } } - _entitiesToRemoveFromPhysics.swap(_entitiesToRelease); + _entitiesToRemoveFromPhysics.clear(); +} + +void PhysicalEntitySimulation::deleteObjectsRemovedFromPhysics() { + QMutexLocker lock(&_mutex); + for (auto entity: _entitiesToRelease) { + EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); + assert(motionState); + entity->setPhysicsInfo(nullptr); + delete motionState; + + if (entity->isSimulated() && entity->isDead()) { + _entitiesToDelete.insert(entity); + } + } + _entitiesToRelease.clear(); } void PhysicalEntitySimulation::getObjectsToAddToPhysics(VectorOfMotionStates& result) { diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 1c783d2f31..7138fb924c 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -49,6 +49,7 @@ public: virtual void prepareEntityForDelete(EntityItemPointer entity) override; void getObjectsToRemoveFromPhysics(VectorOfMotionStates& result); + void deleteObjectsRemovedFromPhysics(); void getObjectsToAddToPhysics(VectorOfMotionStates& result); void setObjectsToChange(const VectorOfMotionStates& objectsToChange); void getObjectsToChange(VectorOfMotionStates& result);