revert the revert of PR-16307

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;

interface/src/avatar/AvatarManager.cpp

@@ -543,26 +543,8 @@ 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());
-        }
-        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<EntityItem>(child);
-                    if (childEntity) {
-                        entityTree->deleteEntity(childEntity->getID(), true, true);
-                        if (avatar) {
-                            avatar->clearAvatarEntity(childEntity->getID(), REQUIRES_REMOVAL_FROM_TREE);
-                        }
-                    }
-                });
-            });
         }
     }
 }

interface/src/avatar/MyAvatar.cpp

@@ -1518,7 +1518,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);
     });
@@ -1526,7 +1527,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.
@@ -1583,13 +1589,16 @@ void MyAvatar::handleChangedAvatarEntityData() {
     // move the lists to minimize lock time
     std::vector<QUuid> cachedBlobsToDelete;
     std::vector<QUuid> cachedBlobsToUpdate;
-    std::vector<QUuid> entitiesToDelete;
+    QSet<EntityItemID> idsToDelete;
     std::vector<QUuid> entitiesToAdd;
     std::vector<QUuid> 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);
     });
@@ -1607,7 +1616,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);
@@ -1621,11 +1630,9 @@ void MyAvatar::handleChangedAvatarEntityData() {
     }
 
     // DELETE real entities
-    for (const auto& id : entitiesToDelete) {
+    entityTree->withWriteLock([&] {
-        entityTree->withWriteLock([&] {
+        entityTree->deleteEntitiesByID(idsToDelete);
-            entityTree->deleteEntity(id);
+    });
-        });
-    }
 
     // ADD real entities
     EntityEditPacketSender* packetSender = qApp->getEntityEditPacketSender();
@@ -1738,7 +1745,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);
                 }
@@ -2469,18 +2476,11 @@ bool isWearableEntity(const EntityItemPointer& entity) {
 void MyAvatar::removeWornAvatarEntity(const EntityItemID& entityID) {
     auto treeRenderer = DependencyManager::get<EntityTreeRenderer>();
     EntityTreePointer entityTree = treeRenderer ? treeRenderer->getTree() : nullptr;
-
     if (entityTree) {
         auto entity = entityTree->findEntityByID(entityID);
         if (entity && isWearableEntity(entity)) {
-            entityTree->withWriteLock([&entityID, &entityTree] {
+            treeRenderer->deleteEntity(entityID);
-                // remove this entity first from the entity tree
+            clearAvatarEntity(entityID);
-                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);
         }
     }
 }
@@ -2490,8 +2490,16 @@ void MyAvatar::clearWornAvatarEntities() {
     _avatarEntitiesLock.withReadLock([&] {
         avatarEntityIDs = _packedAvatarEntityData.keys();
     });
-    for (auto entityID : avatarEntityIDs) {
+    auto treeRenderer = DependencyManager::get<EntityTreeRenderer>();
-        removeWornAvatarEntity(entityID);
+    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);
+            }
+        }
     }
 }
 
@@ -3934,6 +3942,10 @@ float MyAvatar::getGravity() {
 void MyAvatar::setSessionUUID(const QUuid& sessionUUID) {
     QUuid oldSessionID = getSessionUUID();
     Avatar::setSessionUUID(sessionUUID);
+    bool sendPackets = !DependencyManager::get<NodeList>()->getSessionUUID().isNull();
+    if (!sendPackets) {
+        return;
+    }
     QUuid newSessionID = getSessionUUID();
     if (newSessionID != oldSessionID) {
         auto treeRenderer = DependencyManager::get<EntityTreeRenderer>();
@@ -3943,7 +3955,6 @@ void MyAvatar::setSessionUUID(const QUuid& sessionUUID) {
             _avatarEntitiesLock.withReadLock([&] {
                 avatarEntityIDs = _packedAvatarEntityData.keys();
             });
-            bool sendPackets = !DependencyManager::get<NodeList>()->getSessionUUID().isNull();
             EntityEditPacketSender* packetSender = qApp->getEntityEditPacketSender();
             entityTree->withWriteLock([&] {
                 for (const auto& entityID : avatarEntityIDs) {
@@ -3951,11 +3962,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

interface/src/avatar/OtherAvatar.cpp

@@ -561,11 +561,17 @@ void OtherAvatar::handleChangedAvatarEntityData() {
             _avatarEntitiesLock.withReadLock([&] {
                 packedAvatarEntityData = _packedAvatarEntityData;
             });
+            QSet<EntityItemID> 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

interface/src/ui/AvatarCertifyBanner.cpp

@@ -62,16 +62,7 @@ void AvatarCertifyBanner::show(const QUuid& avatarID) {
 
 void AvatarCertifyBanner::clear() {
     if (_active) {
-        auto entityTreeRenderer = DependencyManager::get<EntityTreeRenderer>();
+        DependencyManager::get<EntityTreeRenderer>()->deleteEntity(_bannerID);
-        EntityTreePointer entityTree = entityTreeRenderer->getTree();
-        if (!entityTree) {
-            return;
-        }
-
-        entityTree->withWriteLock([&] {
-            entityTree->deleteEntity(_bannerID);
-        });
-
         _active = false;
     }
 }

libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp

@@ -340,11 +340,13 @@ void Avatar::removeAvatarEntitiesFromTree() {
         _avatarEntitiesLock.withReadLock([&] {
             avatarEntityIDs = _packedAvatarEntityData.keys();
         });
-        entityTree->withWriteLock([&] {
+        QSet<EntityItemID> ids;
-            for (const auto& entityID : avatarEntityIDs) {
+        foreach (auto id, avatarEntityIDs) {
-                entityTree->deleteEntity(entityID, true, true);
+            ids.insert(id);
-            }
+        }
-        });
+        bool force = true;
+        bool ignoreWarnings = true;
+        entityTree->deleteEntitiesByID(ids, force, ignoreWarnings); // locks tree
     }
 }
 

libraries/avatars/src/AvatarData.cpp

@@ -3033,15 +3033,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

libraries/avatars/src/AvatarData.h

@@ -1179,7 +1179,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);

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -226,7 +226,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");
                     }
@@ -240,7 +240,6 @@ void EntityTreeRenderer::stopDomainAndNonOwnedEntities() {
 void EntityTreeRenderer::clearDomainAndNonOwnedEntities() {
     stopDomainAndNonOwnedEntities();
 
-    auto sessionUUID = getTree()->getMyAvatarSessionUUID();
     std::unordered_map<EntityItemID, EntityRendererPointer> savedEntities;
     std::unordered_set<EntityRendererPointer> savedRenderables;
     // remove all entities from the scene
@@ -249,7 +248,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;
@@ -261,6 +260,7 @@ void EntityTreeRenderer::clearDomainAndNonOwnedEntities() {
     _renderablesToUpdate = savedRenderables;
     _entitiesInScene = savedEntities;
 
+    auto sessionUUID = getTree()->getMyAvatarSessionUUID();
     if (_layeredZones.clearDomainAndNonOwnedZones(sessionUUID)) {
         applyLayeredZones();
     }
@@ -683,7 +683,7 @@ void EntityTreeRenderer::leaveDomainAndNonOwnedEntities() {
         QSet<EntityItemID> 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");
@@ -1221,7 +1221,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 {
@@ -1362,6 +1362,10 @@ EntityItemPointer EntityTreeRenderer::getEntity(const EntityItemID& id) {
     return result;
 }
 
+void EntityTreeRenderer::deleteEntity(const EntityItemID& id) const {
+    DependencyManager::get<EntityScriptingInterface>()->deleteEntity(id);
+}
+
 void EntityTreeRenderer::onEntityChanged(const EntityItemID& id) {
     _changedEntitiesGuard.withWriteLock([&] {
         _changedEntities.insert(id);

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

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<NodeList>();
+    // 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 {

libraries/entities/src/DeleteEntityOperator.cpp

@@ -53,6 +53,15 @@ 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();
+    details.cube = details.containingElement->getAACube();
+    _entitiesToDelete << details;
+    _lookingCount++;
+}
 
 // does this entity tree element contain the old entity
 bool DeleteEntityOperator::subTreeContainsSomeEntitiesToDelete(const OctreeElementPointer& element) {

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;
 

libraries/entities/src/EntityEditFilters.cpp

@@ -43,7 +43,7 @@ QList<EntityItemID> 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

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);

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()) {
+        } else if (properties.getOwningAvatarID() == _myAvatar->getID() || properties.getOwningAvatarID() == AVATAR_SELF_ID) {
-            // this is an avatar-based entity --> update our avatar-data rather than sending to the entity-server
+            // 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";

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);
@@ -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<EntityItem>(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,6 +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 here
     properties.setEntityHostType(getEntityHostType());
     properties.setOwningAvatarID(getOwningAvatarID());
     setLastBroadcast(now); // for debug/physics status icons
@@ -3409,9 +3431,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);

libraries/entities/src/EntityItem.h

@@ -18,6 +18,7 @@
 #include <glm/glm.hpp>
 
 #include <QtGui/QWindow>
+#include <QSet>
 
 #include <Octree.h> // for EncodeBitstreamParams class
 #include <OctreeElement.h> // for OctreeElement::AppendState
@@ -49,6 +50,7 @@ typedef std::shared_ptr<EntityTree> EntityTreePointer;
 typedef std::shared_ptr<EntityDynamicInterface> EntityDynamicPointer;
 typedef std::shared_ptr<EntityTreeElement> EntityTreeElementPointer;
 using EntityTreeElementExtraEncodeDataPointer = std::shared_ptr<EntityTreeElementExtraEncodeData>;
+using SetOfEntities = QSet<EntityItemPointer>;
 
 #define DONT_ALLOW_INSTANTIATION virtual void pureVirtualFunctionPlaceHolder() = 0;
 #define ALLOW_INSTANTIATION virtual void pureVirtualFunctionPlaceHolder() override { };
@@ -514,7 +516,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; }
@@ -540,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; }

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
     }
 

libraries/entities/src/EntityScriptingInterface.cpp

@@ -480,17 +480,11 @@ QUuid EntityScriptingInterface::addEntityInternal(const EntityItemProperties& pr
 
     _activityTracking.addedEntityCount++;
 
-    auto nodeList = DependencyManager::get<NodeList>();
-    auto sessionID = nodeList->getSessionUUID();
-
     EntityItemProperties propertiesWithSimID = properties;
     propertiesWithSimID.setEntityHostType(entityHostType);
     if (entityHostType == entity::HostType::AVATAR) {
-        if (sessionID.isNull()) {
+        // only allow adding our own avatar entities from script
-            // null sessionID is unacceptable in this case
+        propertiesWithSimID.setOwningAvatarID(AVATAR_SELF_ID);
-            sessionID = AVATAR_SELF_ID;
-        }
-        propertiesWithSimID.setOwningAvatarID(sessionID);
     } 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<NodeList>();
+    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;
@@ -970,43 +966,52 @@ void EntityScriptingInterface::deleteEntity(const QUuid& id) {
 
     _activityTracking.deletedEntityCount++;
 
-    EntityItemID entityID(id);
+    if (!_entityTree) {
-    bool shouldSendDeleteToServer = true;
+        return;
-
-    // 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<NodeList>();
-                const QUuid myNodeID = nodeList->getSessionUUID();
-                if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) {
-                    // don't delete other avatar's avatarEntities
-                    shouldSendDeleteToServer = false;
-                    return;
-                }
-
-                if (entity->getLocked()) {
-                    shouldSendDeleteToServer = false;
-                } else {
-                    // only delete local entities, server 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 at this point, we know the id, and we should still delete the entity, send the update to the entity server
+    EntityItemID entityID(id);
-    if (shouldSendDeleteToServer) {
+
-        getEntityPacketSender()->queueEraseEntityMessage(entityID);
+    // 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<NodeList>()->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());
     }
 }
 
@@ -1671,7 +1676,7 @@ bool EntityScriptingInterface::actionWorker(const QUuid& entityID,
             return;
         }
 
-        if (entity->isAvatarEntity() && entity->getOwningAvatarID() != myNodeID) {
+        if (entity->isAvatarEntity() && !entity->isMyAvatarEntity()) {
             return;
         }
 

libraries/entities/src/EntitySimulation.cpp

@@ -19,41 +19,36 @@
 
 void EntitySimulation::setEntityTree(EntityTreePointer tree) {
     if (_entityTree && _entityTree != tree) {
-        _mortalEntities.clear();
-        _nextExpiry = std::numeric_limits<uint64_t>::max();
-        _entitiesToUpdate.clear();
         _entitiesToSort.clear();
         _simpleKinematicEntities.clear();
+        _changedEntities.clear();
+        _entitiesToUpdate.clear();
+        _mortalEntities.clear();
+        _nextExpiry = std::numeric_limits<uint64_t>::max();
     }
     _entityTree = 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();
+    processDeadEntities();
 }
 
-void EntitySimulation::takeDeadEntities(SetOfEntities& entitiesToDelete) {
+void EntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
+    // remove from all internal lists except _deadEntitiesToRemoveFromTree
-    entitiesToDelete.swap(_deadEntities);
-    _deadEntities.clear();
-}
-
-void EntitySimulation::removeEntityInternal(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);
 }
 
@@ -62,10 +57,9 @@ void EntitySimulation::prepareEntityForDelete(EntityItemPointer entity) {
     assert(entity->isDead());
     if (entity->isSimulated()) {
         QMutexLocker lock(&_mutex);
-        entity->clearActions(getThisPointer());
+        removeEntityFromInternalLists(entity);
-        removeEntityInternal(entity);
         if (entity->getElement()) {
-            _deadEntities.insert(entity);
+            _deadEntitiesToRemoveFromTree.insert(entity);
             _entityTree->cleanupCloneIDs(entity->getEntityItemID());
         }
     }
@@ -149,10 +143,7 @@ void EntitySimulation::sortEntitiesThatMoved() {
     _entitiesToSort.clear();
 }
 
-void EntitySimulation::addEntity(EntityItemPointer entity) {
+void EntitySimulation::addEntityToInternalLists(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
-    assert(entity);
-    entity->deserializeActions();
     if (entity->isMortal()) {
         _mortalEntities.insert(entity);
         uint64_t expiry = entity->getExpiry();
@@ -163,10 +154,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.
@@ -218,16 +213,14 @@ void EntitySimulation::processChangedEntity(const EntityItemPointer& entity) {
 
 void EntitySimulation::clearEntities() {
     QMutexLocker lock(&_mutex);
-    _mortalEntities.clear();
-    _nextExpiry = std::numeric_limits<uint64_t>::max();
-    _entitiesToUpdate.clear();
     _entitiesToSort.clear();
     _simpleKinematicEntities.clear();
-
+    _changedEntities.clear();
-    clearEntitiesInternal();
-
     _allEntities.clear();
-    _deadEntities.clear();
+    _deadEntitiesToRemoveFromTree.clear();
+    _entitiesToUpdate.clear();
+    _mortalEntities.clear();
+    _nextExpiry = std::numeric_limits<uint64_t>::max();
 }
 
 void EntitySimulation::moveSimpleKinematics(uint64_t now) {
@@ -263,25 +256,22 @@ void EntitySimulation::moveSimpleKinematics(uint64_t now) {
     }
 }
 
-void EntitySimulation::addDynamic(EntityDynamicPointer dynamic) {
+void EntitySimulation::processDeadEntities() {
-    QMutexLocker lock(&_dynamicsMutex);
+    if (_deadEntitiesToRemoveFromTree.empty()) {
-    _dynamicsToAdd += dynamic;
+        return;
-}
-
-void EntitySimulation::removeDynamic(const QUuid dynamicID) {
-    QMutexLocker lock(&_dynamicsMutex);
-    _dynamicsToRemove += dynamicID;
-}
-
-void EntitySimulation::removeDynamics(QList<QUuid> dynamicIDsToRemove) {
-    QMutexLocker lock(&_dynamicsMutex);
-    foreach(QUuid uuid, dynamicIDsToRemove) {
-        _dynamicsToRemove.insert(uuid);
     }
-}
+    SetOfEntities entitiesToDeleteImmediately;
-
+    // NOTE: dummyList will be empty because this base-class implementation is only used server-side
-void EntitySimulation::applyDynamicChanges() {
+    // for which ATM we only process domain-entities, and since we are passing nullSessionID for authorization
-    QMutexLocker lock(&_dynamicsMutex);
+    // EntityItem::collectChildrenForDelete() will not collect domain-entities into this side list.
-    _dynamicsToAdd.clear();
+    SetOfEntities dummyList;
-    _dynamicsToRemove.clear();
+    QUuid nullSessionID;
+    foreach (auto entity, _deadEntitiesToRemoveFromTree) {
+        entitiesToDeleteImmediately.insert(entity);
+        entity->collectChildrenForDelete(entitiesToDeleteImmediately, dummyList, nullSessionID);
+    }
+    if (_entityTree) {
+        _entityTree->deleteEntitiesByPointer(entitiesToDeleteImmediately);
+    }
+    _deadEntitiesToRemoveFromTree.clear();
 }

libraries/entities/src/EntitySimulation.h

@@ -16,17 +16,14 @@
 #include <unordered_set>
 
 #include <QtCore/QObject>
-#include <QSet>
 #include <QVector>
 
 #include <PerfStat.h>
 
-#include "EntityDynamicInterface.h"
 #include "EntityItem.h"
 #include "EntityTree.h"
 
 using EntitySimulationPointer = std::shared_ptr<EntitySimulation>;
-using SetOfEntities = QSet<EntityItemPointer>;
 using VectorOfEntities = QVector<EntityItemPointer>;
 
 // the EntitySimulation needs to know when these things change on an entity,
@@ -47,8 +44,8 @@ const int DIRTY_SIMULATION_FLAGS =
 
 class EntitySimulation : public QObject, public std::enable_shared_from_this<EntitySimulation> {
 public:
-    EntitySimulation() : _mutex(QMutex::Recursive), _entityTree(NULL), _nextExpiry(std::numeric_limits<uint64_t>::max()) { }
+    EntitySimulation() : _mutex(QMutex::Recursive), _nextExpiry(std::numeric_limits<uint64_t>::max()), _entityTree(nullptr) { }
-    virtual ~EntitySimulation() { setEntityTree(NULL); }
+    virtual ~EntitySimulation() { setEntityTree(nullptr); }
 
     inline EntitySimulationPointer getThisPointer() const {
         return std::const_pointer_cast<EntitySimulation>(shared_from_this());
@@ -57,12 +54,12 @@ public:
     /// \param tree pointer to EntityTree which is stored internally
     void setEntityTree(EntityTreePointer tree);
 
-    void updateEntities();
+    virtual void updateEntities();
 
-    virtual void addDynamic(EntityDynamicPointer dynamic);
+    // FIXME: remove these
-    virtual void removeDynamic(const QUuid dynamicID);
+    virtual void addDynamic(EntityDynamicPointer dynamic) {}
-    virtual void removeDynamics(QList<QUuid> dynamicIDsToRemove);
+    virtual void removeDynamic(const QUuid dynamicID) {}
-    virtual void applyDynamicChanges();
+    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
@@ -72,27 +69,22 @@ 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);
 
     EntityTreePointer getEntityTree() { return _entityTree; }
 
-    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();
+    virtual void queueEraseDomainEntities(const SetOfEntities& domainEntities) const { }
 
 protected:
-    // These pure virtual methods are protected because they are not to be called will-nilly. The base class
+    virtual void addEntityToInternalLists(EntityItemPointer entity);
-    // calls them in the right places.
+    virtual void removeEntityFromInternalLists(EntityItemPointer entity);
-    virtual void updateEntitiesInternal(uint64_t now) = 0;
-    virtual void addEntityInternal(EntityItemPointer entity) = 0;
-    virtual void removeEntityInternal(EntityItemPointer entity);
     virtual void processChangedEntity(const EntityItemPointer& entity);
-    virtual void clearEntitiesInternal() = 0;
+    virtual void processDeadEntities();
 
     void expireMortalEntities(uint64_t now);
     void callUpdateOnEntitiesThatNeedIt(uint64_t now);
@@ -102,27 +94,21 @@ protected:
 
     SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree)
     SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion
-    QList<EntityDynamicPointer> _dynamicsToAdd;
+    SetOfEntities _deadEntitiesToRemoveFromTree;
-    QSet<QUuid> _dynamicsToRemove;
-    QMutex _dynamicsMutex { QMutex::Recursive };
-
-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<EntityItemPointer> _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()
+    // back pointer to EntityTree structure
+    EntityTreePointer _entityTree;
 };
 
 #endif // hifi_EntitySimulation_h

libraries/entities/src/EntityTree.cpp

@@ -98,14 +98,15 @@ void EntityTree::eraseDomainAndNonOwnedEntities() {
             if (element) {
                 element->cleanupDomainAndNonOwnedEntities();
             }
-
+            if (!getIsServer()) {
-            if (entity->isLocalEntity() || (entity->isAvatarEntity() && entity->getOwningAvatarID() == getMyAvatarSessionUUID())) {
+                if (entity->isLocalEntity() || entity->isMyAvatarEntity()) {
-                savedEntities[entity->getEntityItemID()] = entity;
+                    savedEntities[entity->getEntityItemID()] = entity;
-            } else {
+                } else {
-                int32_t spaceIndex = entity->getSpaceIndex();
+                    int32_t spaceIndex = entity->getSpaceIndex();
-                if (spaceIndex != -1) {
+                    if (spaceIndex != -1) {
-                    // stale spaceIndices will be freed later
+                        // stale spaceIndices will be freed later
-                    _staleProxies.push_back(spaceIndex);
+                        _staleProxies.push_back(spaceIndex);
+                    }
                 }
             }
         }
@@ -121,7 +122,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);
             }
         }
@@ -144,10 +145,12 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
             if (element) {
                 element->cleanupEntities();
             }
-            int32_t spaceIndex = entity->getSpaceIndex();
+            if (!getIsServer()) {
-            if (spaceIndex != -1) {
+                int32_t spaceIndex = entity->getSpaceIndex();
-                // assume stale spaceIndices will be freed later
+                if (spaceIndex != -1) {
-                _staleProxies.push_back(spaceIndex);
+                    // 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);
+    // NOTE: can be called without lock because deleteEntitiesByID() will lock
-    if (!containingElement) {
+    QSet<EntityItemID> ids;
-        if (!ignoreWarnings) {
+    ids << entityID;
-            qCWarning(entities) << "EntityTree::deleteEntity() on non-existent entityID=" << entityID;
+    deleteEntitiesByID(ids, force, ignoreWarnings);
-        }
-        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<EntityItem>(descendant);
-        if (descendantEntity) {
-            emit deletingEntityPointer(descendantEntity.get());
-        }
-    });
-
-    recurseTreeWithOperator(&theOperator);
-    processRemovedEntities(theOperator);
-    _isDirty = true;
 }
 
 void EntityTree::unhookChildAvatar(const EntityItemID entityID) {
-
+    if (!getIsServer()) {
-    EntityItemPointer entity = findEntityByEntityItemID(entityID);
+        EntityItemPointer entity = findEntityByEntityItemID(entityID);
-
+        entity->forEachDescendant([&](SpatiallyNestablePointer child) {
-    entity->forEachDescendant([&](SpatiallyNestablePointer child) {
+            if (child->getNestableType() == NestableType::Avatar) {
-        if (child->getNestableType() == NestableType::Avatar) {
+                child->setParentID(nullptr);
-            child->setParentID(nullptr);
+            }
-        }
+        });
-    });
+    }
 }
 
 void EntityTree::cleanupCloneIDs(const EntityItemID& entityID) {
@@ -684,39 +647,100 @@ void EntityTree::cleanupCloneIDs(const EntityItemID& entityID) {
     }
 }
 
-void EntityTree::deleteEntities(QSet<EntityItemID> entityIDs, bool force, bool ignoreWarnings) {
+void EntityTree::recursivelyFilterAndCollectForDelete(const EntityItemPointer& entity, SetOfEntities& entitiesToDelete, bool force) const {
-    // NOTE: callers must lock the tree before using this method
+    // 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<EntityItem>(child);
+                    recursivelyFilterAndCollectForDelete(childEntity, entitiesToDelete, force);
+                }
+            }
+        }
+    }
+}
+
+void EntityTree::deleteEntitiesByID(const QSet<EntityItemID>& 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<NodeList>()->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) {
+    for (auto entity : entities) {
-        EntityTreeElementPointer containingElement = getContainingElement(entityID);
+        if (entity->getElement()) {
-        if (!containingElement) {
+            theOperator.addEntityToDeleteList(entity);
-            if (!ignoreWarnings) {
+            emit deletingEntity(entity->getID());
-                qCWarning(entities) << "EntityTree::deleteEntities() on non-existent entityID=" << entityID;
+            emit deletingEntityPointer(entity.get());
-            }
-            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());
     }
 
     if (!theOperator.getEntities().empty()) {
@@ -727,23 +751,11 @@ void EntityTree::deleteEntities(QSet<EntityItemID> 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<EntityItemID> childrenIDs;
-            theEntity->forEachChild([&](SpatiallyNestablePointer child) {
-                if (child->getNestableType() == NestableType::Entity) {
-                    childrenIDs += child->getID();
-                }
-            });
-            deleteEntities(childrenIDs, true, true);
-        }
-
-        theEntity->die();
-
         if (getIsServer()) {
             removeCertifiedEntityOnServer(theEntity);
 
@@ -751,19 +763,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);
-        }
     }
 }
 
@@ -1369,7 +1386,7 @@ void EntityTree::fixupTerseEditLogging(EntityItemProperties& properties, QList<Q
 }
 
 
-bool EntityTree::filterProperties(EntityItemPointer& existingEntity, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged, FilterType filterType) {
+bool EntityTree::filterProperties(const EntityItemPointer& existingEntity, EntityItemProperties& propertiesIn, EntityItemProperties& propertiesOut, bool& wasChanged, FilterType filterType) const {
     bool accepted = true;
     auto entityEditFilters = DependencyManager::get<EntityEditFilters>();
     if (entityEditFilters) {
@@ -1749,9 +1766,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;
@@ -2217,7 +2234,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);
     }
 }
@@ -2249,22 +2268,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<EntityItemID> idsToDelete;
-
-                    for (auto entity : deadEntities) {
-                        idsToDelete.insert(entity->getEntityItemID());
-                    }
-
-                    // delete these things the roundabout way
-                    deleteEntities(idsToDelete, true);
-                }
-            }
         });
     }
 }
@@ -2353,12 +2356,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));
 
@@ -2366,10 +2374,9 @@ int EntityTree::processEraseMessage(ReceivedMessage& message, const SharedNodePo
         message.readPrimitive(&numberOfIDs);
 
         if (numberOfIDs > 0) {
-            QSet<EntityItemID> entityItemIDsToDelete;
+            QSet<EntityItemID> 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
@@ -2381,64 +2388,85 @@ int EntityTree::processEraseMessage(ReceivedMessage& message, const SharedNodePo
                 #endif
 
                 EntityItemID entityItemID(entityID);
+                idsToDelete << entityItemID;
+            }
 
-                if (shouldEraseEntity(entityID, sourceNode)) {
+            // domain-entity deletion can trigger deletion of other entities the entity-server doesn't know about
-                    entityItemIDsToDelete << entityItemID;
+            // so we must recurse down the children and collect consequential deletes however
-                    cleanupCloneIDs(entityItemID);
+            // 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<NodeList>();
+            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.
+// NOTE: Caller must write-lock the tree before calling this.
-// TODO: consider consolidating processEraseMessageDetails() and processEraseMessage()
 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));
+    memcpy(&numberOfIds, dataByteArray.constData(), sizeof(numberOfIds));
-    dataAt += sizeof(numberOfIds);
     processedBytes += sizeof(numberOfIds);
 
     if (numberOfIds > 0) {
-        QSet<EntityItemID> entityItemIDsToDelete;
+        QSet<EntityItemID> 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);
+            QUuid id = QUuid::fromRfc4122(encodedID);
-            dataAt += encodedID.size();
             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);
+            EntityItemID entityID(id);
-
+            ids << entityID;
-            if (shouldEraseEntity(entityID, sourceNode)) {
-                entityItemIDsToDelete << entityItemID;
-                cleanupCloneIDs(entityItemID);
-            }
-
         }
-        deleteEntities(entityItemIDsToDelete, true, true);
+
+        bool force = sourceNode->isAllowedEditor();
+        bool ignoreWarnings = true;
+        deleteEntitiesByID(ids, force, ignoreWarnings);
     }
     return (int)processedBytes;
 }

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<EntityItemID> entityIDs, bool force = false, bool ignoreWarnings = true);
+
+    void deleteEntitiesByID(const QSet<EntityItemID>& 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;
+    mutable quint64 _totalDecodeTime = 0;
-    quint64 _totalLookupTime = 0;
+    mutable quint64 _totalLookupTime = 0;
-    quint64 _totalUpdateTime = 0;
+    mutable quint64 _totalUpdateTime = 0;
-    quint64 _totalCreateTime = 0;
+    mutable quint64 _totalCreateTime = 0;
-    quint64 _totalLoggingTime = 0;
+    mutable quint64 _totalLoggingTime = 0;
-    quint64 _totalFilterTime = 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;
 

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 {

libraries/entities/src/SimpleEntitySimulation.cpp

@@ -47,14 +47,17 @@ 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);
 }
 
-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 +68,6 @@ void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
             }
         }
     } else {
-        QMutexLocker lock(&_mutex);
         _entitiesWithSimulationOwner.insert(entity);
         _nextStaleOwnershipExpiry = glm::min(_nextStaleOwnershipExpiry, entity->getSimulationOwnershipExpiry());
 
@@ -79,10 +81,10 @@ void SimpleEntitySimulation::addEntityInternal(EntityItemPointer entity) {
     }
 }
 
-void SimpleEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
+void SimpleEntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
-    EntitySimulation::removeEntityInternal(entity);
     _entitiesWithSimulationOwner.remove(entity);
     _entitiesThatNeedSimulationOwner.remove(entity);
+    EntitySimulation::removeEntityFromInternalLists(entity);
 }
 
 void SimpleEntitySimulation::processChangedEntity(const EntityItemPointer& entity) {
@@ -135,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() {

libraries/entities/src/SimpleEntitySimulation.h

@@ -23,16 +23,16 @@ using SimpleEntitySimulationPointer = std::shared_ptr<SimpleEntitySimulation>;
 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 addEntityInternal(EntityItemPointer entity) override;
+    void removeEntityFromInternalLists(EntityItemPointer entity) override;
-    void removeEntityInternal(EntityItemPointer entity) override;
     void processChangedEntity(const EntityItemPointer& entity) override;
-    void clearEntitiesInternal() override;
 
     void sortEntitiesThatMoved() override;
 

libraries/physics/src/EntityMotionState.cpp

@@ -62,7 +62,7 @@ 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()) {
+    if (_entity->isAvatarEntity() && !_entity->isMyAvatarEntity()) {
         // avatar entities are always thus, so we cache this fact in _ownershipState
         _ownershipState = EntityMotionState::OwnershipState::Unownable;
     }
@@ -407,8 +407,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
+    // this case is prevented by setting _ownershipState to OwnershipState::Unownable in EntityMotionState::ctor
-    assert(!(_entity->isAvatarEntity() && _entity->getOwningAvatarID() != Physics::getSessionUUID()));
+    assert(!(_entity->isAvatarEntity() && !_entity->isMyAvatarEntity()));
 
     if (_entity->getTransitingWithAvatar()) {
         return false;
@@ -768,7 +768,7 @@ uint8_t EntityMotionState::computeFinalBidPriority() const {
 }
 
 bool EntityMotionState::isLocallyOwned() const {
-    return _entity->getSimulatorID() == Physics::getSessionUUID();
+    return _entity->getSimulatorID() == Physics::getSessionUUID() || _entity->isMyAvatarEntity();
 }
 
 bool EntityMotionState::isLocallyOwnedOrShouldBe() const {
@@ -786,13 +786,21 @@ void EntityMotionState::setRegion(uint8_t region) {
 }
 
 void EntityMotionState::initForBid() {
-    assert(_ownershipState != EntityMotionState::OwnershipState::Unownable);
+    if (_ownershipState != EntityMotionState::OwnershipState::Unownable) {
-    _ownershipState = EntityMotionState::OwnershipState::PendingBid;
+        _ownershipState = EntityMotionState::OwnershipState::PendingBid;
+    }
 }
 
 void EntityMotionState::initForOwned() {
-    assert(_ownershipState != EntityMotionState::OwnershipState::Unownable);
+    if (_ownershipState != EntityMotionState::OwnershipState::Unownable) {
-    _ownershipState = EntityMotionState::OwnershipState::LocallyOwned;
+        _ownershipState = EntityMotionState::OwnershipState::LocallyOwned;
+    }
+}
+
+void EntityMotionState::clearOwnershipState() {
+    if (_ownershipState != OwnershipState::Unownable) {
+        _ownershipState = OwnershipState::NotLocallyOwned;
+    }
 }
 
 void EntityMotionState::clearObjectVelocities() const {

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);
 

libraries/physics/src/PhysicalEntitySimulation.cpp

@@ -40,14 +40,9 @@ void PhysicalEntitySimulation::init(
 }
 
 // begin EntitySimulation overrides
-void PhysicalEntitySimulation::updateEntitiesInternal(uint64_t now) {
+void PhysicalEntitySimulation::addEntityToInternalLists(EntityItemPointer entity) {
-    // Do nothing here because the "internal" update the PhysicsEngine::stepSimulation() which is done elsewhere.
+    EntitySimulation::addEntityToInternalLists(entity);
-}
+    entity->deserializeActions(); // TODO: do this elsewhere
-
-void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
-    QMutexLocker lock(&_mutex);
-    assert(entity);
-    assert(!entity->isDead());
     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;
@@ -66,23 +61,20 @@ void PhysicalEntitySimulation::addEntityInternal(EntityItemPointer entity) {
     }
 }
 
-void PhysicalEntitySimulation::removeEntityInternal(EntityItemPointer entity) {
+void PhysicalEntitySimulation::removeEntityFromInternalLists(EntityItemPointer entity) {
-    if (entity->isSimulated()) {
+    _entitiesToAddToPhysics.remove(entity);
-        EntitySimulation::removeEntityInternal(entity);
+    EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
-        _entitiesToAddToPhysics.remove(entity);
+    if (motionState) {
-
+        removeOwnershipData(motionState);
-        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
+        _entitiesToRemoveFromPhysics.insert(entity);
-        if (motionState) {
+    }
-            removeOwnershipData(motionState);
+    if (entity->isDead() && entity->getElement()) {
-            _entitiesToRemoveFromPhysics.insert(entity);
+        _deadEntitiesToRemoveFromTree.insert(entity);
-        }
-        if (entity->isDead() && entity->getElement()) {
-            _deadEntities.insert(entity);
-        }
     }
     if (entity->isAvatarEntity()) {
         _deadAvatarEntities.insert(entity);
     }
+    EntitySimulation::removeEntityFromInternalLists(entity);
 }
 
 void PhysicalEntitySimulation::removeOwnershipData(EntityMotionState* motionState) {
@@ -115,18 +107,6 @@ void PhysicalEntitySimulation::clearOwnershipData() {
     _bids.clear();
 }
 
-void PhysicalEntitySimulation::takeDeadEntities(SetOfEntities& deadEntities) {
-    QMutexLocker lock(&_mutex);
-    for (auto entity : _deadEntities) {
-        EntityMotionState* motionState = static_cast<EntityMotionState*>(entity->getPhysicsInfo());
-        if (motionState) {
-            _entitiesToRemoveFromPhysics.insert(entity);
-        }
-    }
-    _deadEntities.swap(deadEntities);
-    _deadEntities.clear();
-}
-
 void PhysicalEntitySimulation::takeDeadAvatarEntities(SetOfEntities& deadEntities) {
     _deadAvatarEntities.swap(deadEntities);
     _deadAvatarEntities.clear();
@@ -190,11 +170,44 @@ void PhysicalEntitySimulation::processChangedEntity(const EntityItemPointer& ent
     }
 }
 
-void PhysicalEntitySimulation::clearEntitiesInternal() {
+void PhysicalEntitySimulation::processDeadEntities() {
+    if (_deadEntitiesToRemoveFromTree.empty()) {
+        return;
+    }
+    PROFILE_RANGE(simulation_physics, "Deletes");
+    SetOfEntities entitiesToDeleteImmediately;
+    SetOfEntities domainEntities;
+    QUuid sessionID = Physics::getSessionUUID();
+    QMutexLocker lock(&_mutex);
+    for (auto entity : _deadEntitiesToRemoveFromTree) {
+        EntityMotionState* motionState = static_cast<EntityMotionState*>(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);
+        }
+    }
+    _deadEntitiesToRemoveFromTree.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
     // 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);
 
@@ -216,11 +229,23 @@ void PhysicalEntitySimulation::clearEntitiesInternal() {
     _entitiesToAddToPhysics.clear();
     _incomingChanges.clear();
     _entitiesToDeleteLater.clear();
+
+    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) {
-    // 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);
@@ -228,11 +253,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());
-        removeEntityInternal(entity);
+        EntitySimulation::prepareEntityForDelete(entity);
     }
     _entitiesToDeleteLater.clear();
 }
@@ -647,10 +672,16 @@ 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::applyDynamicChanges() {
     QList<EntityDynamicPointer> dynamicsFailedToAdd;
     if (_physicsEngine) {
@@ -665,8 +696,8 @@ void PhysicalEntitySimulation::applyDynamicChanges() {
                 }
             }
         }
-        // applyDynamicChanges will clear _dynamicsToRemove and _dynamicsToAdd
+        _dynamicsToAdd.clear();
-        EntitySimulation::applyDynamicChanges();
+        _dynamicsToRemove.clear();
     }
 
     // put back the ones that couldn't yet be added

libraries/physics/src/PhysicalEntitySimulation.h

@@ -19,6 +19,7 @@
 #include <btBulletDynamicsCommon.h>
 #include <BulletCollision/CollisionDispatch/btGhostObject.h>
 
+#include <EntityDynamicInterface.h>
 #include <EntityItem.h>
 #include <EntitySimulation.h>
 #include <workload/Space.h>
@@ -58,22 +59,24 @@ public:
     void init(EntityTreePointer tree, PhysicsEnginePointer engine, EntityEditPacketSender* packetSender);
     void setWorkloadSpace(const workload::SpacePointer space) { _space = space; }
 
-    virtual void addDynamic(EntityDynamicPointer dynamic) override;
+    void addDynamic(EntityDynamicPointer dynamic) override;
-    virtual void applyDynamicChanges() override;
+    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 updateEntitiesInternal(uint64_t now) override;
+    void addEntityToInternalLists(EntityItemPointer entity) override;
-    virtual void addEntityInternal(EntityItemPointer entity) override;
+    void removeEntityFromInternalLists(EntityItemPointer entity) override;
-    virtual void removeEntityInternal(EntityItemPointer entity) override;
     void processChangedEntity(const EntityItemPointer& entity) override;
-    virtual void clearEntitiesInternal() override;
+    void processDeadEntities() override;
 
     void removeOwnershipData(EntityMotionState* motionState);
     void clearOwnershipData();
@@ -121,8 +124,13 @@ private:
 
     VectorOfEntityMotionStates _owned;
     VectorOfEntityMotionStates _bids;
-    SetOfEntities _deadAvatarEntities;
+    SetOfEntities _deadAvatarEntities; // to remove from Avatar's lists
     std::vector<EntityItemPointer> _entitiesToDeleteLater;
+
+    QList<EntityDynamicPointer> _dynamicsToAdd;
+    QSet<QUuid> _dynamicsToRemove;
+    QMutex _dynamicsMutex { QMutex::Recursive };
+
     workload::SpacePointer _space;
     uint64_t _nextBidExpiry;
     uint32_t _lastStepSendPackets { 0 };

libraries/shared/src/PhysicsHelpers.cpp

@@ -10,10 +10,12 @@
 //
 
 #include "PhysicsHelpers.h"
-#include "NumericalConstants.h"
+
 #include <QUuid>
 
+#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() {

libraries/shared/src/SpatialParentFinder.h

@@ -21,7 +21,7 @@ using SpatiallyNestableWeakPointer = std::weak_ptr<SpatiallyNestable>;
 using SpatiallyNestablePointer = std::shared_ptr<SpatiallyNestable>;
 class SpatialParentTree {
 public:
-    virtual SpatiallyNestablePointer findByID(const QUuid& id) const { return nullptr; }
+    virtual SpatiallyNestablePointer findByID(const QUuid& id) const = 0;
 };
 class SpatialParentFinder : public Dependency {
 

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);