Merge pull request #6418 from birarda/test-ds-settings-fail

move keepalive pings transparently to NL

assignment-client/src/Agent.cpp

@@ -112,7 +112,6 @@ void Agent::handleAudioPacket(QSharedPointer<NLPacket> packet) {
 }
 
 const QString AGENT_LOGGING_NAME = "agent";
-const int PING_INTERVAL = 1000;
 
 void Agent::run() {
     ThreadedAssignment::commonInit(AGENT_LOGGING_NAME, NodeType::Agent);
@@ -134,10 +133,6 @@ void Agent::run() {
                                                  << NodeType::MessagesMixer
                                                 );
 
-    _pingTimer = new QTimer(this);
-    connect(_pingTimer, SIGNAL(timeout()), SLOT(sendPingRequests()));
-    _pingTimer->start(PING_INTERVAL);
-
     // figure out the URL for the script for this agent assignment
     QUrl scriptURL;
     if (_payload.isEmpty())  {
@@ -393,28 +388,6 @@ void Agent::aboutToFinish() {
         _scriptEngine->stop();
     }
 
-    if (_pingTimer) {
-        _pingTimer->stop();
-        delete _pingTimer;
-    }
-
     // our entity tree is going to go away so tell that to the EntityScriptingInterface
     DependencyManager::get<EntityScriptingInterface>()->setEntityTree(NULL);
 }
-
-void Agent::sendPingRequests() {
-    auto nodeList = DependencyManager::get<NodeList>();
-
-    nodeList->eachMatchingNode([](const SharedNodePointer& node)->bool {
-        switch (node->getType()) {
-        case NodeType::AvatarMixer:
-        case NodeType::AudioMixer:
-        case NodeType::EntityServer:
-            return true;
-        default:
-            return false;
-        }
-    }, [nodeList](const SharedNodePointer& node) {
-        nodeList->sendPacket(nodeList->constructPingPacket(), *node);
-    });
-}

assignment-client/src/Agent.h

@@ -58,14 +58,12 @@ private slots:
     void handleAudioPacket(QSharedPointer<NLPacket> packet);
     void handleOctreePacket(QSharedPointer<NLPacket> packet, SharedNodePointer senderNode);
     void handleJurisdictionPacket(QSharedPointer<NLPacket> packet, SharedNodePointer senderNode);
-    void sendPingRequests();
     void processAgentAvatarAndAudio(float deltaTime);
 
 private:
     std::unique_ptr<ScriptEngine> _scriptEngine;
     EntityEditPacketSender _entityEditSender;
     EntityTreeHeadlessViewer _entityViewer;
-    QTimer* _pingTimer;
     
     MixedAudioStream _receivedAudioStream;
     float _lastReceivedAudioLoudness;

interface/src/Application.cpp

@@ -1004,10 +1004,6 @@ void Application::initializeGL() {
     connect(&_octreeProcessor, &OctreePacketProcessor::packetVersionMismatch, this, &Application::notifyPacketVersionMismatch);
     _entityEditSender.initialize(_enableProcessOctreeThread);
 
-    // call our timer function every second
-    connect(&pingTimer, &QTimer::timeout, this, &Application::ping);
-    pingTimer.start(1000);
-
     _idleLoopStdev.reset();
 
     // update before the first render
@@ -2148,13 +2144,6 @@ bool Application::acceptSnapshot(const QString& urlString) {
     return true;
 }
 
-//  Every second, send a ping, if menu item is checked.
-void Application::ping() {
-    if (Menu::getInstance()->isOptionChecked(MenuOption::TestPing)) {
-        DependencyManager::get<NodeList>()->sendPingPackets();
-    }
-}
-
 void Application::idle(uint64_t now) {
     if (_aboutToQuit) {
         return; // bail early, nothing to do here.

interface/src/Application.h

@@ -306,7 +306,6 @@ public slots:
     
 private slots:
     void clearDomainOctreeDetails();
-    void ping();
     void idle(uint64_t now);
     void aboutToQuit();
 

interface/src/Menu.cpp

@@ -505,7 +505,6 @@ Menu::Menu() {
     addCheckableActionToQMenuAndActionHash(perfTimerMenu, MenuOption::ExpandOtherAvatarTiming, 0, false);
     addCheckableActionToQMenuAndActionHash(perfTimerMenu, MenuOption::ExpandPaintGLTiming, 0, false);
 
-    addCheckableActionToQMenuAndActionHash(timingMenu, MenuOption::TestPing, 0, true);
     addCheckableActionToQMenuAndActionHash(timingMenu, MenuOption::FrameTimer);
     addActionToQMenuAndActionHash(timingMenu, MenuOption::RunTimingTests, 0, qApp, SLOT(runTests()));
     addCheckableActionToQMenuAndActionHash(timingMenu, MenuOption::PipelineWarnings);

interface/src/Menu.h

@@ -281,7 +281,6 @@ namespace MenuOption {
     const QString Stats = "Stats";
     const QString StopAllScripts = "Stop All Scripts";
     const QString SuppressShortTimings = "Suppress Timings Less than 10ms";
-    const QString TestPing = "Test Ping";
     const QString ThirdPerson = "Third Person";
     const QString ThreePointCalibration = "3 Point Calibration";
     const QString ThrottleFPSIfNotFocus = "Throttle FPS If Not Focus"; // FIXME - this value duplicated in Basic2DWindowOpenGLDisplayPlugin.cpp

interface/src/ui/Stats.cpp

@@ -127,36 +127,30 @@ void Stats::updateStats(bool force) {
     STAT_UPDATE_FLOAT(mbpsOut, (float)bandwidthRecorder->getCachedTotalAverageOutputKilobitsPerSecond() / 1000.0f, 0.01f);
 
     // Second column: ping
-    if (Menu::getInstance()->isOptionChecked(MenuOption::TestPing)) {
+    SharedNodePointer audioMixerNode = nodeList->soloNodeOfType(NodeType::AudioMixer);
-        SharedNodePointer audioMixerNode = nodeList->soloNodeOfType(NodeType::AudioMixer);
+    SharedNodePointer avatarMixerNode = nodeList->soloNodeOfType(NodeType::AvatarMixer);
-        SharedNodePointer avatarMixerNode = nodeList->soloNodeOfType(NodeType::AvatarMixer);
+    SharedNodePointer assetServerNode = nodeList->soloNodeOfType(NodeType::AssetServer);
-        SharedNodePointer assetServerNode = nodeList->soloNodeOfType(NodeType::AssetServer);
+    STAT_UPDATE(audioPing, audioMixerNode ? audioMixerNode->getPingMs() : -1);
-        STAT_UPDATE(audioPing, audioMixerNode ? audioMixerNode->getPingMs() : -1);
+    STAT_UPDATE(avatarPing, avatarMixerNode ? avatarMixerNode->getPingMs() : -1);
-        STAT_UPDATE(avatarPing, avatarMixerNode ? avatarMixerNode->getPingMs() : -1);
+    STAT_UPDATE(assetPing, assetServerNode ? assetServerNode->getPingMs() : -1);
-        STAT_UPDATE(assetPing, assetServerNode ? assetServerNode->getPingMs() : -1);
-
-        //// Now handle entity servers, since there could be more than one, we average their ping times
-        int totalPingOctree = 0;
-        int octreeServerCount = 0;
-        int pingOctreeMax = 0;
-        nodeList->eachNode([&](const SharedNodePointer& node) {
-            // TODO: this should also support entities
-            if (node->getType() == NodeType::EntityServer) {
-                totalPingOctree += node->getPingMs();
-                octreeServerCount++;
-                if (pingOctreeMax < node->getPingMs()) {
-                    pingOctreeMax = node->getPingMs();
-                }
-            }
-        });
-        
-        // update the entities ping with the average for all connected entity servers
-        STAT_UPDATE(entitiesPing, octreeServerCount ? totalPingOctree / octreeServerCount : -1);
-    } else {
-        // -2 causes the QML to hide the ping column
-        STAT_UPDATE(audioPing, -2);
-    }
     
+    //// Now handle entity servers, since there could be more than one, we average their ping times
+    int totalPingOctree = 0;
+    int octreeServerCount = 0;
+    int pingOctreeMax = 0;
+    nodeList->eachNode([&](const SharedNodePointer& node) {
+        // TODO: this should also support entities
+        if (node->getType() == NodeType::EntityServer) {
+            totalPingOctree += node->getPingMs();
+            octreeServerCount++;
+            if (pingOctreeMax < node->getPingMs()) {
+                pingOctreeMax = node->getPingMs();
+            }
+        }
+    });
+    
+    // update the entities ping with the average for all connected entity servers
+    STAT_UPDATE(entitiesPing, octreeServerCount ? totalPingOctree / octreeServerCount : -1);
 
     // Third column, avatar stats
     MyAvatar* myAvatar = avatarManager->getMyAvatar();

libraries/networking/src/LimitedNodeList.cpp

@@ -700,22 +700,6 @@ void LimitedNodeList::sendSTUNRequest() {
     _nodeSocket.writeDatagram(stunRequestPacket, sizeof(stunRequestPacket), _stunSockAddr);
 }
 
-void LimitedNodeList::sendPingPackets() {
-    eachMatchingNode([](const SharedNodePointer& node)->bool {
-        switch (node->getType()) {
-            case NodeType::AvatarMixer:
-            case NodeType::AudioMixer:
-            case NodeType::EntityServer:
-            case NodeType::AssetServer:
-                return true;
-            default:
-                return false;
-        }
-    }, [&](const SharedNodePointer& node) {
-        sendPacket(constructPingPacket(), *node);
-    });
-}
-
 void LimitedNodeList::processSTUNResponse(std::unique_ptr<udt::BasePacket> packet) {
     // check the cookie to make sure this is actually a STUN response
     // and read the first attribute and make sure it is a XOR_MAPPED_ADDRESS

libraries/networking/src/LimitedNodeList.h

@@ -228,7 +228,6 @@ public slots:
 
     void startSTUNPublicSocketUpdate();
     virtual void sendSTUNRequest();
-    void sendPingPackets();
 
     bool killNodeWithUUID(const QUuid& nodeUUID);
 

libraries/networking/src/NodeList.cpp

@@ -32,6 +32,7 @@
 #include "udt/PacketHeaders.h"
 #include "SharedUtil.h"
 
+const int KEEPALIVE_PING_INTERVAL_MS = 1000;
 
 NodeList::NodeList(char newOwnerType, unsigned short socketListenPort, unsigned short dtlsListenPort) :
     LimitedNodeList(socketListenPort, dtlsListenPort),
@@ -87,6 +88,12 @@ NodeList::NodeList(char newOwnerType, unsigned short socketListenPort, unsigned
 
     // anytime we get a new node we will want to attempt to punch to it
     connect(this, &LimitedNodeList::nodeAdded, this, &NodeList::startNodeHolePunch);
+    
+    // setup our timer to send keepalive pings (it's started and stopped on domain connect/disconnect)
+    _keepAlivePingTimer.setInterval(KEEPALIVE_PING_INTERVAL_MS);
+    connect(&_keepAlivePingTimer, &QTimer::timeout, this, &NodeList::sendKeepAlivePings);
+    connect(&_domainHandler, SIGNAL(connectedToDomain(QString)), &_keepAlivePingTimer, SLOT(start()));
+    connect(&_domainHandler, &DomainHandler::disconnectedFromDomain, &_keepAlivePingTimer, &QTimer::stop);
 
     // we definitely want STUN to update our public socket, so call the LNL to kick that off
     startSTUNPublicSocketUpdate();
@@ -632,3 +639,11 @@ void NodeList::activateSocketFromNodeCommunication(QSharedPointer<NLPacket> pack
        flagTimeForConnectionStep(LimitedNodeList::ConnectionStep::SetAudioMixerSocket);
     }
 }
+
+void NodeList::sendKeepAlivePings() {
+    eachMatchingNode([this](const SharedNodePointer& node)->bool {
+        return _nodeTypesOfInterest.contains(node->getType());
+    }, [&](const SharedNodePointer& node) {
+        sendPacket(constructPingPacket(), *node);
+    });
+}

libraries/networking/src/NodeList.h

@@ -85,6 +85,7 @@ public slots:
     void processPingReplyPacket(QSharedPointer<NLPacket> packet, SharedNodePointer sendingNode);
 
     void processICEPingPacket(QSharedPointer<NLPacket> packet);
+    
 signals:
     void limitOfSilentDomainCheckInsReached();
 private slots:
@@ -95,6 +96,8 @@ private slots:
     void handleNodePingTimeout();
 
     void pingPunchForDomainServer();
+    
+    void sendKeepAlivePings();
 private:
     NodeList() : LimitedNodeList(0, 0) { assert(false); } // Not implemented, needed for DependencyManager templates compile
     NodeList(char ownerType, unsigned short socketListenPort = 0, unsigned short dtlsListenPort = 0);
@@ -118,6 +121,7 @@ private:
     int _numNoReplyDomainCheckIns;
     HifiSockAddr _assignmentServerSocket;
     bool _isShuttingDown { false };
+    QTimer _keepAlivePingTimer;
 };
 
 #endif // hifi_NodeList_h