From 86c60204fda4301009148a5c6b1c0c7dbba35219 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 20 Sep 2019 15:48:54 -0700 Subject: [PATCH 01/16] move entity Action serialization to PhysicalEntitySimulation --- libraries/entities/src/EntitySimulation.cpp | 2 -- libraries/physics/src/PhysicalEntitySimulation.cpp | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 9f81572a4a..4220779711 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -62,7 +62,6 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { assert(entity->isDead()); if (entity->isSimulated()) { QMutexLocker lock(&_mutex); - entity->clearActions(getThisPointer()); removeEntityInternal(entity); if (entity->getElement()) { _deadEntities.insert(entity); @@ -152,7 +151,6 @@ void EntitySimulation::sortEntitiesThatMoved() { void EntitySimulation::addEntity(EntityItemPointer entity) { QMutexLocker lock(&_mutex); assert(entity); - entity->deserializeActions(); if (entity->isMortal()) { _mortalEntities.insert(entity); uint64_t expiry = entity->getExpiry(); diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index df8c3fa32e..61daa85b42 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -48,6 +48,7 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) { QMutexLocker lock(&_mutex); assert(entity); assert(!entity->isDead()); + entity->deserializeActions(); 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; From 72ba94f2cdd93e5d2b96e6c93780f85c97f5149a Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 20 Sep 2019 17:02:31 -0700 Subject: [PATCH 02/16] unravel more spaghetti into PhysicalEntitySimulation --- libraries/entities/src/EntitySimulation.cpp | 23 ------------------- libraries/entities/src/EntitySimulation.h | 12 ++++------ .../physics/src/PhysicalEntitySimulation.cpp | 21 ++++++++++++++--- .../physics/src/PhysicalEntitySimulation.h | 12 ++++++++-- 4 files changed, 32 insertions(+), 36 deletions(-) diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 4220779711..17ea60ba1c 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -260,26 +260,3 @@ void EntitySimulation::moveSimpleKinematics(uint64_t now) { } } } - -void EntitySimulation::addDynamic(EntityDynamicPointer dynamic) { - QMutexLocker lock(&_dynamicsMutex); - _dynamicsToAdd += dynamic; -} - -void EntitySimulation::removeDynamic(const QUuid dynamicID) { - QMutexLocker lock(&_dynamicsMutex); - _dynamicsToRemove += dynamicID; -} - -void EntitySimulation::removeDynamics(QList dynamicIDsToRemove) { - QMutexLocker lock(&_dynamicsMutex); - foreach(QUuid uuid, dynamicIDsToRemove) { - _dynamicsToRemove.insert(uuid); - } -} - -void EntitySimulation::applyDynamicChanges() { - QMutexLocker lock(&_dynamicsMutex); - _dynamicsToAdd.clear(); - _dynamicsToRemove.clear(); -} diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index 1dd0369561..8312704974 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -21,7 +21,6 @@ #include -#include "EntityDynamicInterface.h" #include "EntityItem.h" #include "EntityTree.h" @@ -59,10 +58,10 @@ public: void updateEntities(); - virtual void addDynamic(EntityDynamicPointer dynamic); - virtual void removeDynamic(const QUuid dynamicID); - virtual void removeDynamics(QList dynamicIDsToRemove); - virtual void applyDynamicChanges(); + // FIXME: remove these + virtual void addDynamic(EntityDynamicPointer dynamic) {} + virtual void removeDynamic(const QUuid dynamicID) {} + virtual void applyDynamicChanges() {}; /// \param entity pointer to EntityItem to be added /// \sideeffect sets relevant backpointers in entity, but maybe later when appropriate data structures are locked @@ -102,9 +101,6 @@ protected: SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree) SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion - QList _dynamicsToAdd; - QSet _dynamicsToRemove; - QMutex _dynamicsMutex { QMutex::Recursive }; protected: SetOfEntities _deadEntities; // dead entities that might still be in the _entityTree diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 61daa85b42..4086369f4c 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -648,10 +648,25 @@ void PhysicalEntitySimulation::addDynamic(EntityDynamicPointer dynamic) { "dynamic that was already in _physicsEngine"; } } - EntitySimulation::addDynamic(dynamic); + QMutexLocker lock(&_dynamicsMutex); + _dynamicsToAdd += dynamic; } } +void PhysicalEntitySimulation::removeDynamic(const QUuid dynamicID) { + QMutexLocker lock(&_dynamicsMutex); + _dynamicsToRemove += dynamicID; +} + +/* +void PhysicalEntitySimulation::removeDynamics(QList dynamicIDsToRemove) { + QMutexLocker lock(&_dynamicsMutex); + foreach(QUuid uuid, dynamicIDsToRemove) { + _dynamicsToRemove.insert(uuid); + } +} +*/ + void PhysicalEntitySimulation::applyDynamicChanges() { QList dynamicsFailedToAdd; if (_physicsEngine) { @@ -666,8 +681,8 @@ void PhysicalEntitySimulation::applyDynamicChanges() { } } } - // applyDynamicChanges will clear _dynamicsToRemove and _dynamicsToAdd - EntitySimulation::applyDynamicChanges(); + _dynamicsToAdd.clear(); + _dynamicsToRemove.clear(); } // put back the ones that couldn't yet be added diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index f5213f7fef..14ace6eac2 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -58,8 +59,10 @@ public: void init(EntityTreePointer tree, PhysicsEnginePointer engine, EntityEditPacketSender* packetSender); void setWorkloadSpace(const workload::SpacePointer space) { _space = space; } - virtual void addDynamic(EntityDynamicPointer dynamic) override; - virtual void applyDynamicChanges() override; + void addDynamic(EntityDynamicPointer dynamic) override; + void removeDynamic(const QUuid dynamicID) override; + //void removeDynamics(QList dynamicIDsToRemove); + void applyDynamicChanges() override; virtual void takeDeadEntities(SetOfEntities& deadEntities) override; void takeDeadAvatarEntities(SetOfEntities& deadEntities); @@ -123,6 +126,11 @@ private: VectorOfEntityMotionStates _bids; SetOfEntities _deadAvatarEntities; std::vector _entitiesToDeleteLater; + + QList _dynamicsToAdd; + QSet _dynamicsToRemove; + QMutex _dynamicsMutex { QMutex::Recursive }; + workload::SpacePointer _space; uint64_t _nextBidExpiry; uint32_t _lastStepSendPackets { 0 }; From c8e875ff726128a974cbe06e1fd3fd65409e0c61 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 20 Sep 2019 17:12:35 -0700 Subject: [PATCH 03/16] remove cruft --- libraries/physics/src/PhysicalEntitySimulation.cpp | 9 --------- libraries/physics/src/PhysicalEntitySimulation.h | 1 - 2 files changed, 10 deletions(-) diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 4086369f4c..e666b353f3 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -658,15 +658,6 @@ void PhysicalEntitySimulation::removeDynamic(const QUuid dynamicID) { _dynamicsToRemove += dynamicID; } -/* -void PhysicalEntitySimulation::removeDynamics(QList dynamicIDsToRemove) { - QMutexLocker lock(&_dynamicsMutex); - foreach(QUuid uuid, dynamicIDsToRemove) { - _dynamicsToRemove.insert(uuid); - } -} -*/ - void PhysicalEntitySimulation::applyDynamicChanges() { QList dynamicsFailedToAdd; if (_physicsEngine) { diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 14ace6eac2..ed530ba9b2 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -61,7 +61,6 @@ public: void addDynamic(EntityDynamicPointer dynamic) override; void removeDynamic(const QUuid dynamicID) override; - //void removeDynamics(QList dynamicIDsToRemove); void applyDynamicChanges() override; virtual void takeDeadEntities(SetOfEntities& deadEntities) override; From 03db88009f4640e77841774a09191dc9e004a78a Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 25 Sep 2019 14:13:47 -0700 Subject: [PATCH 04/16] simplify add/removal to EntitySimulation --- libraries/entities/src/EntitySimulation.cpp | 16 +++++---- libraries/entities/src/EntitySimulation.h | 4 +-- .../entities/src/SimpleEntitySimulation.cpp | 9 +++-- .../entities/src/SimpleEntitySimulation.h | 4 +-- .../physics/src/PhysicalEntitySimulation.cpp | 34 ++++++++----------- .../physics/src/PhysicalEntitySimulation.h | 4 +-- 6 files changed, 34 insertions(+), 37 deletions(-) diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 17ea60ba1c..b23bb4df30 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/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. diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index 8312704974..f80d78412a 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/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; diff --git a/libraries/entities/src/SimpleEntitySimulation.cpp b/libraries/entities/src/SimpleEntitySimulation.cpp index b8e3df2d03..f5e7810858 100644 --- a/libraries/entities/src/SimpleEntitySimulation.cpp +++ b/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) { diff --git a/libraries/entities/src/SimpleEntitySimulation.h b/libraries/entities/src/SimpleEntitySimulation.h index 1b240a8bf0..6194193f5a 100644 --- a/libraries/entities/src/SimpleEntitySimulation.h +++ b/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; diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index e666b353f3..7335f2b1ee 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/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(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(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(); } diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index ed530ba9b2..044eb47dcb 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/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; From 9557ba80c95873b4441c10d8c232c13c5192b8e1 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Wed, 25 Sep 2019 16:31:21 -0700 Subject: [PATCH 05/16] all delete pass through EntitySimulation::prepareEntityForDelete() --- libraries/entities/src/EntitySimulation.cpp | 16 ++++++++-------- libraries/entities/src/EntitySimulation.h | 3 +-- .../physics/src/PhysicalEntitySimulation.cpp | 12 ++++++------ 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index b23bb4df30..8a38905567 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -19,11 +19,11 @@ void EntitySimulation::setEntityTree(EntityTreePointer tree) { if (_entityTree && _entityTree != tree) { - _mortalEntities.clear(); - _nextExpiry = std::numeric_limits::max(); - _entitiesToUpdate.clear(); _entitiesToSort.clear(); _simpleKinematicEntities.clear(); + _entitiesToUpdate.clear(); + _mortalEntities.clear(); + _nextExpiry = std::numeric_limits::max(); } _entityTree = tree; } @@ -49,11 +49,11 @@ void EntitySimulation::takeDeadEntities(SetOfEntities& entitiesToDelete) { void EntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) { // remove from all internal lists except _deadEntities - _mortalEntities.remove(entity); - _entitiesToUpdate.remove(entity); _entitiesToSort.remove(entity); _simpleKinematicEntities.remove(entity); _allEntities.remove(entity); + _entitiesToUpdate.remove(entity); + _mortalEntities.remove(entity); entity->setSimulated(false); } @@ -218,11 +218,11 @@ void EntitySimulation::processChangedEntity(const EntityItemPointer& entity) { void EntitySimulation::clearEntities() { QMutexLocker lock(&_mutex); - _mortalEntities.clear(); - _nextExpiry = std::numeric_limits::max(); - _entitiesToUpdate.clear(); _entitiesToSort.clear(); _simpleKinematicEntities.clear(); + _entitiesToUpdate.clear(); + _mortalEntities.clear(); + _nextExpiry = std::numeric_limits::max(); clearEntitiesInternal(); diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index f80d78412a..22d098168f 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -115,10 +115,9 @@ private: // An entity may be in more than one list. std::unordered_set _changedEntities; // all changes this frame SetOfEntities _allEntities; // tracks all entities added the simulation + SetOfEntities _entitiesToUpdate; // entities that need to call EntityItem::update() SetOfEntities _mortalEntities; // entities that have an expiry uint64_t _nextExpiry; - - SetOfEntities _entitiesToUpdate; // entities that need to call EntityItem::update() }; #endif // hifi_EntitySimulation_h diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 7335f2b1ee..b3f4f28e29 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -71,7 +71,8 @@ void PhysicalEntitySimulation::removeEntityFromInternalLists(EntityItemPointer e if (motionState) { removeOwnershipData(motionState); _entitiesToRemoveFromPhysics.insert(entity); - } else if (entity->isDead() && entity->getElement()) { + } + if (entity->isDead() && entity->getElement()) { _deadEntities.insert(entity); } if (entity->isAvatarEntity()) { @@ -215,7 +216,8 @@ void PhysicalEntitySimulation::clearEntitiesInternal() { // virtual void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { - // this can be called on any thread + // DANGER! this can be called on any thread + // do no dirty deeds here --> assemble list for later assert(entity); assert(entity->isDead()); QMutexLocker lock(&_mutex); @@ -223,13 +225,11 @@ void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) } void PhysicalEntitySimulation::removeDeadEntities() { - // only ever call this on the main thread + // DANGER! only ever call this on the main thread QMutexLocker lock(&_mutex); for (auto& entity : _entitiesToDeleteLater) { entity->clearActions(getThisPointer()); - if (entity->isSimulated()) { - removeEntityFromInternalLists(entity); - } + EntitySimulation::prepareEntityForDelete(entity); } _entitiesToDeleteLater.clear(); } From 99d05790072932a32a204ba25ea5d861d274c913 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 26 Sep 2019 10:40:47 -0700 Subject: [PATCH 06/16] remove updateEntitiesInternal(), make updateEntities() virtual --- libraries/entities/src/EntitySimulation.cpp | 12 +++++------ libraries/entities/src/EntitySimulation.h | 21 +++++++------------ .../entities/src/SimpleEntitySimulation.cpp | 8 +++++-- .../entities/src/SimpleEntitySimulation.h | 6 +++--- .../physics/src/PhysicalEntitySimulation.cpp | 9 ++++---- .../physics/src/PhysicalEntitySimulation.h | 4 ++-- 6 files changed, 27 insertions(+), 33 deletions(-) diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 8a38905567..976cae9bd9 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -21,6 +21,7 @@ void EntitySimulation::setEntityTree(EntityTreePointer tree) { if (_entityTree && _entityTree != tree) { _entitiesToSort.clear(); _simpleKinematicEntities.clear(); + _changedEntities.clear(); _entitiesToUpdate.clear(); _mortalEntities.clear(); _nextExpiry = std::numeric_limits::max(); @@ -29,15 +30,14 @@ void EntitySimulation::setEntityTree(EntityTreePointer tree) { } void EntitySimulation::updateEntities() { + PerformanceTimer perfTimer("EntitySimulation::updateEntities"); QMutexLocker lock(&_mutex); uint64_t now = usecTimestampNow(); - PerformanceTimer perfTimer("EntitySimulation::updateEntities"); // these methods may accumulate entries in _entitiesToBeDeleted expireMortalEntities(now); callUpdateOnEntitiesThatNeedIt(now); moveSimpleKinematics(now); - updateEntitiesInternal(now); sortEntitiesThatMoved(); } @@ -220,14 +220,12 @@ void EntitySimulation::clearEntities() { QMutexLocker lock(&_mutex); _entitiesToSort.clear(); _simpleKinematicEntities.clear(); + _changedEntities.clear(); + _allEntities.clear(); + _deadEntities.clear(); _entitiesToUpdate.clear(); _mortalEntities.clear(); _nextExpiry = std::numeric_limits::max(); - - clearEntitiesInternal(); - - _allEntities.clear(); - _deadEntities.clear(); } void EntitySimulation::moveSimpleKinematics(uint64_t now) { diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index 22d098168f..e9ad7ab2b7 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -46,8 +46,8 @@ const int DIRTY_SIMULATION_FLAGS = class EntitySimulation : public QObject, public std::enable_shared_from_this { public: - EntitySimulation() : _mutex(QMutex::Recursive), _entityTree(NULL), _nextExpiry(std::numeric_limits::max()) { } - virtual ~EntitySimulation() { setEntityTree(NULL); } + EntitySimulation() : _mutex(QMutex::Recursive), _nextExpiry(std::numeric_limits::max()), _entityTree(nullptr) { } + virtual ~EntitySimulation() { setEntityTree(nullptr); } inline EntitySimulationPointer getThisPointer() const { return std::const_pointer_cast(shared_from_this()); @@ -56,7 +56,7 @@ public: /// \param tree pointer to EntityTree which is stored internally void setEntityTree(EntityTreePointer tree); - void updateEntities(); + virtual void updateEntities(); // FIXME: remove these virtual void addDynamic(EntityDynamicPointer dynamic) {} @@ -71,7 +71,7 @@ public: /// call this whenever an entity was changed from some EXTERNAL event (NOT by the EntitySimulation itself) void changeEntity(EntityItemPointer entity); - void clearEntities(); + virtual void clearEntities(); void moveSimpleKinematics(uint64_t now); @@ -79,19 +79,14 @@ public: virtual void takeDeadEntities(SetOfEntities& entitiesToDelete); - /// \param entity pointer to EntityItem that needs to be put on the entitiesToDelete list and removed from others. virtual void prepareEntityForDelete(EntityItemPointer entity); void processChangedEntities(); 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 addEntityToInternalLists(EntityItemPointer entity); virtual void removeEntityFromInternalLists(EntityItemPointer entity); virtual void processChangedEntity(const EntityItemPointer& entity); - virtual void clearEntitiesInternal() = 0; void expireMortalEntities(uint64_t now); void callUpdateOnEntitiesThatNeedIt(uint64_t now); @@ -101,16 +96,11 @@ protected: SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree) SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion - -protected: SetOfEntities _deadEntities; // dead entities that might still be in the _entityTree private: void moveSimpleKinematics(); - // back pointer to EntityTree structure - EntityTreePointer _entityTree; - // We maintain multiple lists, each for its distinct purpose. // An entity may be in more than one list. std::unordered_set _changedEntities; // all changes this frame @@ -118,6 +108,9 @@ private: SetOfEntities _entitiesToUpdate; // entities that need to call EntityItem::update() SetOfEntities _mortalEntities; // entities that have an expiry uint64_t _nextExpiry; + + // back pointer to EntityTree structure + EntityTreePointer _entityTree; }; #endif // hifi_EntitySimulation_h diff --git a/libraries/entities/src/SimpleEntitySimulation.cpp b/libraries/entities/src/SimpleEntitySimulation.cpp index f5e7810858..d64efdf87f 100644 --- a/libraries/entities/src/SimpleEntitySimulation.cpp +++ b/libraries/entities/src/SimpleEntitySimulation.cpp @@ -47,7 +47,10 @@ void SimpleEntitySimulation::clearOwnership(const QUuid& ownerID) { } } -void SimpleEntitySimulation::updateEntitiesInternal(uint64_t now) { +void SimpleEntitySimulation::updateEntities() { + EntitySimulation::updateEntities(); + QMutexLocker lock(&_mutex); + uint64_t now = usecTimestampNow(); expireStaleOwnerships(now); stopOwnerlessEntities(now); } @@ -134,10 +137,11 @@ void SimpleEntitySimulation::processChangedEntity(const EntityItemPointer& entit entity->clearDirtyFlags(); } -void SimpleEntitySimulation::clearEntitiesInternal() { +void SimpleEntitySimulation::clearEntities() { QMutexLocker lock(&_mutex); _entitiesWithSimulationOwner.clear(); _entitiesThatNeedSimulationOwner.clear(); + EntitySimulation::clearEntities(); } void SimpleEntitySimulation::sortEntitiesThatMoved() { diff --git a/libraries/entities/src/SimpleEntitySimulation.h b/libraries/entities/src/SimpleEntitySimulation.h index 6194193f5a..e984b72ed4 100644 --- a/libraries/entities/src/SimpleEntitySimulation.h +++ b/libraries/entities/src/SimpleEntitySimulation.h @@ -23,16 +23,16 @@ using SimpleEntitySimulationPointer = std::shared_ptr; class SimpleEntitySimulation : public EntitySimulation { public: SimpleEntitySimulation() : EntitySimulation() { } - ~SimpleEntitySimulation() { clearEntitiesInternal(); } + ~SimpleEntitySimulation() { clearEntities(); } void clearOwnership(const QUuid& ownerID); + void clearEntities() override; + void updateEntities() override; protected: - void updateEntitiesInternal(uint64_t now) override; void addEntityToInternalLists(EntityItemPointer entity) override; void removeEntityFromInternalLists(EntityItemPointer entity) override; void processChangedEntity(const EntityItemPointer& entity) override; - void clearEntitiesInternal() override; void sortEntitiesThatMoved() override; diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index b3f4f28e29..6356a439d3 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -40,10 +40,6 @@ void PhysicalEntitySimulation::init( } // begin EntitySimulation overrides -void PhysicalEntitySimulation::updateEntitiesInternal(uint64_t now) { - // Do nothing here because the "internal" update the PhysicsEngine::stepSimulation() which is done elsewhere. -} - void PhysicalEntitySimulation::addEntityToInternalLists(EntityItemPointer entity) { EntitySimulation::addEntityToInternalLists(entity); entity->deserializeActions(); // TODO: do this elsewhere @@ -186,11 +182,12 @@ void PhysicalEntitySimulation::processChangedEntity(const EntityItemPointer& ent } } -void PhysicalEntitySimulation::clearEntitiesInternal() { +void PhysicalEntitySimulation::clearEntities() { // 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... + QMutexLocker lock(&_mutex); // remove the objects (aka MotionStates) from physics _physicsEngine->removeSetOfObjects(_physicalObjects); @@ -212,6 +209,8 @@ void PhysicalEntitySimulation::clearEntitiesInternal() { _entitiesToAddToPhysics.clear(); _incomingChanges.clear(); _entitiesToDeleteLater.clear(); + + EntitySimulation::clearEntities(); } // virtual diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 044eb47dcb..7efb000050 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -66,16 +66,16 @@ public: virtual void takeDeadEntities(SetOfEntities& deadEntities) override; void takeDeadAvatarEntities(SetOfEntities& deadEntities); + virtual void clearEntities() override; + signals: void entityCollisionWithEntity(const EntityItemID& idA, const EntityItemID& idB, const Collision& collision); protected: // only called by EntitySimulation // overrides for EntitySimulation - virtual void updateEntitiesInternal(uint64_t now) override; virtual void addEntityToInternalLists(EntityItemPointer entity) override; virtual void removeEntityFromInternalLists(EntityItemPointer entity) override; void processChangedEntity(const EntityItemPointer& entity) override; - virtual void clearEntitiesInternal() override; void removeOwnershipData(EntityMotionState* motionState); void clearOwnershipData(); From 3155ffe07e8d794e7d2c29b72d862b272b78f9aa Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 27 Sep 2019 11:18:18 -0700 Subject: [PATCH 07/16] remove useless unreachable code --- interface/src/InterfaceParentFinder.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/interface/src/InterfaceParentFinder.cpp b/interface/src/InterfaceParentFinder.cpp index 33328f54cc..0f1c8876a9 100644 --- a/interface/src/InterfaceParentFinder.cpp +++ b/interface/src/InterfaceParentFinder.cpp @@ -45,10 +45,6 @@ SpatiallyNestableWeakPointer InterfaceParentFinder::find(QUuid parentID, bool& s success = true; return parent; } - if (parentID == AVATAR_SELF_ID) { - success = true; - return avatarManager->getMyAvatar(); - } success = false; return parent; From aef8ac6bf0557e09095e7ffb7b8955945059a8c8 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 27 Sep 2019 14:07:55 -0700 Subject: [PATCH 08/16] make SpatialParentFinder base class pure virtual --- libraries/shared/src/SpatialParentFinder.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/shared/src/SpatialParentFinder.h b/libraries/shared/src/SpatialParentFinder.h index c19babbc7f..8300359b65 100644 --- a/libraries/shared/src/SpatialParentFinder.h +++ b/libraries/shared/src/SpatialParentFinder.h @@ -21,7 +21,7 @@ using SpatiallyNestableWeakPointer = std::weak_ptr; using SpatiallyNestablePointer = std::shared_ptr; class SpatialParentTree { public: - virtual SpatiallyNestablePointer findByID(const QUuid& id) const { return nullptr; } + virtual SpatiallyNestablePointer findByID(const QUuid& id) const = 0; }; class SpatialParentFinder : public Dependency { From 5a81ed73fe8e5da4d92d578eb80347f53248bcd4 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 27 Sep 2019 14:08:11 -0700 Subject: [PATCH 09/16] don't allow null Physics::sessionUUID --- libraries/shared/src/PhysicsHelpers.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libraries/shared/src/PhysicsHelpers.cpp b/libraries/shared/src/PhysicsHelpers.cpp index 092b9d078a..b7f5242429 100644 --- a/libraries/shared/src/PhysicsHelpers.cpp +++ b/libraries/shared/src/PhysicsHelpers.cpp @@ -10,10 +10,12 @@ // #include "PhysicsHelpers.h" -#include "NumericalConstants.h" + #include +#include "NumericalConstants.h" #include "PhysicsCollisionGroups.h" +#include "SharedUtil.h" // This chunk of code was copied from Bullet-2.82, so we include the Bullet license here: /* @@ -91,7 +93,7 @@ int32_t Physics::getDefaultCollisionMask(int32_t group) { QUuid _sessionID; void Physics::setSessionUUID(const QUuid& sessionID) { - _sessionID = sessionID; + _sessionID = sessionID.isNull() ? AVATAR_SELF_ID : sessionID; } const QUuid& Physics::getSessionUUID() { From 470a45c012d0fe6c3ee41dd8022e10d7b8dc89c3 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 27 Sep 2019 14:14:26 -0700 Subject: [PATCH 10/16] EntityItem::_owningAvatarID always AVATAR_SELF_ID for MyAvatar's entities --- interface/src/avatar/MyAvatar.cpp | 16 +++++++++---- .../src/EntityTreeRenderer.cpp | 10 ++++---- .../src/RenderableEntityItem.cpp | 5 ++-- .../entities/src/EntityEditPacketSender.cpp | 8 +++++-- libraries/entities/src/EntityItem.cpp | 23 +++++++++++++++++-- libraries/entities/src/EntityItem.h | 3 ++- .../entities/src/EntityScriptingInterface.cpp | 21 +++++++---------- libraries/entities/src/EntityTree.cpp | 5 ++-- libraries/entities/src/EntityTreeElement.cpp | 2 +- libraries/physics/src/EntityMotionState.cpp | 15 +++++++++--- libraries/physics/src/EntityMotionState.h | 2 +- 11 files changed, 73 insertions(+), 37 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index de6ae526b4..7aeb0aaa64 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1585,7 +1585,12 @@ void MyAvatar::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFrom void MyAvatar::sanitizeAvatarEntityProperties(EntityItemProperties& properties) const { properties.setEntityHostType(entity::HostType::AVATAR); - properties.setOwningAvatarID(getID()); + + // Note: we store AVATAR_SELF_ID in EntityItem::_owningAvatarID and we usually + // store the actual sessionUUID in EntityItemProperties::_owningAvatarID (for JS + // consumption, for example). However at this context we are preparing properties + // for outgoing packet, in which case we use AVATAR_SELF_ID. + properties.setOwningAvatarID(AVATAR_SELF_ID); // there's no entity-server to tell us we're the simulation owner, so always set the // simulationOwner to the owningAvatarID and a high priority. @@ -4000,6 +4005,10 @@ float MyAvatar::getGravity() { void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { QUuid oldSessionID = getSessionUUID(); Avatar::setSessionUUID(sessionUUID); + bool sendPackets = !DependencyManager::get()->getSessionUUID().isNull(); + if (!sendPackets) { + return; + } QUuid newSessionID = getSessionUUID(); if (newSessionID != oldSessionID) { auto treeRenderer = DependencyManager::get(); @@ -4009,7 +4018,6 @@ void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { _avatarEntitiesLock.withReadLock([&] { avatarEntityIDs = _packedAvatarEntityData.keys(); }); - bool sendPackets = !DependencyManager::get()->getSessionUUID().isNull(); EntityEditPacketSender* packetSender = qApp->getEntityEditPacketSender(); entityTree->withWriteLock([&] { for (const auto& entityID : avatarEntityIDs) { @@ -4017,11 +4025,9 @@ void MyAvatar::setSessionUUID(const QUuid& sessionUUID) { if (!entity) { continue; } - // update OwningAvatarID so entity can be identified as "ours" later - entity->setOwningAvatarID(newSessionID); // NOTE: each attached AvatarEntity already have the correct updated parentID // via magic in SpatiallyNestable, hence we check against newSessionID - if (sendPackets && entity->getParentID() == newSessionID) { + if (entity->getParentID() == newSessionID) { // but when we have a real session and the AvatarEntity is parented to MyAvatar // we need to update the "packedAvatarEntityData" sent to the avatar-mixer // because it contains a stale parentID somewhere deep inside diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 52738bb6cd..6533d462a4 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -221,7 +221,7 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() { EntityItemPointer entityItem = getTree()->findEntityByEntityItemID(entityID); if (entityItem && !entityItem->getScript().isEmpty()) { - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { if (_currentEntitiesInside.contains(entityID)) { _entitiesScriptEngine->callEntityScriptMethod(entityID, "leaveEntity"); } @@ -235,7 +235,6 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() { void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { stopDomainAndNonOwnedEntities(); - auto sessionUUID = getTree()->getMyAvatarSessionUUID(); std::unordered_map savedEntities; std::unordered_set savedRenderables; // remove all entities from the scene @@ -244,7 +243,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { for (const auto& entry : _entitiesInScene) { const auto& renderer = entry.second; const EntityItemPointer& entityItem = renderer->getEntity(); - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == sessionUUID))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { fadeOutRenderable(renderer); } else { savedEntities[entry.first] = entry.second; @@ -256,6 +255,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() { _renderablesToUpdate = savedRenderables; _entitiesInScene = savedEntities; + auto sessionUUID = getTree()->getMyAvatarSessionUUID(); if (_layeredZones.clearDomainAndNonOwnedZones(sessionUUID)) { applyLayeredZones(); } @@ -678,7 +678,7 @@ void EntityTreeRenderer::leaveDomainAndNonOwnedEntities() { QSet currentEntitiesInsideToSave; foreach (const EntityItemID& entityID, _currentEntitiesInside) { EntityItemPointer entityItem = getTree()->findEntityByEntityItemID(entityID); - if (!(entityItem->isLocalEntity() || (entityItem->isAvatarEntity() && entityItem->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entityItem->isLocalEntity() || entityItem->isMyAvatarEntity())) { emit leaveEntity(entityID); if (_entitiesScriptEngine) { _entitiesScriptEngine->callEntityScriptMethod(entityID, "leaveEntity"); @@ -1216,7 +1216,7 @@ bool EntityTreeRenderer::LayeredZones::clearDomainAndNonOwnedZones(const QUuid& auto it = begin(); while (it != end()) { auto zone = it->zone.lock(); - if (!zone || !(zone->isLocalEntity() || (zone->isAvatarEntity() && zone->getOwningAvatarID() == sessionUUID))) { + if (!zone || !(zone->isLocalEntity() || zone->isMyAvatarEntity())) { zonesChanged = true; it = erase(it); } else { diff --git a/libraries/entities-renderer/src/RenderableEntityItem.cpp b/libraries/entities-renderer/src/RenderableEntityItem.cpp index fb3d2f1bf5..0e5dab6524 100644 --- a/libraries/entities-renderer/src/RenderableEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableEntityItem.cpp @@ -43,6 +43,7 @@ const Transform& EntityRenderer::getModelTransform() const { void EntityRenderer::makeStatusGetters(const EntityItemPointer& entity, Item::Status::Getters& statusGetters) { auto nodeList = DependencyManager::get(); + // DANGER: nodeList->getSessionUUID() will return null id when not connected to domain. const QUuid& myNodeID = nodeList->getSessionUUID(); statusGetters.push_back([entity]() -> render::Item::Status::Value { @@ -103,9 +104,9 @@ void EntityRenderer::makeStatusGetters(const EntityItemPointer& entity, Item::St (unsigned char)render::Item::Status::Icon::HAS_ACTIONS); }); - statusGetters.push_back([entity, myNodeID] () -> render::Item::Status::Value { + statusGetters.push_back([entity] () -> render::Item::Status::Value { if (entity->isAvatarEntity()) { - if (entity->getOwningAvatarID() == myNodeID) { + if (entity->isMyAvatarEntity()) { return render::Item::Status::Value(1.0f, render::Item::Status::Value::GREEN, (unsigned char)render::Item::Status::Icon::ENTITY_HOST_TYPE); } else { diff --git a/libraries/entities/src/EntityEditPacketSender.cpp b/libraries/entities/src/EntityEditPacketSender.cpp index dbb3ab076e..aaaf7d645a 100644 --- a/libraries/entities/src/EntityEditPacketSender.cpp +++ b/libraries/entities/src/EntityEditPacketSender.cpp @@ -73,8 +73,12 @@ void EntityEditPacketSender::queueEditEntityMessage(PacketType type, if (properties.getEntityHostType() == entity::HostType::AVATAR) { if (!_myAvatar) { qCWarning(entities) << "Suppressing entity edit message: cannot send avatar entity edit with no myAvatar"; - } else if (properties.getOwningAvatarID() == _myAvatar->getID()) { - // this is an avatar-based entity --> update our avatar-data rather than sending to the entity-server + } else if (properties.getOwningAvatarID() == _myAvatar->getID() || properties.getOwningAvatarID() == AVATAR_SELF_ID) { + // this is a local avatar-entity --> update our avatar-data rather than sending to the entity-server + // Note: we store AVATAR_SELF_ID in EntityItem::_owningAvatarID and we usually + // store the actual sessionUUID in EntityItemProperties::_owningAvatarID. + // However at this context we check for both cases just in case. Really we just want to know + // where to route the data: entity-server or avatar-mixer. queueEditAvatarEntityMessage(entityTree, entityItemID); } else { qCWarning(entities) << "Suppressing entity edit message: cannot send avatar entity edit for another avatar"; diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 598ac17510..60a8cadf6c 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1347,7 +1347,7 @@ EntityItemProperties EntityItem::getProperties(const EntityPropertyFlags& desire COPY_ENTITY_PROPERTY_TO_PROPERTIES(created, getCreated); COPY_ENTITY_PROPERTY_TO_PROPERTIES(lastEditedBy, getLastEditedBy); COPY_ENTITY_PROPERTY_TO_PROPERTIES(entityHostType, getEntityHostType); - COPY_ENTITY_PROPERTY_TO_PROPERTIES(owningAvatarID, getOwningAvatarID); + COPY_ENTITY_PROPERTY_TO_PROPERTIES(owningAvatarID, getOwningAvatarIDForProperties); COPY_ENTITY_PROPERTY_TO_PROPERTIES(queryAACube, getQueryAACube); COPY_ENTITY_PROPERTY_TO_PROPERTIES(canCastShadow, getCanCastShadow); COPY_ENTITY_PROPERTY_TO_PROPERTIES(isVisibleInSecondaryCamera, isVisibleInSecondaryCamera); @@ -3398,6 +3398,7 @@ void EntityItem::prepareForSimulationOwnershipBid(EntityItemProperties& properti properties.setSimulationOwner(Physics::getSessionUUID(), priority); setPendingOwnershipPriority(priority); + // ANDREW TODO: figure out if it would be OK to NOT bother set these properties properties.setEntityHostType(getEntityHostType()); properties.setOwningAvatarID(getOwningAvatarID()); setLastBroadcast(now); // for debug/physics status icons @@ -3409,9 +3410,27 @@ bool EntityItem::isWearable() const { } bool EntityItem::isMyAvatarEntity() const { - return _hostType == entity::HostType::AVATAR && Physics::getSessionUUID() == _owningAvatarID; + return _hostType == entity::HostType::AVATAR && AVATAR_SELF_ID == _owningAvatarID; }; +QUuid EntityItem::getOwningAvatarIDForProperties() const { + if (isMyAvatarEntity()) { + // NOTE: we always store AVATAR_SELF_ID for MyAvatar's avatar entities, + // however for EntityItemProperties to be consumed by outside contexts (e.g. JS) + // we use the actual "sessionUUID" which is conveniently cached in the Physics namespace + return Physics::getSessionUUID(); + } + return _owningAvatarID; +} + +void EntityItem::setOwningAvatarID(const QUuid& owningAvatarID) { + if (!owningAvatarID.isNull() && owningAvatarID == Physics::getSessionUUID()) { + _owningAvatarID = AVATAR_SELF_ID; + } else { + _owningAvatarID = owningAvatarID; + } +} + void EntityItem::addGrab(GrabPointer grab) { enableNoBootstrap(); SpatiallyNestable::addGrab(grab); diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 3274379ee9..bdccd466cb 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -514,7 +514,8 @@ public: // if this entity is an avatar entity, which avatar is it associated with? QUuid getOwningAvatarID() const { return _owningAvatarID; } - virtual void setOwningAvatarID(const QUuid& owningAvatarID) { _owningAvatarID = owningAvatarID; } + QUuid getOwningAvatarIDForProperties() const; + void setOwningAvatarID(const QUuid& owningAvatarID); virtual bool wantsHandControllerPointerEvents() const { return false; } virtual bool wantsKeyboardFocus() const { return false; } diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 7cfdc8a68d..803c1ac4ca 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -480,17 +480,11 @@ QUuid EntityScriptingInterface::addEntityInternal(const EntityItemProperties& pr _activityTracking.addedEntityCount++; - auto nodeList = DependencyManager::get(); - auto sessionID = nodeList->getSessionUUID(); - EntityItemProperties propertiesWithSimID = properties; propertiesWithSimID.setEntityHostType(entityHostType); if (entityHostType == entity::HostType::AVATAR) { - if (sessionID.isNull()) { - // null sessionID is unacceptable in this case - sessionID = AVATAR_SELF_ID; - } - propertiesWithSimID.setOwningAvatarID(sessionID); + // only allow adding our own avatar entities from script + propertiesWithSimID.setOwningAvatarID(AVATAR_SELF_ID); } else if (entityHostType == entity::HostType::LOCAL) { // For now, local entities are always collisionless // TODO: create a separate, local physics simulation that just handles local entities (and MyAvatar?) @@ -498,6 +492,8 @@ QUuid EntityScriptingInterface::addEntityInternal(const EntityItemProperties& pr } // the created time will be set in EntityTree::addEntity by recordCreationTime() + auto nodeList = DependencyManager::get(); + auto sessionID = nodeList->getSessionUUID(); propertiesWithSimID.setLastEditedBy(sessionID); bool scalesWithParent = propertiesWithSimID.getScalesWithParent(); @@ -805,7 +801,7 @@ QUuid EntityScriptingInterface::editEntity(const QUuid& id, const EntityItemProp return; } - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != sessionID && entity->getOwningAvatarID() != AVATAR_SELF_ID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { // don't edit other avatar's avatarEntities properties = EntityItemProperties(); return; @@ -978,10 +974,9 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) { _entityTree->withWriteLock([&] { EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); if (entity) { - auto nodeList = DependencyManager::get(); const QUuid myNodeID = nodeList->getSessionUUID(); - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { // don't delete other avatar's avatarEntities shouldSendDeleteToServer = false; return; @@ -990,7 +985,7 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) { if (entity->getLocked()) { shouldSendDeleteToServer = false; } else { - // only delete local entities, server entities will round trip through the server filters + // only delete non-domain entities, domain entities will round trip through the server filters if (!entity->isDomainEntity() || _entityTree->isServerlessMode()) { shouldSendDeleteToServer = false; _entityTree->deleteEntity(entityID); @@ -1671,7 +1666,7 @@ bool EntityScriptingInterface::actionWorker(const QUuid& entityID, return; } - if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { return; } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 6c12c6d019..6440f39f52 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -99,7 +99,7 @@ void EntityTree::eraseDomainAndNonOwnedEntities() { element->cleanupDomainAndNonOwnedEntities(); } - if (entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getMyAvatarSessionUUID())) { + if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { savedEntities[entity->getEntityItemID()] = entity; } else { int32_t spaceIndex = entity->getSpaceIndex(); @@ -121,7 +121,7 @@ void EntityTree::eraseDomainAndNonOwnedEntities() { foreach (EntityItemWeakPointer entityItem, _needsParentFixup) { auto entity = entityItem.lock(); - if (entity && (entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getMyAvatarSessionUUID()))) { + if (entity && (entity->isLocalEntity() || entity->isMyAvatarEntity())) { needParentFixup.push_back(entityItem); } } @@ -688,6 +688,7 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i // NOTE: callers must lock the tree before using this method DeleteEntityOperator theOperator(getThisPointer()); foreach(const EntityItemID& entityID, entityIDs) { + // ANDREW TODO: rejigger this logic to FIRST get entity THEN get element EntityTreeElementPointer containingElement = getContainingElement(entityID); if (!containingElement) { if (!ignoreWarnings) { diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp index 60eaafc0dd..0096319081 100644 --- a/libraries/entities/src/EntityTreeElement.cpp +++ b/libraries/entities/src/EntityTreeElement.cpp @@ -705,7 +705,7 @@ void EntityTreeElement::cleanupDomainAndNonOwnedEntities() { withWriteLock([&] { EntityItems savedEntities; foreach(EntityItemPointer entity, _entityItems) { - if (!(entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getTree()->getMyAvatarSessionUUID()))) { + if (!(entity->isLocalEntity() || entity->isMyAvatarEntity())) { entity->preDelete(); entity->_element = NULL; } else { diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index e48f0603bd..2ec2eff7f9 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -62,7 +62,8 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer // rather than pass the legit shape pointer to the ObjectMotionState ctor above. setShape(shape); - if (_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID()) { + // ANDREW TODO: maybe all non-domain entities should be unownable? or our avatar entities should have a special OwnershipState + if (_entity->isAvatarEntity() && !_entity->isMyAvatarEntity()) { // avatar entities are always thus, so we cache this fact in _ownershipState _ownershipState = EntityMotionState::OwnershipState::Unownable; } @@ -407,8 +408,8 @@ bool EntityMotionState::shouldSendUpdate(uint32_t simulationStep) { // NOTE: we expect _entity and _body to be valid in this context, since shouldSendUpdate() is only called // after doesNotNeedToSendUpdate() returns false and that call should return 'true' if _entity or _body are NULL. - // this case is prevented by setting _ownershipState to UNOWNABLE in EntityMotionState::ctor - assert(!(_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID())); + // this case is prevented by setting _ownershipState to OwnershipState::Unownable in EntityMotionState::ctor + assert(!(_entity->isAvatarEntity() && !_entity->isMyAvatarEntity())); if (_entity->getTransitingWithAvatar()) { return false; @@ -747,6 +748,7 @@ bool EntityMotionState::shouldSendBid() const { // NOTE: this method is only ever called when the entity's simulation is NOT locally owned return _body->isActive() && (_region == workload::Region::R1) + // ANDREW TODO: make sure _ownershipState to Unownable on creation when applicable && _ownershipState != EntityMotionState::OwnershipState::Unownable && glm::max(glm::max(VOLUNTEER_SIMULATION_PRIORITY, _bumpedPriority), _entity->getScriptSimulationPriority()) >= _entity->getSimulationPriority() && !_entity->getLocked() @@ -768,6 +770,7 @@ uint8_t EntityMotionState::computeFinalBidPriority() const { } bool EntityMotionState::isLocallyOwned() const { + // ANDREW TODO: maybe also always return true for MyAvatar's avatar entities? return _entity->getSimulatorID() == Physics::getSessionUUID(); } @@ -795,6 +798,12 @@ void EntityMotionState::initForOwned() { _ownershipState = EntityMotionState::OwnershipState::LocallyOwned; } +void EntityMotionState::clearOwnershipState() { + if (_ownershipState != OwnershipState::Unownable) { + _ownershipState = OwnershipState::NotLocallyOwned; + } +} + void EntityMotionState::clearObjectVelocities() const { // If transform or velocities are flagged as dirty it means a network or scripted change // occured between the beginning and end of the stepSimulation() and we DON'T want to apply diff --git a/libraries/physics/src/EntityMotionState.h b/libraries/physics/src/EntityMotionState.h index 7456837777..be6f5c0658 100644 --- a/libraries/physics/src/EntityMotionState.h +++ b/libraries/physics/src/EntityMotionState.h @@ -107,7 +107,7 @@ protected: uint64_t getNextBidExpiry() const { return _nextBidExpiry; } void initForBid(); void initForOwned(); - void clearOwnershipState() { _ownershipState = OwnershipState::NotLocallyOwned; } + void clearOwnershipState(); void updateServerPhysicsVariables(); bool remoteSimulationOutOfSync(uint32_t simulationStep); From 8ab70225d93aff8d6bddbc9f90bfe5058e2bc3cf Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 7 Oct 2019 14:34:47 -0700 Subject: [PATCH 11/16] apply delete rules down linked hierarchy --- interface/src/avatar/AvatarManager.cpp | 8 +- interface/src/avatar/MyAvatar.cpp | 45 +-- interface/src/avatar/OtherAvatar.cpp | 8 +- interface/src/ui/AvatarCertifyBanner.cpp | 11 +- .../src/avatars-renderer/Avatar.cpp | 12 +- libraries/avatars/src/AvatarData.cpp | 5 +- libraries/avatars/src/AvatarData.h | 2 +- .../src/EntityTreeRenderer.cpp | 4 + .../src/EntityTreeRenderer.h | 1 + .../entities/src/DeleteEntityOperator.cpp | 8 + libraries/entities/src/DeleteEntityOperator.h | 1 + libraries/entities/src/EntityEditFilters.cpp | 2 +- libraries/entities/src/EntityEditFilters.h | 2 +- libraries/entities/src/EntityItem.cpp | 23 +- libraries/entities/src/EntityItem.h | 4 + .../entities/src/EntityItemProperties.cpp | 2 +- .../entities/src/EntityScriptingInterface.cpp | 78 ++-- libraries/entities/src/EntitySimulation.cpp | 24 +- libraries/entities/src/EntitySimulation.h | 6 +- libraries/entities/src/EntityTree.cpp | 353 ++++++++++-------- libraries/entities/src/EntityTree.h | 19 +- libraries/physics/src/EntityMotionState.cpp | 15 +- .../physics/src/PhysicalEntitySimulation.cpp | 53 ++- .../physics/src/PhysicalEntitySimulation.h | 7 +- tests/octree/src/ModelTests.cpp | 7 +- 25 files changed, 411 insertions(+), 289 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 553033f394..97a24e97f1 100755 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -543,10 +543,13 @@ void AvatarManager::removeDeadAvatarEntities(const SetOfEntities& deadEntities) for (auto entity : deadEntities) { QUuid entityOwnerID = entity->getOwningAvatarID(); AvatarSharedPointer avatar = getAvatarBySessionID(entityOwnerID); - const bool REQUIRES_REMOVAL_FROM_TREE = false; if (avatar) { - avatar->clearAvatarEntity(entity->getID(), REQUIRES_REMOVAL_FROM_TREE); + avatar->clearAvatarEntity(entity->getID()); } + } +} + +/* ADEBUG: I don't think the code below is necessary because any dead entities will have already been removed from tree and simulation if (entityTree && entity->isMyAvatarEntity()) { entityTree->withWriteLock([&] { // We only need to delete the direct children (rather than the descendants) because @@ -566,6 +569,7 @@ void AvatarManager::removeDeadAvatarEntities(const SetOfEntities& deadEntities) } } } +*/ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { auto avatar = std::static_pointer_cast(removedAvatar); diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 7aeb0aaa64..a46b2f46c1 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1577,7 +1577,8 @@ void MyAvatar::storeAvatarEntityDataPayload(const QUuid& entityID, const QByteAr } void MyAvatar::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree) { - AvatarData::clearAvatarEntity(entityID, requiresRemovalFromTree); + // NOTE: the requiresRemovalFromTree argument is unused + AvatarData::clearAvatarEntity(entityID); _avatarEntitiesLock.withWriteLock([&] { _cachedAvatarEntityBlobsToDelete.push_back(entityID); }); @@ -1647,13 +1648,16 @@ void MyAvatar::handleChangedAvatarEntityData() { // move the lists to minimize lock time std::vector cachedBlobsToDelete; std::vector cachedBlobsToUpdate; - std::vector entitiesToDelete; + QSet idsToDelete; std::vector entitiesToAdd; std::vector entitiesToUpdate; _avatarEntitiesLock.withWriteLock([&] { cachedBlobsToDelete = std::move(_cachedAvatarEntityBlobsToDelete); cachedBlobsToUpdate = std::move(_cachedAvatarEntityBlobsToAddOrUpdate); - entitiesToDelete = std::move(_entitiesToDelete); + foreach (auto id, _entitiesToDelete) { + idsToDelete.insert(id); + } + _entitiesToDelete.clear(); entitiesToAdd = std::move(_entitiesToAdd); entitiesToUpdate = std::move(_entitiesToUpdate); }); @@ -1671,7 +1675,7 @@ void MyAvatar::handleChangedAvatarEntityData() { }; // remove delete-add and delete-update overlap - for (const auto& id : entitiesToDelete) { + for (const auto& id : idsToDelete) { removeAllInstancesHelper(id, cachedBlobsToUpdate); removeAllInstancesHelper(id, entitiesToAdd); removeAllInstancesHelper(id, entitiesToUpdate); @@ -1685,11 +1689,9 @@ void MyAvatar::handleChangedAvatarEntityData() { } // DELETE real entities - for (const auto& id : entitiesToDelete) { - entityTree->withWriteLock([&] { - entityTree->deleteEntity(id); - }); - } + entityTree->withWriteLock([&] { + entityTree->deleteEntitiesByID(idsToDelete); + }); // ADD real entities EntityEditPacketSender* packetSender = qApp->getEntityEditPacketSender(); @@ -1802,7 +1804,7 @@ void MyAvatar::handleChangedAvatarEntityData() { // we have a client traits handler // flag removed entities as deleted so that changes are sent next frame _avatarEntitiesLock.withWriteLock([&] { - for (const auto& id : entitiesToDelete) { + for (const auto& id : idsToDelete) { if (_packedAvatarEntityData.find(id) != _packedAvatarEntityData.end()) { _clientTraitsHandler->markInstancedTraitDeleted(AvatarTraits::AvatarEntity, id); } @@ -2531,18 +2533,11 @@ bool isWearableEntity(const EntityItemPointer& entity) { void MyAvatar::removeWornAvatarEntity(const EntityItemID& entityID) { auto treeRenderer = DependencyManager::get(); EntityTreePointer entityTree = treeRenderer ? treeRenderer->getTree() : nullptr; - if (entityTree) { auto entity = entityTree->findEntityByID(entityID); if (entity && isWearableEntity(entity)) { - entityTree->withWriteLock([&entityID, &entityTree] { - // remove this entity first from the entity tree - entityTree->deleteEntity(entityID, true, true); - }); - - // remove the avatar entity from our internal list - // (but indicate it doesn't need to be pulled from the tree) - clearAvatarEntity(entityID, false); + treeRenderer->deleteEntity(entityID); + clearAvatarEntity(entityID); } } } @@ -2552,8 +2547,16 @@ void MyAvatar::clearWornAvatarEntities() { _avatarEntitiesLock.withReadLock([&] { avatarEntityIDs = _packedAvatarEntityData.keys(); }); - for (auto entityID : avatarEntityIDs) { - removeWornAvatarEntity(entityID); + auto treeRenderer = DependencyManager::get(); + EntityTreePointer entityTree = treeRenderer ? treeRenderer->getTree() : nullptr; + if (entityTree) { + for (auto entityID : avatarEntityIDs) { + auto entity = entityTree->findEntityByID(entityID); + if (entity && isWearableEntity(entity)) { + treeRenderer->deleteEntity(entityID); + clearAvatarEntity(entityID); + } + } } } diff --git a/interface/src/avatar/OtherAvatar.cpp b/interface/src/avatar/OtherAvatar.cpp index 5673c2443f..5a9d20bd81 100755 --- a/interface/src/avatar/OtherAvatar.cpp +++ b/interface/src/avatar/OtherAvatar.cpp @@ -558,11 +558,17 @@ void OtherAvatar::handleChangedAvatarEntityData() { _avatarEntitiesLock.withReadLock([&] { packedAvatarEntityData = _packedAvatarEntityData; }); + QSet idsToDelete; foreach (auto entityID, recentlyRemovedAvatarEntities) { if (!packedAvatarEntityData.contains(entityID)) { - entityTree->deleteEntity(entityID, true, true); + idsToDelete.insert(entityID); } } + if (!idsToDelete.empty()) { + bool force = true; + bool ignoreWarnings = true; + entityTree->deleteEntitiesByID(idsToDelete, force, ignoreWarnings); + } // TODO: move this outside of tree lock // remove stale data hashes diff --git a/interface/src/ui/AvatarCertifyBanner.cpp b/interface/src/ui/AvatarCertifyBanner.cpp index 5101188885..3fe2ed2027 100644 --- a/interface/src/ui/AvatarCertifyBanner.cpp +++ b/interface/src/ui/AvatarCertifyBanner.cpp @@ -62,16 +62,7 @@ void AvatarCertifyBanner::show(const QUuid& avatarID) { void AvatarCertifyBanner::clear() { if (_active) { - auto entityTreeRenderer = DependencyManager::get(); - EntityTreePointer entityTree = entityTreeRenderer->getTree(); - if (!entityTree) { - return; - } - - entityTree->withWriteLock([&] { - entityTree->deleteEntity(_bannerID); - }); - + DependencyManager::get()->deleteEntity(_bannerID); _active = false; } } diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index 75a7693de8..a9e9ca3dc6 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -357,11 +357,13 @@ void Avatar::removeAvatarEntitiesFromTree() { _avatarEntitiesLock.withReadLock([&] { avatarEntityIDs = _packedAvatarEntityData.keys(); }); - entityTree->withWriteLock([&] { - for (const auto& entityID : avatarEntityIDs) { - entityTree->deleteEntity(entityID, true, true); - } - }); + QSet ids; + foreach (auto id, avatarEntityIDs) { + ids.insert(id); + } + bool force = true; + bool ignoreWarnings = true; + entityTree->deleteEntitiesByID(ids, force, ignoreWarnings); // locks tree } } diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index a91154ff15..8a7d17fb12 100755 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -2985,15 +2985,12 @@ void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& ent } void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree) { - + // NOTE: requiresRemovalFromTree is unused bool removedEntity = false; - _avatarEntitiesLock.withWriteLock([this, &removedEntity, &entityID] { removedEntity = _packedAvatarEntityData.remove(entityID); }); - insertRemovedEntityID(entityID); - if (removedEntity && _clientTraitsHandler) { // we have a client traits handler, so we need to mark this removed instance trait as deleted // so that changes are sent next frame diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 59a2e2a53e..a66c7d897d 100755 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -1158,7 +1158,7 @@ public: /**jsdoc * @function Avatar.clearAvatarEntity * @param {Uuid} entityID - The entity ID. - * @param {boolean} [requiresRemovalFromTree=true] - Requires removal from tree. + * @param {boolean} [requiresRemovalFromTree=true] - unused * @deprecated This function is deprecated and will be removed. */ Q_INVOKABLE virtual void clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree = true); diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 6533d462a4..f6c02c5285 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -1357,6 +1357,10 @@ EntityItemPointer EntityTreeRenderer::getEntity(const EntityItemID& id) { return result; } +void EntityTreeRenderer::deleteEntity(const EntityItemID& id) const { + DependencyManager::get()->deleteEntity(id); +} + void EntityTreeRenderer::onEntityChanged(const EntityItemID& id) { _changedEntitiesGuard.withWriteLock([&] { _changedEntities.insert(id); diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.h b/libraries/entities-renderer/src/EntityTreeRenderer.h index b9fda690dd..3c69618813 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.h +++ b/libraries/entities-renderer/src/EntityTreeRenderer.h @@ -118,6 +118,7 @@ public: void setProxyWindow(const EntityItemID& id, QWindow* proxyWindow); void setCollisionSound(const EntityItemID& id, const SharedSoundPointer& sound); EntityItemPointer getEntity(const EntityItemID& id); + void deleteEntity(const EntityItemID& id) const; void onEntityChanged(const EntityItemID& id); // Access the workload Space diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index 1dca171ae3..6750db50d2 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/libraries/entities/src/DeleteEntityOperator.cpp @@ -53,6 +53,14 @@ void DeleteEntityOperator::addEntityIDToDeleteList(const EntityItemID& searchEnt } } +void DeleteEntityOperator::addEntityToDeleteList(const EntityItemPointer& entity) { + EntityToDeleteDetails details; + details.entity = entity; + details.containingElement = entity->getElement(); + details.cube = details.containingElement->getAACube(); + _entitiesToDelete << details; + _lookingCount++; +} // does this entity tree element contain the old entity bool DeleteEntityOperator::subTreeContainsSomeEntitiesToDelete(const OctreeElementPointer& element) { diff --git a/libraries/entities/src/DeleteEntityOperator.h b/libraries/entities/src/DeleteEntityOperator.h index 3b3ee2a868..1449e2caad 100644 --- a/libraries/entities/src/DeleteEntityOperator.h +++ b/libraries/entities/src/DeleteEntityOperator.h @@ -42,6 +42,7 @@ public: ~DeleteEntityOperator(); void addEntityIDToDeleteList(const EntityItemID& searchEntityID); + void addEntityToDeleteList(const EntityItemPointer& entity); virtual bool preRecursion(const OctreeElementPointer& element) override; virtual bool postRecursion(const OctreeElementPointer& element) override; diff --git a/libraries/entities/src/EntityEditFilters.cpp b/libraries/entities/src/EntityEditFilters.cpp index a222ca8216..16dace0fc8 100644 --- a/libraries/entities/src/EntityEditFilters.cpp +++ b/libraries/entities/src/EntityEditFilters.cpp @@ -43,7 +43,7 @@ QList EntityEditFilters::getZonesByPosition(glm::vec3& position) { } bool EntityEditFilters::filter(glm::vec3& position, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, - bool& wasChanged, EntityTree::FilterType filterType, EntityItemID& itemID, EntityItemPointer& existingEntity) { + bool& wasChanged, EntityTree::FilterType filterType, EntityItemID& itemID, const EntityItemPointer& existingEntity) { // get the ids of all the zones (plus the global entity edit filter) that the position // lies within diff --git a/libraries/entities/src/EntityEditFilters.h b/libraries/entities/src/EntityEditFilters.h index cb99c97762..69fd920998 100644 --- a/libraries/entities/src/EntityEditFilters.h +++ b/libraries/entities/src/EntityEditFilters.h @@ -55,7 +55,7 @@ public: void removeFilter(EntityItemID entityID); bool filter(glm::vec3& position, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged, - EntityTree::FilterType filterType, EntityItemID& entityID, EntityItemPointer& existingEntity); + EntityTree::FilterType filterType, EntityItemID& entityID, const EntityItemPointer& existingEntity); signals: void filterAdded(EntityItemID id, bool success); diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 60a8cadf6c..15aacd934c 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -3203,6 +3203,7 @@ void EntityItem::somethingChangedNotification() { }); } +// static void EntityItem::retrieveMarketplacePublicKey() { QNetworkAccessManager& networkAccessManager = NetworkAccessManager::getInstance(); QNetworkRequest networkRequest; @@ -3234,6 +3235,26 @@ void EntityItem::retrieveMarketplacePublicKey() { }); } +void EntityItem::collectChildrenForDelete(SetOfEntities& entitiesToDelete, SetOfEntities& domainEntities, const QUuid& sessionID) const { + // Deleting an entity has consequences for its children, however there are rules dictating what can be deleted. + // This method helps enforce those rules for the children of entity (not for this entity). + for (SpatiallyNestablePointer child : getChildren()) { + if (child && child->getNestableType() == NestableType::Entity) { + EntityItemPointer childEntity = std::static_pointer_cast(child); + // NOTE: null sessionID means "collect ALL known entities", else we only collect: local-entities and authorized avatar-entities + if (sessionID.isNull() || childEntity->isLocalEntity() || (childEntity->isAvatarEntity() && + (childEntity->isMyAvatarEntity() || childEntity->getOwningAvatarID() == sessionID))) { + if (entitiesToDelete.find(childEntity) == entitiesToDelete.end()) { + entitiesToDelete.insert(childEntity); + childEntity->collectChildrenForDelete(entitiesToDelete, domainEntities, sessionID); + } + } else if (childEntity->isDomainEntity()) { + domainEntities.insert(childEntity); + } + } + } +} + void EntityItem::setSpaceIndex(int32_t index) { assert(_spaceIndex == -1); _spaceIndex = index; @@ -3398,7 +3419,7 @@ void EntityItem::prepareForSimulationOwnershipBid(EntityItemProperties& properti properties.setSimulationOwner(Physics::getSessionUUID(), priority); setPendingOwnershipPriority(priority); - // ANDREW TODO: figure out if it would be OK to NOT bother set these properties + // ANDREW TODO: figure out if it would be OK to NOT bother set these properties here properties.setEntityHostType(getEntityHostType()); properties.setOwningAvatarID(getOwningAvatarID()); setLastBroadcast(now); // for debug/physics status icons diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index bdccd466cb..fef198c006 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -18,6 +18,7 @@ #include #include +#include #include // for EncodeBitstreamParams class #include // for OctreeElement::AppendState @@ -49,6 +50,7 @@ typedef std::shared_ptr EntityTreePointer; typedef std::shared_ptr EntityDynamicPointer; typedef std::shared_ptr EntityTreeElementPointer; using EntityTreeElementExtraEncodeDataPointer = std::shared_ptr; +using SetOfEntities = QSet; #define DONT_ALLOW_INSTANTIATION virtual void pureVirtualFunctionPlaceHolder() = 0; #define ALLOW_INSTANTIATION virtual void pureVirtualFunctionPlaceHolder() override { }; @@ -541,6 +543,8 @@ public: static QString _marketplacePublicKey; static void retrieveMarketplacePublicKey(); + void collectChildrenForDelete(SetOfEntities& entitiesToDelete, SetOfEntities& domainEntities, const QUuid& sessionID) const; + float getBoundingRadius() const { return _boundingRadius; } void setSpaceIndex(int32_t index); int32_t getSpaceIndex() const { return _spaceIndex; } diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 5cb5ddbbfa..02272a1bfa 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -3936,7 +3936,7 @@ bool EntityItemProperties::decodeCloneEntityMessage(const QByteArray& buffer, in processedBytes = 0; if (NUM_BYTES_RFC4122_UUID * 2 > packetLength) { - qCDebug(entities) << "EntityItemProperties::processEraseMessageDetails().... bailing because not enough bytes in buffer"; + qCDebug(entities) << "EntityItemProperties::decodeCloneEntityMessage().... bailing because not enough bytes in buffer"; return false; // bail to prevent buffer overflow } diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index 803c1ac4ca..c550a7b38b 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -966,42 +966,52 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) { _activityTracking.deletedEntityCount++; - EntityItemID entityID(id); - bool shouldSendDeleteToServer = true; - - // If we have a local entity tree set, then also update it. - if (_entityTree) { - _entityTree->withWriteLock([&] { - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); - if (entity) { - auto nodeList = DependencyManager::get(); - const QUuid myNodeID = nodeList->getSessionUUID(); - if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { - // don't delete other avatar's avatarEntities - shouldSendDeleteToServer = false; - return; - } - - if (entity->getLocked()) { - shouldSendDeleteToServer = false; - } else { - // only delete non-domain entities, domain entities will round trip through the server filters - if (!entity->isDomainEntity() || _entityTree->isServerlessMode()) { - shouldSendDeleteToServer = false; - _entityTree->deleteEntity(entityID); - - if (entity->isAvatarEntity() && getEntityPacketSender()->getMyAvatar()) { - getEntityPacketSender()->getMyAvatar()->clearAvatarEntity(entityID, false); - } - } - } - } - }); + if (!_entityTree) { + return; } - // if at this point, we know the id, and we should still delete the entity, send the update to the entity server - if (shouldSendDeleteToServer) { - getEntityPacketSender()->queueEraseEntityMessage(entityID); + EntityItemID entityID(id); + + // If we have a local entity tree set, then also update it. + SetOfEntities entitiesToDeleteImmediately; + SetOfEntities domainEntities; + _entityTree->withWriteLock([&] { + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + if (entity) { + if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) { + // don't delete other avatar's avatarEntities + return; + } + if (entity->getLocked()) { + return; + } + + // Deleting an entity has consequences for linked children: some can be deleted but others can't. + // Local- and my-avatar-entities can be deleted immediately, but other-avatar-entities can't be deleted + // by this context, and domain-entity deletes must rountrip through the entity-server for authorization. + // So we recurse down the linked hierarchy and snarf children into two categories: + // (a) entitiesToDeleteImmediately and (b) domainEntntities. + if (entity->isDomainEntity()) { + domainEntities.insert(entity); + } else { + entitiesToDeleteImmediately.insert(entity); + const auto sessionID = DependencyManager::get()->getSessionUUID(); + entity->collectChildrenForDelete(entitiesToDeleteImmediately, domainEntities, sessionID); + } + if (!entitiesToDeleteImmediately.empty()) { + _entityTree->deleteEntitiesByPointer(entitiesToDeleteImmediately); + } + } + }); + + foreach (auto entity, entitiesToDeleteImmediately) { + if (entity->isMyAvatarEntity()) { + getEntityPacketSender()->getMyAvatar()->clearAvatarEntity(entityID, false); + } + } + // finally ask entity-server to delete domainEntities + foreach (auto entity, domainEntities) { + getEntityPacketSender()->queueEraseEntityMessage(entity->getID()); } } diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 976cae9bd9..48de811be5 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -39,12 +39,7 @@ void EntitySimulation::updateEntities() { callUpdateOnEntitiesThatNeedIt(now); moveSimpleKinematics(now); sortEntitiesThatMoved(); -} - -void EntitySimulation::takeDeadEntities(SetOfEntities& entitiesToDelete) { - QMutexLocker lock(&_mutex); - entitiesToDelete.swap(_deadEntities); - _deadEntities.clear(); + processDeadEntities(); } void EntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) { @@ -260,3 +255,20 @@ void EntitySimulation::moveSimpleKinematics(uint64_t now) { } } } + +void EntitySimulation::processDeadEntities() { + if (_deadEntities.empty()) { + return; + } + SetOfEntities entitiesToDeleteImmediately; + SetOfEntities dummyList; // ignore this list: it will be empty + foreach (auto entity, _deadEntities) { + QUuid nullSessionID; + entitiesToDeleteImmediately.insert(entity); + entity->collectChildrenForDelete(entitiesToDeleteImmediately, dummyList, nullSessionID); + } + if (_entityTree) { + _entityTree->deleteEntitiesByPointer(entitiesToDeleteImmediately); + } + _deadEntities.clear(); +} diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index e9ad7ab2b7..6c32a7526f 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -16,7 +16,6 @@ #include #include -#include #include #include @@ -25,7 +24,6 @@ #include "EntityTree.h" using EntitySimulationPointer = std::shared_ptr; -using SetOfEntities = QSet; using VectorOfEntities = QVector; // the EntitySimulation needs to know when these things change on an entity, @@ -77,16 +75,16 @@ public: EntityTreePointer getEntityTree() { return _entityTree; } - virtual void takeDeadEntities(SetOfEntities& entitiesToDelete); - virtual void prepareEntityForDelete(EntityItemPointer entity); void processChangedEntities(); + virtual void queueEraseDomainEntities(const SetOfEntities& domainEntities) const { } protected: virtual void addEntityToInternalLists(EntityItemPointer entity); virtual void removeEntityFromInternalLists(EntityItemPointer entity); virtual void processChangedEntity(const EntityItemPointer& entity); + virtual void processDeadEntities(); void expireMortalEntities(uint64_t now); void callUpdateOnEntitiesThatNeedIt(uint64_t now); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 6440f39f52..75a805559f 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -98,14 +98,15 @@ void EntityTree::eraseDomainAndNonOwnedEntities() { if (element) { element->cleanupDomainAndNonOwnedEntities(); } - - if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { - savedEntities[entity->getEntityItemID()] = entity; - } else { - int32_t spaceIndex = entity->getSpaceIndex(); - if (spaceIndex != -1) { - // stale spaceIndices will be freed later - _staleProxies.push_back(spaceIndex); + if (!getIsServer()) { + if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { + savedEntities[entity->getEntityItemID()] = entity; + } else { + int32_t spaceIndex = entity->getSpaceIndex(); + if (spaceIndex != -1) { + // stale spaceIndices will be freed later + _staleProxies.push_back(spaceIndex); + } } } } @@ -144,10 +145,12 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) { if (element) { element->cleanupEntities(); } - int32_t spaceIndex = entity->getSpaceIndex(); - if (spaceIndex != -1) { - // assume stale spaceIndices will be freed later - _staleProxies.push_back(spaceIndex); + if (!getIsServer()) { + int32_t spaceIndex = entity->getSpaceIndex(); + if (spaceIndex != -1) { + // assume stale spaceIndices will be freed later + _staleProxies.push_back(spaceIndex); + } } } }); @@ -605,61 +608,21 @@ void EntityTree::setSimulation(EntitySimulationPointer simulation) { } void EntityTree::deleteEntity(const EntityItemID& entityID, bool force, bool ignoreWarnings) { - EntityTreeElementPointer containingElement = getContainingElement(entityID); - if (!containingElement) { - if (!ignoreWarnings) { - qCWarning(entities) << "EntityTree::deleteEntity() on non-existent entityID=" << entityID; - } - return; - } - - EntityItemPointer existingEntity = containingElement->getEntityWithEntityItemID(entityID); - if (!existingEntity) { - if (!ignoreWarnings) { - qCWarning(entities) << "EntityTree::deleteEntity() on non-existant entity item with entityID=" << entityID; - } - return; - } - - if (existingEntity->getLocked() && !force) { - if (!ignoreWarnings) { - qCDebug(entities) << "ERROR! EntityTree::deleteEntity() trying to delete locked entity. entityID=" << entityID; - } - return; - } - - cleanupCloneIDs(entityID); - unhookChildAvatar(entityID); - emit deletingEntity(entityID); - emit deletingEntityPointer(existingEntity.get()); - - // NOTE: callers must lock the tree before using this method - DeleteEntityOperator theOperator(getThisPointer(), entityID); - - existingEntity->forEachDescendant([&](SpatiallyNestablePointer descendant) { - auto descendantID = descendant->getID(); - theOperator.addEntityIDToDeleteList(descendantID); - emit deletingEntity(descendantID); - EntityItemPointer descendantEntity = std::dynamic_pointer_cast(descendant); - if (descendantEntity) { - emit deletingEntityPointer(descendantEntity.get()); - } - }); - - recurseTreeWithOperator(&theOperator); - processRemovedEntities(theOperator); - _isDirty = true; + // NOTE: can be called without lock because deleteEntitiesByID() will lock + QSet ids; + ids << entityID; + deleteEntitiesByID(ids, force, ignoreWarnings); } void EntityTree::unhookChildAvatar(const EntityItemID entityID) { - - EntityItemPointer entity = findEntityByEntityItemID(entityID); - - entity->forEachDescendant([&](SpatiallyNestablePointer child) { - if (child->getNestableType() == NestableType::Avatar) { - child->setParentID(nullptr); - } - }); + if (!getIsServer()) { + EntityItemPointer entity = findEntityByEntityItemID(entityID); + entity->forEachDescendant([&](SpatiallyNestablePointer child) { + if (child->getNestableType() == NestableType::Avatar) { + child->setParentID(nullptr); + } + }); + } } void EntityTree::cleanupCloneIDs(const EntityItemID& entityID) { @@ -684,40 +647,98 @@ void EntityTree::cleanupCloneIDs(const EntityItemID& entityID) { } } -void EntityTree::deleteEntities(QSet entityIDs, bool force, bool ignoreWarnings) { - // NOTE: callers must lock the tree before using this method +void EntityTree::recursivelyFilterAndCollectForDelete(const EntityItemPointer& entity, SetOfEntities& entitiesToDelete, bool force) const { + // tree must be read-locked before calling this method + //TODO: assert(treeIsLocked); + assert(entity); + if (entity->getElement() && (entitiesToDelete.find(entity) == entitiesToDelete.end())) { + // filter + bool allowed = force; + if (!allowed) { + bool wasChanged = false; + auto startFilter = usecTimestampNow(); + EntityItemProperties dummyProperties; + allowed = force || filterProperties(entity, dummyProperties, dummyProperties, wasChanged, FilterType::Delete); + auto endFilter = usecTimestampNow(); + _totalFilterTime += endFilter - startFilter; + } + if (allowed) { + entitiesToDelete.insert(entity); + for (SpatiallyNestablePointer child : entity->getChildren()) { + if (child && child->getNestableType() == NestableType::Entity) { + EntityItemPointer childEntity = std::static_pointer_cast(child); + recursivelyFilterAndCollectForDelete(childEntity, entitiesToDelete, force); + } + } + } + } +} + +void EntityTree::deleteEntitiesByID(const QSet& ids, bool force, bool ignoreWarnings) { + // this method has two paths: + // (a) entity-server: applies delete filter + // (b) interface-client: deletes local- and my-avatar-entities immediately, submits domainEntity deletes to the entity-server + if (getIsServer()) { + withWriteLock([&] { + SetOfEntities entitiesToDelete; + for (auto id : ids) { + EntityItemPointer entity; + { + QReadLocker locker(&_entityMapLock); + entity = _entityMap.value(id); + } + if (entity) { + recursivelyFilterAndCollectForDelete(entity, entitiesToDelete, force); + } + } + if (!entitiesToDelete.empty()) { + deleteEntitiesByPointer(entitiesToDelete); + } + }); + } else { + SetOfEntities entitiesToDelete; + SetOfEntities domainEntities; + QUuid sessionID = DependencyManager::get()->getSessionUUID(); + withWriteLock([&] { + for (auto id : ids) { + EntityItemPointer entity; + { + QReadLocker locker(&_entityMapLock); + entity = _entityMap.value(id); + } + if (entity) { + if (entity->isDomainEntity()) { + domainEntities.insert(entity); + } else if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { + entitiesToDelete.insert(entity); + entity->collectChildrenForDelete(entitiesToDelete, domainEntities, sessionID); + } + } + } + if (!entitiesToDelete.empty()) { + deleteEntitiesByPointer(entitiesToDelete); + } + }); + if (!domainEntities.empty() && _simulation) { + // interface-client can't delete domainEntities outright, they must roundtrip through the entity-server + _simulation->queueEraseDomainEntities(domainEntities); + } + } +} + +void EntityTree::deleteEntitiesByPointer(const SetOfEntities& entities) { + // tree must be write-locked before calling this method + //TODO: assert(treeIsLocked); + // NOTE: there is no entity validation (i.e. is entity in tree?) nor snarfing of children beyond this point. + // Get those done BEFORE calling this method. + for (auto entity : entities) { + cleanupCloneIDs(entity->getID()); + } DeleteEntityOperator theOperator(getThisPointer()); - foreach(const EntityItemID& entityID, entityIDs) { - // ANDREW TODO: rejigger this logic to FIRST get entity THEN get element - EntityTreeElementPointer containingElement = getContainingElement(entityID); - if (!containingElement) { - if (!ignoreWarnings) { - qCWarning(entities) << "EntityTree::deleteEntities() on non-existent entityID=" << entityID; - } - continue; - } - - EntityItemPointer existingEntity = containingElement->getEntityWithEntityItemID(entityID); - if (!existingEntity) { - if (!ignoreWarnings) { - qCWarning(entities) << "EntityTree::deleteEntities() on non-existent entity item with entityID=" << entityID; - } - continue; - } - - if (existingEntity->getLocked() && !force) { - if (!ignoreWarnings) { - qCDebug(entities) << "ERROR! EntityTree::deleteEntities() trying to delete locked entity. entityID=" << entityID; - } - continue; - } - - // tell our delete operator about this entityID - cleanupCloneIDs(entityID); - unhookChildAvatar(entityID); - theOperator.addEntityIDToDeleteList(entityID); - emit deletingEntity(entityID); - emit deletingEntityPointer(existingEntity.get()); + for (auto entity : entities) { + theOperator.addEntityToDeleteList(entity); + emit deletingEntity(entity->getID()); + emit deletingEntityPointer(entity.get()); } if (!theOperator.getEntities().empty()) { @@ -728,23 +749,11 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i } void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) { + // NOTE: assume tree already write-locked because this method only called in deleteEntitiesByPointer() quint64 deletedAt = usecTimestampNow(); const RemovedEntities& entities = theOperator.getEntities(); foreach(const EntityToDeleteDetails& details, entities) { EntityItemPointer theEntity = details.entity; - - if (getIsServer()) { - QSet childrenIDs; - theEntity->forEachChild([&](SpatiallyNestablePointer child) { - if (child->getNestableType() == NestableType::Entity) { - childrenIDs += child->getID(); - } - }); - deleteEntities(childrenIDs, true, true); - } - - theEntity->die(); - if (getIsServer()) { removeCertifiedEntityOnServer(theEntity); @@ -752,19 +761,24 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) QWriteLocker recentlyDeletedEntitiesLocker(&_recentlyDeletedEntitiesLock); _recentlyDeletedEntityItemIDs.insert(deletedAt, theEntity->getEntityItemID()); } else { + theEntity->forEachDescendant([&](SpatiallyNestablePointer child) { + if (child->getNestableType() == NestableType::Avatar) { + child->setParentID(nullptr); + } + }); + // on the client side, we also remember that we deleted this entity, we don't care about the time trackDeletedEntity(theEntity->getEntityItemID()); - } + int32_t spaceIndex = theEntity->getSpaceIndex(); + if (spaceIndex != -1) { + // stale spaceIndices will be freed later + _staleProxies.push_back(spaceIndex); + } + } if (theEntity->isSimulated()) { _simulation->prepareEntityForDelete(theEntity); } - - int32_t spaceIndex = theEntity->getSpaceIndex(); - if (spaceIndex != -1) { - // stale spaceIndices will be freed later - _staleProxies.push_back(spaceIndex); - } } } @@ -1370,7 +1384,7 @@ void EntityTree::fixupTerseEditLogging(EntityItemProperties& properties, QList(); if (entityEditFilters) { @@ -1750,9 +1764,9 @@ void EntityTree::processChallengeOwnershipPacket(ReceivedMessage& message, const } } +// NOTE: Caller must lock the tree before calling this. int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned char* editData, int maxLength, const SharedNodePointer& senderNode) { - if (!getIsServer()) { qCWarning(entities) << "EntityTree::processEditPacketData() should only be called on a server tree."; return 0; @@ -2218,7 +2232,9 @@ void EntityTree::fixupNeedsParentFixups() { void EntityTree::deleteDescendantsOfAvatar(QUuid avatarID) { if (_childrenOfAvatars.contains(avatarID)) { - deleteEntities(_childrenOfAvatars[avatarID]); + bool force = true; + bool ignoreWarnings = true; + deleteEntitiesByID(_childrenOfAvatars[avatarID], force, ignoreWarnings); _childrenOfAvatars.remove(avatarID); } } @@ -2250,22 +2266,6 @@ void EntityTree::update(bool simulate) { if (simulate && _simulation) { withWriteLock([&] { _simulation->updateEntities(); - { - PROFILE_RANGE(simulation_physics, "Deletes"); - SetOfEntities deadEntities; - _simulation->takeDeadEntities(deadEntities); - if (!deadEntities.empty()) { - // translate into list of ID's - QSet idsToDelete; - - for (auto entity : deadEntities) { - idsToDelete.insert(entity->getEntityItemID()); - } - - // delete these things the roundabout way - deleteEntities(idsToDelete, true); - } - } }); } } @@ -2354,12 +2354,17 @@ bool EntityTree::shouldEraseEntity(EntityItemID entityID, const SharedNodePointe return allowed; } - -// TODO: consider consolidating processEraseMessageDetails() and processEraseMessage() int EntityTree::processEraseMessage(ReceivedMessage& message, const SharedNodePointer& sourceNode) { + // NOTE: this is only called by the interface-client on receipt of deleteEntity message from entity-server. + // Which means this is a state synchronization message from the the entity-server. It is saying + // "The following domain-entities have already been deleted". While need to perform sanity checking + // (e.g. verify these are domain entities) permissions need NOT checked for the domain-entities. + assert(!getIsServer()); + // TODO: remove this stuff out of EntityTree:: and into interface-client code. #ifdef EXTRA_ERASE_DEBUGGING qCDebug(entities) << "EntityTree::processEraseMessage()"; #endif + SetOfEntities consequentialDomainEntities; withWriteLock([&] { message.seek(sizeof(OCTREE_PACKET_FLAGS) + sizeof(OCTREE_PACKET_SEQUENCE) + sizeof(OCTREE_PACKET_SENT_TIME)); @@ -2367,10 +2372,9 @@ int EntityTree::processEraseMessage(ReceivedMessage& message, const SharedNodePo message.readPrimitive(&numberOfIDs); if (numberOfIDs > 0) { - QSet entityItemIDsToDelete; + QSet idsToDelete; for (size_t i = 0; i < numberOfIDs; i++) { - if (NUM_BYTES_RFC4122_UUID > message.getBytesLeftToRead()) { qCDebug(entities) << "EntityTree::processEraseMessage().... bailing because not enough bytes in buffer"; break; // bail to prevent buffer overflow @@ -2382,64 +2386,85 @@ int EntityTree::processEraseMessage(ReceivedMessage& message, const SharedNodePo #endif EntityItemID entityItemID(entityID); + idsToDelete << entityItemID; + } - if (shouldEraseEntity(entityID, sourceNode)) { - entityItemIDsToDelete << entityItemID; - cleanupCloneIDs(entityItemID); + // domain-entity deletion can trigger deletion of other entities the entity-server doesn't know about + // so we must recurse down the children and collect consequential deletes however + // we must first identify all domain-entities in idsToDelete so as to not overstep entity-server's authority + SetOfEntities domainEntities; + for (auto id : idsToDelete) { + EntityItemPointer entity = _entityMap.value(id); + if (entity && entity->isDomainEntity()) { + domainEntities.insert(entity); } } - deleteEntities(entityItemIDsToDelete, true, true); + // now we recurse domain-entities children and snarf consequential entities + auto nodeList = DependencyManager::get(); + QUuid sessionID = nodeList->getSessionUUID(); + // NOTE: normally a null sessionID would be bad, as that would cause the collectDhildrenForDelete() method below + // to snarf domain entities for which the interface-client is not authorized to delete without explicit instructions + // from the entity-server, however it is ok here because that would mean: + // (a) interface-client is not connected to a domain which means... + // (b) we should never get here (since this would correspond to a message from the entity-server) but... + // (c) who cares? When not connected to a domain the interface-client can do whatever it wants. + SetOfEntities entitiesToDelete; + for (auto entity : domainEntities) { + entitiesToDelete.insert(entity); + entity->collectChildrenForDelete(entitiesToDelete, consequentialDomainEntities, sessionID); + } + + if (!entitiesToDelete.empty()) { + deleteEntitiesByPointer(entitiesToDelete); + } } }); + if (!consequentialDomainEntities.empty() && _simulation) { + _simulation->queueEraseDomainEntities(consequentialDomainEntities); + } return message.getPosition(); } // This version skips over the header -// NOTE: Caller must lock the tree before calling this. -// TODO: consider consolidating processEraseMessageDetails() and processEraseMessage() +// NOTE: Caller must write-lock the tree before calling this. int EntityTree::processEraseMessageDetails(const QByteArray& dataByteArray, const SharedNodePointer& sourceNode) { + //TODO: assert(treeIsLocked); + assert(getIsServer()); #ifdef EXTRA_ERASE_DEBUGGING qCDebug(entities) << "EntityTree::processEraseMessageDetails()"; #endif - const unsigned char* packetData = (const unsigned char*)dataByteArray.constData(); - const unsigned char* dataAt = packetData; size_t packetLength = dataByteArray.size(); size_t processedBytes = 0; uint16_t numberOfIds = 0; // placeholder for now - memcpy(&numberOfIds, dataAt, sizeof(numberOfIds)); - dataAt += sizeof(numberOfIds); + memcpy(&numberOfIds, dataByteArray.constData(), sizeof(numberOfIds)); processedBytes += sizeof(numberOfIds); if (numberOfIds > 0) { - QSet entityItemIDsToDelete; + QSet ids; + // extract ids from packet for (size_t i = 0; i < numberOfIds; i++) { - - if (processedBytes + NUM_BYTES_RFC4122_UUID > packetLength) { qCDebug(entities) << "EntityTree::processEraseMessageDetails().... bailing because not enough bytes in buffer"; break; // bail to prevent buffer overflow } QByteArray encodedID = dataByteArray.mid((int)processedBytes, NUM_BYTES_RFC4122_UUID); - QUuid entityID = QUuid::fromRfc4122(encodedID); - dataAt += encodedID.size(); + QUuid id = QUuid::fromRfc4122(encodedID); processedBytes += encodedID.size(); #ifdef EXTRA_ERASE_DEBUGGING - qCDebug(entities) << " ---- EntityTree::processEraseMessageDetails() contains id:" << entityID; + qCDebug(entities) << " ---- EntityTree::processEraseMessageDetails() contains id:" << id; #endif - EntityItemID entityItemID(entityID); - - if (shouldEraseEntity(entityID, sourceNode)) { - entityItemIDsToDelete << entityItemID; - cleanupCloneIDs(entityItemID); - } - + EntityItemID entityID(id); + ids << entityID; } - deleteEntities(entityItemIDsToDelete, true, true); + + bool force = sourceNode->isAllowedEditor(); + bool ignoreWarnings = true; + deleteEntitiesByID(ids, force, ignoreWarnings); } return (int)processedBytes; } diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 9108f8d8d2..6574b9d601 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -126,7 +126,9 @@ public: void unhookChildAvatar(const EntityItemID entityID); void cleanupCloneIDs(const EntityItemID& entityID); void deleteEntity(const EntityItemID& entityID, bool force = false, bool ignoreWarnings = true); - void deleteEntities(QSet entityIDs, bool force = false, bool ignoreWarnings = true); + + void deleteEntitiesByID(const QSet& entityIDs, bool force = false, bool ignoreWarnings = true); + void deleteEntitiesByPointer(const SetOfEntities& entities); EntityItemPointer findEntityByID(const QUuid& id) const; EntityItemPointer findEntityByEntityItemID(const EntityItemID& entityID) const; @@ -291,6 +293,7 @@ signals: protected: + void recursivelyFilterAndCollectForDelete(const EntityItemPointer& entity, SetOfEntities& entitiesToDelete, bool force) const; void processRemovedEntities(const DeleteEntityOperator& theOperator); bool updateEntity(EntityItemPointer entity, const EntityItemProperties& properties, const SharedNodePointer& senderNode = SharedNodePointer(nullptr)); @@ -339,12 +342,12 @@ protected: int _totalEditMessages = 0; int _totalUpdates = 0; int _totalCreates = 0; - quint64 _totalDecodeTime = 0; - quint64 _totalLookupTime = 0; - quint64 _totalUpdateTime = 0; - quint64 _totalCreateTime = 0; - quint64 _totalLoggingTime = 0; - quint64 _totalFilterTime = 0; + mutable quint64 _totalDecodeTime = 0; + mutable quint64 _totalLookupTime = 0; + mutable quint64 _totalUpdateTime = 0; + mutable quint64 _totalCreateTime = 0; + mutable quint64 _totalLoggingTime = 0; + mutable quint64 _totalFilterTime = 0; // these performance statistics are only used in the client void resetClientEditStats(); @@ -364,7 +367,7 @@ protected: float _maxTmpEntityLifetime { DEFAULT_MAX_TMP_ENTITY_LIFETIME }; - bool filterProperties(EntityItemPointer& existingEntity, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged, FilterType filterType); + bool filterProperties(const EntityItemPointer& existingEntity, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged, FilterType filterType) const; bool _hasEntityEditFilter{ false }; QStringList _entityScriptSourceWhitelist; diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index 2ec2eff7f9..f3d129871f 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -62,7 +62,6 @@ EntityMotionState::EntityMotionState(btCollisionShape* shape, EntityItemPointer // rather than pass the legit shape pointer to the ObjectMotionState ctor above. setShape(shape); - // ANDREW TODO: maybe all non-domain entities should be unownable? or our avatar entities should have a special OwnershipState if (_entity->isAvatarEntity() && !_entity->isMyAvatarEntity()) { // avatar entities are always thus, so we cache this fact in _ownershipState _ownershipState = EntityMotionState::OwnershipState::Unownable; @@ -748,7 +747,6 @@ bool EntityMotionState::shouldSendBid() const { // NOTE: this method is only ever called when the entity's simulation is NOT locally owned return _body->isActive() && (_region == workload::Region::R1) - // ANDREW TODO: make sure _ownershipState to Unownable on creation when applicable && _ownershipState != EntityMotionState::OwnershipState::Unownable && glm::max(glm::max(VOLUNTEER_SIMULATION_PRIORITY, _bumpedPriority), _entity->getScriptSimulationPriority()) >= _entity->getSimulationPriority() && !_entity->getLocked() @@ -770,8 +768,7 @@ uint8_t EntityMotionState::computeFinalBidPriority() const { } bool EntityMotionState::isLocallyOwned() const { - // ANDREW TODO: maybe also always return true for MyAvatar's avatar entities? - return _entity->getSimulatorID() == Physics::getSessionUUID(); + return _entity->getSimulatorID() == Physics::getSessionUUID() || _entity->isMyAvatarEntity(); } bool EntityMotionState::isLocallyOwnedOrShouldBe() const { @@ -789,13 +786,15 @@ void EntityMotionState::setRegion(uint8_t region) { } void EntityMotionState::initForBid() { - assert(_ownershipState != EntityMotionState::OwnershipState::Unownable); - _ownershipState = EntityMotionState::OwnershipState::PendingBid; + if (_ownershipState != EntityMotionState::OwnershipState::Unownable) { + _ownershipState = EntityMotionState::OwnershipState::PendingBid; + } } void EntityMotionState::initForOwned() { - assert(_ownershipState != EntityMotionState::OwnershipState::Unownable); - _ownershipState = EntityMotionState::OwnershipState::LocallyOwned; + if (_ownershipState != EntityMotionState::OwnershipState::Unownable) { + _ownershipState = EntityMotionState::OwnershipState::LocallyOwned; + } } void EntityMotionState::clearOwnershipState() { diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 6356a439d3..a7f66929ed 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -107,18 +107,6 @@ void PhysicalEntitySimulation::clearOwnershipData() { _bids.clear(); } -void PhysicalEntitySimulation::takeDeadEntities(SetOfEntities& deadEntities) { - QMutexLocker lock(&_mutex); - for (auto entity : _deadEntities) { - EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); - if (motionState) { - _entitiesToRemoveFromPhysics.insert(entity); - } - } - _deadEntities.swap(deadEntities); - _deadEntities.clear(); -} - void PhysicalEntitySimulation::takeDeadAvatarEntities(SetOfEntities& deadEntities) { _deadAvatarEntities.swap(deadEntities); _deadAvatarEntities.clear(); @@ -182,6 +170,38 @@ void PhysicalEntitySimulation::processChangedEntity(const EntityItemPointer& ent } } +void PhysicalEntitySimulation::processDeadEntities() { + if (_deadEntities.empty()) { + return; + } + PROFILE_RANGE(simulation_physics, "Deletes"); + SetOfEntities entitiesToDeleteImmediately; + SetOfEntities domainEntities; + QUuid sessionID = Physics::getSessionUUID(); + QMutexLocker lock(&_mutex); + for (auto entity : _deadEntities) { + EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); + if (motionState) { + _entitiesToRemoveFromPhysics.insert(entity); + } + if (entity->isDomainEntity()) { + domainEntities.insert(entity); + } else if (entity->isLocalEntity() || entity->isMyAvatarEntity()) { + entitiesToDeleteImmediately.insert(entity); + entity->collectChildrenForDelete(entitiesToDeleteImmediately, domainEntities, sessionID); + } + } + _deadEntities.clear(); + + // interface-client can't delete domainEntities outright, they must roundtrip through the entity-server + for (auto entity : domainEntities) { + _entityPacketSender->queueEraseEntityMessage(entity->getID()); + } + if (!entitiesToDeleteImmediately.empty()) { + getEntityTree()->deleteEntitiesByPointer(entitiesToDeleteImmediately); + } +} + void PhysicalEntitySimulation::clearEntities() { // 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 @@ -213,6 +233,15 @@ void PhysicalEntitySimulation::clearEntities() { EntitySimulation::clearEntities(); } +void PhysicalEntitySimulation::queueEraseDomainEntities(const SetOfEntities& domainEntities) const { + if (_entityPacketSender) { + for (auto domainEntity : domainEntities) { + assert(domainEntity->isDomainEntity()); + _entityPacketSender->queueEraseEntityMessage(domainEntity->getID()); + } + } +} + // virtual void PhysicalEntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { // DANGER! this can be called on any thread diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 7efb000050..869ad1f108 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -63,19 +63,20 @@ public: void removeDynamic(const QUuid dynamicID) override; void applyDynamicChanges() override; - virtual void takeDeadEntities(SetOfEntities& deadEntities) override; void takeDeadAvatarEntities(SetOfEntities& deadEntities); virtual void clearEntities() override; + void queueEraseDomainEntities(const SetOfEntities& domainEntities) const override; signals: void entityCollisionWithEntity(const EntityItemID& idA, const EntityItemID& idB, const Collision& collision); protected: // only called by EntitySimulation // overrides for EntitySimulation - virtual void addEntityToInternalLists(EntityItemPointer entity) override; - virtual void removeEntityFromInternalLists(EntityItemPointer entity) override; + void addEntityToInternalLists(EntityItemPointer entity) override; + void removeEntityFromInternalLists(EntityItemPointer entity) override; void processChangedEntity(const EntityItemPointer& entity) override; + void processDeadEntities() override; void removeOwnershipData(EntityMotionState* motionState); void clearOwnershipData(); diff --git a/tests/octree/src/ModelTests.cpp b/tests/octree/src/ModelTests.cpp index f3e17a56a5..1ab32217d9 100644 --- a/tests/octree/src/ModelTests.cpp +++ b/tests/octree/src/ModelTests.cpp @@ -363,7 +363,8 @@ void EntityTests::entityTreeTests(bool verbose) { } quint64 startDelete = usecTimestampNow(); - tree.deleteEntity(entityID); + bool force = true; + tree.deleteEntity(entityID, force); quint64 endDelete = usecTimestampNow(); totalElapsedDelete += (endDelete - startDelete); @@ -446,7 +447,9 @@ void EntityTests::entityTreeTests(bool verbose) { } quint64 startDelete = usecTimestampNow(); - tree.deleteEntities(entitiesToDelete); + bool force = true; + bool ignoreWarnings = true; + tree.deleteEntitiesByID(entitiesToDelete, force, ignoreWarnings); quint64 endDelete = usecTimestampNow(); totalElapsedDelete += (endDelete - startDelete); From 052a0c3ebe6b3b9cf17ffaa19a1abf9c431253a2 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 7 Oct 2019 16:23:30 -0700 Subject: [PATCH 12/16] remove cruft, add comments, change variable name --- interface/src/avatar/AvatarManager.cpp | 22 ------------------- libraries/entities/src/EntitySimulation.cpp | 19 +++++++++------- libraries/entities/src/EntitySimulation.h | 2 +- .../physics/src/PhysicalEntitySimulation.cpp | 8 +++---- .../physics/src/PhysicalEntitySimulation.h | 2 +- 5 files changed, 17 insertions(+), 36 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 97a24e97f1..32e725388c 100755 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -549,28 +549,6 @@ void AvatarManager::removeDeadAvatarEntities(const SetOfEntities& deadEntities) } } -/* ADEBUG: I don't think the code below is necessary because any dead entities will have already been removed from tree and simulation - if (entityTree && entity->isMyAvatarEntity()) { - entityTree->withWriteLock([&] { - // We only need to delete the direct children (rather than the descendants) because - // when the child is deleted, it will take care of its own children. If the child - // is also an avatar-entity, we'll end up back here. If it's not, the entity-server - // will take care of it in the usual way. - entity->forEachChild([&](SpatiallyNestablePointer child) { - EntityItemPointer childEntity = std::dynamic_pointer_cast(child); - if (childEntity) { - entityTree->deleteEntity(childEntity->getID(), true, true); - if (avatar) { - avatar->clearAvatarEntity(childEntity->getID(), REQUIRES_REMOVAL_FROM_TREE); - } - } - }); - }); - } - } -} -*/ - void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { auto avatar = std::static_pointer_cast(removedAvatar); AvatarHashMap::handleRemovedAvatar(avatar, removalReason); diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 48de811be5..5576f21cc5 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -43,7 +43,7 @@ void EntitySimulation::updateEntities() { } void EntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) { - // remove from all internal lists except _deadEntities + // remove from all internal lists except _deadEntitiesToRemoveFromTree _entitiesToSort.remove(entity); _simpleKinematicEntities.remove(entity); _allEntities.remove(entity); @@ -59,7 +59,7 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) { QMutexLocker lock(&_mutex); removeEntityFromInternalLists(entity); if (entity->getElement()) { - _deadEntities.insert(entity); + _deadEntitiesToRemoveFromTree.insert(entity); _entityTree->cleanupCloneIDs(entity->getEntityItemID()); } } @@ -217,7 +217,7 @@ void EntitySimulation::clearEntities() { _simpleKinematicEntities.clear(); _changedEntities.clear(); _allEntities.clear(); - _deadEntities.clear(); + _deadEntitiesToRemoveFromTree.clear(); _entitiesToUpdate.clear(); _mortalEntities.clear(); _nextExpiry = std::numeric_limits::max(); @@ -257,18 +257,21 @@ void EntitySimulation::moveSimpleKinematics(uint64_t now) { } void EntitySimulation::processDeadEntities() { - if (_deadEntities.empty()) { + if (_deadEntitiesToRemoveFromTree.empty()) { return; } SetOfEntities entitiesToDeleteImmediately; - SetOfEntities dummyList; // ignore this list: it will be empty - foreach (auto entity, _deadEntities) { - QUuid nullSessionID; + // NOTE: dummyList will be empty because this base-class implementation is only used server-side + // for which ATM we only process domain-entities, and since we are passing nullSessionID for authorization + // EntityItem::collectChildrenForDelete() will not collect domain-entities into this side list. + SetOfEntities dummyList; + QUuid nullSessionID; + foreach (auto entity, _deadEntitiesToRemoveFromTree) { entitiesToDeleteImmediately.insert(entity); entity->collectChildrenForDelete(entitiesToDeleteImmediately, dummyList, nullSessionID); } if (_entityTree) { _entityTree->deleteEntitiesByPointer(entitiesToDeleteImmediately); } - _deadEntities.clear(); + _deadEntitiesToRemoveFromTree.clear(); } diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index 6c32a7526f..646e5a0f67 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -94,7 +94,7 @@ protected: SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree) SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion - SetOfEntities _deadEntities; // dead entities that might still be in the _entityTree + SetOfEntities _deadEntitiesToRemoveFromTree; private: void moveSimpleKinematics(); diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index a7f66929ed..0375e1dfd9 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -69,7 +69,7 @@ void PhysicalEntitySimulation::removeEntityFromInternalLists(EntityItemPointer e _entitiesToRemoveFromPhysics.insert(entity); } if (entity->isDead() && entity->getElement()) { - _deadEntities.insert(entity); + _deadEntitiesToRemoveFromTree.insert(entity); } if (entity->isAvatarEntity()) { _deadAvatarEntities.insert(entity); @@ -171,7 +171,7 @@ void PhysicalEntitySimulation::processChangedEntity(const EntityItemPointer& ent } void PhysicalEntitySimulation::processDeadEntities() { - if (_deadEntities.empty()) { + if (_deadEntitiesToRemoveFromTree.empty()) { return; } PROFILE_RANGE(simulation_physics, "Deletes"); @@ -179,7 +179,7 @@ void PhysicalEntitySimulation::processDeadEntities() { SetOfEntities domainEntities; QUuid sessionID = Physics::getSessionUUID(); QMutexLocker lock(&_mutex); - for (auto entity : _deadEntities) { + for (auto entity : _deadEntitiesToRemoveFromTree) { EntityMotionState* motionState = static_cast(entity->getPhysicsInfo()); if (motionState) { _entitiesToRemoveFromPhysics.insert(entity); @@ -191,7 +191,7 @@ void PhysicalEntitySimulation::processDeadEntities() { entity->collectChildrenForDelete(entitiesToDeleteImmediately, domainEntities, sessionID); } } - _deadEntities.clear(); + _deadEntitiesToRemoveFromTree.clear(); // interface-client can't delete domainEntities outright, they must roundtrip through the entity-server for (auto entity : domainEntities) { diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 869ad1f108..a3bd60b96e 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -124,7 +124,7 @@ private: VectorOfEntityMotionStates _owned; VectorOfEntityMotionStates _bids; - SetOfEntities _deadAvatarEntities; + SetOfEntities _deadAvatarEntities; // to remove from Avatar's lists std::vector _entitiesToDeleteLater; QList _dynamicsToAdd; From 960646f117fd11d3b18b5c42d87ff3b897537929 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Tue, 8 Oct 2019 16:12:14 -0700 Subject: [PATCH 13/16] skip delete entities not in the tree --- libraries/entities/src/DeleteEntityOperator.cpp | 1 + libraries/entities/src/EntityTree.cpp | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index 6750db50d2..a4ecb532e5 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/libraries/entities/src/DeleteEntityOperator.cpp @@ -54,6 +54,7 @@ void DeleteEntityOperator::addEntityIDToDeleteList(const EntityItemID& searchEnt } void DeleteEntityOperator::addEntityToDeleteList(const EntityItemPointer& entity) { + assert(entity && entity->getElement()); EntityToDeleteDetails details; details.entity = entity; details.containingElement = entity->getElement(); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 75a805559f..f44f2eb7c5 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -736,9 +736,11 @@ void EntityTree::deleteEntitiesByPointer(const SetOfEntities& entities) { } DeleteEntityOperator theOperator(getThisPointer()); for (auto entity : entities) { - theOperator.addEntityToDeleteList(entity); - emit deletingEntity(entity->getID()); - emit deletingEntityPointer(entity.get()); + if (entity->getElement()) { + theOperator.addEntityToDeleteList(entity); + emit deletingEntity(entity->getID()); + emit deletingEntityPointer(entity.get()); + } } if (!theOperator.getEntities().empty()) { From 240f904b85f5199e9e709a0df1c0f42e11e8a400 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Thu, 10 Oct 2019 11:11:27 -0700 Subject: [PATCH 14/16] DEV-2386: Remove 'Run Client Script Test' --- interface/src/Menu.cpp | 5 ----- interface/src/Menu.h | 1 - interface/src/ui/DialogsManager.cpp | 9 --------- interface/src/ui/DialogsManager.h | 1 - 4 files changed, 16 deletions(-) diff --git a/interface/src/Menu.cpp b/interface/src/Menu.cpp index b66bc9c1c4..5be4db46a0 100644 --- a/interface/src/Menu.cpp +++ b/interface/src/Menu.cpp @@ -640,11 +640,6 @@ Menu::Menu() { } #endif - - // Developer >> Tests >>> - MenuWrapper* testMenu = developerMenu->addMenu("Tests"); - addActionToQMenuAndActionHash(testMenu, MenuOption::RunClientScriptTests, 0, dialogsManager.data(), SLOT(showTestingResults())); - // Developer > Timing >>> MenuWrapper* timingMenu = developerMenu->addMenu("Timing"); MenuWrapper* perfTimerMenu = timingMenu->addMenu("Performance Timer"); diff --git a/interface/src/Menu.h b/interface/src/Menu.h index 0ba1159052..e3080e60aa 100644 --- a/interface/src/Menu.h +++ b/interface/src/Menu.h @@ -178,7 +178,6 @@ namespace MenuOption { const QString ResetAvatarSize = "Reset Avatar Size"; const QString ResetSensors = "Reset Sensors"; const QString RunningScripts = "Running Scripts..."; - const QString RunClientScriptTests = "Run Client Script Tests"; const QString RunTimingTests = "Run Timing Tests"; const QString ScriptedMotorControl = "Enable Scripted Motor Control"; const QString ShowTrackedObjects = "Show Tracked Objects"; diff --git a/interface/src/ui/DialogsManager.cpp b/interface/src/ui/DialogsManager.cpp index e34b82e0a1..0a655de5e5 100644 --- a/interface/src/ui/DialogsManager.cpp +++ b/interface/src/ui/DialogsManager.cpp @@ -186,15 +186,6 @@ void DialogsManager::setAddressBarVisible(bool addressBarVisible) { emit addressBarShown(_addressBarVisible); } -void DialogsManager::showTestingResults() { - if (!_testingDialog) { - _testingDialog = new TestingDialog(qApp->getWindow()); - connect(_testingDialog, SIGNAL(closed()), _testingDialog, SLOT(deleteLater())); - } - _testingDialog->show(); - _testingDialog->raise(); -} - void DialogsManager::showDomainConnectionDialog() { // if the dialog already exists we delete it so the connection data is refreshed if (_domainConnectionDialog) { diff --git a/interface/src/ui/DialogsManager.h b/interface/src/ui/DialogsManager.h index b11264444b..949c86c240 100644 --- a/interface/src/ui/DialogsManager.h +++ b/interface/src/ui/DialogsManager.h @@ -53,7 +53,6 @@ public slots: void lodTools(); void hmdTools(bool showTools); void showDomainConnectionDialog(); - void showTestingResults(); void toggleAddressBar(); // Application Update From 3f00c524fbbb73a3770d29be38c739c8ac4bdfd6 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Thu, 10 Oct 2019 11:47:40 -0700 Subject: [PATCH 15/16] DEV-2379: Modify Shadow-related tooltips in Create --- scripts/system/create/assets/data/createAppTooltips.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/system/create/assets/data/createAppTooltips.json b/scripts/system/create/assets/data/createAppTooltips.json index cda6fc9be0..24f443f901 100644 --- a/scripts/system/create/assets/data/createAppTooltips.json +++ b/scripts/system/create/assets/data/createAppTooltips.json @@ -76,7 +76,7 @@ "tooltip": "The angle in deg at which light emits. Starts in the entity's -z direction, and rotates around its x axis." }, "keyLight.castShadows": { - "tooltip": "If enabled, shadows are cast. The entity or avatar casting the shadow must also have Cast Shadows enabled." + "tooltip": "If enabled, shadows are cast. The entity or avatar casting the shadow must also have Cast Shadows enabled. Note: Shadows are rendered only on high-profiled computers. This setting will have no effect on computers profiled to medium or low graphics." }, "keyLight.shadowBias": { "tooltip": "The bias of the shadows cast by the light. Use this to fine-tune your shadows to your scene to prevent shadow acne and peter panning." @@ -500,7 +500,7 @@ "tooltip": "If enabled, grabbed entities will follow the movements of your hand controller instead of your avatar's hand." }, "canCastShadow": { - "tooltip": "If enabled, this geometry of this entity casts shadows when a shadow-casting light source shines on it." + "tooltip": "If enabled, the geometry of this entity casts shadows when a shadow-casting light source shines on it. Note: Shadows are rendered only on high-profiled computers. This setting will have no effect on computers profiled to medium or low graphics.." }, "ignorePickIntersection": { "tooltip": "If enabled, this entity will not be considered for ray picks, and will also not occlude other entities when picking." From ec67b67966bea03b6ce35967472f7782633b1066 Mon Sep 17 00:00:00 2001 From: Ken Cooke Date: Thu, 10 Oct 2019 14:10:47 -0700 Subject: [PATCH 16/16] On waking from sleep/hibernate, restart the audio devices --- interface/src/Application.cpp | 1 + libraries/audio-client/src/AudioClient.cpp | 6 ++++++ libraries/audio-client/src/AudioClient.h | 1 + 3 files changed, 8 insertions(+) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 421741b0a2..92bc54d43f 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -974,6 +974,7 @@ bool setupEssentials(int& argc, char** argv, bool runningMarkerExisted) { QObject::connect(PlatformHelper::instance(), &PlatformHelper::systemWillWake, [] { QMetaObject::invokeMethod(DependencyManager::get().data(), "noteAwakening", Qt::QueuedConnection); + QMetaObject::invokeMethod(DependencyManager::get().data(), "noteAwakening", Qt::QueuedConnection); }); diff --git a/libraries/audio-client/src/AudioClient.cpp b/libraries/audio-client/src/AudioClient.cpp index 5e1f285a6c..d29045c99b 100644 --- a/libraries/audio-client/src/AudioClient.cpp +++ b/libraries/audio-client/src/AudioClient.cpp @@ -1998,6 +1998,12 @@ void AudioClient::outputNotify() { } } +void AudioClient::noteAwakening() { + qCDebug(audioclient) << "Restarting the audio devices."; + switchInputToAudioDevice(_inputDeviceInfo); + switchOutputToAudioDevice(_outputDeviceInfo); +} + bool AudioClient::switchOutputToAudioDevice(const HifiAudioDeviceInfo outputDeviceInfo, bool isShutdownRequest) { Q_ASSERT_X(QThread::currentThread() == thread(), Q_FUNC_INFO, "Function invoked on wrong thread"); diff --git a/libraries/audio-client/src/AudioClient.h b/libraries/audio-client/src/AudioClient.h index 28a4f23968..b4ddb1018e 100644 --- a/libraries/audio-client/src/AudioClient.h +++ b/libraries/audio-client/src/AudioClient.h @@ -255,6 +255,7 @@ public slots: void setOutputGain(float gain) { _outputGain = gain; }; void outputNotify(); + void noteAwakening(); void loadSettings(); void saveSettings();