From fcd0d8fa0a8e28f60b52ddab435cda431bf9b8c4 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Sat, 6 May 2017 14:34:28 -0700 Subject: [PATCH 01/35] Fix sped up animations --- interface/src/avatar/MySkeletonModel.cpp | 4 +--- .../src/avatars-renderer/SkeletonModel.cpp | 6 +++--- .../avatars-renderer/src/avatars-renderer/SkeletonModel.h | 1 + libraries/render-utils/src/CauterizedModel.h | 8 ++++---- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/interface/src/avatar/MySkeletonModel.cpp b/interface/src/avatar/MySkeletonModel.cpp index 639c9f003f..1b9aa4dc18 100644 --- a/interface/src/avatar/MySkeletonModel.cpp +++ b/interface/src/avatar/MySkeletonModel.cpp @@ -140,9 +140,6 @@ void MySkeletonModel::updateRig(float deltaTime, glm::mat4 parentTransform) { auto orientation = myAvatar->getLocalOrientation(); _rig->computeMotionAnimationState(deltaTime, position, velocity, orientation, ccState); - // evaluate AnimGraph animation and update jointStates. - Model::updateRig(deltaTime, parentTransform); - Rig::EyeParameters eyeParams; eyeParams.eyeLookAt = lookAt; eyeParams.eyeSaccade = head->getSaccade(); @@ -153,6 +150,7 @@ void MySkeletonModel::updateRig(float deltaTime, glm::mat4 parentTransform) { _rig->updateFromEyeParameters(eyeParams); + // evaluate AnimGraph animation and update jointStates. Parent::updateRig(deltaTime, parentTransform); } diff --git a/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.cpp b/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.cpp index 8fcc859b62..e1e5dc4282 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.cpp @@ -120,7 +120,7 @@ void SkeletonModel::updateRig(float deltaTime, glm::mat4 parentTransform) { } // evaluate AnimGraph animation and update jointStates. - Model::updateRig(deltaTime, parentTransform); + Parent::updateRig(deltaTime, parentTransform); } void SkeletonModel::updateAttitude() { @@ -136,7 +136,7 @@ void SkeletonModel::simulate(float deltaTime, bool fullUpdate) { if (fullUpdate) { setBlendshapeCoefficients(_owningAvatar->getHead()->getSummedBlendshapeCoefficients()); - Model::simulate(deltaTime, fullUpdate); + Parent::simulate(deltaTime, fullUpdate); // let rig compute the model offset glm::vec3 registrationPoint; @@ -144,7 +144,7 @@ void SkeletonModel::simulate(float deltaTime, bool fullUpdate) { setOffset(registrationPoint); } } else { - Model::simulate(deltaTime, fullUpdate); + Parent::simulate(deltaTime, fullUpdate); } if (!isActive() || !_owningAvatar->isMyAvatar()) { diff --git a/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.h b/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.h index 23af04e964..059dd245fd 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.h +++ b/libraries/avatars-renderer/src/avatars-renderer/SkeletonModel.h @@ -23,6 +23,7 @@ using SkeletonModelWeakPointer = std::weak_ptr; /// A skeleton loaded from a model. class SkeletonModel : public CauterizedModel { + using Parent = CauterizedModel; Q_OBJECT public: diff --git a/libraries/render-utils/src/CauterizedModel.h b/libraries/render-utils/src/CauterizedModel.h index 39bcedf639..dcff7bd12d 100644 --- a/libraries/render-utils/src/CauterizedModel.h +++ b/libraries/render-utils/src/CauterizedModel.h @@ -28,10 +28,10 @@ public: const std::unordered_set& getCauterizeBoneSet() const { return _cauterizeBoneSet; } void setCauterizeBoneSet(const std::unordered_set& boneSet) { _cauterizeBoneSet = boneSet; } - void deleteGeometry() override; - bool updateGeometry() override; + void deleteGeometry() override; + bool updateGeometry() override; - void createVisibleRenderItemSet() override; + void createVisibleRenderItemSet() override; void createCollisionRenderItemSet() override; virtual void updateClusterMatrices() override; @@ -41,7 +41,7 @@ public: protected: std::unordered_set _cauterizeBoneSet; - QVector _cauterizeMeshStates; + QVector _cauterizeMeshStates; bool _isCauterized { false }; bool _enableCauterization { false }; }; From 1a27dae884d86862f3923634eeb7fdb4f5b8606f Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 4 May 2017 15:43:44 -0700 Subject: [PATCH 02/35] fix typos: RenderItemsMap not RenderItems --- libraries/render-utils/src/Model.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp index eddda41d5e..acc84646c5 100644 --- a/libraries/render-utils/src/Model.cpp +++ b/libraries/render-utils/src/Model.cpp @@ -573,7 +573,7 @@ bool Model::addToScene(const render::ScenePointer& scene, bool somethingAdded = false; if (_collisionGeometry) { - if (_collisionRenderItems.empty()) { + if (_collisionRenderItemsMap.empty()) { foreach (auto renderItem, _collisionRenderItems) { auto item = scene->allocateID(); auto renderPayload = std::make_shared(renderItem); @@ -583,7 +583,7 @@ bool Model::addToScene(const render::ScenePointer& scene, transaction.resetItem(item, renderPayload); _collisionRenderItemsMap.insert(item, renderPayload); } - somethingAdded = !_collisionRenderItems.empty(); + somethingAdded = !_collisionRenderItemsMap.empty(); } } else { if (_modelMeshRenderItemsMap.empty()) { @@ -632,7 +632,7 @@ void Model::removeFromScene(const render::ScenePointer& scene, render::Transacti transaction.removeItem(item); } _collisionRenderItems.clear(); - _collisionRenderItems.clear(); + _collisionRenderItemsMap.clear(); _addedToScene = false; _renderInfoVertexCount = 0; From 25f5fcb480e0bbc1f18feec38b04b2d619ea606a Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Mon, 8 May 2017 18:53:04 -0700 Subject: [PATCH 03/35] Fixing crash in texture transfer logic, again --- libraries/gpu-gl/CMakeLists.txt | 2 +- libraries/gpu-gl/src/gpu/gl/GLTexture.cpp | 355 +++++++++++++------- libraries/gpu-gl/src/gpu/gl/GLTexture.h | 34 +- libraries/gpu-gl/src/gpu/gl45/GL45Backend.h | 1 - 4 files changed, 247 insertions(+), 145 deletions(-) diff --git a/libraries/gpu-gl/CMakeLists.txt b/libraries/gpu-gl/CMakeLists.txt index 3e3853532a..65130d6d07 100644 --- a/libraries/gpu-gl/CMakeLists.txt +++ b/libraries/gpu-gl/CMakeLists.txt @@ -1,5 +1,5 @@ set(TARGET_NAME gpu-gl) -setup_hifi_library() +setup_hifi_library(Concurrent) link_hifi_libraries(shared gl gpu) if (UNIX) target_link_libraries(${TARGET_NAME} pthread) diff --git a/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp b/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp index 5534419eaa..84dc49deba 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp +++ b/libraries/gpu-gl/src/gpu/gl/GLTexture.cpp @@ -160,8 +160,6 @@ const uvec3 GLVariableAllocationSupport::INITIAL_MIP_TRANSFER_DIMENSIONS { 64, 6 WorkQueue GLVariableAllocationSupport::_transferQueue; WorkQueue GLVariableAllocationSupport::_promoteQueue; WorkQueue GLVariableAllocationSupport::_demoteQueue; -TexturePointer GLVariableAllocationSupport::_currentTransferTexture; -TransferJobPointer GLVariableAllocationSupport::_currentTransferJob; size_t GLVariableAllocationSupport::_frameTexturesCreated { 0 }; #define OVERSUBSCRIBED_PRESSURE_VALUE 0.95f @@ -176,30 +174,19 @@ const uvec3 GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS { 1024, 1024, 1 const size_t GLVariableAllocationSupport::MAX_TRANSFER_SIZE = GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS.x * GLVariableAllocationSupport::MAX_TRANSFER_DIMENSIONS.y * 4; #if THREADED_TEXTURE_BUFFERING -std::shared_ptr TransferJob::_bufferThread { nullptr }; -std::atomic TransferJob::_shutdownBufferingThread { false }; -Mutex TransferJob::_mutex; -TransferJob::VoidLambdaQueue TransferJob::_bufferLambdaQueue; -void TransferJob::startTransferLoop() { - if (_bufferThread) { - return; - } - _shutdownBufferingThread = false; - _bufferThread = std::make_shared([] { - TransferJob::bufferLoop(); +TexturePointer GLVariableAllocationSupport::_currentTransferTexture; +TransferJobPointer GLVariableAllocationSupport::_currentTransferJob; +QThreadPool* TransferJob::_bufferThreadPool { nullptr }; + +void TransferJob::startBufferingThread() { + static std::once_flag once; + std::call_once(once, [&] { + _bufferThreadPool = new QThreadPool(qApp); + _bufferThreadPool->setMaxThreadCount(1); }); } -void TransferJob::stopTransferLoop() { - if (!_bufferThread) { - return; - } - _shutdownBufferingThread = true; - _bufferThread->join(); - _bufferThread.reset(); - _shutdownBufferingThread = false; -} #endif TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t targetMip, uint8_t face, uint32_t lines, uint32_t lineOffset) @@ -233,7 +220,6 @@ TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t t // Buffering can invoke disk IO, so it should be off of the main and render threads _bufferingLambda = [=] { _mipData = _parent._gpuObject.accessStoredMipFace(sourceMip, face)->createView(_transferSize, _transferOffset); - _bufferingCompleted = true; }; _transferLambda = [=] { @@ -243,65 +229,66 @@ TransferJob::TransferJob(const GLTexture& parent, uint16_t sourceMip, uint16_t t } TransferJob::TransferJob(const GLTexture& parent, std::function transferLambda) - : _parent(parent), _bufferingCompleted(true), _transferLambda(transferLambda) { + : _parent(parent), _bufferingRequired(false), _transferLambda(transferLambda) { } TransferJob::~TransferJob() { Backend::updateTextureTransferPendingSize(_transferSize, 0); } - bool TransferJob::tryTransfer() { - // Disable threaded texture transfer for now #if THREADED_TEXTURE_BUFFERING // Are we ready to transfer - if (_bufferingCompleted) { - _transferLambda(); + if (!bufferingCompleted()) { + startBuffering(); + return false; + } +#else + if (_bufferingRequired) { + _bufferingLambda(); + } +#endif + _transferLambda(); + return true; +} + +#if THREADED_TEXTURE_BUFFERING +bool TransferJob::bufferingRequired() const { + if (!_bufferingRequired) { + return false; + } + + // The default state of a QFuture is with status Canceled | Started | Finished, + // so we have to check isCancelled before we check the actual state + if (_bufferingStatus.isCanceled()) { return true; } - startBuffering(); - return false; -#else - if (!_bufferingCompleted) { - _bufferingLambda(); - _bufferingCompleted = true; - } - _transferLambda(); - return true; -#endif + return !_bufferingStatus.isStarted(); } -#if THREADED_TEXTURE_BUFFERING +bool TransferJob::bufferingCompleted() const { + if (!_bufferingRequired) { + return true; + } + + // The default state of a QFuture is with status Canceled | Started | Finished, + // so we have to check isCancelled before we check the actual state + if (_bufferingStatus.isCanceled()) { + return false; + } + + return _bufferingStatus.isFinished(); +} void TransferJob::startBuffering() { - if (_bufferingStarted) { - return; - } - _bufferingStarted = true; - { - Lock lock(_mutex); - _bufferLambdaQueue.push(_bufferingLambda); - } -} - -void TransferJob::bufferLoop() { - while (!_shutdownBufferingThread) { - VoidLambdaQueue workingQueue; - { - Lock lock(_mutex); - _bufferLambdaQueue.swap(workingQueue); - } - - if (workingQueue.empty()) { - QThread::msleep(5); - continue; - } - - while (!workingQueue.empty()) { - workingQueue.front()(); - workingQueue.pop(); - } + if (bufferingRequired()) { + assert(_bufferingStatus.isCanceled()); + _bufferingStatus = QtConcurrent::run(_bufferThreadPool, [=] { + _bufferingLambda(); + }); + assert(!_bufferingStatus.isCanceled()); + assert(_bufferingStatus.isStarted()); } } #endif @@ -316,7 +303,9 @@ GLVariableAllocationSupport::~GLVariableAllocationSupport() { void GLVariableAllocationSupport::addMemoryManagedTexture(const TexturePointer& texturePointer) { _memoryManagedTextures.push_back(texturePointer); - addToWorkQueue(texturePointer); + if (MemoryPressureState::Idle != _memoryPressureState) { + addToWorkQueue(texturePointer); + } } void GLVariableAllocationSupport::addToWorkQueue(const TexturePointer& texturePointer) { @@ -345,10 +334,8 @@ void GLVariableAllocationSupport::addToWorkQueue(const TexturePointer& texturePo break; case MemoryPressureState::Idle: - break; - - default: Q_UNREACHABLE(); + break; } } @@ -364,10 +351,10 @@ WorkQueue& GLVariableAllocationSupport::getActiveWorkQueue() { case MemoryPressureState::Transfer: return _transferQueue; - default: + case MemoryPressureState::Idle: + Q_UNREACHABLE(); break; } - Q_UNREACHABLE(); return empty; } @@ -460,16 +447,11 @@ void GLVariableAllocationSupport::updateMemoryPressure() { } if (newState != _memoryPressureState) { + _memoryPressureState = newState; #if THREADED_TEXTURE_BUFFERING if (MemoryPressureState::Transfer == _memoryPressureState) { - TransferJob::stopTransferLoop(); + TransferJob::startBufferingThread(); } - _memoryPressureState = newState; - if (MemoryPressureState::Transfer == _memoryPressureState) { - TransferJob::startTransferLoop(); - } -#else - _memoryPressureState = newState; #endif // Clear the existing queue _transferQueue = WorkQueue(); @@ -487,49 +469,111 @@ void GLVariableAllocationSupport::updateMemoryPressure() { } } +TexturePointer GLVariableAllocationSupport::getNextWorkQueueItem(WorkQueue& workQueue) { + while (!workQueue.empty()) { + auto workTarget = workQueue.top(); + + auto texture = workTarget.first.lock(); + if (!texture) { + workQueue.pop(); + continue; + } + + // Check whether the resulting texture can actually have work performed + GLTexture* gltexture = Backend::getGPUObject(*texture); + GLVariableAllocationSupport* vartexture = dynamic_cast(gltexture); + switch (_memoryPressureState) { + case MemoryPressureState::Oversubscribed: + if (vartexture->canDemote()) { + return texture; + } + break; + + case MemoryPressureState::Undersubscribed: + if (vartexture->canPromote()) { + return texture; + } + break; + + case MemoryPressureState::Transfer: + if (vartexture->hasPendingTransfers()) { + return texture; + } + break; + + case MemoryPressureState::Idle: + Q_UNREACHABLE(); + break; + } + + // If we got here, then the texture has no work to do in the current state, + // so pop it off the queue and continue + workQueue.pop(); + } + + return TexturePointer(); +} + +void GLVariableAllocationSupport::processWorkQueue(WorkQueue& workQueue) { + if (workQueue.empty()) { + return; + } + + // Get the front of the work queue to perform work + auto texture = getNextWorkQueueItem(workQueue); + if (!texture) { + return; + } + + // Grab the first item off the demote queue + PROFILE_RANGE(render_gpu_gl, __FUNCTION__); + + GLTexture* gltexture = Backend::getGPUObject(*texture); + GLVariableAllocationSupport* vartexture = dynamic_cast(gltexture); + switch (_memoryPressureState) { + case MemoryPressureState::Oversubscribed: + vartexture->demote(); + workQueue.pop(); + addToWorkQueue(texture); + break; + + case MemoryPressureState::Undersubscribed: + vartexture->promote(); + workQueue.pop(); + addToWorkQueue(texture); + break; + + case MemoryPressureState::Transfer: + if (vartexture->executeNextTransfer(texture)) { + workQueue.pop(); + addToWorkQueue(texture); + +#if THREADED_TEXTURE_BUFFERING + // Eagerly start the next buffering job if possible + texture = getNextWorkQueueItem(workQueue); + if (texture) { + gltexture = Backend::getGPUObject(*texture); + vartexture = dynamic_cast(gltexture); + vartexture->executeNextBuffer(texture); + } +#endif + } + break; + + case MemoryPressureState::Idle: + Q_UNREACHABLE(); + break; + } +} + void GLVariableAllocationSupport::processWorkQueues() { if (MemoryPressureState::Idle == _memoryPressureState) { return; } auto& workQueue = getActiveWorkQueue(); - PROFILE_RANGE(render_gpu_gl, __FUNCTION__); - while (!workQueue.empty()) { - auto workTarget = workQueue.top(); - workQueue.pop(); - auto texture = workTarget.first.lock(); - if (!texture) { - continue; - } - - // Grab the first item off the demote queue - GLTexture* gltexture = Backend::getGPUObject(*texture); - GLVariableAllocationSupport* vartexture = dynamic_cast(gltexture); - if (MemoryPressureState::Oversubscribed == _memoryPressureState) { - if (!vartexture->canDemote()) { - continue; - } - vartexture->demote(); - _memoryPressureStateStale = true; - } else if (MemoryPressureState::Undersubscribed == _memoryPressureState) { - if (!vartexture->canPromote()) { - continue; - } - vartexture->promote(); - _memoryPressureStateStale = true; - } else if (MemoryPressureState::Transfer == _memoryPressureState) { - if (!vartexture->hasPendingTransfers()) { - continue; - } - vartexture->executeNextTransfer(texture); - } else { - Q_UNREACHABLE(); - } - - // Reinject into the queue if more work to be done - addToWorkQueue(texture); - break; - } + // Do work on the front of the queue + processWorkQueue(workQueue); if (workQueue.empty()) { _memoryPressureState = MemoryPressureState::Idle; @@ -543,28 +587,83 @@ void GLVariableAllocationSupport::manageMemory() { processWorkQueues(); } +bool GLVariableAllocationSupport::executeNextTransfer(const TexturePointer& currentTexture) { +#if THREADED_TEXTURE_BUFFERING + // If a transfer job is active on the buffering thread, but has not completed it's buffering lambda, + // then we need to exit early, since we don't want to have the transfer job leave scope while it's + // being used in another thread -- See https://highfidelity.fogbugz.com/f/cases/4626 + if (_currentTransferJob && !_currentTransferJob->bufferingCompleted()) { + return false; + } +#endif -void GLVariableAllocationSupport::executeNextTransfer(const TexturePointer& currentTexture) { if (_populatedMip <= _allocatedMip) { +#if THREADED_TEXTURE_BUFFERING + _currentTransferJob.reset(); + _currentTransferTexture.reset(); +#endif + return true; + } + + // If the transfer queue is empty, rebuild it + if (_pendingTransfers.empty()) { + populateTransferQueue(); + } + + bool result = false; + if (!_pendingTransfers.empty()) { +#if THREADED_TEXTURE_BUFFERING + // If there is a current transfer, but it's not the top of the pending transfer queue, then it's an orphan, so we want to abandon it. + if (_currentTransferJob && _currentTransferJob != _pendingTransfers.front()) { + _currentTransferJob.reset(); + } + + if (!_currentTransferJob) { + // Keeping hold of a strong pointer to the transfer job ensures that if the pending transfer queue is rebuilt, the transfer job + // doesn't leave scope, causing a crash in the buffering thread + _currentTransferJob = _pendingTransfers.front(); + + // Keeping hold of a strong pointer during the transfer ensures that the transfer thread cannot try to access a destroyed texture + _currentTransferTexture = currentTexture; + } + + // transfer jobs use asynchronous buffering of the texture data because it may involve disk IO, so we execute a try here to determine if the buffering + // is complete + if (_currentTransferJob->tryTransfer()) { + _pendingTransfers.pop(); + // Once a given job is finished, release the shared pointers keeping them alive + _currentTransferTexture.reset(); + _currentTransferJob.reset(); + result = true; + } +#else + if (_pendingTransfers.front()->tryTransfer()) { + _pendingTransfers.pop(); + result = true; + } +#endif + } + return result; +} + +#if THREADED_TEXTURE_BUFFERING +void GLVariableAllocationSupport::executeNextBuffer(const TexturePointer& currentTexture) { + if (_currentTransferJob && !_currentTransferJob->bufferingCompleted()) { return; } + // If the transfer queue is empty, rebuild it if (_pendingTransfers.empty()) { populateTransferQueue(); } if (!_pendingTransfers.empty()) { - // Keeping hold of a strong pointer during the transfer ensures that the transfer thread cannot try to access a destroyed texture - _currentTransferTexture = currentTexture; - // Keeping hold of a strong pointer to the transfer job ensures that if the pending transfer queue is rebuilt, the transfer job - // doesn't leave scope, causing a crash in the buffering thread - _currentTransferJob = _pendingTransfers.front(); - // transfer jobs use asynchronous buffering of the texture data because it may involve disk IO, so we execute a try here to determine if the buffering - // is complete - if (_currentTransferJob->tryTransfer()) { - _pendingTransfers.pop(); - _currentTransferTexture.reset(); - _currentTransferJob.reset(); + if (!_currentTransferJob) { + _currentTransferJob = _pendingTransfers.front(); + _currentTransferTexture = currentTexture; } + + _currentTransferJob->startBuffering(); } } +#endif diff --git a/libraries/gpu-gl/src/gpu/gl/GLTexture.h b/libraries/gpu-gl/src/gpu/gl/GLTexture.h index 877966f2d9..c6ce2a2495 100644 --- a/libraries/gpu-gl/src/gpu/gl/GLTexture.h +++ b/libraries/gpu-gl/src/gpu/gl/GLTexture.h @@ -8,6 +8,9 @@ #ifndef hifi_gpu_gl_GLTexture_h #define hifi_gpu_gl_GLTexture_h +#include +#include + #include "GLShared.h" #include "GLBackend.h" #include "GLTexelFormat.h" @@ -47,24 +50,19 @@ public: class TransferJob { using VoidLambda = std::function; using VoidLambdaQueue = std::queue; - using ThreadPointer = std::shared_ptr; const GLTexture& _parent; Texture::PixelsPointer _mipData; size_t _transferOffset { 0 }; size_t _transferSize { 0 }; - // Indicates if a transfer from backing storage to interal storage has started - bool _bufferingStarted { false }; - bool _bufferingCompleted { false }; + bool _bufferingRequired { true }; VoidLambda _transferLambda; VoidLambda _bufferingLambda; #if THREADED_TEXTURE_BUFFERING - static Mutex _mutex; - static VoidLambdaQueue _bufferLambdaQueue; - static ThreadPointer _bufferThread; - static std::atomic _shutdownBufferingThread; - static void bufferLoop(); + // Indicates if a transfer from backing storage to interal storage has started + QFuture _bufferingStatus; + static QThreadPool* _bufferThreadPool; #endif public: @@ -75,14 +73,13 @@ public: bool tryTransfer(); #if THREADED_TEXTURE_BUFFERING - static void startTransferLoop(); - static void stopTransferLoop(); + void startBuffering(); + bool bufferingRequired() const; + bool bufferingCompleted() const; + static void startBufferingThread(); #endif private: -#if THREADED_TEXTURE_BUFFERING - void startBuffering(); -#endif void transfer(); }; @@ -100,8 +97,10 @@ protected: static WorkQueue _transferQueue; static WorkQueue _promoteQueue; static WorkQueue _demoteQueue; +#if THREADED_TEXTURE_BUFFERING static TexturePointer _currentTransferTexture; static TransferJobPointer _currentTransferJob; +#endif static const uvec3 INITIAL_MIP_TRANSFER_DIMENSIONS; static const uvec3 MAX_TRANSFER_DIMENSIONS; static const size_t MAX_TRANSFER_SIZE; @@ -109,6 +108,8 @@ protected: static void updateMemoryPressure(); static void processWorkQueues(); + static void processWorkQueue(WorkQueue& workQueue); + static TexturePointer getNextWorkQueueItem(WorkQueue& workQueue); static void addToWorkQueue(const TexturePointer& texture); static WorkQueue& getActiveWorkQueue(); @@ -118,7 +119,10 @@ protected: bool canPromote() const { return _allocatedMip > _minAllocatedMip; } bool canDemote() const { return _allocatedMip < _maxAllocatedMip; } bool hasPendingTransfers() const { return _populatedMip > _allocatedMip; } - void executeNextTransfer(const TexturePointer& currentTexture); +#if THREADED_TEXTURE_BUFFERING + void executeNextBuffer(const TexturePointer& currentTexture); +#endif + bool executeNextTransfer(const TexturePointer& currentTexture); virtual void populateTransferQueue() = 0; virtual void promote() = 0; virtual void demote() = 0; diff --git a/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h b/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h index 8319e61382..fe2761b37d 100644 --- a/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h +++ b/libraries/gpu-gl/src/gpu/gl45/GL45Backend.h @@ -17,7 +17,6 @@ #include #define INCREMENTAL_TRANSFER 0 -#define THREADED_TEXTURE_BUFFERING 1 #define GPU_SSBO_TRANSFORM_OBJECT 1 namespace gpu { namespace gl45 { From 3a20c47f397a62cdced2b11c751cbb89f7c41492 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Tue, 9 May 2017 11:09:39 -0700 Subject: [PATCH 04/35] fix a bug that could cause OBJ models with external mtl references to hang the loading thread --- libraries/fbx/src/OBJReader.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libraries/fbx/src/OBJReader.cpp b/libraries/fbx/src/OBJReader.cpp index 167cb8caac..1445d14d84 100644 --- a/libraries/fbx/src/OBJReader.cpp +++ b/libraries/fbx/src/OBJReader.cpp @@ -273,10 +273,9 @@ std::tuple requestData(QUrl& url) { return std::make_tuple(false, QByteArray()); } - request->send(); - QEventLoop loop; QObject::connect(request, &ResourceRequest::finished, &loop, &QEventLoop::quit); + request->send(); loop.exec(); if (request->getResult() == ResourceRequest::Success) { From a36565d5b2454c6fbcdd25033b82516bd06fe79d Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 12:02:12 -0700 Subject: [PATCH 05/35] Fix possible corruption of ktx cache files When creating a file like this: 1. Create file 2. Write file 3. Close file there is an opening before the file is written and closed where, if the application were to crash (or not finish for any other reason), the file would be left in an undefined state. Instead, this change updates it to do this: 1. Create temp file 2. Write temp file 3. Close temp file 4. Move temp file to final path This ensures that at step 3 we have a valid file, before we rename it. We should never end up with a final file in an undefined state, but it is possible to end up with a temp file that is an undefined state if we, for instance, crash during step 2. --- libraries/networking/src/FileCache.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0a859d511b..0ceda7a745 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -110,10 +110,16 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { return file; } - // write the new file - FILE* saveFile = fopen(filepath.c_str(), "wb"); + // write the data to a temporary file + std::string tmpFilepath = filepath + "_tmp"; + FILE* saveFile = fopen(tmpFilepath.c_str(), "wb"); if (saveFile != nullptr && fwrite(data, metadata.length, 1, saveFile) && fclose(saveFile) == 0) { - file = addFile(std::move(metadata), filepath); + int result = rename(tmpFilepath.c_str(), filepath.c_str()); + if (result == 0) { + file = addFile(std::move(metadata), filepath); + } else { + qCWarning(file_cache, "[%s] Failed to rename %s to %s (%s)", tmpFilepath.c_str(), filepath.c_str(), strerror(errno)); + } } else { qCWarning(file_cache, "[%s] Failed to write %s (%s)", _dirname.c_str(), metadata.key.c_str(), strerror(errno)); errno = 0; From c8e173e15922d7cc23e16ec47b71d7e8ea0671f2 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 14:08:43 -0700 Subject: [PATCH 06/35] Update FileCache to use QSaveFile for atomic writes --- libraries/networking/src/FileCache.cpp | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0ceda7a745..348206a863 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -17,6 +17,7 @@ #include #include +#include #include @@ -110,19 +111,13 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { return file; } - // write the data to a temporary file - std::string tmpFilepath = filepath + "_tmp"; - FILE* saveFile = fopen(tmpFilepath.c_str(), "wb"); - if (saveFile != nullptr && fwrite(data, metadata.length, 1, saveFile) && fclose(saveFile) == 0) { - int result = rename(tmpFilepath.c_str(), filepath.c_str()); - if (result == 0) { - file = addFile(std::move(metadata), filepath); - } else { - qCWarning(file_cache, "[%s] Failed to rename %s to %s (%s)", tmpFilepath.c_str(), filepath.c_str(), strerror(errno)); - } + QSaveFile saveFile(QString::fromStdString(filepath)); + saveFile.open(QIODevice::WriteOnly); + saveFile.write(data, metadata.length); + if (saveFile.commit()) { + file = addFile(std::move(metadata), filepath); } else { - qCWarning(file_cache, "[%s] Failed to write %s (%s)", _dirname.c_str(), metadata.key.c_str(), strerror(errno)); - errno = 0; + qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); } return file; From 99ee84bc311a9587e1bc7dc4c7036353e317bcb9 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 9 May 2017 16:27:02 -0700 Subject: [PATCH 07/35] Update FileCache writing to check QSaveFile::write return value --- libraries/networking/src/FileCache.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 348206a863..0f90807d08 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -112,9 +112,8 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { } QSaveFile saveFile(QString::fromStdString(filepath)); - saveFile.open(QIODevice::WriteOnly); - saveFile.write(data, metadata.length); - if (saveFile.commit()) { + if (saveFile.open(QIODevice::WriteOnly) && saveFile.write(data, metadata.length) == metadata.length + && saveFile.commit()) { file = addFile(std::move(metadata), filepath); } else { qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); From b517c9f7be10b93466083c664439082d3c5442fb Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 10 May 2017 09:43:55 -0700 Subject: [PATCH 08/35] Fix unsigned/signed comparision warning --- libraries/networking/src/FileCache.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 0f90807d08..43055e7ed6 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -112,8 +112,10 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { } QSaveFile saveFile(QString::fromStdString(filepath)); - if (saveFile.open(QIODevice::WriteOnly) && saveFile.write(data, metadata.length) == metadata.length + if (saveFile.open(QIODevice::WriteOnly) + && saveFile.write(data, metadata.length) == static_cast(metadata.length) && saveFile.commit()) { + file = addFile(std::move(metadata), filepath); } else { qCWarning(file_cache, "[%s] Failed to write %s", _dirname.c_str(), metadata.key.c_str()); From d1d3bd0fc1dbf668fe6a18b2d48899033532a20b Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Tue, 9 May 2017 14:00:18 -0700 Subject: [PATCH 09/35] always call Head::simulate() for Avatar in view --- .../src/avatars-renderer/Avatar.cpp | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp index be55653f64..664b0094f4 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Avatar.cpp @@ -369,23 +369,25 @@ void Avatar::simulate(float deltaTime, bool inView) { PerformanceTimer perfTimer("simulate"); { PROFILE_RANGE(simulation, "updateJoints"); - if (inView && _hasNewJointData) { - _skeletonModel->getRig()->copyJointsFromJointData(_jointData); - glm::mat4 rootTransform = glm::scale(_skeletonModel->getScale()) * glm::translate(_skeletonModel->getOffset()); - _skeletonModel->getRig()->computeExternalPoses(rootTransform); - _jointDataSimulationRate.increment(); - - _skeletonModel->simulate(deltaTime, true); - - locationChanged(); // joints changed, so if there are any children, update them. - _hasNewJointData = false; - - glm::vec3 headPosition = getPosition(); - if (!_skeletonModel->getHeadPosition(headPosition)) { - headPosition = getPosition(); - } + if (inView) { Head* head = getHead(); - head->setPosition(headPosition); + if (_hasNewJointData) { + _skeletonModel->getRig()->copyJointsFromJointData(_jointData); + glm::mat4 rootTransform = glm::scale(_skeletonModel->getScale()) * glm::translate(_skeletonModel->getOffset()); + _skeletonModel->getRig()->computeExternalPoses(rootTransform); + _jointDataSimulationRate.increment(); + + _skeletonModel->simulate(deltaTime, true); + + locationChanged(); // joints changed, so if there are any children, update them. + _hasNewJointData = false; + + glm::vec3 headPosition = getPosition(); + if (!_skeletonModel->getHeadPosition(headPosition)) { + headPosition = getPosition(); + } + head->setPosition(headPosition); + } head->setScale(getUniformScale()); head->simulate(deltaTime); } else { From a5935b6fbc13d4cb27e920a98e0015a4b6a806e8 Mon Sep 17 00:00:00 2001 From: David Rowe Date: Wed, 10 May 2017 11:40:42 +1200 Subject: [PATCH 10/35] Fix unreachable eye blink code --- libraries/avatars-renderer/src/avatars-renderer/Head.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/Head.cpp b/libraries/avatars-renderer/src/avatars-renderer/Head.cpp index a90c9cd5f7..1c54ea269a 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/Head.cpp +++ b/libraries/avatars-renderer/src/avatars-renderer/Head.cpp @@ -89,8 +89,7 @@ void Head::simulate(float deltaTime) { _timeWithoutTalking += deltaTime; if ((_averageLoudness - _longTermAverageLoudness) > TALKING_LOUDNESS) { _timeWithoutTalking = 0.0f; - - } else if (_timeWithoutTalking < BLINK_AFTER_TALKING && _timeWithoutTalking >= BLINK_AFTER_TALKING) { + } else if (_timeWithoutTalking - deltaTime < BLINK_AFTER_TALKING && _timeWithoutTalking >= BLINK_AFTER_TALKING) { forceBlink = true; } From 1414254fc29573f65f89f114e62b57a7159edfd4 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 8 May 2017 19:33:09 -0400 Subject: [PATCH 11/35] add launch.tester to user_actions --- interface/src/Application.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 3be55e82cd..0d07c50e07 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -941,10 +941,12 @@ Application::Application(int& argc, char** argv, QElapsedTimer& startupTimer, bo // sessionRunTime will be reset soon by loadSettings. Grab it now to get previous session value. // The value will be 0 if the user blew away settings this session, which is both a feature and a bug. + static const QString TESTER = "HIFI_TESTER"; auto gpuIdent = GPUIdent::getInstance(); auto glContextData = getGLContextData(); QJsonObject properties = { { "version", applicationVersion() }, + { "tester", QProcessEnvironment::systemEnvironment().contains(TESTER) }, { "previousSessionCrashed", _previousSessionCrashed }, { "previousSessionRuntime", sessionRunTime.get() }, { "cpu_architecture", QSysInfo::currentCpuArchitecture() }, From 757b3be03ecb27781a87d68ece5ef9d544fa2417 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 10 May 2017 18:14:50 -0700 Subject: [PATCH 12/35] force data in MemoryStorage to be word aligned to avoid crash in glTextureSubImage2D --- .../src/avatars-renderer/OtherAvatar.h | 2 +- libraries/shared/src/SimpleMovingAverage.h | 2 +- libraries/shared/src/shared/Storage.cpp | 5 +++-- libraries/shared/src/shared/Storage.h | 13 ++++++++----- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/libraries/avatars-renderer/src/avatars-renderer/OtherAvatar.h b/libraries/avatars-renderer/src/avatars-renderer/OtherAvatar.h index 2f6c9a38aa..22a7e1863a 100644 --- a/libraries/avatars-renderer/src/avatars-renderer/OtherAvatar.h +++ b/libraries/avatars-renderer/src/avatars-renderer/OtherAvatar.h @@ -14,7 +14,7 @@ class OtherAvatar : public Avatar { public: explicit OtherAvatar(QThread* thread, RigPointer rig = nullptr); - void instantiableAvatar() {}; + virtual void instantiableAvatar() override {}; }; #endif // hifi_OtherAvatar_h diff --git a/libraries/shared/src/SimpleMovingAverage.h b/libraries/shared/src/SimpleMovingAverage.h index 0404ab9646..3855375f4c 100644 --- a/libraries/shared/src/SimpleMovingAverage.h +++ b/libraries/shared/src/SimpleMovingAverage.h @@ -71,7 +71,7 @@ public: void addSample(T sample) { if (numSamples > 0) { - average = (sample * WEIGHTING) + (average * ONE_MINUS_WEIGHTING); + average = (sample * (T)WEIGHTING) + (average * (T)ONE_MINUS_WEIGHTING); } else { average = sample; } diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index aae1f8455f..46c631c72a 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -38,8 +38,9 @@ StoragePointer Storage::toFileStorage(const QString& filename) const { return FileStorage::create(filename, size(), data()); } -MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) { - _data.resize(size); +MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) : _size(size) { + _data.resize((size + 3) / 4); // alloc smallest number of 4-byte chunks that will cover size bytes + if (data) { memcpy(_data.data(), data, size); } diff --git a/libraries/shared/src/shared/Storage.h b/libraries/shared/src/shared/Storage.h index da5b773d52..e0519c98f6 100644 --- a/libraries/shared/src/shared/Storage.h +++ b/libraries/shared/src/shared/Storage.h @@ -41,13 +41,16 @@ namespace storage { class MemoryStorage : public Storage { public: MemoryStorage(size_t size, const uint8_t* data = nullptr); - const uint8_t* data() const override { return _data.data(); } - uint8_t* data() { return _data.data(); } - uint8_t* mutableData() override { return _data.data(); } - size_t size() const override { return _data.size(); } + const uint8_t* data() const override { return reinterpret_cast(_data.data()); } + uint8_t* data() { return reinterpret_cast(_data.data()); } + uint8_t* mutableData() override { return reinterpret_cast(_data.data()); } + + size_t _size { 0 }; + size_t size() const override { return _size; } operator bool() const override { return true; } private: - std::vector _data; + // the vector is of uint32_t rather than uint8_t to force alignment + std::vector _data; }; class FileStorage : public Storage { From 0915c9ad9e3c8eb207e59f6b015902e8f4678e67 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 11 May 2017 07:11:40 -0700 Subject: [PATCH 13/35] pad MemoryStorage size to multiple of 4 rather than using a vector of uint32_t -- this gets the same alignment without needs casts --- libraries/shared/src/shared/Storage.cpp | 5 +++-- libraries/shared/src/shared/Storage.h | 9 ++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index 46c631c72a..ec29a8d7f8 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -39,8 +39,9 @@ StoragePointer Storage::toFileStorage(const QString& filename) const { } MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) : _size(size) { - _data.resize((size + 3) / 4); // alloc smallest number of 4-byte chunks that will cover size bytes - + // alloc smallest number of 4-byte chunks that will cover size bytes. The buffer is padded out to a multiple + // of 4 to force an alignment that glTextureSubImage2D can later use. + _data.resize((size + 3) & ~0x3); if (data) { memcpy(_data.data(), data, size); } diff --git a/libraries/shared/src/shared/Storage.h b/libraries/shared/src/shared/Storage.h index e0519c98f6..f51d95a95a 100644 --- a/libraries/shared/src/shared/Storage.h +++ b/libraries/shared/src/shared/Storage.h @@ -41,16 +41,15 @@ namespace storage { class MemoryStorage : public Storage { public: MemoryStorage(size_t size, const uint8_t* data = nullptr); - const uint8_t* data() const override { return reinterpret_cast(_data.data()); } - uint8_t* data() { return reinterpret_cast(_data.data()); } - uint8_t* mutableData() override { return reinterpret_cast(_data.data()); } + const uint8_t* data() const override { return _data.data(); } + uint8_t* data() { return _data.data(); } + uint8_t* mutableData() override { return _data.data(); } size_t _size { 0 }; size_t size() const override { return _size; } operator bool() const override { return true; } private: - // the vector is of uint32_t rather than uint8_t to force alignment - std::vector _data; + std::vector _data; }; class FileStorage : public Storage { From 351ddc3b2af7da50dc0bd2028a0ae71de97e4a1c Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 11 May 2017 13:36:05 -0700 Subject: [PATCH 14/35] make _size const --- libraries/shared/src/shared/Storage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/shared/src/shared/Storage.h b/libraries/shared/src/shared/Storage.h index f51d95a95a..ed1c1dd0f3 100644 --- a/libraries/shared/src/shared/Storage.h +++ b/libraries/shared/src/shared/Storage.h @@ -45,7 +45,7 @@ namespace storage { uint8_t* data() { return _data.data(); } uint8_t* mutableData() override { return _data.data(); } - size_t _size { 0 }; + const size_t _size; size_t size() const override { return _size; } operator bool() const override { return true; } private: From 3585edaa643da6117657ef122b1332c4ead1756e Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 11 May 2017 14:10:03 -0700 Subject: [PATCH 15/35] try, try again --- libraries/shared/src/shared/Storage.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index ec29a8d7f8..6a6393ff73 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -39,9 +39,11 @@ StoragePointer Storage::toFileStorage(const QString& filename) const { } MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) : _size(size) { - // alloc smallest number of 4-byte chunks that will cover size bytes. The buffer is padded out to a multiple - // of 4 to force an alignment that glTextureSubImage2D can later use. - _data.resize((size + 3) & ~0x3); + // we end up calling glCompressedTextureSubImage2D with this, and the rows of the image are expected + // to be word aligned. This is fine in all cases except for 1x1 images and 2x2 images. For 1x1, + // there is only one row, so the problem is avoided. For 2x2, we add 2 extra bytes so there's + // room for the second row. + _data.resize(size == 4 ? 6 : size); if (data) { memcpy(_data.data(), data, size); } From 9646569630f425f77f13b669ed1014934baebc85 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 11 May 2017 16:22:12 -0700 Subject: [PATCH 16/35] back out previous changes, do compressionOptions.setPitchAlignment(4) instead --- libraries/image/src/image/Image.cpp | 6 ++++++ libraries/shared/src/shared/Storage.cpp | 8 ++------ libraries/shared/src/shared/Storage.h | 4 +--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libraries/image/src/image/Image.cpp b/libraries/image/src/image/Image.cpp index 68add428c1..32184dfe79 100644 --- a/libraries/image/src/image/Image.cpp +++ b/libraries/image/src/image/Image.cpp @@ -383,6 +383,7 @@ void generateMips(gpu::Texture* texture, QImage& image, int face = -1) { } else if (mipFormat == gpu::Element::COLOR_RGBA_32) { compressionOptions.setFormat(nvtt::Format_RGBA); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(32, 0x000000FF, 0x0000FF00, @@ -393,6 +394,7 @@ void generateMips(gpu::Texture* texture, QImage& image, int face = -1) { } else if (mipFormat == gpu::Element::COLOR_BGRA_32) { compressionOptions.setFormat(nvtt::Format_RGBA); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(32, 0x00FF0000, 0x0000FF00, @@ -403,6 +405,7 @@ void generateMips(gpu::Texture* texture, QImage& image, int face = -1) { } else if (mipFormat == gpu::Element::COLOR_SRGBA_32) { compressionOptions.setFormat(nvtt::Format_RGBA); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(32, 0x000000FF, 0x0000FF00, @@ -411,6 +414,7 @@ void generateMips(gpu::Texture* texture, QImage& image, int face = -1) { } else if (mipFormat == gpu::Element::COLOR_SBGRA_32) { compressionOptions.setFormat(nvtt::Format_RGBA); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(32, 0x00FF0000, 0x0000FF00, @@ -419,11 +423,13 @@ void generateMips(gpu::Texture* texture, QImage& image, int face = -1) { } else if (mipFormat == gpu::Element::COLOR_R_8) { compressionOptions.setFormat(nvtt::Format_RGB); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(8, 0, 0, 0); } else if (mipFormat == gpu::Element::VEC2NU8_XY) { inputOptions.setNormalMap(true); compressionOptions.setFormat(nvtt::Format_RGBA); compressionOptions.setPixelType(nvtt::PixelType_UnsignedNorm); + compressionOptions.setPitchAlignment(4); compressionOptions.setPixelFormat(8, 8, 0, 0); } else { qCWarning(imagelogging) << "Unknown mip format"; diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index 6a6393ff73..aae1f8455f 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -38,12 +38,8 @@ StoragePointer Storage::toFileStorage(const QString& filename) const { return FileStorage::create(filename, size(), data()); } -MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) : _size(size) { - // we end up calling glCompressedTextureSubImage2D with this, and the rows of the image are expected - // to be word aligned. This is fine in all cases except for 1x1 images and 2x2 images. For 1x1, - // there is only one row, so the problem is avoided. For 2x2, we add 2 extra bytes so there's - // room for the second row. - _data.resize(size == 4 ? 6 : size); +MemoryStorage::MemoryStorage(size_t size, const uint8_t* data) { + _data.resize(size); if (data) { memcpy(_data.data(), data, size); } diff --git a/libraries/shared/src/shared/Storage.h b/libraries/shared/src/shared/Storage.h index ed1c1dd0f3..da5b773d52 100644 --- a/libraries/shared/src/shared/Storage.h +++ b/libraries/shared/src/shared/Storage.h @@ -44,9 +44,7 @@ namespace storage { const uint8_t* data() const override { return _data.data(); } uint8_t* data() { return _data.data(); } uint8_t* mutableData() override { return _data.data(); } - - const size_t _size; - size_t size() const override { return _size; } + size_t size() const override { return _data.size(); } operator bool() const override { return true; } private: std::vector _data; From aa4e100346405f7c1eff584949831353d608ffdb Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 5 May 2017 16:47:08 -0700 Subject: [PATCH 17/35] I think this is all that must be done. --- libraries/avatars/src/AvatarData.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index cb819c6b20..c4d72efa43 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1497,7 +1497,7 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged) { - if (identity.updatedAt < _identityUpdatedAt) { + if (identity.updatedAt < _identityUpdatedAt && !(DependencyManager::get()->getRequestsDomainListData())) { qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() << "identity.updatedAt:" << identity.updatedAt << "_identityUpdatedAt:" << _identityUpdatedAt; return; From 97f67e27d1f8c317a40ba61a90ecf5d2a3cf9354 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Tue, 9 May 2017 12:23:40 -0700 Subject: [PATCH 18/35] This is a better method. --- assignment-client/src/avatars/AvatarMixer.cpp | 2 +- libraries/avatars/src/AvatarData.cpp | 6 +++--- libraries/avatars/src/AvatarData.h | 2 +- libraries/avatars/src/AvatarHashMap.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 05dbfee912..b4d0864581 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -402,7 +402,7 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes AvatarData::parseAvatarIdentityPacket(message->getMessage(), identity); bool identityChanged = false; bool displayNameChanged = false; - avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged); + avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged, 0); if (identityChanged) { QMutexLocker nodeDataLocker(&nodeData->getMutex()); nodeData->flagIdentityChange(); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index c4d72efa43..5d5455327c 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1495,11 +1495,11 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { return _skeletonModelURL.scheme() == "file" ? emptyURL : _skeletonModelURL; } -void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged) { +void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { - if (identity.updatedAt < _identityUpdatedAt && !(DependencyManager::get()->getRequestsDomainListData())) { + if (identity.updatedAt < _identityUpdatedAt + clockSkew) { qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() - << "identity.updatedAt:" << identity.updatedAt << "_identityUpdatedAt:" << _identityUpdatedAt; + << "identity.updatedAt:" << identity.updatedAt << "_identityUpdatedAt:" << _identityUpdatedAt << "clockSkew:" << clockSkew; return; } diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 6d801793b7..e6e0571878 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -538,7 +538,7 @@ public: // identityChanged returns true if identity has changed, false otherwise. // displayNameChanged returns true if displayName has changed, false otherwise. - void processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged); + void processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew); QByteArray identityByteArray() const; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 0d341c684e..f4dc30a10c 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -148,7 +148,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer auto avatar = newOrExistingAvatar(identity.uuid, sendingNode); bool identityChanged = false; bool displayNameChanged = false; - avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged); + avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged, sendingNode->getClockSkewUsec()); } } From 81706c15020634aac5e320700cafd3da08b86b28 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 10 May 2017 14:46:14 -0700 Subject: [PATCH 19/35] Finally, the actual fix? --- assignment-client/src/avatars/AvatarMixer.cpp | 2 +- libraries/avatars/src/AvatarData.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index b4d0864581..998799f5e6 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -402,7 +402,7 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes AvatarData::parseAvatarIdentityPacket(message->getMessage(), identity); bool identityChanged = false; bool displayNameChanged = false; - avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged, 0); + avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged, senderNode->getClockSkewUsec()); if (identityChanged) { QMutexLocker nodeDataLocker(&nodeData->getMutex()); nodeData->flagIdentityChange(); diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 5d5455327c..ec36a81a43 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1497,9 +1497,9 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { - if (identity.updatedAt < _identityUpdatedAt + clockSkew) { + if ((_identityUpdatedAt > identity.updatedAt - clockSkew) && (_identityUpdatedAt != 0)) { qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() - << "identity.updatedAt:" << identity.updatedAt << "_identityUpdatedAt:" << _identityUpdatedAt << "clockSkew:" << clockSkew; + << "_identityUpdatedAt (" << _identityUpdatedAt << ") is greater than identity.updatedAt - clockSkew (" << identity.updatedAt << "-" << clockSkew << ")"; return; } @@ -1535,7 +1535,7 @@ void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityC // use the timestamp from this identity, since we want to honor the updated times in "server clock" // this will overwrite any changes we made locally to this AvatarData's _identityUpdatedAt - _identityUpdatedAt = identity.updatedAt; + _identityUpdatedAt = identity.updatedAt - clockSkew; } QByteArray AvatarData::identityByteArray() const { From 806b6e1acfed6576d2217729f59fabd2d5b5cb18 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Thu, 11 May 2017 11:45:07 -0700 Subject: [PATCH 20/35] Clarifying comments --- libraries/avatars/src/AvatarData.cpp | 3 +++ libraries/avatars/src/AvatarHashMap.cpp | 1 + 2 files changed, 4 insertions(+) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index ec36a81a43..392d3752e8 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1497,6 +1497,9 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { + // Consider the case where this packet is being processed on Client A, and Client A is connected to Sandbox B. + // If Client A's system clock is *ahead of* Sandbox B's system clock, "clockSkew" will be *negative*. + // If Client A's system clock is *behind* Sandbox B's system clock, "clockSkew" will be *positive*. if ((_identityUpdatedAt > identity.updatedAt - clockSkew) && (_identityUpdatedAt != 0)) { qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() << "_identityUpdatedAt (" << _identityUpdatedAt << ") is greater than identity.updatedAt - clockSkew (" << identity.updatedAt << "-" << clockSkew << ")"; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index f4dc30a10c..155ef9a0a2 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -148,6 +148,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer auto avatar = newOrExistingAvatar(identity.uuid, sendingNode); bool identityChanged = false; bool displayNameChanged = false; + // In this case, the "sendingNode" is the Avatar Mixer. avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged, sendingNode->getClockSkewUsec()); } } From 6720c0c02b94184244692fb70f1d838d90599a39 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Fri, 12 May 2017 12:39:10 -0700 Subject: [PATCH 21/35] Fix the twitter mention --- scripts/system/html/js/SnapshotReview.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/system/html/js/SnapshotReview.js b/scripts/system/html/js/SnapshotReview.js index 32a0956615..946e04beef 100644 --- a/scripts/system/html/js/SnapshotReview.js +++ b/scripts/system/html/js/SnapshotReview.js @@ -415,7 +415,7 @@ function updateShareInfo(containerID, storyID) { facebookButton.setAttribute("href", 'https://www.facebook.com/dialog/feed?app_id=1585088821786423&link=' + shareURL); twitterButton.setAttribute("target", "_blank"); - twitterButton.setAttribute("href", 'https://twitter.com/intent/tweet?text=I%20just%20took%20a%20snapshot!&url=' + shareURL + '&via=highfidelity&hashtags=VR,HiFi'); + twitterButton.setAttribute("href", 'https://twitter.com/intent/tweet?text=I%20just%20took%20a%20snapshot!&url=' + shareURL + '&via=highfidelityinc&hashtags=VR,HiFi'); hideUploadingMessageAndShare(containerID, storyID); } From 665316a5748660d3415b448cbd3e19205c0a1e28 Mon Sep 17 00:00:00 2001 From: samcake Date: Fri, 12 May 2017 18:04:22 -0700 Subject: [PATCH 22/35] Adding a validation step at runtime for the cached KTX file in order to regenerate them if anything seems wrong --- libraries/gpu/src/gpu/Texture.cpp | 7 ++++++- libraries/gpu/src/gpu/Texture_ktx.cpp | 7 +++++++ libraries/ktx/src/ktx/KTX.cpp | 6 ++++++ libraries/ktx/src/ktx/Reader.cpp | 19 ++++++++++++++++++- libraries/ktx/src/ktx/Writer.cpp | 3 ++- .../src/model-networking/KTXCache.cpp | 2 +- .../src/model-networking/TextureCache.cpp | 4 +++- libraries/networking/src/FileCache.cpp | 11 ++++++++--- libraries/networking/src/FileCache.h | 2 +- libraries/shared/src/shared/Storage.cpp | 4 ++++ 10 files changed, 56 insertions(+), 9 deletions(-) diff --git a/libraries/gpu/src/gpu/Texture.cpp b/libraries/gpu/src/gpu/Texture.cpp index 0f84d2a3c9..a94a0e1621 100755 --- a/libraries/gpu/src/gpu/Texture.cpp +++ b/libraries/gpu/src/gpu/Texture.cpp @@ -441,7 +441,10 @@ void Texture::assignStoredMip(uint16 level, storage::StoragePointer& storage) { // THen check that the mem texture passed make sense with its format Size expectedSize = evalStoredMipSize(level, getStoredMipFormat()); auto size = storage->size(); - if (storage->size() <= expectedSize) { + if (storage->size() < expectedSize) { + _storage->assignMipData(level, storage); + _stamp++; + } else if (size == expectedSize) { _storage->assignMipData(level, storage); _stamp++; } else if (size > expectedSize) { @@ -469,6 +472,8 @@ void Texture::assignStoredMipFace(uint16 level, uint8 face, storage::StoragePoin Size expectedSize = evalStoredMipFaceSize(level, getStoredMipFormat()); auto size = storage->size(); if (size <= expectedSize) { + _stamp++; + } else if (size == expectedSize) { _storage->assignMipFaceData(level, face, storage); _stamp++; } else if (size > expectedSize) { diff --git a/libraries/gpu/src/gpu/Texture_ktx.cpp b/libraries/gpu/src/gpu/Texture_ktx.cpp index 3fc4e0d432..524fd0a88c 100644 --- a/libraries/gpu/src/gpu/Texture_ktx.cpp +++ b/libraries/gpu/src/gpu/Texture_ktx.cpp @@ -542,6 +542,13 @@ bool Texture::evalTextureFormat(const ktx::Header& header, Element& mipFormat, E } else { return false; } + } else if (header.getGLFormat() == ktx::GLFormat::RG && header.getGLType() == ktx::GLType::UNSIGNED_BYTE && header.getTypeSize() == 1) { + mipFormat = Format::VEC2NU8_XY; + if (header.getGLInternaFormat_Uncompressed() == ktx::GLInternalFormat_Uncompressed::RG8) { + texelFormat = Format::VEC2NU8_XY; + } else { + return false; + } } else if (header.getGLFormat() == ktx::GLFormat::COMPRESSED_FORMAT && header.getGLType() == ktx::GLType::COMPRESSED_TYPE) { if (header.getGLInternaFormat_Compressed() == ktx::GLInternalFormat_Compressed::COMPRESSED_SRGB_S3TC_DXT1_EXT) { mipFormat = Format::COLOR_COMPRESSED_SRGB; diff --git a/libraries/ktx/src/ktx/KTX.cpp b/libraries/ktx/src/ktx/KTX.cpp index 38bb91e5c2..ff80695280 100644 --- a/libraries/ktx/src/ktx/KTX.cpp +++ b/libraries/ktx/src/ktx/KTX.cpp @@ -114,6 +114,9 @@ size_t Header::evalFaceSize(uint32_t level) const { } size_t Header::evalImageSize(uint32_t level) const { auto faceSize = evalFaceSize(level); + if ((faceSize < 4) || ((faceSize & 0x3) != 0)) { + return 0; + } if (numberOfFaces == NUM_CUBEMAPFACES && numberOfArrayElements == 0) { return faceSize; } else { @@ -139,6 +142,9 @@ ImageDescriptors Header::generateImageDescriptors() const { size_t imageOffset = 0; for (uint32_t level = 0; level < numberOfMipmapLevels; ++level) { auto imageSize = static_cast(evalImageSize(level)); + if ((imageSize < 4) || ((imageSize & 0x3) != 0)) { + return ImageDescriptors(); + } if (imageSize == 0) { return ImageDescriptors(); } diff --git a/libraries/ktx/src/ktx/Reader.cpp b/libraries/ktx/src/ktx/Reader.cpp index 440e2f048c..49fc8bac70 100644 --- a/libraries/ktx/src/ktx/Reader.cpp +++ b/libraries/ktx/src/ktx/Reader.cpp @@ -148,12 +148,24 @@ namespace ktx { size_t imageSize = *reinterpret_cast(currentPtr); currentPtr += sizeof(uint32_t); + auto expectedImageSize = header.evalImageSize(images.size()); + if (imageSize != expectedImageSize) { + break; + } else if ((imageSize < 4) || (imageSize & 0x3)) { + break; + } + + // The image size is the face size, beware! + size_t faceSize = imageSize; + if (numFaces == NUM_CUBEMAPFACES) { + imageSize = NUM_CUBEMAPFACES * faceSize; + } + // If enough data ahead then capture the pointer if ((currentPtr - srcBytes) + imageSize <= (srcSize)) { auto padding = Header::evalPadding(imageSize); if (numFaces == NUM_CUBEMAPFACES) { - size_t faceSize = imageSize / NUM_CUBEMAPFACES; Image::FaceBytes faces(NUM_CUBEMAPFACES); for (uint32_t face = 0; face < NUM_CUBEMAPFACES; face++) { faces[face] = currentPtr; @@ -166,6 +178,7 @@ namespace ktx { currentPtr += imageSize + padding; } } else { + // Stop here break; } } @@ -190,6 +203,10 @@ namespace ktx { // populate image table result->_images = parseImages(result->getHeader(), result->getTexelsDataSize(), result->getTexelsData()); + if (result->_images.size() != result->getHeader().getNumberOfLevels()) { + // Fail if the number of images produced doesn't match the header number of levels + return nullptr; + } return result; } diff --git a/libraries/ktx/src/ktx/Writer.cpp b/libraries/ktx/src/ktx/Writer.cpp index 4226b8fa84..50f63767a0 100644 --- a/libraries/ktx/src/ktx/Writer.cpp +++ b/libraries/ktx/src/ktx/Writer.cpp @@ -210,7 +210,8 @@ namespace ktx { if (currentDataSize + sizeof(uint32_t) < allocatedImagesDataSize) { uint32_t imageOffset = currentPtr - destBytes; size_t imageSize = srcImages[l]._imageSize; - *(reinterpret_cast (currentPtr)) = (uint32_t) imageSize; + size_t imageFaceSize = srcImages[l]._faceSize; + *(reinterpret_cast (currentPtr)) = (uint32_t)imageFaceSize; // the imageSize written in the ktx is the FACE size currentPtr += sizeof(uint32_t); currentDataSize += sizeof(uint32_t); diff --git a/libraries/model-networking/src/model-networking/KTXCache.cpp b/libraries/model-networking/src/model-networking/KTXCache.cpp index 8ec1c4e41c..e0447af8e6 100644 --- a/libraries/model-networking/src/model-networking/KTXCache.cpp +++ b/libraries/model-networking/src/model-networking/KTXCache.cpp @@ -22,7 +22,7 @@ KTXCache::KTXCache(const std::string& dir, const std::string& ext) : } KTXFilePointer KTXCache::writeFile(const char* data, Metadata&& metadata) { - FilePointer file = FileCache::writeFile(data, std::move(metadata)); + FilePointer file = FileCache::writeFile(data, std::move(metadata), true); return std::static_pointer_cast(file); } diff --git a/libraries/model-networking/src/model-networking/TextureCache.cpp b/libraries/model-networking/src/model-networking/TextureCache.cpp index 9653cde7d8..47fc62a9b5 100644 --- a/libraries/model-networking/src/model-networking/TextureCache.cpp +++ b/libraries/model-networking/src/model-networking/TextureCache.cpp @@ -792,6 +792,8 @@ void ImageReader::read() { texture = gpu::Texture::unserialize(ktxFile->getFilepath()); if (texture) { texture = textureCache->cacheTextureByHash(hash, texture); + } else { + qCWarning(modelnetworking) << "Invalid cached KTX " << _url << " under hash " << hash.c_str() << ", recreating..."; } } } @@ -835,7 +837,7 @@ void ImageReader::read() { const char* data = reinterpret_cast(memKtx->_storage->data()); size_t length = memKtx->_storage->size(); auto& ktxCache = textureCache->_ktxCache; - networkTexture->_file = ktxCache.writeFile(data, KTXCache::Metadata(hash, length)); + networkTexture->_file = ktxCache.writeFile(data, KTXCache::Metadata(hash, length)); // if (!networkTexture->_file) { qCWarning(modelnetworking) << _url << "file cache failed"; } else { diff --git a/libraries/networking/src/FileCache.cpp b/libraries/networking/src/FileCache.cpp index 43055e7ed6..8f3509d8f3 100644 --- a/libraries/networking/src/FileCache.cpp +++ b/libraries/networking/src/FileCache.cpp @@ -97,7 +97,7 @@ FilePointer FileCache::addFile(Metadata&& metadata, const std::string& filepath) return file; } -FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { +FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata, bool overwrite) { assert(_initialized); std::string filepath = getFilepath(metadata.key); @@ -107,8 +107,13 @@ FilePointer FileCache::writeFile(const char* data, File::Metadata&& metadata) { // if file already exists, return it FilePointer file = getFile(metadata.key); if (file) { - qCWarning(file_cache, "[%s] Attempted to overwrite %s", _dirname.c_str(), metadata.key.c_str()); - return file; + if (!overwrite) { + qCWarning(file_cache, "[%s] Attempted to overwrite %s", _dirname.c_str(), metadata.key.c_str()); + return file; + } else { + qCWarning(file_cache, "[%s] Overwriting %s", _dirname.c_str(), metadata.key.c_str()); + file.reset(); + } } QSaveFile saveFile(QString::fromStdString(filepath)); diff --git a/libraries/networking/src/FileCache.h b/libraries/networking/src/FileCache.h index f77db555bc..908ddcd285 100644 --- a/libraries/networking/src/FileCache.h +++ b/libraries/networking/src/FileCache.h @@ -80,7 +80,7 @@ protected: /// must be called after construction to create the cache on the fs and restore persisted files void initialize(); - FilePointer writeFile(const char* data, Metadata&& metadata); + FilePointer writeFile(const char* data, Metadata&& metadata, bool overwrite = false); FilePointer getFile(const Key& key); /// create a file diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index aae1f8455f..d78c124349 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -21,6 +21,10 @@ ViewStorage::ViewStorage(const storage::StoragePointer& owner, size_t size, cons StoragePointer Storage::createView(size_t viewSize, size_t offset) const { auto selfSize = size(); + if ((viewSize < 4) || ((viewSize & 0x3) != 0)) { + throw std::runtime_error("Invalid mapping range"); + } + if (0 == viewSize) { viewSize = selfSize; } From 833eb0b5277460761f98ca9887fff5b8d05bf15f Mon Sep 17 00:00:00 2001 From: samcake Date: Fri, 12 May 2017 18:10:48 -0700 Subject: [PATCH 23/35] Fixing the test... --- libraries/gpu/src/gpu/Texture.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libraries/gpu/src/gpu/Texture.cpp b/libraries/gpu/src/gpu/Texture.cpp index a94a0e1621..b027c25907 100755 --- a/libraries/gpu/src/gpu/Texture.cpp +++ b/libraries/gpu/src/gpu/Texture.cpp @@ -471,7 +471,8 @@ void Texture::assignStoredMipFace(uint16 level, uint8 face, storage::StoragePoin // THen check that the mem texture passed make sense with its format Size expectedSize = evalStoredMipFaceSize(level, getStoredMipFormat()); auto size = storage->size(); - if (size <= expectedSize) { + if (size < expectedSize) { + _storage->assignMipFaceData(level, face, storage); _stamp++; } else if (size == expectedSize) { _storage->assignMipFaceData(level, face, storage); From 62f2bf722d96e27f4c82d8a75ed92e088436b68f Mon Sep 17 00:00:00 2001 From: samcake Date: Fri, 12 May 2017 18:15:00 -0700 Subject: [PATCH 24/35] Adding a comment for debug sake --- libraries/gpu/src/gpu/Texture.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/gpu/src/gpu/Texture.cpp b/libraries/gpu/src/gpu/Texture.cpp index b027c25907..a545be9088 100755 --- a/libraries/gpu/src/gpu/Texture.cpp +++ b/libraries/gpu/src/gpu/Texture.cpp @@ -441,6 +441,7 @@ void Texture::assignStoredMip(uint16 level, storage::StoragePointer& storage) { // THen check that the mem texture passed make sense with its format Size expectedSize = evalStoredMipSize(level, getStoredMipFormat()); auto size = storage->size(); + // NOTE: doing the same thing in all the next block but beeing able to breakpoint with more accuracy if (storage->size() < expectedSize) { _storage->assignMipData(level, storage); _stamp++; @@ -471,6 +472,7 @@ void Texture::assignStoredMipFace(uint16 level, uint8 face, storage::StoragePoin // THen check that the mem texture passed make sense with its format Size expectedSize = evalStoredMipFaceSize(level, getStoredMipFormat()); auto size = storage->size(); + // NOTE: doing the same thing in all the next block but beeing able to breakpoint with more accuracy if (size < expectedSize) { _storage->assignMipFaceData(level, face, storage); _stamp++; From 05ad4b13a5b474d7e5471118dbe338f03feea00a Mon Sep 17 00:00:00 2001 From: samcake Date: Fri, 12 May 2017 18:25:07 -0700 Subject: [PATCH 25/35] Replace the alignment test by a nice function --- libraries/ktx/src/ktx/KTX.cpp | 7 +++++-- libraries/ktx/src/ktx/KTX.h | 1 + libraries/ktx/src/ktx/Reader.cpp | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/libraries/ktx/src/ktx/KTX.cpp b/libraries/ktx/src/ktx/KTX.cpp index ff80695280..b43d015d65 100644 --- a/libraries/ktx/src/ktx/KTX.cpp +++ b/libraries/ktx/src/ktx/KTX.cpp @@ -22,6 +22,9 @@ uint32_t Header::evalPadding(size_t byteSize) { return (uint32_t) (3 - (byteSize + 3) % PACKING_SIZE);// padding ? PACKING_SIZE - padding : 0); } +bool Header::checkAlignment(size_t byteSize) { + return ((byteSize & 0x3) == 0); +} const Header::Identifier ktx::Header::IDENTIFIER {{ 0xAB, 0x4B, 0x54, 0x58, 0x20, 0x31, 0x31, 0xBB, 0x0D, 0x0A, 0x1A, 0x0A @@ -114,7 +117,7 @@ size_t Header::evalFaceSize(uint32_t level) const { } size_t Header::evalImageSize(uint32_t level) const { auto faceSize = evalFaceSize(level); - if ((faceSize < 4) || ((faceSize & 0x3) != 0)) { + if (!checkAlignment(faceSize)) { return 0; } if (numberOfFaces == NUM_CUBEMAPFACES && numberOfArrayElements == 0) { @@ -142,7 +145,7 @@ ImageDescriptors Header::generateImageDescriptors() const { size_t imageOffset = 0; for (uint32_t level = 0; level < numberOfMipmapLevels; ++level) { auto imageSize = static_cast(evalImageSize(level)); - if ((imageSize < 4) || ((imageSize & 0x3) != 0)) { + if (!checkAlignment(imageSize)) { return ImageDescriptors(); } if (imageSize == 0) { diff --git a/libraries/ktx/src/ktx/KTX.h b/libraries/ktx/src/ktx/KTX.h index e8fa019a07..656cf93f34 100644 --- a/libraries/ktx/src/ktx/KTX.h +++ b/libraries/ktx/src/ktx/KTX.h @@ -309,6 +309,7 @@ namespace ktx { static const uint32_t REVERSE_ENDIAN_TEST = 0x01020304; static uint32_t evalPadding(size_t byteSize); + static bool checkAlignment(size_t byteSize); Header(); diff --git a/libraries/ktx/src/ktx/Reader.cpp b/libraries/ktx/src/ktx/Reader.cpp index 49fc8bac70..e69ce53551 100644 --- a/libraries/ktx/src/ktx/Reader.cpp +++ b/libraries/ktx/src/ktx/Reader.cpp @@ -151,7 +151,7 @@ namespace ktx { auto expectedImageSize = header.evalImageSize(images.size()); if (imageSize != expectedImageSize) { break; - } else if ((imageSize < 4) || (imageSize & 0x3)) { + } else if (!Header::checkAlignment(imageSize)) { break; } From 9d6cf5b7f8dc112c4715d37c418c50e1bff7a56d Mon Sep 17 00:00:00 2001 From: samcake Date: Fri, 12 May 2017 18:27:58 -0700 Subject: [PATCH 26/35] not testing in storage for alignment --- libraries/shared/src/shared/Storage.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libraries/shared/src/shared/Storage.cpp b/libraries/shared/src/shared/Storage.cpp index d78c124349..aae1f8455f 100644 --- a/libraries/shared/src/shared/Storage.cpp +++ b/libraries/shared/src/shared/Storage.cpp @@ -21,10 +21,6 @@ ViewStorage::ViewStorage(const storage::StoragePointer& owner, size_t size, cons StoragePointer Storage::createView(size_t viewSize, size_t offset) const { auto selfSize = size(); - if ((viewSize < 4) || ((viewSize & 0x3) != 0)) { - throw std::runtime_error("Invalid mapping range"); - } - if (0 == viewSize) { viewSize = selfSize; } From 91d439442d5089ff5697912790d02ff1f77add0c Mon Sep 17 00:00:00 2001 From: Sam Cake Date: Fri, 12 May 2017 20:20:03 -0700 Subject: [PATCH 27/35] warning -1 --- libraries/ktx/src/ktx/Reader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/ktx/src/ktx/Reader.cpp b/libraries/ktx/src/ktx/Reader.cpp index e69ce53551..1b63af5262 100644 --- a/libraries/ktx/src/ktx/Reader.cpp +++ b/libraries/ktx/src/ktx/Reader.cpp @@ -148,7 +148,7 @@ namespace ktx { size_t imageSize = *reinterpret_cast(currentPtr); currentPtr += sizeof(uint32_t); - auto expectedImageSize = header.evalImageSize(images.size()); + auto expectedImageSize = header.evalImageSize((uint32_t) images.size()); if (imageSize != expectedImageSize) { break; } else if (!Header::checkAlignment(imageSize)) { From a2950be5e73918b3d3842d90ae5a0534d3456df9 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Mon, 15 May 2017 14:12:48 -0700 Subject: [PATCH 28/35] Force _identityUpdatedAt to stay above 0 --- libraries/avatars/src/AvatarData.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 392d3752e8..1990903650 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1538,7 +1538,14 @@ void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityC // use the timestamp from this identity, since we want to honor the updated times in "server clock" // this will overwrite any changes we made locally to this AvatarData's _identityUpdatedAt - _identityUpdatedAt = identity.updatedAt - clockSkew; + // Additionally, ensure that the timestamp that we try to record isn't negative, as + // "_identityUpdatedAt" is an *unsigned* 64-bit integer. Furthermore, negative timestamps + // wouldn't make sense. + if (identity.updatedAt - clockSkew >= 0) { + _identityUpdatedAt = identity.updatedAt - clockSkew; + } else { + _identityUpdatedAt = 0; + } } QByteArray AvatarData::identityByteArray() const { From 0738c7fce7c5ab83938e303b3759b09fdde4243d Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Mon, 15 May 2017 14:55:05 -0700 Subject: [PATCH 29/35] Improve the test - thanks Dave! --- libraries/avatars/src/AvatarData.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 1990903650..5d5f3bcd6e 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1541,7 +1541,7 @@ void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityC // Additionally, ensure that the timestamp that we try to record isn't negative, as // "_identityUpdatedAt" is an *unsigned* 64-bit integer. Furthermore, negative timestamps // wouldn't make sense. - if (identity.updatedAt - clockSkew >= 0) { + if (identity.updatedAt > clockSkew) { _identityUpdatedAt = identity.updatedAt - clockSkew; } else { _identityUpdatedAt = 0; From 0510572a2fc60f005c6a9ddc7ce8957b92d59a93 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Mon, 15 May 2017 17:39:19 -0700 Subject: [PATCH 30/35] Thanks to Andrew! --- libraries/avatars/src/AvatarData.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 5d5f3bcd6e..9a01934b5b 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1496,13 +1496,18 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { } void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { + quint64 identityPacketUpdatedAt = identity.updatedAt; + + if (identityPacketUpdatedAt <= (uint64_t)(abs(clockSkew))) { // Incoming timestamp is bad - compute our own timestamp + identityPacketUpdatedAt = usecTimestampNow() + clockSkew; + } // Consider the case where this packet is being processed on Client A, and Client A is connected to Sandbox B. // If Client A's system clock is *ahead of* Sandbox B's system clock, "clockSkew" will be *negative*. // If Client A's system clock is *behind* Sandbox B's system clock, "clockSkew" will be *positive*. - if ((_identityUpdatedAt > identity.updatedAt - clockSkew) && (_identityUpdatedAt != 0)) { + if ((_identityUpdatedAt > identityPacketUpdatedAt - clockSkew) && (_identityUpdatedAt != 0)) { qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() - << "_identityUpdatedAt (" << _identityUpdatedAt << ") is greater than identity.updatedAt - clockSkew (" << identity.updatedAt << "-" << clockSkew << ")"; + << "_identityUpdatedAt (" << _identityUpdatedAt << ") is greater than identityPacketUpdatedAt - clockSkew (" << identityPacketUpdatedAt << "-" << clockSkew << ")"; return; } @@ -1538,14 +1543,7 @@ void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityC // use the timestamp from this identity, since we want to honor the updated times in "server clock" // this will overwrite any changes we made locally to this AvatarData's _identityUpdatedAt - // Additionally, ensure that the timestamp that we try to record isn't negative, as - // "_identityUpdatedAt" is an *unsigned* 64-bit integer. Furthermore, negative timestamps - // wouldn't make sense. - if (identity.updatedAt > clockSkew) { - _identityUpdatedAt = identity.updatedAt - clockSkew; - } else { - _identityUpdatedAt = 0; - } + _identityUpdatedAt = identityPacketUpdatedAt - clockSkew; } QByteArray AvatarData::identityByteArray() const { From 00fb328f2fc129d135fce6df8cb4c2ae15912096 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 17 May 2017 15:26:29 -0700 Subject: [PATCH 31/35] set wireless scan interval to -1 --- libraries/shared/src/SharedUtil.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libraries/shared/src/SharedUtil.cpp b/libraries/shared/src/SharedUtil.cpp index 81a0eed9f3..a68d27e620 100644 --- a/libraries/shared/src/SharedUtil.cpp +++ b/libraries/shared/src/SharedUtil.cpp @@ -768,9 +768,10 @@ bool similarStrings(const QString& stringA, const QString& stringB) { } void disableQtBearerPoll() { - // to work around the Qt constant wireless scanning, set the env for polling interval very high - const QByteArray EXTREME_BEARER_POLL_TIMEOUT = QString::number(INT16_MAX).toLocal8Bit(); - qputenv("QT_BEARER_POLL_TIMEOUT", EXTREME_BEARER_POLL_TIMEOUT); + // to disable the Qt constant wireless scanning, set the env for polling interval + qDebug() << "Disabling Qt wireless polling by using a negative value for QTimer::setInterval"; + const QByteArray DISABLE_BEARER_POLL_TIMEOUT = QString::number(-1).toLocal8Bit(); + qputenv("QT_BEARER_POLL_TIMEOUT", DISABLE_BEARER_POLL_TIMEOUT); } void printSystemInformation() { From 7ec073dce4536b47d6943786725823773a962039 Mon Sep 17 00:00:00 2001 From: howard-stearns Date: Wed, 17 May 2017 13:47:24 -0700 Subject: [PATCH 32/35] Ensure avatar identity packet at mixer. --- assignment-client/src/avatars/AvatarMixerClientData.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.h b/assignment-client/src/avatars/AvatarMixerClientData.h index 1449005246..76519466b5 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.h +++ b/assignment-client/src/avatars/AvatarMixerClientData.h @@ -145,7 +145,7 @@ private: std::unordered_map> _lastOtherAvatarSentJoints; uint64_t _identityChangeTimestamp; - bool _avatarSessionDisplayNameMustChange{ false }; + bool _avatarSessionDisplayNameMustChange{ true }; int _numAvatarsSentLastFrame = 0; int _numFramesSinceAdjustment = 0; From 8213a1b280814b4c3407846e95beb73d534e257e Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 17 May 2017 13:11:37 -0700 Subject: [PATCH 33/35] First pass - checking performance --- interface/src/Application.cpp | 3 +-- libraries/avatars/src/AvatarData.cpp | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 0d07c50e07..11467c27f9 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -5284,9 +5284,8 @@ void Application::nodeActivated(SharedNodePointer node) { } if (node->getType() == NodeType::AvatarMixer) { - // new avatar mixer, send off our identity packet right away + // new avatar mixer, send off our identity packet on next update loop getMyAvatar()->markIdentityDataChanged(); - getMyAvatar()->sendIdentityPacket(); getMyAvatar()->resetLastSent(); } } diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 9a01934b5b..0f9f4ebfe4 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1584,8 +1584,6 @@ void AvatarData::setDisplayName(const QString& displayName) { _displayName = displayName; _sessionDisplayName = ""; - sendIdentityPacket(); - qCDebug(avatars) << "Changing display name for avatar to" << displayName; markIdentityDataChanged(); } From 1fcf3b3a5f2a876225cca0eb2e34eba8651a9271 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Wed, 17 May 2017 14:22:36 -0700 Subject: [PATCH 34/35] Switch from timestamp to sequence id for avatar identity --- libraries/avatars/src/AvatarData.cpp | 45 ++++++++------------ libraries/avatars/src/AvatarData.h | 36 ++++++++-------- libraries/networking/src/udt/PacketHeaders.h | 3 +- 3 files changed, 38 insertions(+), 46 deletions(-) diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 0f9f4ebfe4..94d27a6922 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -170,13 +170,13 @@ QByteArray AvatarData::toByteArrayStateful(AvatarDataDetail dataDetail) { AvatarDataPacket::HasFlags hasFlagsOut; auto lastSentTime = _lastToByteArray; _lastToByteArray = usecTimestampNow(); - return AvatarData::toByteArray(dataDetail, lastSentTime, getLastSentJointData(), + return AvatarData::toByteArray(dataDetail, lastSentTime, getLastSentJointData(), hasFlagsOut, false, false, glm::vec3(0), nullptr, &_outboundDataRate); } QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSentTime, const QVector& lastSentJointData, - AvatarDataPacket::HasFlags& hasFlagsOut, bool dropFaceTracking, bool distanceAdjust, + AvatarDataPacket::HasFlags& hasFlagsOut, bool dropFaceTracking, bool distanceAdjust, glm::vec3 viewerPosition, QVector* sentJointDataOut, AvatarDataRate* outboundDataRateOut) const { bool cullSmallChanges = (dataDetail == CullSmallData); @@ -199,7 +199,7 @@ QByteArray AvatarData::toByteArray(AvatarDataDetail dataDetail, quint64 lastSent // FIXME - // - // BUG -- if you enter a space bubble, and then back away, the avatar has wrong orientation until "send all" happens... + // BUG -- if you enter a space bubble, and then back away, the avatar has wrong orientation until "send all" happens... // this is an iFrame issue... what to do about that? // // BUG -- Resizing avatar seems to "take too long"... the avatar doesn't redraw at smaller size right away @@ -1471,13 +1471,13 @@ QStringList AvatarData::getJointNames() const { void AvatarData::parseAvatarIdentityPacket(const QByteArray& data, Identity& identityOut) { QDataStream packetStream(data); - packetStream >> identityOut.uuid - >> identityOut.skeletonModelURL + packetStream >> identityOut.uuid + >> identityOut.skeletonModelURL >> identityOut.attachmentData >> identityOut.displayName >> identityOut.sessionDisplayName >> identityOut.avatarEntityData - >> identityOut.updatedAt; + >> identityOut.sequenceId; #ifdef WANT_DEBUG qCDebug(avatars) << __FUNCTION__ @@ -1496,20 +1496,14 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { } void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { - quint64 identityPacketUpdatedAt = identity.updatedAt; - if (identityPacketUpdatedAt <= (uint64_t)(abs(clockSkew))) { // Incoming timestamp is bad - compute our own timestamp - identityPacketUpdatedAt = usecTimestampNow() + clockSkew; - } - - // Consider the case where this packet is being processed on Client A, and Client A is connected to Sandbox B. - // If Client A's system clock is *ahead of* Sandbox B's system clock, "clockSkew" will be *negative*. - // If Client A's system clock is *behind* Sandbox B's system clock, "clockSkew" will be *positive*. - if ((_identityUpdatedAt > identityPacketUpdatedAt - clockSkew) && (_identityUpdatedAt != 0)) { - qCDebug(avatars) << "Ignoring late identity packet for avatar " << getSessionUUID() - << "_identityUpdatedAt (" << _identityUpdatedAt << ") is greater than identityPacketUpdatedAt - clockSkew (" << identityPacketUpdatedAt << "-" << clockSkew << ")"; + if (identity.sequenceId < _identitySequenceId) { + qCDebug(avatars) << "Ignoring older identity packet for avatar" << getSessionUUID() + << "_identitySequenceId (" << _identitySequenceId << ") is greater than" << identity.sequenceId; return; } + // otherwise, set the identitySequenceId to match the incoming identity + _identitySequenceId = identity.sequenceId; if (_firstSkeletonCheck || (identity.skeletonModelURL != cannonicalSkeletonModelURL(emptyURL))) { setSkeletonModelURL(identity.skeletonModelURL); @@ -1541,9 +1535,6 @@ void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityC identityChanged = true; } - // use the timestamp from this identity, since we want to honor the updated times in "server clock" - // this will overwrite any changes we made locally to this AvatarData's _identityUpdatedAt - _identityUpdatedAt = identityPacketUpdatedAt - clockSkew; } QByteArray AvatarData::identityByteArray() const { @@ -1552,13 +1543,13 @@ QByteArray AvatarData::identityByteArray() const { const QUrl& urlToSend = cannonicalSkeletonModelURL(emptyURL); // depends on _skeletonModelURL _avatarEntitiesLock.withReadLock([&] { - identityStream << getSessionUUID() - << urlToSend + identityStream << getSessionUUID() + << urlToSend << _attachmentData << _displayName << getSessionDisplayNameForTransport() // depends on _sessionDisplayName << _avatarEntityData - << _identityUpdatedAt; + << _identitySequenceId; }); return identityData; @@ -2467,11 +2458,11 @@ QScriptValue AvatarEntityMapToScriptValue(QScriptEngine* engine, const AvatarEnt if (!jsonEntityProperties.isObject()) { qCDebug(avatars) << "bad AvatarEntityData in AvatarEntityMap" << QString(entityProperties.toHex()); } - + QVariant variantEntityProperties = jsonEntityProperties.toVariant(); QVariantMap entityPropertiesMap = variantEntityProperties.toMap(); QScriptValue scriptEntityProperties = variantMapToScriptValue(entityPropertiesMap, *engine); - + QString key = entityID.toString(); obj.setProperty(key, scriptEntityProperties); } @@ -2483,12 +2474,12 @@ void AvatarEntityMapFromScriptValue(const QScriptValue& object, AvatarEntityMap& while (itr.hasNext()) { itr.next(); QUuid EntityID = QUuid(itr.name()); - + QScriptValue scriptEntityProperties = itr.value(); QVariant variantEntityProperties = scriptEntityProperties.toVariant(); QJsonDocument jsonEntityProperties = QJsonDocument::fromVariant(variantEntityProperties); QByteArray binaryEntityProperties = jsonEntityProperties.toBinaryData(); - + value[EntityID] = binaryEntityProperties; } } diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index e6e0571878..875ab3abae 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -165,15 +165,15 @@ namespace AvatarDataPacket { const size_t AVATAR_ORIENTATION_SIZE = 6; PACKED_BEGIN struct AvatarScale { - SmallFloat scale; // avatar's scale, compressed by packFloatRatioToTwoByte() + SmallFloat scale; // avatar's scale, compressed by packFloatRatioToTwoByte() } PACKED_END; const size_t AVATAR_SCALE_SIZE = 2; PACKED_BEGIN struct LookAtPosition { float lookAtPosition[3]; // world space position that eyes are focusing on. - // FIXME - unless the person has an eye tracker, this is simulated... + // FIXME - unless the person has an eye tracker, this is simulated... // a) maybe we can just have the client calculate this - // b) at distance this will be hard to discern and can likely be + // b) at distance this will be hard to discern and can likely be // descimated or dropped completely // // POTENTIAL SAVINGS - 12 bytes @@ -376,10 +376,10 @@ public: glm::vec3 getHandPosition() const; void setHandPosition(const glm::vec3& handPosition); - typedef enum { + typedef enum { NoData, PALMinimum, - MinimumData, + MinimumData, CullSmallData, IncludeSmallData, SendAllData @@ -388,7 +388,7 @@ public: virtual QByteArray toByteArrayStateful(AvatarDataDetail dataDetail); virtual QByteArray toByteArray(AvatarDataDetail dataDetail, quint64 lastSentTime, const QVector& lastSentJointData, - AvatarDataPacket::HasFlags& hasFlagsOut, bool dropFaceTracking, bool distanceAdjust, glm::vec3 viewerPosition, + AvatarDataPacket::HasFlags& hasFlagsOut, bool dropFaceTracking, bool distanceAdjust, glm::vec3 viewerPosition, QVector* sentJointDataOut, AvatarDataRate* outboundDataRateOut = nullptr) const; virtual void doneEncoding(bool cullSmallChanges); @@ -417,23 +417,23 @@ public: void nextAttitude(glm::vec3 position, glm::quat orientation); // Can be safely called at any time. virtual void updateAttitude() {} // Tell skeleton mesh about changes - glm::quat getHeadOrientation() const { + glm::quat getHeadOrientation() const { lazyInitHeadData(); - return _headData->getOrientation(); + return _headData->getOrientation(); } - void setHeadOrientation(const glm::quat& orientation) { + void setHeadOrientation(const glm::quat& orientation) { if (_headData) { _headData->setOrientation(orientation); } } - void setLookAtPosition(const glm::vec3& lookAtPosition) { + void setLookAtPosition(const glm::vec3& lookAtPosition) { if (_headData) { - _headData->setLookAtPosition(lookAtPosition); + _headData->setLookAtPosition(lookAtPosition); } } - void setBlendshapeCoefficients(const QVector& blendshapeCoefficients) { + void setBlendshapeCoefficients(const QVector& blendshapeCoefficients) { if (_headData) { _headData->setBlendshapeCoefficients(blendshapeCoefficients); } @@ -470,7 +470,7 @@ public: void setDomainMinimumScale(float domainMinimumScale) { _domainMinimumScale = glm::clamp(domainMinimumScale, MIN_AVATAR_SCALE, MAX_AVATAR_SCALE); _scaleChanged = usecTimestampNow(); } - void setDomainMaximumScale(float domainMaximumScale) + void setDomainMaximumScale(float domainMaximumScale) { _domainMaximumScale = glm::clamp(domainMaximumScale, MIN_AVATAR_SCALE, MAX_AVATAR_SCALE); _scaleChanged = usecTimestampNow(); } // Hand State @@ -531,7 +531,7 @@ public: QString displayName; QString sessionDisplayName; AvatarEntityMap avatarEntityData; - quint64 updatedAt; + quint64 sequenceId; }; static void parseAvatarIdentityPacket(const QByteArray& data, Identity& identityOut); @@ -548,8 +548,8 @@ public: virtual void setSkeletonModelURL(const QUrl& skeletonModelURL); virtual void setDisplayName(const QString& displayName); - virtual void setSessionDisplayName(const QString& sessionDisplayName) { - _sessionDisplayName = sessionDisplayName; + virtual void setSessionDisplayName(const QString& sessionDisplayName) { + _sessionDisplayName = sessionDisplayName; markIdentityDataChanged(); } @@ -623,7 +623,7 @@ public: bool getIdentityDataChanged() const { return _identityDataChanged; } // has the identity data changed since the last time sendIdentityPacket() was called void markIdentityDataChanged() { _identityDataChanged = true; - _identityUpdatedAt = usecTimestampNow(); + _identitySequenceId++; } signals: @@ -781,7 +781,7 @@ protected: float _audioAverageLoudness { 0.0f }; bool _identityDataChanged { false }; - quint64 _identityUpdatedAt { 0 }; + quint64 _identitySequenceId { 0 }; private: friend void avatarStateFromFrame(const QByteArray& frameData, AvatarData* _avatar); diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 746ae80361..f88015a4e4 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -234,7 +234,8 @@ enum class AvatarMixerPacketVersion : PacketVersion { VariableAvatarData, AvatarAsChildFixes, StickAndBallDefaultAvatar, - IdentityPacketsIncludeUpdateTime + IdentityPacketsIncludeUpdateTime, + AvatarIdentitySequenceId }; enum class DomainConnectRequestVersion : PacketVersion { From df5928c04a9f5c08eb3401a08377169e4c227eb9 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Wed, 17 May 2017 15:14:56 -0700 Subject: [PATCH 35/35] remove clockSkew, reference new version for Avatar packets --- assignment-client/src/avatars/AvatarMixer.cpp | 10 +++++----- libraries/avatars/src/AvatarData.cpp | 2 +- libraries/avatars/src/AvatarData.h | 2 +- libraries/avatars/src/AvatarHashMap.cpp | 2 +- libraries/networking/src/udt/PacketHeaders.cpp | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 998799f5e6..870149f1bc 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/assignment-client/src/avatars/AvatarMixer.cpp @@ -402,7 +402,7 @@ void AvatarMixer::handleAvatarIdentityPacket(QSharedPointer mes AvatarData::parseAvatarIdentityPacket(message->getMessage(), identity); bool identityChanged = false; bool displayNameChanged = false; - avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged, senderNode->getClockSkewUsec()); + avatar.processAvatarIdentity(identity, identityChanged, displayNameChanged); if (identityChanged) { QMutexLocker nodeDataLocker(&nodeData->getMutex()); nodeData->flagIdentityChange(); @@ -665,12 +665,12 @@ void AvatarMixer::sendStatsPacket() { void AvatarMixer::run() { qCDebug(avatars) << "Waiting for connection to domain to request settings from domain-server."; - + // wait until we have the domain-server settings, otherwise we bail DomainHandler& domainHandler = DependencyManager::get()->getDomainHandler(); connect(&domainHandler, &DomainHandler::settingsReceived, this, &AvatarMixer::domainSettingsRequestComplete); connect(&domainHandler, &DomainHandler::settingsReceiveFail, this, &AvatarMixer::domainSettingsRequestFailed); - + ThreadedAssignment::commonInit(AVATAR_MIXER_LOGGING_NAME, NodeType::AvatarMixer); } @@ -695,7 +695,7 @@ void AvatarMixer::domainSettingsRequestComplete() { // parse the settings to pull out the values we need parseDomainServerSettings(nodeList->getDomainHandler().getSettingsObject()); - + // start our tight loop... start(); } @@ -745,7 +745,7 @@ void AvatarMixer::parseDomainServerSettings(const QJsonObject& domainSettings) { } else { qCDebug(avatars) << "Avatar mixer will automatically determine number of threads to use. Using:" << _slavePool.numThreads() << "threads."; } - + const QString AVATARS_SETTINGS_KEY = "avatars"; static const QString MIN_SCALE_OPTION = "min_avatar_scale"; diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 94d27a6922..ed17e049b8 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1495,7 +1495,7 @@ QUrl AvatarData::cannonicalSkeletonModelURL(const QUrl& emptyURL) const { return _skeletonModelURL.scheme() == "file" ? emptyURL : _skeletonModelURL; } -void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew) { +void AvatarData::processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged) { if (identity.sequenceId < _identitySequenceId) { qCDebug(avatars) << "Ignoring older identity packet for avatar" << getSessionUUID() diff --git a/libraries/avatars/src/AvatarData.h b/libraries/avatars/src/AvatarData.h index 875ab3abae..a587dced3f 100644 --- a/libraries/avatars/src/AvatarData.h +++ b/libraries/avatars/src/AvatarData.h @@ -538,7 +538,7 @@ public: // identityChanged returns true if identity has changed, false otherwise. // displayNameChanged returns true if displayName has changed, false otherwise. - void processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged, const qint64 clockSkew); + void processAvatarIdentity(const Identity& identity, bool& identityChanged, bool& displayNameChanged); QByteArray identityByteArray() const; diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index 155ef9a0a2..2ccc64fee2 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -149,7 +149,7 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer bool identityChanged = false; bool displayNameChanged = false; // In this case, the "sendingNode" is the Avatar Mixer. - avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged, sendingNode->getClockSkewUsec()); + avatar->processAvatarIdentity(identity, identityChanged, displayNameChanged); } } diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index 82b4bf703d..9d970fa318 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -56,7 +56,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::AvatarData: case PacketType::BulkAvatarData: case PacketType::KillAvatar: - return static_cast(AvatarMixerPacketVersion::IdentityPacketsIncludeUpdateTime); + return static_cast(AvatarMixerPacketVersion::AvatarIdentitySequenceId); case PacketType::MessagesData: return static_cast(MessageDataVersion::TextOrBinaryData); case PacketType::ICEServerHeartbeat: