Merge pull request #6154 from howard-stearns/safe-avatar-list-access

Safe avatar list access

interface/src/avatar/AvatarManager.cpp

@@ -77,7 +77,10 @@ AvatarManager::AvatarManager(QObject* parent) :
 
 void AvatarManager::init() {
     _myAvatar->init();
-    _avatarHash.insert(MY_AVATAR_KEY, _myAvatar);
+    {
+        QWriteLocker locker(&_hashLock);
+        _avatarHash.insert(MY_AVATAR_KEY, _myAvatar);
+    }
 
     connect(DependencyManager::get<SceneScriptingInterface>().data(), &SceneScriptingInterface::shouldRenderAvatarsChanged, this, &AvatarManager::updateAvatarRenderStatus, Qt::QueuedConnection);
 
@@ -127,6 +130,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) {
         } else if (avatar->shouldDie()) {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
         } else {
             avatar->startUpdate();
@@ -202,6 +206,7 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
         if (avatar != _myAvatar && avatar->isInitialized()) {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             _avatarHash.erase(avatarIterator);
         }
     }
@@ -218,6 +223,7 @@ void AvatarManager::clearOtherAvatars() {
         } else {
             removeAvatarMotionState(avatar);
             _avatarFades.push_back(avatarIterator.value());
+            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
         }
     }
@@ -349,5 +355,6 @@ AvatarSharedPointer AvatarManager::getAvatarBySessionID(const QUuid& sessionID)
     if (sessionID == _myAvatar->getSessionUUID()) {
         return std::static_pointer_cast<Avatar>(_myAvatar);
     }
-    return getAvatarHash()[sessionID];
+    QReadLocker locker(&_hashLock);
+    return _avatarHash[sessionID];
 }

interface/src/avatar/MyAvatar.cpp

@@ -1020,7 +1020,11 @@ void MyAvatar::updateLookAtTargetAvatar() {
     const float KEEP_LOOKING_AT_CURRENT_ANGLE_FACTOR = 1.3f;
     const float GREATEST_LOOKING_AT_DISTANCE = 10.0f;
 
-    foreach (const AvatarSharedPointer& avatarPointer, DependencyManager::get<AvatarManager>()->getAvatarHash()) {
+    AvatarHash hash;
+    DependencyManager::get<AvatarManager>()->withAvatarHash([&] (const AvatarHash& locked) {
+        hash = locked; // make a shallow copy and operate on that, to minimize lock time
+    });
+    foreach (const AvatarSharedPointer& avatarPointer, hash) {
         auto avatar = static_pointer_cast<Avatar>(avatarPointer);
         bool isCurrentTarget = avatar->getIsLookAtTarget();
         float distanceTo = glm::length(avatar->getHead()->getEyePosition() - cameraPosition);
@@ -1052,7 +1056,6 @@ void MyAvatar::updateLookAtTargetAvatar() {
                 glm::vec3 humanLeftEye = humanSystem->getPosition() + (humanSystem->getOrientation() * leftEyeHeadLocal);
                 glm::vec3 humanRightEye = humanSystem->getPosition() + (humanSystem->getOrientation() * rightEyeHeadLocal);
 
-
                 // First find out where (in world space) the person is looking relative to that bridge-of-the-avatar point.
                 // (We will be adding that offset to the camera position, after making some other adjustments.)
                 glm::vec3 gazeOffset = lookAtPosition - getHead()->getEyePosition();
@@ -1064,14 +1067,14 @@ void MyAvatar::updateLookAtTargetAvatar() {
 
                 // If the camera is also not oriented with the head, adjust by getting the offset in head-space...
                 /* Not needed (i.e., code is a no-op), but I'm leaving the example code here in case something like this is needed someday.
-                glm::quat avatarHeadOrientation = getHead()->getOrientation();
+                 glm::quat avatarHeadOrientation = getHead()->getOrientation();
-                glm::vec3 gazeOffsetLocalToHead = glm::inverse(avatarHeadOrientation) * gazeOffset;
+                 glm::vec3 gazeOffsetLocalToHead = glm::inverse(avatarHeadOrientation) * gazeOffset;
-                // ... and treat that as though it were in camera space, bringing it back to world space.
+                 // ... and treat that as though it were in camera space, bringing it back to world space.
-                // But camera is fudged to make the picture feel like the avatar's orientation.
+                 // But camera is fudged to make the picture feel like the avatar's orientation.
-                glm::quat humanOrientation = humanSystem->getOrientation(); // or just avatar getOrienation() ?
+                 glm::quat humanOrientation = humanSystem->getOrientation(); // or just avatar getOrienation() ?
-                gazeOffset = humanOrientation * gazeOffsetLocalToHead;
+                 gazeOffset = humanOrientation * gazeOffsetLocalToHead;
-                glm::vec3 corrected = humanSystem->getPosition() + gazeOffset;
+                 glm::vec3 corrected = humanSystem->getPosition() + gazeOffset;
-               */
+                 */
 
                 // And now we can finally add that offset to the camera.
                 glm::vec3 corrected = qApp->getViewFrustum()->getPosition() + gazeOffset;

libraries/avatars/src/AvatarHashMap.cpp

@@ -22,7 +22,12 @@ AvatarHashMap::AvatarHashMap() {
     connect(DependencyManager::get<NodeList>().data(), &NodeList::uuidChanged, this, &AvatarHashMap::sessionUUIDChanged);
 }
 
+void AvatarHashMap::withAvatarHash(std::function<void(const AvatarHash& hash)> callback) {
+    QReadLocker locker(&_hashLock);
+    callback(_avatarHash);
+}
 bool AvatarHashMap::isAvatarInRange(const glm::vec3& position, const float range) {
+    QReadLocker locker(&_hashLock);
     foreach(const AvatarSharedPointer& sharedAvatar, _avatarHash) {
         glm::vec3 avatarPosition = sharedAvatar->getPosition();
         float distance = glm::distance(avatarPosition, position);
@@ -43,6 +48,7 @@ AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWe
     AvatarSharedPointer avatar = newSharedAvatar();
     avatar->setSessionUUID(sessionUUID);
     avatar->setOwningAvatarMixer(mixerWeakPointer);
+    QWriteLocker locker(&_hashLock);
     _avatarHash.insert(sessionUUID, avatar);
 
     return avatar;
@@ -134,6 +140,7 @@ void AvatarHashMap::processKillAvatar(QSharedPointer<NLPacket> packet, SharedNod
 }
 
 void AvatarHashMap::removeAvatar(const QUuid& sessionUUID) {
+    QWriteLocker locker(&_hashLock);
     _avatarHash.remove(sessionUUID);
 }
 

libraries/avatars/src/AvatarHashMap.h

@@ -16,6 +16,7 @@
 #include <QtCore/QSharedPointer>
 #include <QtCore/QUuid>
 
+#include <functional>
 #include <memory>
 
 #include <DependencyManager.h>
@@ -30,7 +31,7 @@ class AvatarHashMap : public QObject, public Dependency {
     SINGLETON_DEPENDENCY
 
 public:
-    const AvatarHash& getAvatarHash() { return _avatarHash; }
+    void withAvatarHash(std::function<void(const AvatarHash& hash)>);
     int size() { return _avatarHash.size(); }
 
 public slots:
@@ -52,6 +53,9 @@ protected:
     virtual void removeAvatar(const QUuid& sessionUUID);
 
     AvatarHash _avatarHash;
+    // "Case-based safety": Most access to the _avatarHash is on the same thread. Write access is protected by a write-lock.
+    // If you read from a different thread, you must read-lock the _hashLock. (Scripted write access is not supported).
+    QReadWriteLock _hashLock;
 
 private:
     QUuid _lastOwnerSessionUUID;