Update model URL on render thread

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<Model>(std::make_shared<Rig>());
     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 EntityTreeRenderer::updateModel(ModelPointer model, const QString& newUrl, const QString& collisionUrl) {
-    ModelPointer model = nullptr;
+    // Only create and delete models on the thread that owns the EntityTreeRenderer
-
-    // 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
     if (QThread::currentThread() != thread()) {
         QMetaObject::invokeMethod(this, "updateModel", Qt::BlockingQueuedConnection,
             Q_RETURN_ARG(ModelPointer, model),
-                Q_ARG(ModelPointer, original),
+                Q_ARG(ModelPointer, model),
-                Q_ARG(const QString&, newUrl));
+                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<Model>(std::make_shared<Rig>());
-    model->init();
     model->setURL(QUrl(newUrl));
     model->setCollisionModelURL(QUrl(collisionUrl));
-        
     return model;
 }
 

libraries/entities-renderer/src/RenderableModelEntityItem.cpp

@@ -69,14 +69,10 @@ void RenderableModelEntityItem::loader() {
     _needsModelReload = true;
     EntityTreeRenderer* renderer = DependencyManager::get<EntityTreeRenderer>().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 don't have a model, allocate one *immediately*
-        // if we have a previously allocated model, but its URL doesn't match
+        if (!_model) {
-        // then we need to let our renderer update our model for us.
+            _model = _myRenderer->allocateModel(getModelURL(), getCompoundShapeURL());
-        if (_model && (QUrl(getModelURL()) != _model->getURL() ||
-                       QUrl(getCompoundShapeURL()) != _model->getCollisionURL())) {
-            result = _model = _myRenderer->updateModel(_model, getModelURL(), getCompoundShapeURL());
             _needsInitialSimulation = true;
-        } else if (!_model) { // if we don't yet have a model, then we want our renderer to allocate one
+        // If we need to change URLs, update it *after rendering* (to avoid access violations)
-            result = _model = _myRenderer->allocateModel(getModelURL(), getCompoundShapeURL());
+        } 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 {

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; }