From e33f349d5390ec7f4535d1fca9b050a6fd99bdfa Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Fri, 10 Aug 2018 16:06:09 -0700 Subject: [PATCH] fix flagging for avatar entity update/delete --- interface/src/Application.cpp | 18 +++----- interface/src/avatar/MyAvatar.cpp | 1 - .../src/avatars-renderer/Avatar.cpp | 20 ++++++--- libraries/avatars/src/AvatarData.cpp | 41 +++++++++++-------- libraries/avatars/src/AvatarData.h | 6 +-- .../entities/src/EntityEditPacketSender.cpp | 7 +--- .../entities/src/EntityEditPacketSender.h | 1 - .../entities/src/EntityScriptingInterface.cpp | 13 ++++-- 8 files changed, 56 insertions(+), 51 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index e556fd734f..60af79bfda 100644 --- a/interface/src/Application.cpp +++ b/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(); - 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() || - (_octreeProcessor.isLoadSequenceComplete() )) { + if (isServerlessMode() || + (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(); } diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 2c88f917a1..deef69d980 100755 --- a/interface/src/avatar/MyAvatar.cpp +++ b/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 firstRunVal { Settings::firstRun, true }; diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index 0b43fd5433..150ecb6d44 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/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 - // - MyAvatar::update notices _avatarEntityDataLocallyEdited and calls sendIdentityPacket - // - sendIdentityPacket sends the entity bytes to the server which relays them to other interfaces - // - AvatarHashMap::processAvatarIdentityPacket on other interfaces call avatar->setAvatarEntityData() - // - setAvatarEntityData saves the bytes and sets _avatarEntityDataChanged = true + // - updateAvatarEntity saves the bytes and flags the trait instance for the entity as updated + // - ClientTraitsHandler::sendChangedTraitsToMixer sends the entity bytes to the mixer which relays them to other interfaces + // - AvatarHashMap::processBulkAvatarTraits on other interfaces calls avatar->processTraitInstace + // - AvatarData::processTraitInstance calls updateAvatarEntity, which 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; } diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 4e396e2a9e..f1b9986186 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/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) { - _avatarEntitiesLock.withWriteLock([&] { - _avatarEntityData.remove(entityID); +void AvatarData::clearAvatarEntity(const QUuid& entityID, bool requiresRemovalFromTree) { + + bool removedEntity = false; + + _avatarEntitiesLock.withWriteLock([this, &removedEntity, &entityID] { + removedEntity = _avatarEntityData.remove(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); + if (removedEntity) { + if (requiresRemovalFromTree) { + insertDetachedEntityID(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) { diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 97ae90f694..6f1871974d 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/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 }; diff --git a/libraries/entities/src/EntityEditPacketSender.cpp b/libraries/entities/src/EntityEditPacketSender.cpp index 0982775b09..d288126348 100644 --- a/libraries/entities/src/EntityEditPacketSender.cpp +++ b/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)) { diff --git a/libraries/entities/src/EntityEditPacketSender.h b/libraries/entities/src/EntityEditPacketSender.h index 31f91707b8..9bf9095f7f 100644 --- a/libraries/entities/src/EntityEditPacketSender.h +++ b/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 diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index c080fbbb88..d9924cb9fd 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/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(); 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); } }