fix flagging for avatar entity update/delete

interface/src/Application.cpp

@@ -1013,7 +1013,7 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo
                 // This is done so as not break previous command line scripts
                 if (testScriptPath.left(URL_SCHEME_HTTP.length()) == URL_SCHEME_HTTP ||
                     testScriptPath.left(URL_SCHEME_FTP.length()) == URL_SCHEME_FTP) {
-                    
+
                     setProperty(hifi::properties::TEST, QUrl::fromUserInput(testScriptPath));
                 } else if (QFileInfo(testScriptPath).exists()) {
                     setProperty(hifi::properties::TEST, QUrl::fromLocalFile(testScriptPath));
@@ -1830,14 +1830,6 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo
         }
     });
 
-    connect(getEntities()->getTree().get(), &EntityTree::deletingEntity, [](const EntityItemID& entityItemID) {
-        auto avatarManager = DependencyManager::get<AvatarManager>();
-        auto myAvatar = avatarManager ? avatarManager->getMyAvatar() : nullptr;
-        if (myAvatar) {
-            myAvatar->clearAvatarEntity(entityItemID);
-        }
-    });
-
     EntityTree::setAddMaterialToEntityOperator([this](const QUuid& entityID, graphics::MaterialLayer material, const std::string& parentMaterialName) {
         // try to find the renderable
         auto renderable = getEntities()->renderableForEntityId(entityID);
@@ -2617,7 +2609,7 @@ Application::~Application() {
 
     // Can't log to file passed this point, FileLogger about to be deleted
     qInstallMessageHandler(LogHandler::verboseMessageHandler);
-    
+
     _renderEventHandler->deleteLater();
 }
 
@@ -5498,8 +5490,8 @@ void Application::update(float deltaTime) {
         quint64 now = usecTimestampNow();
         // Check for flagged EntityData having arrived.
         auto entityTreeRenderer = getEntities();
-        if (isServerlessMode() || 
+        if (isServerlessMode() ||
-            (_octreeProcessor.isLoadSequenceComplete() )) {
+            (entityTreeRenderer && _octreeProcessor.octreeSequenceIsComplete(entityTreeRenderer->getLastOctreeMessageSequence()) )) {
             // we've received a new full-scene octree stats packet, or it's been long enough to try again anyway
             _lastPhysicsCheckTime = now;
             _fullSceneCounterAtLastPhysicsCheck = _fullSceneReceivedCounter;
@@ -6393,7 +6385,7 @@ void Application::nodeActivated(SharedNodePointer node) {
         if (_avatarOverrideUrl.isValid()) {
             getMyAvatar()->useFullAvatarURL(_avatarOverrideUrl);
         }
-        
+
         if (getMyAvatar()->getFullAvatarURLFromPreferences() != getMyAvatar()->getSkeletonModelURL()) {
             getMyAvatar()->resetFullAvatarURL();
         }

interface/src/avatar/MyAvatar.cpp

@@ -1292,7 +1292,6 @@ void MyAvatar::loadData() {
         // HACK: manually remove empty 'avatarEntityData' else legacy data may persist in settings file
         settings.remove("avatarEntityData");
     }
-    setAvatarEntityDataChanged(true);
 
     // Flying preferences must be loaded before calling setFlyingEnabled()
     Setting::Handle<bool> firstRunVal { Settings::firstRun, true };

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

@@ -224,14 +224,24 @@ void Avatar::setAvatarEntityDataChanged(bool value) {
 
 void Avatar::updateAvatarEntities() {
     PerformanceTimer perfTimer("attachments");
+
+    // AVATAR ENTITY UPDATE FLOW
     // - if queueEditEntityMessage sees clientOnly flag it does _myAvatar->updateAvatarEntity()
-    // - updateAvatarEntity saves the bytes and sets _avatarEntityDataLocallyEdited
+    // - updateAvatarEntity saves the bytes and flags the trait instance for the entity as updated
-    // - MyAvatar::update notices _avatarEntityDataLocallyEdited and calls sendIdentityPacket
+    // - ClientTraitsHandler::sendChangedTraitsToMixer sends the entity bytes to the mixer which relays them to other interfaces
-    // - sendIdentityPacket sends the entity bytes to the server which relays them to other interfaces
+    // - AvatarHashMap::processBulkAvatarTraits on other interfaces calls avatar->processTraitInstace
-    // - AvatarHashMap::processAvatarIdentityPacket on other interfaces call avatar->setAvatarEntityData()
+    // - AvatarData::processTraitInstance calls updateAvatarEntity, which sets _avatarEntityDataChanged = true
-    // - setAvatarEntityData saves the bytes and sets _avatarEntityDataChanged = true
     // - (My)Avatar::simulate notices _avatarEntityDataChanged and here we are...
 
+    // AVATAR ENTITY DELETE FLOW
+    // - EntityScriptingInterface::deleteEntity calls _myAvatar->clearAvatarEntity() for deleted avatar entities
+    // - clearAvatarEntity removes the avatar entity and flags the trait instance for the entity as deleted
+    // - ClientTraitsHandler::sendChangedTraitsToMixer sends a deletion to the mixer which relays to other interfaces
+    // - AvatarHashMap::processBulkAvatarTraits on other interfaces calls avatar->processDeletedTraitInstace
+    // - AvatarData::processDeletedTraitInstance calls clearAvatarEntity
+    // - AvatarData::clearAvatarEntity sets _avatarEntityDataChanged = true and adds the ID to the detached list
+    // - Avatar::simulate notices _avatarEntityDataChanged and here we are...
+
     if (!_avatarEntityDataChanged) {
         return;
     }

libraries/avatars/src/AvatarData.cpp

@@ -1898,7 +1898,7 @@ void AvatarData::processTraitInstance(AvatarTraits::TraitType traitType,
 
 void AvatarData::processDeletedTraitInstance(AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID instanceID) {
     if (traitType == AvatarTraits::AvatarEntity) {
-        removeAvatarEntityAndDetach(instanceID);
+        clearAvatarEntity(instanceID);
     }
 }
 
@@ -2687,7 +2687,7 @@ void AvatarData::setAttachmentsVariant(const QVariantList& variant) {
 
 const int MAX_NUM_AVATAR_ENTITIES = 42;
 
-void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& entityData) {
+void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& entityData, bool requiresTreeUpdate) {
     _avatarEntitiesLock.withWriteLock([&] {
         AvatarEntityMap::iterator itr = _avatarEntityData.find(entityID);
         if (itr == _avatarEntityData.end()) {
@@ -2699,6 +2699,10 @@ void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& ent
         }
     });
 
+    if (requiresTreeUpdate) {
+        _avatarEntityDataChanged = true;
+    }
+
     if (_clientTraitsHandler) {
         // we have a client traits handler, so we need to mark this instanced trait as changed
         // so that changes will be sent next frame
@@ -2706,26 +2710,27 @@ void AvatarData::updateAvatarEntity(const QUuid& entityID, const QByteArray& ent
     }
 }
 
-void AvatarData::clearAvatarEntity(const QUuid& entityID) {
+void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree) {
-    _avatarEntitiesLock.withWriteLock([&] {
+
-        _avatarEntityData.remove(entityID);
+    bool removedEntity = false;
+
+    _avatarEntitiesLock.withWriteLock([this, &removedEntity, &entityID] {
+        removedEntity = _avatarEntityData.remove(entityID);
     });
 
-    if (_clientTraitsHandler) {
+    if (removedEntity) {
-        // we have a client traits handler, so we need to mark this removed instance trait as changed
+        if (requiresRemovalFromTree) {
-        // so that changes are sent next frame
+            insertDetachedEntityID(entityID);
-        _clientTraitsHandler->markInstancedTraitDeleted(AvatarTraits::AvatarEntity, entityID);
+        }
+
+        if (_clientTraitsHandler) {
+            // we have a client traits handler, so we need to mark this removed instance trait as changed
+            // so that changes are sent next frame
+            _clientTraitsHandler->markInstancedTraitDeleted(AvatarTraits::AvatarEntity, entityID);
+        }
     }
 }
 
-void AvatarData::removeAvatarEntityAndDetach(const QUuid &entityID) {
-    _avatarEntitiesLock.withWriteLock([this, &entityID]{
-        _avatarEntityData.remove(entityID);
-    });
-
-    insertDetachedEntityID(entityID);
-}
-
 AvatarEntityMap AvatarData::getAvatarEntityData() const {
     AvatarEntityMap result;
     _avatarEntitiesLock.withReadLock([&] {
@@ -2738,6 +2743,8 @@ void AvatarData::insertDetachedEntityID(const QUuid entityID) {
     _avatarEntitiesLock.withWriteLock([&] {
         _avatarEntityDetached.insert(entityID);
     });
+
+    _avatarEntityDataChanged = true;
 }
 
 void AvatarData::setAvatarEntityData(const AvatarEntityMap& avatarEntityData) {

libraries/avatars/src/AvatarData.h

@@ -925,13 +925,13 @@ public:
      * @param {Uuid} entityID
      * @param {string} entityData
      */
-    Q_INVOKABLE void updateAvatarEntity(const QUuid& entityID, const QByteArray& entityData);
+    Q_INVOKABLE void updateAvatarEntity(const QUuid& entityID, const QByteArray& entityData, bool requiresTreeUpdate = true);
 
     /**jsdoc
      * @function MyAvatar.clearAvatarEntity
      * @param {Uuid} entityID
      */
-    Q_INVOKABLE void clearAvatarEntity(const QUuid& entityID);
+    Q_INVOKABLE void clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree = true);
 
 
     /**jsdoc
@@ -1318,8 +1318,6 @@ protected:
     virtual const QString& getSessionDisplayNameForTransport() const { return _sessionDisplayName; }
     virtual void maybeUpdateSessionDisplayNameFromTransport(const QString& sessionDisplayName) { } // No-op in AvatarMixer
 
-    void removeAvatarEntityAndDetach(const QUuid& entityID);
-
     // Body scale
     float _targetScale;
     float _domainMinimumHeight { MIN_AVATAR_HEIGHT };

libraries/entities/src/EntityEditPacketSender.cpp

@@ -74,7 +74,7 @@ void EntityEditPacketSender::queueEditAvatarEntityMessage(PacketType type,
     jsonProperties = QJsonDocument(jsonObject);
 
     QByteArray binaryProperties = jsonProperties.toBinaryData();
-    _myAvatar->updateAvatarEntity(entityItemID, binaryProperties);
+    _myAvatar->updateAvatarEntity(entityItemID, binaryProperties, false);
 
     entity->setLastBroadcast(usecTimestampNow());
 }
@@ -149,11 +149,6 @@ void EntityEditPacketSender::queueEditEntityMessage(PacketType type,
 
 void EntityEditPacketSender::queueEraseEntityMessage(const EntityItemID& entityItemID) {
 
-    // in case this was a clientOnly entity:
-    if(_myAvatar) {
-        _myAvatar->clearAvatarEntity(entityItemID);
-    }
-
     QByteArray bufferOut(NLPacket::maxPayloadSize(PacketType::EntityErase), 0);
 
     if (EntityItemProperties::encodeEraseEntityMessage(entityItemID, bufferOut)) {

libraries/entities/src/EntityEditPacketSender.h

@@ -27,7 +27,6 @@ public:
 
     void setMyAvatar(AvatarData* myAvatar) { _myAvatar = myAvatar; }
     AvatarData* getMyAvatar() { return _myAvatar; }
-    void clearAvatarEntity(QUuid entityID) { assert(_myAvatar); _myAvatar->clearAvatarEntity(entityID); }
 
     /// Queues an array of several voxel edit messages. Will potentially send a pending multi-command packet. Determines
     /// which voxel-server node or nodes the packet should be sent to. Can be called even before voxel servers are known, in

libraries/entities/src/EntityScriptingInterface.cpp

@@ -574,7 +574,7 @@ void EntityScriptingInterface::deleteEntity(QUuid id) {
     _activityTracking.deletedEntityCount++;
 
     EntityItemID entityID(id);
-    bool shouldDelete = true;
+    bool shouldSendDeleteToServer = true;
 
     // If we have a local entity tree set, then also update it.
     if (_entityTree) {
@@ -591,16 +591,21 @@ void EntityScriptingInterface::deleteEntity(QUuid id) {
                     auto avatarHashMap = DependencyManager::get<AvatarHashMap>();
                     AvatarSharedPointer myAvatar = avatarHashMap->getAvatarBySessionID(myNodeID);
                     myAvatar->insertDetachedEntityID(id);
-                    shouldDelete = false;
+                    shouldSendDeleteToServer = false;
                     return;
                 }
 
                 if (entity->getLocked()) {
-                    shouldDelete = false;
+                    shouldSendDeleteToServer = false;
                 } else {
                     // only delete local entities, server entities will round trip through the server filters
                     if (entity->getClientOnly() || _entityTree->isServerlessMode()) {
+                        shouldSendDeleteToServer = false;
                         _entityTree->deleteEntity(entityID);
+
+                        if (entity->getClientOnly() && getEntityPacketSender()->getMyAvatar()) {
+                            getEntityPacketSender()->getMyAvatar()->clearAvatarEntity(entityID, false);
+                        }
                     }
                 }
             }
@@ -608,7 +613,7 @@ void EntityScriptingInterface::deleteEntity(QUuid id) {
     }
 
     // if at this point, we know the id, and we should still delete the entity, send the update to the entity server
-    if (shouldDelete) {
+    if (shouldSendDeleteToServer) {
         getEntityPacketSender()->queueEraseEntityMessage(entityID);
     }
 }