From 29dedd55241e08bc7f6458dbfbed5e7b15f856b6 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 28 Mar 2016 19:19:16 -0700 Subject: [PATCH 1/3] Update model URL on render thread --- .../src/EntityTreeRenderer.cpp | 34 ++++-------- .../src/RenderableModelEntityItem.cpp | 53 +++++++++---------- libraries/render-utils/src/Model.h | 4 +- 3 files changed, 38 insertions(+), 53 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 07d19dbd92..65ac5197c8 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -460,14 +460,17 @@ void EntityTreeRenderer::processEraseMessage(ReceivedMessage& message, const Sha ModelPointer EntityTreeRenderer::allocateModel(const QString& url, const QString& collisionUrl) { ModelPointer model = nullptr; - // Make sure we only create and delete models on the thread that owns the EntityTreeRenderer + + // Only create and delete models on the thread that owns the EntityTreeRenderer if (QThread::currentThread() != thread()) { QMetaObject::invokeMethod(this, "allocateModel", Qt::BlockingQueuedConnection, Q_RETURN_ARG(ModelPointer, model), - Q_ARG(const QString&, url)); + Q_ARG(const QString&, url), + Q_ARG(const QString&, collisionUrl)); return model; } + model = std::make_shared(std::make_shared()); model->init(); model->setURL(QUrl(url)); @@ -475,37 +478,20 @@ ModelPointer EntityTreeRenderer::allocateModel(const QString& url, const QString return model; } -ModelPointer EntityTreeRenderer::updateModel(ModelPointer original, const QString& newUrl, const QString& collisionUrl) { - ModelPointer model = nullptr; - - // The caller shouldn't call us if the URL doesn't need to change. But if they - // do, we just return their original back to them. - if (!original || (QUrl(newUrl) == original->getURL())) { - return original; - } - - // Before we do any creating or deleting, make sure we're on our renderer thread +ModelPointer EntityTreeRenderer::updateModel(ModelPointer model, const QString& newUrl, const QString& collisionUrl) { + // Only create and delete models on the thread that owns the EntityTreeRenderer if (QThread::currentThread() != thread()) { QMetaObject::invokeMethod(this, "updateModel", Qt::BlockingQueuedConnection, Q_RETURN_ARG(ModelPointer, model), - Q_ARG(ModelPointer, original), - Q_ARG(const QString&, newUrl)); + Q_ARG(ModelPointer, model), + Q_ARG(const QString&, newUrl), + Q_ARG(const QString&, collisionUrl)); return model; } - // at this point we know we need to replace the model, and we know we're on the - // correct thread, so we can do all our work. - if (original) { - original.reset(); // delete the old model... - } - - // create the model and correctly initialize it with the new url - model = std::make_shared(std::make_shared()); - model->init(); model->setURL(QUrl(newUrl)); model->setCollisionModelURL(QUrl(collisionUrl)); - return model; } diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index ae29d1faf7..e2db598d40 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -69,14 +69,10 @@ void RenderableModelEntityItem::loader() { _needsModelReload = true; EntityTreeRenderer* renderer = DependencyManager::get().data(); assert(renderer); - if (!_model || _needsModelReload) { + { PerformanceTimer perfTimer("getModel"); getModel(renderer); } - if (_model) { - _model->setURL(getParsedModelURL()); - _model->setCollisionModelURL(QUrl(getCompoundShapeURL())); - } } void RenderableModelEntityItem::setDimensions(const glm::vec3& value) { @@ -364,13 +360,6 @@ void RenderableModelEntityItem::render(RenderArgs* args) { if (hasModel()) { if (_model) { - // check if the URL has changed - auto& currentURL = getParsedModelURL(); - if (currentURL != _model->getURL()) { - qCDebug(entitiesrenderer).noquote() << "Updating model URL: " << currentURL.toDisplayString(); - _model->setURL(currentURL); - } - render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); // check to see if when we added our models to the scene they were ready, if they were not ready, then @@ -435,6 +424,15 @@ void RenderableModelEntityItem::render(RenderArgs* args) { } }); updateModelBounds(); + + // Check if the URL has changed + // Do this last as the getModel is queued for the next frame, + // and we need to keep state directing the model to reinitialize + auto& currentURL = getParsedModelURL(); + if (currentURL != _model->getURL()) { + // Defer setting the url to the render thread + getModel(_myRenderer); + } } } } else { @@ -450,10 +448,8 @@ void RenderableModelEntityItem::render(RenderArgs* args) { } ModelPointer RenderableModelEntityItem::getModel(EntityTreeRenderer* renderer) { - ModelPointer result = nullptr; - if (!renderer) { - return result; + return nullptr; } // make sure our renderer is setup @@ -468,21 +464,22 @@ ModelPointer RenderableModelEntityItem::getModel(EntityTreeRenderer* renderer) { _needsModelReload = false; // this is the reload - // if we have a URL, then we will want to end up returning a model... + // If we have a URL, then we will want to end up returning a model... if (!getModelURL().isEmpty()) { - - // if we have a previously allocated model, but its URL doesn't match - // then we need to let our renderer update our model for us. - if (_model && (QUrl(getModelURL()) != _model->getURL() || - QUrl(getCompoundShapeURL()) != _model->getCollisionURL())) { - result = _model = _myRenderer->updateModel(_model, getModelURL(), getCompoundShapeURL()); + // If we don't have a model, allocate one *immediately* + if (!_model) { + _model = _myRenderer->allocateModel(getModelURL(), getCompoundShapeURL()); _needsInitialSimulation = true; - } else if (!_model) { // if we don't yet have a model, then we want our renderer to allocate one - result = _model = _myRenderer->allocateModel(getModelURL(), getCompoundShapeURL()); + // If we need to change URLs, update it *after rendering* (to avoid access violations) + } else if ((QUrl(getModelURL()) != _model->getURL() || QUrl(getCompoundShapeURL()) != _model->getCollisionURL())) { + QMetaObject::invokeMethod(_myRenderer, "updateModel", Qt::QueuedConnection, + Q_ARG(ModelPointer, _model), + Q_ARG(const QString&, getModelURL()), + Q_ARG(const QString&, getCompoundShapeURL())); _needsInitialSimulation = true; - } else { // we already have the model we want... - result = _model; } + // Else we can just return the _model + // If we have no URL, then we can delete any model we do have... } else if (_model) { // remove from scene render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); @@ -492,11 +489,11 @@ ModelPointer RenderableModelEntityItem::getModel(EntityTreeRenderer* renderer) { // release interest _myRenderer->releaseModel(_model); - result = _model = nullptr; + _model = nullptr; _needsInitialSimulation = true; } - return result; + return _model; } bool RenderableModelEntityItem::needsToCallUpdate() const { diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h index 744a4ce605..785b902b3a 100644 --- a/libraries/render-utils/src/Model.h +++ b/libraries/render-utils/src/Model.h @@ -73,6 +73,7 @@ public: } /// Sets the URL of the model to render. + // Should only be called from the model's rendering thread to avoid access violations of changed geometry. Q_INVOKABLE void setURL(const QUrl& url); const QUrl& getURL() const { return _url; } @@ -133,7 +134,8 @@ public: /// Provided as a convenience, will crash if !isCollisionLoaded() const FBXGeometry& getCollisionFBXGeometry() const { assert(isCollisionLoaded()); return getCollisionGeometry()->getGeometry()->getGeometry(); } - // Set the model to use for collisions + // Set the model to use for collisions. + // Should only be called from the model's rendering thread to avoid access violations of changed geometry. Q_INVOKABLE void setCollisionModelURL(const QUrl& url); const QUrl& getCollisionURL() const { return _collisionUrl; } From 96a35a8f43dc562fac2376b879daf92880a763a3 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 29 Mar 2016 11:33:02 -0700 Subject: [PATCH 2/3] Fix bug where a pending asset request could receive a pending message after deletion --- libraries/networking/src/AssetClient.cpp | 12 +++++++++++- libraries/networking/src/AssetClient.h | 5 +++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 8cb1f31090..446009ac23 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -228,7 +228,8 @@ bool AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end nodeList->sendPacket(std::move(packet), *assetServer); - _pendingRequests[assetServer][messageID] = { callback, progressCallback }; + _pendingRequests[assetServer][messageID] = { QSharedPointer(), callback, progressCallback }; + return true; } else { @@ -326,6 +327,9 @@ void AssetClient::handleAssetGetReply(QSharedPointer message, S if (requestIt != messageCallbackMap.end()) { auto& callbacks = requestIt->second; + // Store message in case we need to disconnect from it later. + callbacks.message = message; + if (message->isComplete()) { callbacks.completeCallback(true, error, message->readAll()); } else { @@ -550,6 +554,12 @@ void AssetClient::handleNodeKilled(SharedNodePointer node) { auto messageMapIt = _pendingRequests.find(node); if (messageMapIt != _pendingRequests.end()) { for (const auto& value : messageMapIt->second) { + auto& message = value.second.message; + if (message) { + // Disconnect from all signals emitting from the pending message + disconnect(message.data(), nullptr, this, nullptr); + } + value.second.completeCallback(false, AssetServerError::NoError, QByteArray()); } messageMapIt->second.clear(); diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index e46c8c6524..f1890377dc 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -86,14 +86,15 @@ private: ReceivedAssetCallback callback, ProgressCallback progressCallback); bool uploadAsset(const QByteArray& data, UploadResultCallback callback); - struct GetAssetCallbacks { + struct GetAssetRequestData { + QSharedPointer message; ReceivedAssetCallback completeCallback; ProgressCallback progressCallback; }; static MessageID _currentID; std::unordered_map> _pendingMappingRequests; - std::unordered_map> _pendingRequests; + std::unordered_map> _pendingRequests; std::unordered_map> _pendingInfoRequests; std::unordered_map> _pendingUploads; From ad9027f9d650bff58b1f4bdd0fd4415b4d9f3cd2 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Tue, 29 Mar 2016 15:09:27 -0700 Subject: [PATCH 3/3] Log OffscreenQml lifetime --- .../src/RenderableWebEntityItem.cpp | 2 +- libraries/gl/src/gl/OffscreenQmlSurface.cpp | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index ce3faeb196..855fd16408 100644 --- a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp @@ -55,8 +55,8 @@ bool RenderableWebEntityItem::buildWebSurface(EntityTreeRenderer* renderer) { qWarning() << "Too many concurrent web views to create new view"; return false; } - qDebug() << "Building web surface"; + ++_currentWebCount; // Save the original GL context, because creating a QML surface will create a new context QOpenGLContext * currentContext = QOpenGLContext::currentContext(); diff --git a/libraries/gl/src/gl/OffscreenQmlSurface.cpp b/libraries/gl/src/gl/OffscreenQmlSurface.cpp index 818b3c6ca8..1d9e6d0149 100644 --- a/libraries/gl/src/gl/OffscreenQmlSurface.cpp +++ b/libraries/gl/src/gl/OffscreenQmlSurface.cpp @@ -70,7 +70,7 @@ public: virtual bool event(QEvent *e) override; protected: - class Queue : public QQueue { + class Queue : private QQueue { public: void add(QEvent::Type type); QEvent* take(); @@ -134,12 +134,14 @@ QEvent* OffscreenQmlRenderThread::Queue::take() { } OffscreenQmlRenderThread::OffscreenQmlRenderThread(OffscreenQmlSurface* surface, QOpenGLContext* shareContext) : _surface(surface) { + qDebug() << "Building QML Renderer: creating context"; if (!_canvas.create(shareContext)) { static const char* error = "Failed to create OffscreenGLCanvas"; qWarning() << error; throw error; }; + qDebug() << "Building QML Renderer: creating render control"; _renderControl = new QMyQuickRenderControl(); QQuickWindow::setDefaultAlphaBuffer(true); // Create a QQuickWindow that is associated with our render control. @@ -147,19 +149,25 @@ OffscreenQmlRenderThread::OffscreenQmlRenderThread(OffscreenQmlSurface* surface, // NOTE: Must be created on the main thread so that OffscreenQmlSurface can send it events // NOTE: Must be created on the rendering thread or it will refuse to render, // so we wait until after its ctor to move object/context to this thread. + qDebug() << "Building QML Renderer: creating window"; _quickWindow = new QQuickWindow(_renderControl); _quickWindow->setColor(QColor(255, 255, 255, 0)); _quickWindow->setFlags(_quickWindow->flags() | static_cast(Qt::WA_TranslucentBackground)); // We can prepare, but we must wait to start() the thread until after the ctor + qDebug() << "Building QML Renderer: moving to own thread"; _renderControl->prepareThread(this); _canvas.getContextObject()->moveToThread(this); moveToThread(this); + qDebug() << "Building QML Renderer: complete"; + _queue.add(INIT); } void OffscreenQmlRenderThread::run() { + qDebug() << "Starting QML Renderer thread"; + while (!_quit) { QEvent* e = _queue.take(); event(e); @@ -208,12 +216,14 @@ void OffscreenQmlRenderThread::setupFbo() { } void OffscreenQmlRenderThread::init() { + qDebug() << "Initializing QML Renderer"; + connect(_renderControl, &QQuickRenderControl::renderRequested, _surface, &OffscreenQmlSurface::requestRender); connect(_renderControl, &QQuickRenderControl::sceneChanged, _surface, &OffscreenQmlSurface::requestUpdate); if (!_canvas.makeCurrent()) { - // Failed to make GL context current, this OffscreenQmlSurface is basically dead qWarning("Failed to make context current on QML Renderer Thread"); + _quit = true; return; } @@ -360,6 +370,8 @@ void OffscreenQmlSurface::onAboutToQuit() { } void OffscreenQmlSurface::create(QOpenGLContext* shareContext) { + qDebug() << "Building QML surface"; + _renderer = new OffscreenQmlRenderThread(this, shareContext); _renderer->moveToThread(_renderer); _renderer->setObjectName("QML Renderer Thread");