Merge pull request #6436 from birarda/ghost-avatar

fix for ghosted avatars thanks to check/add race

interface/src/avatar/Avatar.cpp

@@ -1187,7 +1187,7 @@ void Avatar::computeShapeInfo(ShapeInfo& shapeInfo) {
 
 // virtual
 void Avatar::rebuildSkeletonBody() {
-    DependencyManager::get<AvatarManager>()->updateAvatarPhysicsShape(getSessionUUID());
+    DependencyManager::get<AvatarManager>()->updateAvatarPhysicsShape(this);
 }
 
 glm::vec3 Avatar::getLeftPalmPosition() {

interface/src/avatar/AvatarManager.cpp

@@ -110,28 +110,34 @@ 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
-    AvatarHash::iterator avatarIterator = _avatarHash.begin();
+    auto hashCopy = getHashCopy();
-    while (avatarIterator != _avatarHash.end()) {
+    
-        auto avatar = std::dynamic_pointer_cast<Avatar>(avatarIterator.value());
+    AvatarHash::iterator avatarIterator = hashCopy.begin();
+    while (avatarIterator != hashCopy.end()) {
+        auto avatar = std::static_pointer_cast<Avatar>(avatarIterator.value());
 
         if (avatar == _myAvatar || !avatar->isInitialized()) {
             // DO NOT update _myAvatar!  Its update has already been done earlier in the main loop.
             // DO NOT update or fade out uninitialized Avatars
             ++avatarIterator;
         } else if (avatar->shouldDie()) {
-            removeAvatarMotionState(avatar);
+            removeAvatar(avatarIterator.key());
-            _avatarFades.push_back(avatarIterator.value());
+            ++avatarIterator;
-            QWriteLocker locker(&_hashLock);
-            avatarIterator = _avatarHash.erase(avatarIterator);
         } else {
             avatar->startUpdate();
             avatar->simulate(deltaTime);
@@ -169,19 +175,21 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
 }
 
 AvatarSharedPointer AvatarManager::newSharedAvatar() {
-    return AvatarSharedPointer(std::make_shared<Avatar>(std::make_shared<AvatarRig>()));
+    return std::make_shared<Avatar>(std::make_shared<AvatarRig>());
 }
 
-// virtual
 AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
-    auto avatar = std::dynamic_pointer_cast<Avatar>(AvatarHashMap::addAvatar(sessionUUID, mixerWeakPointer));
+    auto newAvatar = AvatarHashMap::addAvatar(sessionUUID, mixerWeakPointer);
+    auto rawRenderableAvatar = std::static_pointer_cast<Avatar>(newAvatar);
+    
     render::ScenePointer scene = qApp->getMain3DScene();
     render::PendingChanges pendingChanges;
     if (DependencyManager::get<SceneScriptingInterface>()->shouldRenderAvatars()) {
-        avatar->addToScene(avatar, scene, pendingChanges);
+        rawRenderableAvatar->addToScene(rawRenderableAvatar, scene, pendingChanges);
     }
     scene->enqueuePendingChanges(pendingChanges);
-    return avatar;
+    
+    return newAvatar;
 }
 
 // protected
@@ -200,20 +208,25 @@ void AvatarManager::removeAvatarMotionState(AvatarSharedPointer avatar) {
 
 // virtual
 void AvatarManager::removeAvatar(const QUuid& sessionUUID) {
-    AvatarHash::iterator avatarIterator = _avatarHash.find(sessionUUID);
+    QWriteLocker locker(&_hashLock);
-    if (avatarIterator != _avatarHash.end()) {
+    
-        std::shared_ptr<Avatar> avatar = std::dynamic_pointer_cast<Avatar>(avatarIterator.value());
+    auto removedAvatar = _avatarHash.take(sessionUUID);
-        if (avatar != _myAvatar && avatar->isInitialized()) {
+    if (removedAvatar) {
-            removeAvatarMotionState(avatar);
+        handleRemovedAvatar(removedAvatar);
-            _avatarFades.push_back(avatarIterator.value());
-            QWriteLocker locker(&_hashLock);
-            _avatarHash.erase(avatarIterator);
-        }
     }
 }
 
+void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar) {
+    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<Avatar>(avatarIterator.value());
@@ -221,10 +234,10 @@ void AvatarManager::clearOtherAvatars() {
             // don't remove myAvatar or uninitialized avatars from the list
             ++avatarIterator;
         } else {
-            removeAvatarMotionState(avatar);
+            auto removedAvatar = avatarIterator.value();
-            _avatarFades.push_back(avatarIterator.value());
-            QWriteLocker locker(&_hashLock);
             avatarIterator = _avatarHash.erase(avatarIterator);
+            
+            handleRemovedAvatar(removedAvatar);
         }
     }
     _myAvatar->clearLookAtTargetAvatar();
@@ -252,6 +265,7 @@ QVector<QUuid> AvatarManager::getAvatarIdentifiers() {
     QReadLocker locker(&_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.
@@ -317,23 +331,19 @@ void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents
     }
 }
 
-void AvatarManager::updateAvatarPhysicsShape(const QUuid& id) {
+void AvatarManager::updateAvatarPhysicsShape(Avatar* avatar) {
-    AvatarHash::iterator avatarItr = _avatarHash.find(id);
+    AvatarMotionState* motionState = avatar->getMotionState();
-    if (avatarItr != _avatarHash.end()) {
+    if (motionState) {
-        auto avatar = std::static_pointer_cast<Avatar>(avatarItr.value());
+        motionState->addDirtyFlags(Simulation::DIRTY_SHAPE);
-        AvatarMotionState* motionState = avatar->getMotionState();
+    } else {
-        if (motionState) {
+        ShapeInfo shapeInfo;
-            motionState->addDirtyFlags(Simulation::DIRTY_SHAPE);
+        avatar->computeShapeInfo(shapeInfo);
-        } else {
+        btCollisionShape* shape = ObjectMotionState::getShapeManager()->getShape(shapeInfo);
-            ShapeInfo shapeInfo;
+        if (shape) {
-            avatar->computeShapeInfo(shapeInfo);
+            AvatarMotionState* motionState = new AvatarMotionState(avatar, shape);
-            btCollisionShape* shape = ObjectMotionState::getShapeManager()->getShape(shapeInfo);
+            avatar->setMotionState(motionState);
-            if (shape) {
+            _motionStatesToAdd.insert(motionState);
-                AvatarMotionState* motionState = new AvatarMotionState(avatar.get(), shape);
+            _avatarMotionStates.insert(motionState);
-                avatar->setMotionState(motionState);
-                _motionStatesToAdd.insert(motionState);
-                _avatarMotionStates.insert(motionState);
-            }
         }
     }
 }
@@ -341,7 +351,7 @@ void AvatarManager::updateAvatarPhysicsShape(const QUuid& id) {
 void AvatarManager::updateAvatarRenderStatus(bool shouldRenderAvatars) {
     if (DependencyManager::get<SceneScriptingInterface>()->shouldRenderAvatars()) {
         for (auto avatarData : _avatarHash) {
-            auto avatar = std::dynamic_pointer_cast<Avatar>(avatarData);
+            auto avatar = std::static_pointer_cast<Avatar>(avatarData);
             render::ScenePointer scene = qApp->getMain3DScene();
             render::PendingChanges pendingChanges;
             avatar->addToScene(avatar, scene, pendingChanges);
@@ -349,7 +359,7 @@ void AvatarManager::updateAvatarRenderStatus(bool shouldRenderAvatars) {
         }
     } else {
         for (auto avatarData : _avatarHash) {
-            auto avatar = std::dynamic_pointer_cast<Avatar>(avatarData);
+            auto avatar = std::static_pointer_cast<Avatar>(avatarData);
             render::ScenePointer scene = qApp->getMain3DScene();
             render::PendingChanges pendingChanges;
             avatar->removeFromScene(avatar, scene, pendingChanges);
@@ -363,11 +373,6 @@ AvatarSharedPointer AvatarManager::getAvatarBySessionID(const QUuid& sessionID)
     if (sessionID == _myAvatar->getSessionUUID()) {
         return std::static_pointer_cast<Avatar>(_myAvatar);
     }
-    QReadLocker locker(&_hashLock);
+    
-    auto iter = _avatarHash.find(sessionID);
+    return findAvatar(sessionID);
-    if (iter != _avatarHash.end()) {
-        return iter.value();
-    } else {
-        return AvatarSharedPointer();
-    }
 }

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; }
@@ -79,7 +79,9 @@ private:
     virtual AvatarSharedPointer newSharedAvatar();
     virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
     void removeAvatarMotionState(AvatarSharedPointer avatar);
+    
     virtual void removeAvatar(const QUuid& sessionUUID);
+    virtual void handleRemovedAvatar(const AvatarSharedPointer& removedAvatar);
     
     QVector<AvatarSharedPointer> _avatarFades;
     std::shared_ptr<MyAvatar> _myAvatar;

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;
+    AvatarHash hash = DependencyManager::get<AvatarManager>()->getHashCopy();
-    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();

libraries/avatars/src/AvatarHashMap.cpp

@@ -22,13 +22,9 @@ 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);
+    auto hashCopy = getHashCopy();
-    foreach(const AvatarSharedPointer& sharedAvatar, _avatarHash) {
+    foreach(const AvatarSharedPointer& sharedAvatar, hashCopy) {
         glm::vec3 avatarPosition = sharedAvatar->getPosition();
         float distance = glm::distance(avatarPosition, position);
         if (distance < range) {
@@ -44,16 +40,34 @@ AvatarSharedPointer AvatarHashMap::newSharedAvatar() {
 
 AvatarSharedPointer AvatarHashMap::addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
     qCDebug(avatars) << "Adding avatar with sessionUUID " << sessionUUID << "to AvatarHashMap.";
-
+    
-    AvatarSharedPointer avatar = newSharedAvatar();
+    auto avatar = newSharedAvatar();
     avatar->setSessionUUID(sessionUUID);
     avatar->setOwningAvatarMixer(mixerWeakPointer);
-    QWriteLocker locker(&_hashLock);
+    
     _avatarHash.insert(sessionUUID, avatar);
     emit avatarAddedEvent(sessionUUID);
+    
     return avatar;
 }
 
+AvatarSharedPointer AvatarHashMap::newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer) {
+    QWriteLocker locker(&_hashLock);
+    
+    auto avatar = _avatarHash.value(sessionUUID);
+    
+    if (!avatar) {
+        avatar = addAvatar(sessionUUID, mixerWeakPointer);
+    }
+    
+    return avatar;
+}
+
+AvatarSharedPointer AvatarHashMap::findAvatar(const QUuid& sessionUUID) {
+    QReadLocker locker(&_hashLock);
+    return _avatarHash.value(sessionUUID);
+}
+
 void AvatarHashMap::processAvatarDataPacket(QSharedPointer<NLPacket> packet, SharedNodePointer sendingNode) {
 
     // enumerate over all of the avatars in this packet
@@ -66,10 +80,7 @@ void AvatarHashMap::processAvatarDataPacket(QSharedPointer<NLPacket> packet, Sha
         QByteArray byteArray = packet->readWithoutCopy(packet->bytesLeftToRead());
         
         if (sessionUUID != _lastOwnerSessionUUID) {
-            AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+            auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-            if (!avatar) {
-                avatar = addAvatar(sessionUUID, sendingNode);
-            }
             
             // have the matching (or new) avatar parse the data from the packet
             int bytesRead = avatar->parseDataFromBuffer(byteArray);
@@ -97,10 +108,8 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer<NLPacket> packet,
         identityStream >> sessionUUID >> faceMeshURL >> skeletonURL >> attachmentData >> displayName;
 
         // mesh URL for a UUID, find avatar in our list
-        AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+        auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-        if (!avatar) {
+        
-            avatar = addAvatar(sessionUUID, sendingNode);
-        }
         if (avatar->getFaceModelURL() != faceMeshURL) {
             avatar->setFaceModelURL(faceMeshURL);
         }
@@ -122,10 +131,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer<NLPacket> packet,
 void AvatarHashMap::processAvatarBillboardPacket(QSharedPointer<NLPacket> packet, SharedNodePointer sendingNode) {
     QUuid sessionUUID = QUuid::fromRfc4122(packet->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
 
-    AvatarSharedPointer avatar = _avatarHash.value(sessionUUID);
+    auto avatar = newOrExistingAvatar(sessionUUID, sendingNode);
-    if (!avatar) {
-        avatar = addAvatar(sessionUUID, sendingNode);
-    }
 
     QByteArray billboard = packet->read(packet->bytesLeftToRead());
     if (avatar->getBillboard() != billboard) {
@@ -137,13 +143,22 @@ void AvatarHashMap::processKillAvatar(QSharedPointer<NLPacket> 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) {

libraries/avatars/src/AvatarHashMap.h

@@ -31,7 +31,7 @@ class AvatarHashMap : public QObject, public Dependency {
     SINGLETON_DEPENDENCY
 
 public:
-    void withAvatarHash(std::function<void(const AvatarHash& hash)>);
+    AvatarHash getHashCopy() { QReadLocker lock(&_hashLock); return _avatarHash; }
     int size() { return _avatarHash.size(); }
 
 signals:
@@ -55,7 +55,11 @@ protected:
 
     virtual AvatarSharedPointer newSharedAvatar();
     virtual AvatarSharedPointer addAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& mixerWeakPointer);
+    AvatarSharedPointer newOrExistingAvatar(const QUuid& sessionUUID, const QWeakPointer<Node>& 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.