address code review comments

assignment-client/src/audio/AudioMixerClientData.cpp

@@ -121,8 +121,6 @@ void AudioMixerClientData::optionallyReplicatePacket(ReceivedMessage& message, c
     // first, make sure that this is a packet from a node we are supposed to replicate
     if (node.isReplicated()) {
 
-        auto nodeList = DependencyManager::get<NodeList>();
-
         // now make sure it's a packet type that we want to replicate
 
         // first check if it is an original type that we should replicate
@@ -139,6 +137,7 @@ void AudioMixerClientData::optionallyReplicatePacket(ReceivedMessage& message, c
         }
 
         std::unique_ptr<NLPacket> packet;
+        auto nodeList = DependencyManager::get<NodeList>();
 
         // enumerate the downstream audio mixers and send them the replicated version of this packet
         nodeList->unsafeEachNode([&](const SharedNodePointer& downstreamNode) {

assignment-client/src/avatars/AvatarMixer.cpp

@@ -57,7 +57,7 @@ AvatarMixer::AvatarMixer(ReceivedMessage& message) :
     packetReceiver.registerListenerForTypes({
         PacketType::ReplicatedAvatarIdentity,
         PacketType::ReplicatedKillAvatar
-    }, this, "handleReplicatedPackets");
+    }, this, "handleReplicatedPacket");
 
     packetReceiver.registerListener(PacketType::ReplicatedBulkAvatarData, this, "handleReplicatedBulkAvatarPacket");
 
@@ -82,7 +82,7 @@ SharedNodePointer addOrUpdateReplicatedNode(const QUuid& nodeID, const HifiSockA
     return replicatedNode;
 }
 
-void AvatarMixer::handleReplicatedPackets(QSharedPointer<ReceivedMessage> message) {
+void AvatarMixer::handleReplicatedPacket(QSharedPointer<ReceivedMessage> message) {
     auto nodeList = DependencyManager::get<NodeList>();
     auto nodeID = QUuid::fromRfc4122(message->peek(NUM_BYTES_RFC4122_UUID));
 
@@ -96,8 +96,6 @@ void AvatarMixer::handleReplicatedPackets(QSharedPointer<ReceivedMessage> messag
 }
 
 void AvatarMixer::handleReplicatedBulkAvatarPacket(QSharedPointer<ReceivedMessage> message) {
-    auto nodeList = DependencyManager::get<NodeList>();
-
     while (message->getBytesLeftToRead()) {
         // first, grab the node ID for this replicated avatar
         auto nodeID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID));

assignment-client/src/avatars/AvatarMixer.h

@@ -46,7 +46,7 @@ private slots:
     void handleNodeIgnoreRequestPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer senderNode);
     void handleRadiusIgnoreRequestPacket(QSharedPointer<ReceivedMessage> packet, SharedNodePointer sendingNode);
     void handleRequestsDomainListDataPacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer senderNode);
-    void handleReplicatedPackets(QSharedPointer<ReceivedMessage> message);
+    void handleReplicatedPacket(QSharedPointer<ReceivedMessage> message);
     void handleReplicatedBulkAvatarPacket(QSharedPointer<ReceivedMessage> message);
     void domainSettingsRequestComplete();
     void handlePacketVersionMismatch(PacketType type, const HifiSockAddr& senderSockAddr, const QUuid& senderUUID);

assignment-client/src/avatars/AvatarMixerSlave.cpp

@@ -67,7 +67,7 @@ void AvatarMixerSlave::processIncomingPackets(const SharedNodePointer& node) {
 
 int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) {
     if (destinationNode->getType() == NodeType::Agent && !destinationNode->isUpstream()) {
-        QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(true);
+        QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray();
         individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious
         auto identityPackets = NLPacketList::create(PacketType::AvatarIdentity, QByteArray(), true, true);
         identityPackets->write(individualData);
@@ -81,7 +81,7 @@ int AvatarMixerSlave::sendIdentityPacket(const AvatarMixerClientData* nodeData,
 
 int AvatarMixerSlave::sendReplicatedIdentityPacket(const AvatarMixerClientData* nodeData, const SharedNodePointer& destinationNode) {
     if (destinationNode->getType() == NodeType::DownstreamAvatarMixer) {
-        QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray(true);
+        QByteArray individualData = nodeData->getConstAvatarData()->identityByteArray();
         individualData.replace(0, NUM_BYTES_RFC4122_UUID, nodeData->getNodeID().toRfc4122()); // FIXME, this looks suspicious
         auto identityPacket = NLPacket::create(PacketType::ReplicatedAvatarIdentity);
         identityPacket->write(individualData);

domain-server/src/DomainServer.cpp

@@ -2277,6 +2277,10 @@ void DomainServer::updateDownstreamNodes() {
 
             }
         }
+
+        // enumerate the nodes to determine which are no longer downstream for this domain
+        // collect them in a vector to separately remove them with handleKillNode (since eachNode has a read lock and
+        // we cannot recursively take the write lock required by handleKillNode)
         std::vector<SharedNodePointer> nodesToKill;
         nodeList->eachNode([&](const SharedNodePointer& otherNode) {
             if (NodeType::isDownstream(otherNode->getType())) {
@@ -2288,6 +2292,7 @@ void DomainServer::updateDownstreamNodes() {
                 }
             }
         });
+
         for (auto& node : nodesToKill) {
             handleKillNode(node);
         }
@@ -2296,12 +2301,11 @@ void DomainServer::updateDownstreamNodes() {
 
 void DomainServer::updateReplicatedNodes() {
     // Make sure we have downstream nodes in our list
-    // TODO Move this to a different function
-    _replicatedUsernames.clear();
     auto settings = _settingsManager.getSettingsMap();
 
     static const QString REPLICATED_USERS_KEY = "users";
     _replicatedUsernames.clear();
+    
     if (settings.contains(BROADCASTING_SETTINGS_KEY)) {
         auto replicationSettings = settings.value(BROADCASTING_SETTINGS_KEY).toMap();
         if (replicationSettings.contains(REPLICATED_USERS_KEY)) {
@@ -2319,9 +2323,6 @@ void DomainServer::updateReplicatedNodes() {
             auto shouldReplicate = shouldReplicateNode(*otherNode);
             auto isReplicated = otherNode->isReplicated();
             if (isReplicated && !shouldReplicate) {
-                qDebug() << "Setting node to NOT be replicated:" << otherNode->getUUID();
-            } else if (!isReplicated && shouldReplicate) {
-                qDebug() << "Setting node to replicated:" << otherNode->getUUID();
                 qDebug() << "Setting node to NOT be replicated:"
                     << otherNode->getPermissions().getVerifiedUserName() << otherNode->getUUID();
             } else if (!isReplicated && shouldReplicate) {
@@ -2334,11 +2335,16 @@ void DomainServer::updateReplicatedNodes() {
 }
 
 bool DomainServer::shouldReplicateNode(const Node& node) {
-    QString verifiedUsername = node.getPermissions().getVerifiedUserName();
-    // Both the verified username and usernames in _replicatedUsernames are lowercase, so
-    // comparisons here are case-insensitive.
-    auto it = find(_replicatedUsernames.cbegin(), _replicatedUsernames.cend(), verifiedUsername);
-    return it != _replicatedUsernames.end() && node.getType() == NodeType::Agent;
+    if (node.getType() == NodeType::Agent) {
+        QString verifiedUsername = node.getPermissions().getVerifiedUserName();
+
+        // Both the verified username and usernames in _replicatedUsernames are lowercase, so
+        // comparisons here are case-insensitive.
+        auto it = find(_replicatedUsernames.cbegin(), _replicatedUsernames.cend(), verifiedUsername);
+        return it != _replicatedUsernames.end();
+    } else {
+        return false;
+    }
 };
 
 void DomainServer::nodeAdded(SharedNodePointer node) {

libraries/avatars/src/AvatarData.cpp

@@ -1496,13 +1496,13 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide
     udt::SequenceNumber incomingSequenceNumber(incomingSequenceNumberType);
 
     if (!_hasProcessedFirstIdentity) {
-        _lastIncomingSequenceNumber = incomingSequenceNumber - 1;
+        _lastSequenceNumber = incomingSequenceNumber - 1;
         _hasProcessedFirstIdentity = true;
         qCDebug(avatars) << "Processing first identity packet for" << avatarSessionID << "-"
             << (udt::SequenceNumber::Type) incomingSequenceNumber;
     }
 
-    if (incomingSequenceNumber > _lastIncomingSequenceNumber) {
+    if (incomingSequenceNumber > _lastSequenceNumber) {
         Identity identity;
 
         packetStream >> identity.skeletonModelURL
@@ -1512,7 +1512,7 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide
             >> identity.avatarEntityData;
 
         // set the store identity sequence number to match the incoming identity
-        _lastIncomingSequenceNumber = incomingSequenceNumber;
+        _lastSequenceNumber = incomingSequenceNumber;
 
         if (_firstSkeletonCheck || (identity.skeletonModelURL != cannonicalSkeletonModelURL(emptyURL))) {
             setSkeletonModelURL(identity.skeletonModelURL);
@@ -1552,34 +1552,26 @@ void AvatarData::processAvatarIdentity(const QByteArray& identityData, bool& ide
             << "identity.skeletonModelURL:" << identity.skeletonModelURL
             << "identity.displayName:" << identity.displayName
             << "identity.sessionDisplayName:" << identity.sessionDisplayName;
-#endif
-
     } else {
-#ifdef WANT_DEBUG
+
         qCDebug(avatars) << "Refusing to process identity for" << uuidStringWithoutCurlyBraces(avatarSessionID) << "since"
-            << (udt::SequenceNumber::Type) _lastIncomingSequenceNumber
+            << (udt::SequenceNumber::Type) _lastSequenceNumber
             << "is >=" << (udt::SequenceNumber::Type) incomingSequenceNumber;
 #endif
     }
 }
 
-QByteArray AvatarData::identityByteArray(bool shouldForwardIncomingSequenceNumber) const {
+QByteArray AvatarData::identityByteArray() const {
     QByteArray identityData;
     QDataStream identityStream(&identityData, QIODevice::Append);
     const QUrl& urlToSend = cannonicalSkeletonModelURL(emptyURL); // depends on _skeletonModelURL
 
-    // we use the boolean flag to determine if this is an identity byte array for a mixer to send to an agent
-    // or an agent to send to a mixer
-
     // when mixers send identity packets to agents, they simply forward along the last incoming sequence number they received
     // whereas agents send a fresh outgoing sequence number when identity data has changed
 
-    udt::SequenceNumber identitySequenceNumber =
-        shouldForwardIncomingSequenceNumber ? _lastIncomingSequenceNumber : _lastOutgoingSequenceNumber;
-
     _avatarEntitiesLock.withReadLock([&] {
         identityStream << getSessionUUID()
-            << (udt::SequenceNumber::Type) identitySequenceNumber
+            << (udt::SequenceNumber::Type) _lastSequenceNumber
             << urlToSend
             << _attachmentData
             << _displayName
@@ -1763,7 +1755,7 @@ void AvatarData::sendIdentityPacket() {
 
     if (_identityDataChanged) {
         // if the identity data has changed, push the sequence number forwards
-        ++_lastOutgoingSequenceNumber;
+        ++_lastSequenceNumber;
     }
 
     QByteArray identityData = identityByteArray();

libraries/avatars/src/AvatarData.h

@@ -539,7 +539,7 @@ public:
     void processAvatarIdentity(const QByteArray& identityData, bool& identityChanged,
                                bool& displayNameChanged, bool& skeletonModelUrlChanged);
 
-    QByteArray identityByteArray(bool shouldForwardIncomingSequenceNumber = false) const;
+    QByteArray identityByteArray() const;
 
     const QUrl& getSkeletonModelURL() const { return _skeletonModelURL; }
     const QString& getDisplayName() const { return _displayName; }
@@ -781,8 +781,7 @@ protected:
     float _audioAverageLoudness { 0.0f };
 
     bool _identityDataChanged { false };
-    udt::SequenceNumber _lastIncomingSequenceNumber { 0 };
-    udt::SequenceNumber _lastOutgoingSequenceNumber { 0 };
+    udt::SequenceNumber _lastSequenceNumber { 0 };
     bool _hasProcessedFirstIdentity { false };
     float _density;
 

libraries/networking/src/LimitedNodeList.h

@@ -289,9 +289,6 @@ public:
     void sendFakedHandshakeRequestToNode(SharedNodePointer node);
 #endif
 
-    void setMirrorSocket(const HifiSockAddr& mirrorSocket) { _mirrorSocket = mirrorSocket; }
-    const HifiSockAddr& getMirrorSocket() { return _mirrorSocket; }
-
 public slots:
     void reset();
     void eraseAllNodes();
@@ -401,8 +398,6 @@ protected:
     }
 
 
-    HifiSockAddr _mirrorSocket;
-
 private slots:
     void flagTimeForConnectionStep(ConnectionStep connectionStep, quint64 timestamp);
     void possiblyTimeoutSTUNAddressLookup();

libraries/networking/src/NetworkPeer.h

@@ -76,6 +76,11 @@ public:
     float getOutboundBandwidth() const; // in kbps
     float getInboundBandwidth() const; // in kbps
 
+    // Typically the LimitedNodeList removes nodes after they are "silent"
+    // meaning that we have not received any packets (including simple keepalive pings) from them for a set interval.
+    // The _isForcedNeverSilent flag tells the LimitedNodeList that a Node should never be killed by removeSilentNodes()
+    // even if its the timestamp of when it was last heard from has never been updated.
+
     bool isForcedNeverSilent() const { return _isForcedNeverSilent; }
     void setIsForcedNeverSilent(bool isForcedNeverSilent) { _isForcedNeverSilent = isForcedNeverSilent; }