Merge pull request #15928 from AndrewMeadows/fix-safe-landing-003

BUGZ-942: fix SafeLanding to not accidentally "neglect to track" incoming entity packet when connecting to new domain

interface/src/Application.cpp

@@ -5986,6 +5986,7 @@ void Application::resetPhysicsReadyInformation() {
     _gpuTextureMemSizeStabilityCount = 0;
     _gpuTextureMemSizeAtLastCheck = 0;
     _physicsEnabled = false;
+    _octreeProcessor.stopSafeLanding();
 }
 
 void Application::reloadResourceCaches() {
@@ -6245,6 +6246,7 @@ void Application::tryToEnablePhysics() {
         // We keep physics disabled until we've received a full scene and everything near the avatar in that
         // scene is ready to compute its collision shape.
         if (getMyAvatar()->isReadyForPhysics()) {
+            _octreeProcessor.resetSafeLanding();
             _physicsEnabled = true;
             setIsInterstitialMode(false);
             getMyAvatar()->updateMotionBehaviorFromMenu();
@@ -6980,13 +6982,16 @@ int Application::sendNackPackets() {
 }
 
 void Application::queryOctree(NodeType_t serverType, PacketType packetType) {
-
     if (!_settingsLoaded) {
         return; // bail early if settings are not loaded
     }
 
     const bool isModifiedQuery = !_physicsEnabled;
     if (isModifiedQuery) {
+        if (!_octreeProcessor.safeLandingIsActive()) {
+            // don't send the octreeQuery until SafeLanding knows it has started
+            return;
+        }
         // Create modified view that is a simple sphere.
         bool interstitialModeEnabled = DependencyManager::get<NodeList>()->getDomainHandler().getInterstitialModeEnabled();
 
@@ -7209,7 +7214,6 @@ void Application::clearDomainOctreeDetails(bool clearAll) {
 
 void Application::domainURLChanged(QUrl domainURL) {
     // disable physics until we have enough information about our new location to not cause craziness.
-    resetPhysicsReadyInformation();
     setIsServerlessMode(domainURL.scheme() != URL_SCHEME_HIFI);
     if (isServerlessMode()) {
         loadServerlessDomain(domainURL);
@@ -7219,7 +7223,6 @@ void Application::domainURLChanged(QUrl domainURL) {
 
 void Application::goToErrorDomainURL(QUrl errorDomainURL) {
     // disable physics until we have enough information about our new location to not cause craziness.
-    resetPhysicsReadyInformation();
     setIsServerlessMode(errorDomainURL.scheme() != URL_SCHEME_HIFI);
     if (isServerlessMode()) {
         loadErrorDomain(errorDomainURL);
@@ -7236,12 +7239,12 @@ void Application::resettingDomain() {
 void Application::nodeAdded(SharedNodePointer node) {
     if (node->getType() == NodeType::EntityServer) {
         if (_failedToConnectToEntityServer && !_entityServerConnectionTimer.isActive()) {
-            _failedToConnectToEntityServer = false;
             _octreeProcessor.stopSafeLanding();
-            _octreeProcessor.startSafeLanding();
+            _failedToConnectToEntityServer = false;
         } else if (_entityServerConnectionTimer.isActive()) {
             _entityServerConnectionTimer.stop();
         }
+        _octreeProcessor.startSafeLanding();
         _entityServerConnectionTimer.setInterval(ENTITY_SERVER_CONNECTION_TIMEOUT);
         _entityServerConnectionTimer.start();
     }

interface/src/octree/OctreePacketProcessor.cpp

@@ -114,7 +114,11 @@ void OctreePacketProcessor::processPacket(QSharedPointer<ReceivedMessage> messag
                 if (renderer) {
                     renderer->processDatagram(*message, sendingNode);
                     if (_safeLanding && _safeLanding->isTracking()) {
-                        _safeLanding->addToSequence(renderer->getLastOctreeMessageSequence());
+                        OCTREE_PACKET_SEQUENCE thisSequence = renderer->getLastOctreeMessageSequence();
+                        _safeLanding->addToSequence(thisSequence);
+                        if (_safeLandingSequenceStart == SafeLanding::INVALID_SEQUENCE) {
+                            _safeLandingSequenceStart = thisSequence;
+                        }
                     }
                 }
             }
@@ -124,8 +128,8 @@ void OctreePacketProcessor::processPacket(QSharedPointer<ReceivedMessage> messag
             // Read sequence #
             OCTREE_PACKET_SEQUENCE completionNumber;
             message->readPrimitive(&completionNumber);
-            if (_safeLanding) {
+            if (_safeLanding && _safeLanding->isTracking()) {
-                _safeLanding->finishSequence(0, completionNumber);
+                _safeLanding->finishSequence(_safeLandingSequenceStart, completionNumber);
             }
         } break;
 
@@ -153,6 +157,13 @@ void OctreePacketProcessor::stopSafeLanding() {
     }
 }
 
+void OctreePacketProcessor::resetSafeLanding() {
+    if (_safeLanding) {
+        _safeLanding->reset();
+    }
+    _safeLandingSequenceStart = SafeLanding::INVALID_SEQUENCE;
+}
+
 bool OctreePacketProcessor::safeLandingIsActive() const {
     return _safeLanding && _safeLanding->isTracking();
 }

interface/src/octree/OctreePacketProcessor.h

@@ -28,6 +28,7 @@ public:
     void startSafeLanding();
     void updateSafeLanding();
     void stopSafeLanding();
+    void resetSafeLanding();
     bool safeLandingIsActive() const;
     bool safeLandingIsComplete() const;
 
@@ -43,6 +44,7 @@ private slots:
     void handleOctreePacket(QSharedPointer<ReceivedMessage> message, SharedNodePointer senderNode);
 
 private:
+    OCTREE_PACKET_SEQUENCE _safeLandingSequenceStart { SafeLanding::INVALID_SEQUENCE };
     std::unique_ptr<SafeLanding> _safeLanding;
 };
 #endif  // hifi_OctreePacketProcessor_h

interface/src/octree/SafeLanding.cpp

@@ -17,7 +17,6 @@
 #include "InterfaceLogging.h"
 #include "Application.h"
 
-const int SafeLanding::SEQUENCE_MODULO = std::numeric_limits<OCTREE_PACKET_SEQUENCE>::max() + 1;
 
 CalculateEntityLoadingPriority SafeLanding::entityLoadingOperatorElevateCollidables = [](const EntityItem& entityItem) {
     const int COLLIDABLE_ENTITY_PRIORITY = 10.0f;
@@ -25,8 +24,8 @@ CalculateEntityLoadingPriority SafeLanding::entityLoadingOperatorElevateCollidab
 };
 
 namespace {
-    template<typename T> bool lessThanWraparound(int a, int b) {
+    template<typename T> bool lessThanWraparound(int32_t a, int32_t b) {
-        constexpr int MAX_T_VALUE = std::numeric_limits<T>::max();
+        constexpr int32_t MAX_T_VALUE = std::numeric_limits<T>::max();
         if (b <= a) {
             b += MAX_T_VALUE;
         }
@@ -34,7 +33,7 @@ namespace {
     }
 }
 
-bool SafeLanding::SequenceLessThan::operator()(const int& a, const int& b) const {
+bool SafeLanding::SequenceLessThan::operator()(const OCTREE_PACKET_SEQUENCE& a, const OCTREE_PACKET_SEQUENCE& b) const {
     return lessThanWraparound<OCTREE_PACKET_SEQUENCE>(a, b);
 }
 
@@ -46,8 +45,8 @@ void SafeLanding::startTracking(QSharedPointer<EntityTreeRenderer> entityTreeRen
             _entityTreeRenderer = entityTreeRenderer;
             _trackedEntities.clear();
             _maxTrackedEntityCount = 0;
-            _initialStart = INVALID_SEQUENCE;
+            _sequenceStart = SafeLanding::INVALID_SEQUENCE;
-            _initialEnd = INVALID_SEQUENCE;
+            _sequenceEnd = SafeLanding::INVALID_SEQUENCE;
             _sequenceNumbers.clear();
             _trackingEntities = true;
             _startTime = usecTimestampNow();
@@ -72,7 +71,7 @@ void SafeLanding::addTrackedEntity(const EntityItemID& entityID) {
             if (entity && !entity->isLocalEntity() && entity->getCreated() < _startTime) {
                 _trackedEntities.emplace(entityID, entity);
 
-                int trackedEntityCount = (int)_trackedEntities.size();
+                int32_t trackedEntityCount = (int32_t)_trackedEntities.size();
                 if (trackedEntityCount > _maxTrackedEntityCount) {
                     _maxTrackedEntityCount = trackedEntityCount;
                     _trackedEntityStabilityCount = 0;
@@ -87,15 +86,15 @@ void SafeLanding::deleteTrackedEntity(const EntityItemID& entityID) {
     _trackedEntities.erase(entityID);
 }
 
-void SafeLanding::finishSequence(int first, int last) {
+void SafeLanding::finishSequence(OCTREE_PACKET_SEQUENCE first, OCTREE_PACKET_SEQUENCE last) {
     Locker lock(_lock);
     if (_trackingEntities) {
-        _initialStart = first;
+        _sequenceStart = first;
-        _initialEnd = last;
+        _sequenceEnd = last;
     }
 }
 
-void SafeLanding::addToSequence(int sequenceNumber) {
+void SafeLanding::addToSequence(OCTREE_PACKET_SEQUENCE sequenceNumber) {
     Locker lock(_lock);
     _sequenceNumbers.insert(sequenceNumber);
 }
@@ -135,14 +134,13 @@ void SafeLanding::updateTracking() {
 
     if (_trackedEntities.empty()) {
         // no more tracked entities --> check sequenceNumbers
-        if (_initialStart != INVALID_SEQUENCE) {
+        if (_sequenceStart != SafeLanding::INVALID_SEQUENCE) {
             bool shouldStop = false;
             {
                 Locker lock(_lock);
-                int sequenceSize = _initialStart <= _initialEnd ? _initialEnd - _initialStart:
+                auto sequenceSize = _sequenceEnd - _sequenceStart; // this works even in rollover case
-                    _initialEnd + SEQUENCE_MODULO - _initialStart;
+                auto startIter = _sequenceNumbers.find(_sequenceStart);
-                auto startIter = _sequenceNumbers.find(_initialStart);
+                auto endIter = _sequenceNumbers.find(_sequenceEnd - 1);
-                auto endIter = _sequenceNumbers.find(_initialEnd - 1);
 
                 bool missingSequenceNumbers = qApp->isMissingSequenceNumbers();
                 shouldStop = (sequenceSize == 0 ||
@@ -159,6 +157,7 @@ void SafeLanding::updateTracking() {
 
 void SafeLanding::stopTracking() {
     Locker lock(_lock);
+    if (_trackingEntities) {
         _trackingEntities = false;
         if (_entityTreeRenderer) {
             auto entityTree = _entityTreeRenderer->getTree();
@@ -169,21 +168,30 @@ void SafeLanding::stopTracking() {
             _entityTreeRenderer.reset();
         }
         EntityTreeRenderer::setEntityLoadingPriorityFunction(_prevEntityLoadingPriorityOperator);
+    }
+}
+
+void SafeLanding::reset() {
+    _trackingEntities = false;
+    _trackedEntities.clear();
+    _maxTrackedEntityCount = 0;
+    _sequenceStart = SafeLanding::INVALID_SEQUENCE;
+    _sequenceEnd = SafeLanding::INVALID_SEQUENCE;
 }
 
 bool SafeLanding::trackingIsComplete() const {
-    return !_trackingEntities && (_initialStart != INVALID_SEQUENCE);
+    return !_trackingEntities && (_sequenceStart != SafeLanding::INVALID_SEQUENCE);
 }
 
 float SafeLanding::loadingProgressPercentage() {
     Locker lock(_lock);
-    static const int MINIMUM_TRACKED_ENTITY_STABILITY_COUNT = 15;
 
     float entityReadyPercentage = 0.0f;
     if (_maxTrackedEntityCount > 0) {
         entityReadyPercentage = ((_maxTrackedEntityCount - _trackedEntities.size()) / (float)_maxTrackedEntityCount);
     }
 
+    constexpr int32_t MINIMUM_TRACKED_ENTITY_STABILITY_COUNT = 15;
     if (_trackedEntityStabilityCount < MINIMUM_TRACKED_ENTITY_STABILITY_COUNT) {
         entityReadyPercentage *= 0.20f;
     }
@@ -226,20 +234,24 @@ bool SafeLanding::isEntityPhysicsReady(const EntityItemPointer& entity) {
 }
 
 void SafeLanding::debugDumpSequenceIDs() const {
-    int p = -1;
     qCDebug(interfaceapp) << "Sequence set size:" << _sequenceNumbers.size();
-    for (auto s: _sequenceNumbers) {
+
-        if (p == -1) {
+    auto itr = _sequenceNumbers.begin();
-            p = s;
+    OCTREE_PACKET_SEQUENCE p = SafeLanding::INVALID_SEQUENCE;
-            qCDebug(interfaceapp) << "First:" << s;
+    if (itr != _sequenceNumbers.end()) {
-        } else {
+        p = (*itr);
+        qCDebug(interfaceapp) << "First:" << (int32_t)p;
+        ++itr;
+        while (itr != _sequenceNumbers.end()) {
+            OCTREE_PACKET_SEQUENCE s = *itr;
             if (s != p + 1) {
-                qCDebug(interfaceapp) << "Gap from" << p << "to" << s << "(exclusive)";
+                qCDebug(interfaceapp) << "Gap from" << (int32_t)p << "to" << (int32_t)s << "(exclusive)";
                 p = s;
             }
+            ++itr;
         }
-    }
+        if (p != SafeLanding::INVALID_SEQUENCE) {
-    if (p != -1) {
             qCDebug(interfaceapp) << "Last:" << p;
         }
+    }
 }

interface/src/octree/SafeLanding.h

@@ -17,6 +17,8 @@
 #include <QtCore/QObject>
 #include <QtCore/QSharedPointer>
 
+#include <set>
+
 #include "EntityItem.h"
 #include "EntityDynamicInterface.h"
 
@@ -27,14 +29,18 @@ class EntityItemID;
 
 class SafeLanding : public QObject {
 public:
+    static constexpr OCTREE_PACKET_SEQUENCE MAX_SEQUENCE = std::numeric_limits<OCTREE_PACKET_SEQUENCE>::max();
+    static constexpr OCTREE_PACKET_SEQUENCE INVALID_SEQUENCE = MAX_SEQUENCE; // not technically invalid, but close enough
+
     void startTracking(QSharedPointer<EntityTreeRenderer> entityTreeRenderer);
     void updateTracking();
     void stopTracking();
+    void reset();
     bool isTracking() const { return _trackingEntities; }
     bool trackingIsComplete() const;
 
-    void finishSequence(int first, int last);  // 'last' exclusive.
+    void finishSequence(OCTREE_PACKET_SEQUENCE first, OCTREE_PACKET_SEQUENCE last);  // 'last' exclusive.
-    void addToSequence(int sequenceNumber);
+    void addToSequence(OCTREE_PACKET_SEQUENCE sequenceNumber);
     float loadingProgressPercentage();
 
 private slots:
@@ -52,24 +58,22 @@ private:
     using EntityMap = std::map<EntityItemID, EntityItemPointer>;
     EntityMap _trackedEntities;
 
-    static constexpr int INVALID_SEQUENCE = -1;
+    OCTREE_PACKET_SEQUENCE _sequenceStart { INVALID_SEQUENCE };
-    int _initialStart { INVALID_SEQUENCE };
+    OCTREE_PACKET_SEQUENCE _sequenceEnd { INVALID_SEQUENCE };
-    int _initialEnd { INVALID_SEQUENCE };
+    int32_t _maxTrackedEntityCount { 0 };
-    int _maxTrackedEntityCount { 0 };
+    int32_t _trackedEntityStabilityCount { 0 };
-    int _trackedEntityStabilityCount { 0 };
 
     quint64 _startTime { 0 };
 
     struct SequenceLessThan {
-        bool operator()(const int& a, const int& b) const;
+        bool operator()(const OCTREE_PACKET_SEQUENCE& a, const OCTREE_PACKET_SEQUENCE& b) const;
     };
 
-    std::set<int, SequenceLessThan> _sequenceNumbers;
+    using SequenceSet = std::set<OCTREE_PACKET_SEQUENCE, SequenceLessThan>;
+    SequenceSet _sequenceNumbers;
 
     static CalculateEntityLoadingPriority entityLoadingOperatorElevateCollidables;
     CalculateEntityLoadingPriority _prevEntityLoadingPriorityOperator { nullptr };
-
-    static const int SEQUENCE_MODULO;
 };
 
 #endif  // hifi_SafeLanding_h