From 2a05ec650b0bd1a878a620115b4c7876552fa7f0 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 14:17:48 -0800 Subject: [PATCH] fix locking in AvatarHashMap/AvatarManager --- interface/src/avatar/AvatarManager.cpp | 62 ++++++++++++++----------- interface/src/avatar/AvatarManager.h | 4 +- libraries/avatars/src/AvatarHashMap.cpp | 59 ++++++++++++++--------- libraries/avatars/src/AvatarHashMap.h | 5 +- 4 files changed, 78 insertions(+), 52 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 4b9bfd21a3..ae4c978877 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -119,6 +119,8 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { PerformanceTimer perfTimer("otherAvatars"); // simulate avatars + QWriteLocker writeLock(&_hashLock); + AvatarHash::iterator avatarIterator = _avatarHash.begin(); while (avatarIterator != _avatarHash.end()) { auto avatar = std::dynamic_pointer_cast(avatarIterator.value()); @@ -128,10 +130,9 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { // DO NOT update or fade out uninitialized Avatars ++avatarIterator; } else if (avatar->shouldDie()) { - removeAvatarMotionState(avatar); - _avatarFades.push_back(avatarIterator.value()); - QWriteLocker locker(&_hashLock); + auto removedAvatar = avatarIterator.value(); avatarIterator = _avatarHash.erase(avatarIterator); + handleRemovedAvatar(removedAvatar); } else { avatar->startUpdate(); avatar->simulate(deltaTime); @@ -139,6 +140,8 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { ++avatarIterator; } } + + writeLock.unlock(); // simulate avatar fades simulateAvatarFades(deltaTime); @@ -173,8 +176,8 @@ AvatarSharedPointer AvatarManager::newSharedAvatar() { } // virtual -AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { - auto avatar = std::dynamic_pointer_cast(AvatarHashMap::addAvatar(sessionUUID, mixerWeakPointer)); +AvatarSharedPointer AvatarManager::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { + auto avatar = std::dynamic_pointer_cast(AvatarHashMap::newOrExistingAvatar(sessionUUID, mixerWeakPointer)); render::ScenePointer scene = qApp->getMain3DScene(); render::PendingChanges pendingChanges; if (DependencyManager::get()->shouldRenderAvatars()) { @@ -200,20 +203,28 @@ void AvatarManager::removeAvatarMotionState(AvatarSharedPointer avatar) { // virtual void AvatarManager::removeAvatar(const QUuid& sessionUUID) { - AvatarHash::iterator avatarIterator = _avatarHash.find(sessionUUID); - if (avatarIterator != _avatarHash.end()) { - std::shared_ptr avatar = std::dynamic_pointer_cast(avatarIterator.value()); - if (avatar != _myAvatar && avatar->isInitialized()) { - removeAvatarMotionState(avatar); - _avatarFades.push_back(avatarIterator.value()); - QWriteLocker locker(&_hashLock); - _avatarHash.erase(avatarIterator); - } + QWriteLocker locker(&_hashLock); + + auto removedAvatar = _avatarHash.take(sessionUUID); + if (removedAvatar) { + handleRemovedAvatar(removedAvatar); } } +void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) { + qDebug() << "Removed avatar with UUID" << uuidStringWithoutCurlyBraces(removedAvatar->getSessionUUID()) + << "from AvatarManager"; + + AvatarHashMap::handleRemovedAvatar(removedAvatar); + + removeAvatarMotionState(removedAvatar); + _avatarFades.push_back(removedAvatar); +} + void AvatarManager::clearOtherAvatars() { // clear any avatars that came from an avatar-mixer + QWriteLocker locker(&_hashLock); + AvatarHash::iterator avatarIterator = _avatarHash.begin(); while (avatarIterator != _avatarHash.end()) { auto avatar = std::static_pointer_cast(avatarIterator.value()); @@ -221,10 +232,10 @@ void AvatarManager::clearOtherAvatars() { // don't remove myAvatar or uninitialized avatars from the list ++avatarIterator; } else { - removeAvatarMotionState(avatar); - _avatarFades.push_back(avatarIterator.value()); - QWriteLocker locker(&_hashLock); + auto removedAvatar = avatarIterator.value(); avatarIterator = _avatarHash.erase(avatarIterator); + + handleRemovedAvatar(removedAvatar); } } _myAvatar->clearLookAtTargetAvatar(); @@ -318,9 +329,11 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents } void AvatarManager::updateAvatarPhysicsShape(const QUuid& id) { - AvatarHash::iterator avatarItr = _avatarHash.find(id); - if (avatarItr != _avatarHash.end()) { - auto avatar = std::static_pointer_cast(avatarItr.value()); + auto avatarData = findAvatar(id); + + if (avatarData) { + auto avatar = std::dynamic_pointer_cast(avatarData); + AvatarMotionState* motionState = avatar->getMotionState(); if (motionState) { motionState->addDirtyFlags(Simulation::DIRTY_SHAPE); @@ -363,11 +376,6 @@ AvatarSharedPointer AvatarManager::getAvatarBySessionID(const QUuid& sessionID) if (sessionID == _myAvatar->getSessionUUID()) { return std::static_pointer_cast(_myAvatar); } - QReadLocker locker(&_hashLock); - auto iter = _avatarHash.find(sessionID); - if (iter != _avatarHash.end()) { - return iter.value(); - } else { - return AvatarSharedPointer(); - } + + return findAvatar(sessionID); } diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index fa0593368b..71f579abc2 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -77,9 +77,11 @@ private: // virtual overrides virtual AvatarSharedPointer newSharedAvatar(); - virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); void removeAvatarMotionState(AvatarSharedPointer avatar); + virtual void removeAvatar(const QUuid& sessionUUID); + virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar); QVector _avatarFades; std::shared_ptr _myAvatar; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 342f995de7..cc83ddf678 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -42,18 +42,30 @@ AvatarSharedPointer AvatarHashMap::newSharedAvatar() { return std::make_shared(); } -AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { - qCDebug(avatars) << "Adding avatar with sessionUUID " << sessionUUID << "to AvatarHashMap."; - - AvatarSharedPointer avatar = newSharedAvatar(); - avatar->setSessionUUID(sessionUUID); - avatar->setOwningAvatarMixer(mixerWeakPointer); +AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { QWriteLocker locker(&_hashLock); - _avatarHash.insert(sessionUUID, avatar); - emit avatarAddedEvent(sessionUUID); + + auto avatar = _avatarHash.value(sessionUUID); + + if (!avatar) { + qCDebug(avatars) << "Adding avatar with sessionUUID " << sessionUUID << "to AvatarHashMap."; + + avatar = newSharedAvatar(); + avatar->setSessionUUID(sessionUUID); + avatar->setOwningAvatarMixer(mixerWeakPointer); + + _avatarHash.insert(sessionUUID, avatar); + emit avatarAddedEvent(sessionUUID); + } + return avatar; } +AvatarSharedPointer AvatarHashMap::findAvatar(const QUuid& sessionUUID) { + QReadLocker locker(&_hashLock); + return _avatarHash.value(sessionUUID); +} + void AvatarHashMap::processAvatarDataPacket(QSharedPointer packet, SharedNodePointer sendingNode) { // enumerate over all of the avatars in this packet @@ -66,10 +78,7 @@ void AvatarHashMap::processAvatarDataPacket(QSharedPointer packet, Sha QByteArray byteArray = packet->readWithoutCopy(packet->bytesLeftToRead()); if (sessionUUID != _lastOwnerSessionUUID) { - AvatarSharedPointer avatar = _avatarHash.value(sessionUUID); - if (!avatar) { - avatar = addAvatar(sessionUUID, sendingNode); - } + auto avatar = newOrExistingAvatar(sessionUUID, sendingNode); // have the matching (or new) avatar parse the data from the packet int bytesRead = avatar->parseDataFromBuffer(byteArray); @@ -97,10 +106,8 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer packet, identityStream >> sessionUUID >> faceMeshURL >> skeletonURL >> attachmentData >> displayName; // mesh URL for a UUID, find avatar in our list - AvatarSharedPointer avatar = _avatarHash.value(sessionUUID); - if (!avatar) { - avatar = addAvatar(sessionUUID, sendingNode); - } + auto avatar = newOrExistingAvatar(sessionUUID, sendingNode); + if (avatar->getFaceModelURL() != faceMeshURL) { avatar->setFaceModelURL(faceMeshURL); } @@ -122,10 +129,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer packet, void AvatarHashMap::processAvatarBillboardPacket(QSharedPointer packet, SharedNodePointer sendingNode) { QUuid sessionUUID = QUuid::fromRfc4122(packet->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); - AvatarSharedPointer avatar = _avatarHash.value(sessionUUID); - if (!avatar) { - avatar = addAvatar(sessionUUID, sendingNode); - } + auto avatar = newOrExistingAvatar(sessionUUID, sendingNode); QByteArray billboard = packet->read(packet->bytesLeftToRead()); if (avatar->getBillboard() != billboard) { @@ -137,13 +141,22 @@ void AvatarHashMap::processKillAvatar(QSharedPointer packet, SharedNod // read the node id QUuid sessionUUID = QUuid::fromRfc4122(packet->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); removeAvatar(sessionUUID); - } void AvatarHashMap::removeAvatar(const QUuid& sessionUUID) { QWriteLocker locker(&_hashLock); - _avatarHash.remove(sessionUUID); - emit avatarRemovedEvent(sessionUUID); + + auto removedAvatar = _avatarHash.take(sessionUUID); + + if (removedAvatar) { + handleRemovedAvatar(removedAvatar); + } +} + +void AvatarHashMap::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) { + qDebug() << "Removed avatar with UUID" << uuidStringWithoutCurlyBraces(removedAvatar->getSessionUUID()) + << "from AvatarHashMap"; + emit avatarRemovedEvent(removedAvatar->getSessionUUID()); } void AvatarHashMap::sessionUUIDChanged(const QUuid& sessionUUID, const QUuid& oldUUID) { diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index a125784cd8..0f230a7022 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -54,8 +54,11 @@ protected: AvatarHashMap(); virtual AvatarSharedPointer newSharedAvatar(); - virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + virtual AvatarSharedPointer findAvatar(const QUuid& sessionUUID); // uses a QReadLocker on the hashLock virtual void removeAvatar(const QUuid& sessionUUID); + + virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar); AvatarHash _avatarHash; // "Case-based safety": Most access to the _avatarHash is on the same thread. Write access is protected by a write-lock.