From f133713d0e2dabb114bde522e8d4defc7362d542 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Fri, 21 Apr 2017 10:21:34 -0700 Subject: [PATCH] CR feedback --- interface/src/avatar/Avatar.cpp | 5 +++-- libraries/avatars/src/AvatarData.cpp | 2 +- libraries/avatars/src/AvatarData.h | 2 +- .../src/model-networking/ModelCache.h | 1 + libraries/networking/src/ReceivedMessage.cpp | 1 - libraries/networking/src/ReceivedMessage.h | 2 +- libraries/networking/src/ResourceCache.cpp | 12 +++++++----- libraries/networking/src/ResourceCache.h | 6 +++++- libraries/render-utils/src/Model.h | 1 + 9 files changed, 20 insertions(+), 12 deletions(-) diff --git a/interface/src/avatar/Avatar.cpp b/interface/src/avatar/Avatar.cpp index 9d79a40c11..0a4cdefef1 100644 --- a/interface/src/avatar/Avatar.cpp +++ b/interface/src/avatar/Avatar.cpp @@ -1112,8 +1112,9 @@ void Avatar::setSkeletonModelURL(const QUrl& skeletonModelURL) { void Avatar::setModelURLFinished(bool success) { if (!success && _skeletonModelURL != AvatarData::defaultFullAvatarModelUrl()) { - const int MAX_SKELETON_DOWNLOAD_ATTEMPTS = 4; // NOTE: must be less than MAX_ATTEMPTS in ResourceCache.cpp - if (_skeletonModel->getResourceDownloadAttempts() > MAX_SKELETON_DOWNLOAD_ATTEMPTS) { + const int MAX_SKELETON_DOWNLOAD_ATTEMPTS = 4; // NOTE: we don't want to be as generous as ResourceCache is, we only want 4 attempts + if (_skeletonModel->getResourceDownloadAttemptsRemaining() <= 0 || + _skeletonModel->getResourceDownloadAttempts() > MAX_SKELETON_DOWNLOAD_ATTEMPTS) { qCWarning(interfaceapp) << "Using default after failing to load Avatar model: " << _skeletonModelURL; // call _skeletonModel.setURL, but leave our copy of _skeletonModelURL alone. This is so that // we don't redo this every time we receive an identity packet from the avatar with the bad url. diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index b904c2b6e8..f541610ae9 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1454,7 +1454,7 @@ QStringList AvatarData::getJointNames() const { return _jointNames; } -void AvatarData::parseAvatarIdentityPacket(QSharedPointer message, Identity& identityOut, quint64& messageNumberOut) { +void AvatarData::parseAvatarIdentityPacket(const QSharedPointer& message, Identity& identityOut, quint64& messageNumberOut) { const QByteArray& data = message->getMessage(); messageNumberOut = message->getMessageNumber(); QDataStream packetStream(data); diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 0eb22fd938..2ffa3c020c 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -532,7 +532,7 @@ public: AvatarEntityMap avatarEntityData; }; - static void parseAvatarIdentityPacket(QSharedPointer message, Identity& identityOut, quint64& messageNumberOut); + static void parseAvatarIdentityPacket(const QSharedPointer& message, Identity& identityOut, quint64& messageNumberOut); // identityChanged returns true if identity has changed, false otherwise. // displayNameChanged returns true if displayName has changed, false otherwise. diff --git a/libraries/model-networking/src/model-networking/ModelCache.h b/libraries/model-networking/src/model-networking/ModelCache.h index 67d57eab19..4c68e1b6c3 100644 --- a/libraries/model-networking/src/model-networking/ModelCache.h +++ b/libraries/model-networking/src/model-networking/ModelCache.h @@ -113,6 +113,7 @@ public: QUrl getURL() const { return (bool)_resource ? _resource->getURL() : QUrl(); } int getResourceDownloadAttempts() { return _resource ? _resource->getDownloadAttempts() : 0; } + int getResourceDownloadAttemptsRemaining() { return _resource ? _resource->getDownloadAttemptsRemaining() : 0; } private: void startWatching(); diff --git a/libraries/networking/src/ReceivedMessage.cpp b/libraries/networking/src/ReceivedMessage.cpp index 5c8239f227..ecf7eb9fde 100644 --- a/libraries/networking/src/ReceivedMessage.cpp +++ b/libraries/networking/src/ReceivedMessage.cpp @@ -27,7 +27,6 @@ ReceivedMessage::ReceivedMessage(const NLPacketList& packetList) _packetType(packetList.getType()), _packetVersion(packetList.getVersion()), _senderSockAddr(packetList.getSenderSockAddr()), - _isComplete(true), _messageNumber(packetList.getMessageNumber()) { } diff --git a/libraries/networking/src/ReceivedMessage.h b/libraries/networking/src/ReceivedMessage.h index d154794719..ba61ef7044 100644 --- a/libraries/networking/src/ReceivedMessage.h +++ b/libraries/networking/src/ReceivedMessage.h @@ -101,7 +101,7 @@ private: std::atomic _isComplete { true }; std::atomic _failed { false }; - udt::Packet::MessageNumber _messageNumber { 0 }; + const udt::Packet::MessageNumber _messageNumber; // only settable on construction }; diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index ed0d5fd20b..1e09bd4608 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -621,8 +621,6 @@ void Resource::init() { } } -const int MAX_ATTEMPTS = 8; - void Resource::attemptRequest() { _startedLoading = true; @@ -724,14 +722,17 @@ void Resource::handleReplyFinished() { } else { switch (result) { case ResourceRequest::Result::Timeout: { - qCDebug(networking) << "Timed out loading" << _url << "received" << _bytesReceived << "total" << _bytesTotal << "attempt:" << _attempts << "of" << MAX_ATTEMPTS; + qCDebug(networking) << "Timed out loading" << _url << "received" << _bytesReceived << "total" << _bytesTotal + << "attempt:" << _attempts << "attemptsRemaining:" << _attemptsRemaining; // Fall through to other cases } case ResourceRequest::Result::ServerUnavailable: { - qCDebug(networking) << "Server Unavailable loading" << _url << "attempt:" << _attempts << "of" << MAX_ATTEMPTS; + qCDebug(networking) << "Server Unavailable loading" << _url << "attempt:" << _attempts << "attemptsRemaining:" << _attemptsRemaining; // retry with increasing delays const int BASE_DELAY_MS = 1000; if (_attempts++ < MAX_ATTEMPTS) { + _attemptsRemaining--; + auto waitTime = BASE_DELAY_MS * (int)pow(2.0, _attempts); qCDebug(networking).noquote() << "Server unavailable for" << _url << "- may retry in" << waitTime << "ms" @@ -743,7 +744,8 @@ void Resource::handleReplyFinished() { // fall through to final failure } default: { - qCDebug(networking) << "Error loading " << _url << "attempt:" << _attempts << "of" << MAX_ATTEMPTS; + _attemptsRemaining = 0; + qCDebug(networking) << "Error loading " << _url << "attempt:" << _attempts << "attemptsRemaining:" << _attemptsRemaining; auto error = (result == ResourceRequest::Timeout) ? QNetworkReply::TimeoutError : QNetworkReply::UnknownNetworkError; emit failed(error); diff --git a/libraries/networking/src/ResourceCache.h b/libraries/networking/src/ResourceCache.h index df21a296a6..8531560d99 100644 --- a/libraries/networking/src/ResourceCache.h +++ b/libraries/networking/src/ResourceCache.h @@ -396,7 +396,7 @@ public: const QUrl& getURL() const { return _url; } int getDownloadAttempts() { return _attempts; } - + int getDownloadAttemptsRemaining() { return _attemptsRemaining; } signals: /// Fired when the resource begins downloading. @@ -477,6 +477,10 @@ private: qint64 _bytesTotal{ 0 }; qint64 _bytes{ 0 }; int _attempts{ 0 }; + + const int MAX_ATTEMPTS = 8; + + int _attemptsRemaining { MAX_ATTEMPTS }; bool _isInScript{ false }; }; diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h index 3e67f2a296..5899ccf6b5 100644 --- a/libraries/render-utils/src/Model.h +++ b/libraries/render-utils/src/Model.h @@ -253,6 +253,7 @@ public: void renderDebugMeshBoxes(gpu::Batch& batch); int getResourceDownloadAttempts() { return _renderWatcher.getResourceDownloadAttempts(); } + int getResourceDownloadAttemptsRemaining() { return _renderWatcher.getResourceDownloadAttemptsRemaining(); } public slots: void loadURLFinished(bool success);