Merge pull request #9753 from zfox23/FB_3234_TakeTwo

Optimize AvatarMixer with respect to Identity and Avatar Data

assignment-client/src/avatars/AvatarMixer.cpp

@@ -365,6 +365,28 @@ void AvatarMixer::handleRequestsDomainListDataPacket(QSharedPointer<ReceivedMess
             message->readPrimitive(&isRequesting);
             nodeData->setRequestsDomainListData(isRequesting);
             qCDebug(avatars) << "node" << nodeData->getNodeID() << "requestsDomainListData" << isRequesting;
+
+            // If we just opened the PAL...
+            if (isRequesting) {
+                // For each node in the NodeList...
+                auto nodeList = DependencyManager::get<NodeList>();
+                nodeList->eachMatchingNode(
+                    // Discover the valid nodes we're ignoring...
+                    [&](const SharedNodePointer& node)->bool {
+                    if (node->getUUID() != senderNode->getUUID() &&
+                        (nodeData->isRadiusIgnoring(node->getUUID()) ||
+                        senderNode->isIgnoringNodeWithID(node->getUUID()))) {
+                        return true;
+                    }
+                    return false;
+                },
+                    // ...For those nodes, reset the lastBroadcastTime to 0
+                    // so that the AvatarMixer will send Identity data to us
+                    [&](const SharedNodePointer& node) {
+                    nodeData->setLastBroadcastTime(node->getUUID(), 0);
+                }
+                );
+            }
         }
     }
     auto end = usecTimestampNow();
@@ -409,7 +431,31 @@ void AvatarMixer::handleKillAvatarPacket(QSharedPointer<ReceivedMessage> message
 
 void AvatarMixer::handleNodeIgnoreRequestPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer senderNode) {
     auto start = usecTimestampNow();
-    senderNode->parseIgnoreRequestMessage(message);
+    auto nodeList = DependencyManager::get<NodeList>();
+    AvatarMixerClientData* nodeData = reinterpret_cast<AvatarMixerClientData*>(senderNode->getLinkedData());
+    bool addToIgnore;
+    message->readPrimitive(&addToIgnore);
+    while (message->getBytesLeftToRead()) {
+        // parse out the UUID being ignored from the packet
+        QUuid ignoredUUID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
+        // Reset the lastBroadcastTime for the ignored avatar to 0
+        // so the AvatarMixer knows it'll have to send identity data about the ignored avatar
+        // to the ignorer if the ignorer unignores.
+        nodeData->setLastBroadcastTime(ignoredUUID, 0);
+
+        // Reset the lastBroadcastTime for the ignorer (FROM THE PERSPECTIVE OF THE IGNORED) to 0
+        // so the AvatarMixer knows it'll have to send identity data about the ignorer
+        // to the ignored if the ignorer unignores.
+        auto ignoredNode = nodeList->nodeWithUUID(ignoredUUID);
+        AvatarMixerClientData* ignoredNodeData = reinterpret_cast<AvatarMixerClientData*>(ignoredNode->getLinkedData());
+        ignoredNodeData->setLastBroadcastTime(senderNode->getUUID(), 0);
+
+        if (addToIgnore) {
+            senderNode->addIgnoredNode(ignoredUUID);
+        } else {
+            senderNode->removeIgnoredNode(ignoredUUID);
+        }
+    }
     auto end = usecTimestampNow();
     _handleNodeIgnoreRequestPacketElapsedTime += (end - start);
 }

assignment-client/src/avatars/AvatarMixerClientData.cpp

@@ -65,15 +65,6 @@ int AvatarMixerClientData::parseData(ReceivedMessage& message) {
     // compute the offset to the data payload
     return _avatar->parseDataFromBuffer(message.readWithoutCopy(message.getBytesLeftToRead()));
 }
-
-bool AvatarMixerClientData::checkAndSetHasReceivedFirstPacketsFrom(const QUuid& uuid) {
-    if (_hasReceivedFirstPacketsFrom.find(uuid) == _hasReceivedFirstPacketsFrom.end()) {
-        _hasReceivedFirstPacketsFrom.insert(uuid);
-        return false;
-    }
-    return true;
-}
-
 uint64_t AvatarMixerClientData::getLastBroadcastTime(const QUuid& nodeUUID) const {
     // return the matching PacketSequenceNumber, or the default if we don't have it
     auto nodeMatch = _lastBroadcastTimes.find(nodeUUID);
@@ -102,8 +93,8 @@ void AvatarMixerClientData::ignoreOther(SharedNodePointer self, SharedNodePointe
         } else {
             killPacket->writePrimitive(KillAvatarReason::YourAvatarEnteredTheirBubble);
         }
+        setLastBroadcastTime(other->getUUID(), 0);
         DependencyManager::get<NodeList>()->sendUnreliablePacket(*killPacket, *self);
-        _hasReceivedFirstPacketsFrom.erase(other->getUUID());
     }
 }
 

assignment-client/src/avatars/AvatarMixerClientData.h

@@ -45,8 +45,6 @@ public:
     const AvatarData* getConstAvatarData() const { return _avatar.get(); }
     AvatarSharedPointer getAvatarSharedPointer() const { return _avatar; }
 
-    bool checkAndSetHasReceivedFirstPacketsFrom(const QUuid& uuid);
-
     uint16_t getLastBroadcastSequenceNumber(const QUuid& nodeUUID) const;
     void setLastBroadcastSequenceNumber(const QUuid& nodeUUID, uint16_t sequenceNumber)
         { _lastBroadcastSequenceNumbers[nodeUUID] = sequenceNumber; }
@@ -63,8 +61,8 @@ public:
 
     uint16_t getLastReceivedSequenceNumber() const { return _lastReceivedSequenceNumber; }
 
-    HRCTime getIdentityChangeTimestamp() const { return _identityChangeTimestamp; }
-    void flagIdentityChange() { _identityChangeTimestamp = p_high_resolution_clock::now(); }
+    uint64_t getIdentityChangeTimestamp() const { return _identityChangeTimestamp; }
+    void flagIdentityChange() { _identityChangeTimestamp = usecTimestampNow(); }
     bool getAvatarSessionDisplayNameMustChange() const { return _avatarSessionDisplayNameMustChange; }
     void setAvatarSessionDisplayNameMustChange(bool set = true) { _avatarSessionDisplayNameMustChange = set; }
 
@@ -139,7 +137,6 @@ private:
 
     uint16_t _lastReceivedSequenceNumber { 0 };
     std::unordered_map<QUuid, uint16_t> _lastBroadcastSequenceNumbers;
-    std::unordered_set<QUuid> _hasReceivedFirstPacketsFrom;
     std::unordered_map<QUuid, uint64_t> _lastBroadcastTimes;
 
     // this is a map of the last time we encoded an "other" avatar for
@@ -147,7 +144,7 @@ private:
     std::unordered_map<QUuid, quint64> _lastOtherAvatarEncodeTime;
     std::unordered_map<QUuid, QVector<JointData>> _lastOtherAvatarSentJoints;
 
-    HRCTime _identityChangeTimestamp;
+    uint64_t _identityChangeTimestamp;
     bool _avatarSessionDisplayNameMustChange{ false };
 
     int _numAvatarsSentLastFrame = 0;

assignment-client/src/avatars/AvatarMixerSlave.cpp

@@ -80,16 +80,6 @@ int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData,
 
 static const int AVATAR_MIXER_BROADCAST_FRAMES_PER_SECOND = 45;
 
-// FIXME - There is some old logic (unchanged as of 2/17/17) that randomly decides to send an identity
-// packet. That logic had the following comment about the constants it uses...
-//
-//         An 80% chance of sending a identity packet within a 5 second interval.
-//         assuming 60 htz update rate.
-//
-// Assuming the calculation of the constant is in fact correct for 80% and 60hz and 5 seconds (an assumption
-// that I have not verified) then the constant is definitely wrong now, since we send at 45hz.
-const float IDENTITY_SEND_PROBABILITY = 1.0f / 187.0f;
-
 void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) {
     quint64 start = usecTimestampNow();
 
@@ -137,14 +127,18 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) {
         // keep track of the number of other avatar frames skipped
         int numAvatarsWithSkippedFrames = 0;
 
-        // When this is true, the AvatarMixer will send Avatar data to a client about avatars that are not in the view frustrum
-        bool getsOutOfView = nodeData->getRequestsDomainListData();
-
-        // When this is true, the AvatarMixer will send Avatar data to a client about avatars that they've ignored
-        bool getsIgnoredByMe = getsOutOfView;
+        // When this is true, the AvatarMixer will send Avatar data to a client
+        // about avatars they've ignored or that are out of view
+        bool PALIsOpen = nodeData->getRequestsDomainListData();
 
         // When this is true, the AvatarMixer will send Avatar data to a client about avatars that have ignored them
-        bool getsAnyIgnored = getsIgnoredByMe && node->getCanKick();
+        bool getsAnyIgnored = PALIsOpen && node->getCanKick();
+
+        if (PALIsOpen) {
+            // Increase minimumBytesPerAvatar if the PAL is open
+            minimumBytesPerAvatar += sizeof(AvatarDataPacket::AvatarGlobalPosition) +
+                sizeof(AvatarDataPacket::AudioLoudness);
+        }
 
         // setup a PacketList for the avatarPackets
         auto avatarPacketList = NLPacketList::create(PacketType::BulkAvatarData);
@@ -222,13 +216,14 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) {
                     // or that has ignored the viewing node
                     if (!avatarNode->getLinkedData()
                         || avatarNode->getUUID() == node->getUUID()
-                        || (node->isIgnoringNodeWithID(avatarNode->getUUID()) && !getsIgnoredByMe)
+                        || (node->isIgnoringNodeWithID(avatarNode->getUUID()) && !PALIsOpen)
                         || (avatarNode->isIgnoringNodeWithID(node->getUUID()) && !getsAnyIgnored)) {
                         shouldIgnore = true;
                     } else {
 
                         // Check to see if the space bubble is enabled
-                        if (node->isIgnoreRadiusEnabled() || avatarNode->isIgnoreRadiusEnabled()) {
+                        // Don't bother with these checks if the other avatar has their bubble enabled and we're gettingAnyIgnored
+                        if (node->isIgnoreRadiusEnabled() || (avatarNode->isIgnoreRadiusEnabled() && !getsAnyIgnored)) {
 
                             // Define the scale of the box for the current other node
                             glm::vec3 otherNodeBoxScale = (avatarNodeData->getPosition() - avatarNodeData->getGlobalBoundingBoxCorner()) * 2.0f;
@@ -306,16 +301,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) {
 
             const AvatarMixerClientData* otherNodeData = reinterpret_cast<const AvatarMixerClientData*>(otherNode->getLinkedData());
 
-            // make sure we send out identity packets to and from new arrivals.
-            bool forceSend = !nodeData->checkAndSetHasReceivedFirstPacketsFrom(otherNode->getUUID());
-
-            // FIXME - this clause seems suspicious "... || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp ..."
-            if (!overBudget
-                && otherNodeData->getIdentityChangeTimestamp().time_since_epoch().count() > 0
-                && (forceSend
-                || otherNodeData->getIdentityChangeTimestamp() > _lastFrameTimestamp
-                || distribution(generator) < IDENTITY_SEND_PROBABILITY)) {
-
+            // If the time that the mixer sent AVATAR DATA about Avatar B to Avatar A is BEFORE OR EQUAL TO
+            // the time that Avatar B flagged an IDENTITY DATA change, send IDENTITY DATA about Avatar B to Avatar A.
+            if (nodeData->getLastBroadcastTime(otherNode->getUUID()) <= otherNodeData->getIdentityChangeTimestamp()) {
                 identityBytesSent += sendIdentityPacket(otherNodeData, node);
             }
 
@@ -335,9 +323,9 @@ void AvatarMixerSlave::broadcastAvatarData(const SharedNodePointer& node) {
             if (overBudget) {
                 overBudgetAvatars++;
                 _stats.overBudgetAvatars++;
-                detail = AvatarData::NoData;
-            } else  if (!isInView && !getsOutOfView) {
-                detail = AvatarData::NoData;
+                detail = PALIsOpen ? AvatarData::PALMinimum : AvatarData::NoData;
+            } else if (!isInView) {
+                detail = PALIsOpen ? AvatarData::PALMinimum : AvatarData::NoData;
                 nodeData->incrementAvatarOutOfView();
             } else {
                 detail = distribution(generator) < AVATAR_SEND_FULL_UPDATE_RATIO

interface/src/avatar/AvatarManager.cpp

@@ -329,7 +329,7 @@ void AvatarManager::removeAvatar(const QUuid& sessionUUID, KillAvatarReason remo
 }
 
 void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) {
-    AvatarHashMap::handleRemovedAvatar(removedAvatar);
+    AvatarHashMap::handleRemovedAvatar(removedAvatar, removalReason);
 
     // removedAvatar is a shared pointer to an AvatarData but we need to get to the derived Avatar
     // class in this context so we can call methods that don't exist at the base class.

libraries/avatars/src/AvatarData.cpp

@@ -186,6 +186,7 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent
     bool cullSmallChanges = (dataDetail == CullSmallData);
     bool sendAll = (dataDetail == SendAllData);
     bool sendMinimum = (dataDetail == MinimumData);
+    bool sendPALMinimum = (dataDetail == PALMinimum);
 
     lazyInitHeadData();
 
@@ -222,24 +223,41 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent
     auto parentID = getParentID();
 
     bool hasAvatarGlobalPosition = true; // always include global position
-    bool hasAvatarOrientation = sendAll || rotationChangedSince(lastSentTime);
-    bool hasAvatarBoundingBox = sendAll || avatarBoundingBoxChangedSince(lastSentTime);
-    bool hasAvatarScale = sendAll || avatarScaleChangedSince(lastSentTime);
-    bool hasLookAtPosition = sendAll || lookAtPositionChangedSince(lastSentTime);
-    bool hasAudioLoudness = sendAll || audioLoudnessChangedSince(lastSentTime);
-    bool hasSensorToWorldMatrix = sendAll || sensorToWorldMatrixChangedSince(lastSentTime);
-    bool hasAdditionalFlags = sendAll || additionalFlagsChangedSince(lastSentTime);
+    bool hasAvatarOrientation = false;
+    bool hasAvatarBoundingBox = false;
+    bool hasAvatarScale = false;
+    bool hasLookAtPosition = false;
+    bool hasAudioLoudness = false;
+    bool hasSensorToWorldMatrix = false;
+    bool hasAdditionalFlags = false;
 
     // local position, and parent info only apply to avatars that are parented. The local position
     // and the parent info can change independently though, so we track their "changed since"
     // separately
-    bool hasParentInfo = sendAll || parentInfoChangedSince(lastSentTime);
-    bool hasAvatarLocalPosition = hasParent() && (sendAll ||
-        tranlationChangedSince(lastSentTime) ||
-        parentInfoChangedSince(lastSentTime));
+    bool hasParentInfo = false;
+    bool hasAvatarLocalPosition = false;
 
-    bool hasFaceTrackerInfo = !dropFaceTracking && hasFaceTracker() && (sendAll || faceTrackerInfoChangedSince(lastSentTime));
-    bool hasJointData = sendAll || !sendMinimum;
+    bool hasFaceTrackerInfo = false;
+    bool hasJointData = false;
+
+    if (sendPALMinimum) {
+        hasAudioLoudness = true;
+    } else {
+        hasAvatarOrientation = sendAll || rotationChangedSince(lastSentTime);
+        hasAvatarBoundingBox = sendAll || avatarBoundingBoxChangedSince(lastSentTime);
+        hasAvatarScale = sendAll || avatarScaleChangedSince(lastSentTime);
+        hasLookAtPosition = sendAll || lookAtPositionChangedSince(lastSentTime);
+        hasAudioLoudness = sendAll || audioLoudnessChangedSince(lastSentTime);
+        hasSensorToWorldMatrix = sendAll || sensorToWorldMatrixChangedSince(lastSentTime);
+        hasAdditionalFlags = sendAll || additionalFlagsChangedSince(lastSentTime);
+        hasParentInfo = sendAll || parentInfoChangedSince(lastSentTime);
+        hasAvatarLocalPosition = hasParent() && (sendAll ||
+            tranlationChangedSince(lastSentTime) ||
+            parentInfoChangedSince(lastSentTime));
+
+        hasFaceTrackerInfo = !dropFaceTracking && hasFaceTracker() && (sendAll || faceTrackerInfoChangedSince(lastSentTime));
+        hasJointData = sendAll || !sendMinimum;
+    }
 
     // Leading flags, to indicate how much data is actually included in the packet...
     AvatarDataPacket::HasFlags packetStateFlags =

libraries/avatars/src/AvatarData.h

@@ -376,6 +376,7 @@ public:
 
     typedef enum { 
         NoData,
+        PALMinimum,
         MinimumData, 
         CullSmallData,
         IncludeSmallData,