From 2a05ec650b0bd1a878a620115b4c7876552fa7f0 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 14:17:48 -0800 Subject: [PATCH 1/8] 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. From 5140a480a15556bbd7b52ea77810fef74dd6d396 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 15:35:41 -0800 Subject: [PATCH 2/8] fix for recursive write then read lock --- interface/src/avatar/Avatar.cpp | 2 +- interface/src/avatar/AvatarManager.cpp | 32 +++++++++++--------------- interface/src/avatar/AvatarManager.h | 2 +- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/interface/src/avatar/Avatar.cpp b/interface/src/avatar/Avatar.cpp index b979334383..bfa32b8223 100644 --- a/interface/src/avatar/Avatar.cpp +++ b/interface/src/avatar/Avatar.cpp @@ -1187,7 +1187,7 @@ void Avatar::computeShapeInfo(ShapeInfo& shapeInfo) { // virtual void Avatar::rebuildSkeletonBody() { - DependencyManager::get()->updateAvatarPhysicsShape(getSessionUUID()); + DependencyManager::get()->updateAvatarPhysicsShape(this); } glm::vec3 Avatar::getLeftPalmPosition() { diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index ae4c978877..af5937d3eb 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -328,25 +328,19 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents } } -void AvatarManager::updateAvatarPhysicsShape(const QUuid& id) { - auto avatarData = findAvatar(id); - - if (avatarData) { - auto avatar = std::dynamic_pointer_cast(avatarData); - - AvatarMotionState* motionState = avatar->getMotionState(); - if (motionState) { - motionState->addDirtyFlags(Simulation::DIRTY_SHAPE); - } else { - ShapeInfo shapeInfo; - avatar->computeShapeInfo(shapeInfo); - btCollisionShape* shape = ObjectMotionState::getShapeManager()->getShape(shapeInfo); - if (shape) { - AvatarMotionState* motionState = new AvatarMotionState(avatar.get(), shape); - avatar->setMotionState(motionState); - _motionStatesToAdd.insert(motionState); - _avatarMotionStates.insert(motionState); - } +void AvatarManager::updateAvatarPhysicsShape(Avatar* avatar) { + AvatarMotionState* motionState = avatar->getMotionState(); + if (motionState) { + motionState->addDirtyFlags(Simulation::DIRTY_SHAPE); + } else { + ShapeInfo shapeInfo; + avatar->computeShapeInfo(shapeInfo); + btCollisionShape* shape = ObjectMotionState::getShapeManager()->getShape(shapeInfo); + if (shape) { + AvatarMotionState* motionState = new AvatarMotionState(avatar, shape); + avatar->setMotionState(motionState); + _motionStatesToAdd.insert(motionState); + _avatarMotionStates.insert(motionState); } } } diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 71f579abc2..35e37656d8 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -63,7 +63,7 @@ public: void handleOutgoingChanges(const VectorOfMotionStates& motionStates); void handleCollisionEvents(const CollisionEvents& collisionEvents); - void updateAvatarPhysicsShape(const QUuid& id); + void updateAvatarPhysicsShape(Avatar* avatar); public slots: void setShouldShowReceiveStats(bool shouldShowReceiveStats) { _shouldShowReceiveStats = shouldShowReceiveStats; } From 6398a922c656316c001c3990cb71f1f450cb9776 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 15:43:29 -0800 Subject: [PATCH 3/8] leverage COW for AvatarHash --- interface/src/avatar/AvatarManager.cpp | 14 ++++++-------- interface/src/avatar/MyAvatar.cpp | 6 ++---- libraries/avatars/src/AvatarHashMap.cpp | 4 ---- libraries/avatars/src/AvatarHashMap.h | 2 +- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index af5937d3eb..4a69e9010a 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -119,10 +119,10 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { PerformanceTimer perfTimer("otherAvatars"); // simulate avatars - QWriteLocker writeLock(&_hashLock); + auto hashCopy = _avatarHash; - AvatarHash::iterator avatarIterator = _avatarHash.begin(); - while (avatarIterator != _avatarHash.end()) { + AvatarHash::iterator avatarIterator = hashCopy.begin(); + while (avatarIterator != hashCopy.end()) { auto avatar = std::dynamic_pointer_cast(avatarIterator.value()); if (avatar == _myAvatar || !avatar->isInitialized()) { @@ -130,9 +130,8 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { // DO NOT update or fade out uninitialized Avatars ++avatarIterator; } else if (avatar->shouldDie()) { - auto removedAvatar = avatarIterator.value(); - avatarIterator = _avatarHash.erase(avatarIterator); - handleRemovedAvatar(removedAvatar); + removeAvatar(avatarIterator.key()); + ++avatarIterator; } else { avatar->startUpdate(); avatar->simulate(deltaTime); @@ -140,8 +139,6 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { ++avatarIterator; } } - - writeLock.unlock(); // simulate avatar fades simulateAvatarFades(deltaTime); @@ -329,6 +326,7 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents } void AvatarManager::updateAvatarPhysicsShape(Avatar* avatar) { + qDebug() << "Update physics state called for" << avatar; AvatarMotionState* motionState = avatar->getMotionState(); if (motionState) { motionState->addDirtyFlags(Simulation::DIRTY_SHAPE); diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 1c151bcd3f..6521a0c891 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -982,10 +982,8 @@ void MyAvatar::updateLookAtTargetAvatar() { const float KEEP_LOOKING_AT_CURRENT_ANGLE_FACTOR = 1.3f; const float GREATEST_LOOKING_AT_DISTANCE = 10.0f; - AvatarHash hash; - DependencyManager::get()->withAvatarHash([&] (const AvatarHash& locked) { - hash = locked; // make a shallow copy and operate on that, to minimize lock time - }); + AvatarHash hash = DependencyManager::get()->getCopy(); + foreach (const AvatarSharedPointer& avatarPointer, hash) { auto avatar = static_pointer_cast(avatarPointer); bool isCurrentTarget = avatar->getIsLookAtTarget(); diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index cc83ddf678..1da9abacdd 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -22,10 +22,6 @@ AvatarHashMap::AvatarHashMap() { connect(DependencyManager::get().data(), &NodeList::uuidChanged, this, &AvatarHashMap::sessionUUIDChanged); } -void AvatarHashMap::withAvatarHash(std::function callback) { - QReadLocker locker(&_hashLock); - callback(_avatarHash); -} bool AvatarHashMap::isAvatarInRange(const glm::vec3& position, const float range) { QReadLocker locker(&_hashLock); foreach(const AvatarSharedPointer& sharedAvatar, _avatarHash) { diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index 0f230a7022..c3c9160f12 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -31,7 +31,7 @@ class AvatarHashMap : public QObject, public Dependency { SINGLETON_DEPENDENCY public: - void withAvatarHash(std::function); + AvatarHash getCopy() { return _avatarHash; } int size() { return _avatarHash.size(); } signals: From 40397add433f25c0dfa4baf39b711adf2dd673bc Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 16:15:21 -0800 Subject: [PATCH 4/8] fix for extra adds in AvatarManager --- interface/src/avatar/AvatarManager.cpp | 24 +++++++++++------------- interface/src/avatar/AvatarManager.h | 2 +- libraries/avatars/src/AvatarHashMap.cpp | 22 ++++++++++++++-------- libraries/avatars/src/AvatarHashMap.h | 3 ++- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 4a69e9010a..a64263aec2 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -123,7 +123,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { AvatarHash::iterator avatarIterator = hashCopy.begin(); while (avatarIterator != hashCopy.end()) { - auto avatar = std::dynamic_pointer_cast(avatarIterator.value()); + auto avatar = std::static_pointer_cast(avatarIterator.value()); if (avatar == _myAvatar || !avatar->isInitialized()) { // DO NOT update _myAvatar! Its update has already been done earlier in the main loop. @@ -169,19 +169,21 @@ void AvatarManager::simulateAvatarFades(float deltaTime) { } AvatarSharedPointer AvatarManager::newSharedAvatar() { - return AvatarSharedPointer(std::make_shared(std::make_shared())); + return std::make_shared(std::make_shared()); } -// virtual -AvatarSharedPointer AvatarManager::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { - auto avatar = std::dynamic_pointer_cast(AvatarHashMap::newOrExistingAvatar(sessionUUID, mixerWeakPointer)); +AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { + auto newAvatar = AvatarHashMap::addAvatar(sessionUUID, mixerWeakPointer); + auto rawRenderableAvatar = std::static_pointer_cast(newAvatar); + render::ScenePointer scene = qApp->getMain3DScene(); render::PendingChanges pendingChanges; if (DependencyManager::get()->shouldRenderAvatars()) { - avatar->addToScene(avatar, scene, pendingChanges); + rawRenderableAvatar->addToScene(rawRenderableAvatar, scene, pendingChanges); } scene->enqueuePendingChanges(pendingChanges); - return avatar; + + return newAvatar; } // protected @@ -209,9 +211,6 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID) { } void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) { - qDebug() << "Removed avatar with UUID" << uuidStringWithoutCurlyBraces(removedAvatar->getSessionUUID()) - << "from AvatarManager"; - AvatarHashMap::handleRemovedAvatar(removedAvatar); removeAvatarMotionState(removedAvatar); @@ -326,7 +325,6 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents } void AvatarManager::updateAvatarPhysicsShape(Avatar* avatar) { - qDebug() << "Update physics state called for" << avatar; AvatarMotionState* motionState = avatar->getMotionState(); if (motionState) { motionState->addDirtyFlags(Simulation::DIRTY_SHAPE); @@ -346,7 +344,7 @@ void AvatarManager::updateAvatarPhysicsShape(Avatar* avatar) { void AvatarManager::updateAvatarRenderStatus(bool shouldRenderAvatars) { if (DependencyManager::get()->shouldRenderAvatars()) { for (auto avatarData : _avatarHash) { - auto avatar = std::dynamic_pointer_cast(avatarData); + auto avatar = std::static_pointer_cast(avatarData); render::ScenePointer scene = qApp->getMain3DScene(); render::PendingChanges pendingChanges; avatar->addToScene(avatar, scene, pendingChanges); @@ -354,7 +352,7 @@ void AvatarManager::updateAvatarRenderStatus(bool shouldRenderAvatars) { } } else { for (auto avatarData : _avatarHash) { - auto avatar = std::dynamic_pointer_cast(avatarData); + auto avatar = std::static_pointer_cast(avatarData); render::ScenePointer scene = qApp->getMain3DScene(); render::PendingChanges pendingChanges; avatar->removeFromScene(avatar, scene, pendingChanges); diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 35e37656d8..f911dacc4d 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -77,7 +77,7 @@ private: // virtual overrides virtual AvatarSharedPointer newSharedAvatar(); - virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); void removeAvatarMotionState(AvatarSharedPointer avatar); virtual void removeAvatar(const QUuid& sessionUUID); diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 1da9abacdd..8f2ea82373 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -38,20 +38,26 @@ 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."; + + auto avatar = newSharedAvatar(); + avatar->setSessionUUID(sessionUUID); + avatar->setOwningAvatarMixer(mixerWeakPointer); + + _avatarHash.insert(sessionUUID, avatar); + emit avatarAddedEvent(sessionUUID); + + return avatar; +} + AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer) { QWriteLocker locker(&_hashLock); 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); + avatar = addAvatar(sessionUUID, mixerWeakPointer); } return avatar; diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index c3c9160f12..c2fecf5bd9 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -54,7 +54,8 @@ protected: AvatarHashMap(); virtual AvatarSharedPointer newSharedAvatar(); - virtual AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer& mixerWeakPointer); + 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); From 430cb7876dd99cfb7832620b2da65e40f0bf004f Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 16:22:56 -0800 Subject: [PATCH 5/8] remove a couple of read lockers for AvatarHashMap --- interface/src/avatar/AvatarManager.cpp | 1 - libraries/avatars/src/AvatarHashMap.cpp | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index a64263aec2..bd45561b38 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -256,7 +256,6 @@ QVector AvatarManager::getLocalLights() const { } QVector AvatarManager::getAvatarIdentifiers() { - QReadLocker locker(&_hashLock); return _avatarHash.keys().toVector(); } AvatarData* AvatarManager::getAvatar(QUuid avatarID) { diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 8f2ea82373..035d29f344 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -23,8 +23,8 @@ AvatarHashMap::AvatarHashMap() { } bool AvatarHashMap::isAvatarInRange(const glm::vec3& position, const float range) { - QReadLocker locker(&_hashLock); - foreach(const AvatarSharedPointer& sharedAvatar, _avatarHash) { + auto hashCopy = _avatarHash; + foreach(const AvatarSharedPointer& sharedAvatar, hashCopy) { glm::vec3 avatarPosition = sharedAvatar->getPosition(); float distance = glm::distance(avatarPosition, position); if (distance < range) { From 22f5d4df6dece4e1c9f3c42d1cdced28e4b2dfa9 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 19 Nov 2015 16:25:33 -0800 Subject: [PATCH 6/8] change signature of copy return --- interface/src/avatar/MyAvatar.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 6521a0c891..a2102bd010 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -982,7 +982,7 @@ void MyAvatar::updateLookAtTargetAvatar() { const float KEEP_LOOKING_AT_CURRENT_ANGLE_FACTOR = 1.3f; const float GREATEST_LOOKING_AT_DISTANCE = 10.0f; - AvatarHash hash = DependencyManager::get()->getCopy(); + AvatarHash hash = DependencyManager::get()->getHashCopy(); foreach (const AvatarSharedPointer& avatarPointer, hash) { auto avatar = static_pointer_cast(avatarPointer); diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index c2fecf5bd9..7795072ec2 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -31,7 +31,7 @@ class AvatarHashMap : public QObject, public Dependency { SINGLETON_DEPENDENCY public: - AvatarHash getCopy() { return _avatarHash; } + AvatarHash getHashCopy() { return _avatarHash; } int size() { return _avatarHash.size(); } signals: From 19d3d80ff97ec6e2aafff260fcc14b47482beaff Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Fri, 20 Nov 2015 11:03:52 -0800 Subject: [PATCH 7/8] fixes for some incorrect assumptions --- interface/src/avatar/AvatarManager.cpp | 10 +++++++++- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/avatars/src/AvatarHashMap.h | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index bd45561b38..c17590c4ac 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -110,16 +110,22 @@ void AvatarManager::updateMyAvatar(float deltaTime) { } void AvatarManager::updateOtherAvatars(float deltaTime) { + // lock the hash for read to check the size + QReadLocker lock(&_hashLock); + if (_avatarHash.size() < 2 && _avatarFades.isEmpty()) { return; } + + lock.unlock(); + bool showWarnings = Menu::getInstance()->isOptionChecked(MenuOption::PipelineWarnings); PerformanceWarning warn(showWarnings, "Application::updateAvatars()"); PerformanceTimer perfTimer("otherAvatars"); // simulate avatars - auto hashCopy = _avatarHash; + auto hashCopy = getHashCopy(); AvatarHash::iterator avatarIterator = hashCopy.begin(); while (avatarIterator != hashCopy.end()) { @@ -256,8 +262,10 @@ QVector AvatarManager::getLocalLights() const { } QVector AvatarManager::getAvatarIdentifiers() { + QReadLocker lock(&_hashLock); return _avatarHash.keys().toVector(); } + AvatarData* AvatarManager::getAvatar(QUuid avatarID) { QReadLocker locker(&_hashLock); return _avatarHash[avatarID].get(); // Non-obvious: A bogus avatarID answers your own avatar. diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 035d29f344..c195ab4c32 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -23,7 +23,7 @@ AvatarHashMap::AvatarHashMap() { } bool AvatarHashMap::isAvatarInRange(const glm::vec3& position, const float range) { - auto hashCopy = _avatarHash; + auto hashCopy = getHashCopy(); foreach(const AvatarSharedPointer& sharedAvatar, hashCopy) { glm::vec3 avatarPosition = sharedAvatar->getPosition(); float distance = glm::distance(avatarPosition, position); diff --git a/libraries/avatars/src/AvatarHashMap.h b/libraries/avatars/src/AvatarHashMap.h index 7795072ec2..5881b779a1 100644 --- a/libraries/avatars/src/AvatarHashMap.h +++ b/libraries/avatars/src/AvatarHashMap.h @@ -31,7 +31,7 @@ class AvatarHashMap : public QObject, public Dependency { SINGLETON_DEPENDENCY public: - AvatarHash getHashCopy() { return _avatarHash; } + AvatarHash getHashCopy() { QReadLocker lock(&_hashLock); return _avatarHash; } int size() { return _avatarHash.size(); } signals: From 9b5bfd45bc4a822ff365d646b4d65c449655f649 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Fri, 20 Nov 2015 11:04:46 -0800 Subject: [PATCH 8/8] change lock back to locker to remove change --- interface/src/avatar/AvatarManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index c17590c4ac..769b1d56a2 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -262,7 +262,7 @@ QVector AvatarManager::getLocalLights() const { } QVector AvatarManager::getAvatarIdentifiers() { - QReadLocker lock(&_hashLock); + QReadLocker locker(&_hashLock); return _avatarHash.keys().toVector(); }