diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 1faf17ea9a..0fdd246d7b 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -456,31 +456,37 @@ void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar } void AvatarManager::clearOtherAvatars() { - // Remove other avatars from the world but don't actually remove them from _avatarHash - // each will either be removed on timeout or will re-added to the world on receipt of update. - const render::ScenePointer& scene = qApp->getMain3DScene(); - render::Transaction transaction; - - QReadLocker locker(&_hashLock); - AvatarHash::iterator avatarIterator = _avatarHash.begin(); - while (avatarIterator != _avatarHash.end()) { - auto avatar = std::static_pointer_cast(avatarIterator.value()); - if (avatar != _myAvatar) { - handleRemovedAvatar(avatar); - avatarIterator = _avatarHash.erase(avatarIterator); - } else { - ++avatarIterator; - } - } - assert(scene); - scene->enqueueTransaction(transaction); _myAvatar->clearLookAtTargetAvatar(); + + // setup a vector of removed avatars outside the scope of the hash lock + std::vector removedAvatars; + + { + QWriteLocker locker(&_hashLock); + + removedAvatars.reserve(_avatarHash.size()); + + auto avatarIterator = _avatarHash.begin(); + while (avatarIterator != _avatarHash.end()) { + auto avatar = std::static_pointer_cast(avatarIterator.value()); + if (avatar != _myAvatar) { + removedAvatars.push_back(avatar); + avatarIterator = _avatarHash.erase(avatarIterator); + } else { + ++avatarIterator; + } + } + } + + for (auto& av : removedAvatars) { + handleRemovedAvatar(av); + } } void AvatarManager::deleteAllAvatars() { assert(_avatarsToChangeInPhysics.empty()); - QReadLocker locker(&_hashLock); + QWriteLocker locker(&_hashLock); AvatarHash::iterator avatarIterator = _avatarHash.begin(); while (avatarIterator != _avatarHash.end()) { auto avatar = std::static_pointer_cast(avatarIterator.value()); diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 306ba6f39b..9c4287728d 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -204,7 +204,12 @@ private: void simulateAvatarFades(float deltaTime); AvatarSharedPointer newSharedAvatar() override; - void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason) override; + + // called only from the AvatarHashMap thread - cannot be called while this thread holds the + // hash lock, since handleRemovedAvatar needs a write lock on the entity tree and the entity tree + // frequently grabs a read lock on the hash to get a given avatar by ID + void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, + KillAvatarReason removalReason = KillAvatarReason::NoReason) override; QVector _avatarsToFade; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index d205a915f8..01557e307e 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -66,6 +66,22 @@ void AvatarReplicas::removeReplicas(const QUuid& parentID) { } } +std::vector AvatarReplicas::takeReplicas(const QUuid& parentID) { + std::vector replicas; + + auto it = _replicasMap.find(parentID); + + if (it != _replicasMap.end()) { + // take a copy of the replica shared pointers for this parent + replicas.swap(it->second); + + // erase the replicas for this parent from our map + _replicasMap.erase(it); + } + + return replicas; +} + void AvatarReplicas::processAvatarIdentity(const QUuid& parentID, const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged) { if (_replicasMap.find(parentID) != _replicasMap.end()) { auto &replicas = _replicasMap[parentID]; @@ -386,24 +402,31 @@ void AvatarHashMap::processKillAvatar(QSharedPointer message, S } void AvatarHashMap::removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason) { - QWriteLocker locker(&_hashLock); + std::vector removedAvatars; - auto replicaIDs = _replicas.getReplicaIDs(sessionUUID); - _replicas.removeReplicas(sessionUUID); - for (auto id : replicaIDs) { - auto removedReplica = _avatarHash.take(id); - if (removedReplica) { - handleRemovedAvatar(removedReplica, removalReason); + { + QWriteLocker locker(&_hashLock); + + auto replicas = _replicas.takeReplicas(sessionUUID); + + for (auto& replica : replicas) { + auto removedReplica = _avatarHash.take(replica->getID()); + if (removedReplica) { + removedAvatars.push_back(removedReplica); + } + } + + _pendingAvatars.remove(sessionUUID); + auto removedAvatar = _avatarHash.take(sessionUUID); + + if (removedAvatar) { + removedAvatars.push_back(removedAvatar); } } - _pendingAvatars.remove(sessionUUID); - auto removedAvatar = _avatarHash.take(sessionUUID); - - if (removedAvatar) { + for (auto& removedAvatar: removedAvatars) { handleRemovedAvatar(removedAvatar, removalReason); } - } void AvatarHashMap::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { @@ -421,11 +444,18 @@ void AvatarHashMap::sessionUUIDChanged(const QUuid& sessionUUID, const QUuid& ol } void AvatarHashMap::clearOtherAvatars() { - QWriteLocker locker(&_hashLock); + QList removedAvatars; - for (auto& av : _avatarHash) { - handleRemovedAvatar(av); + { + QWriteLocker locker(&_hashLock); + + // grab a copy of the current avatars so we can call handleRemoveAvatar for them + removedAvatars = _avatarHash.values(); + + _avatarHash.clear(); } - _avatarHash.clear(); + for (auto& av : removedAvatars) { + handleRemovedAvatar(av); + } } diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index 70d7f8c04d..c2cb448e52 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -49,6 +49,7 @@ public: void parseDataFromBuffer(const QUuid& parentID, const QByteArray& buffer); void processAvatarIdentity(const QUuid& parentID, const QByteArray& identityData, bool& identityChanged, bool& displayNameChanged); void removeReplicas(const QUuid& parentID); + std::vector takeReplicas(const QUuid& parentID); void processTrait(const QUuid& parentID, AvatarTraits::TraitType traitType, QByteArray traitBinaryData); void processDeletedTraitInstance(const QUuid& parentID, AvatarTraits::TraitType traitType, AvatarTraits::TraitInstanceID instanceID); void processTraitInstance(const QUuid& parentID, AvatarTraits::TraitType traitType, @@ -179,7 +180,7 @@ protected: bool& isNew); virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID) const; // uses a QReadLocker on the hashLock virtual void removeAvatar(const QUuid& sessionUUID, KillAvatarReason removalReason = KillAvatarReason::NoReason); - + virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason = KillAvatarReason::NoReason); AvatarHash _avatarHash;