From 29dedd55241e08bc7f6458dbfbed5e7b15f856b6 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 28 Mar 2016 19:19:16 -0700 Subject: [PATCH] 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; }