fix Model mew/delete and thread issues

interface/src/entities/EntityTreeRenderer.cpp

@@ -67,6 +67,7 @@ void EntityTreeRenderer::update() {
 
 void EntityTreeRenderer::render(RenderMode renderMode) {
     OctreeRenderer::render(renderMode);
+    deleteReleasedModels(); // seems like as good as any other place to do some memory cleanup
 }
 
 const FBXGeometry* EntityTreeRenderer::getGeometryForEntity(const EntityItem* entityItem) {
@@ -76,7 +77,7 @@ const FBXGeometry* EntityTreeRenderer::getGeometryForEntity(const EntityItem* en
         const RenderableModelEntityItem* constModelEntityItem = dynamic_cast<const RenderableModelEntityItem*>(entityItem);
         RenderableModelEntityItem* modelEntityItem = const_cast<RenderableModelEntityItem*>(constModelEntityItem);
         assert(modelEntityItem); // we need this!!!
-        Model* model = modelEntityItem->getModel();
+        Model* model = modelEntityItem->getModel(this);
         if (model) {
             result = &model->getGeometry()->getFBXGeometry();
         }
@@ -91,7 +92,7 @@ const Model* EntityTreeRenderer::getModelForEntityItem(const EntityItem* entityI
         RenderableModelEntityItem* modelEntityItem = const_cast<RenderableModelEntityItem*>(constModelEntityItem);
         assert(modelEntityItem); // we need this!!!
     
-        result = modelEntityItem->getModel();
+        result = modelEntityItem->getModel(this);
     }
     return result;
 }
@@ -225,7 +226,71 @@ void EntityTreeRenderer::processEraseMessage(const QByteArray& dataByteArray, co
     static_cast<EntityTree*>(_tree)->processEraseMessage(dataByteArray, sourceNode);
 }
 
-
-
+Model* EntityTreeRenderer::allocateModel(const QString& url) {
+    Model* model = NULL;
+    // Make sure we 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(Model*, model),
+                Q_ARG(const QString&, url));
+
+        return model;
+    }
+    model = new Model();
+    model->init();
+    model->setURL(QUrl(url));
+    return model;
+}
+
+Model* EntityTreeRenderer::updateModel(Model* original, const QString& newUrl) {
+    Model* model = NULL;
+
+    // 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(Model*, model),
+                Q_ARG(Model*, original),
+                Q_ARG(const QString&, newUrl));
+
+        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) {
+        delete original; // delete the old model...
+    }
+
+    // create the model and correctly initialize it with the new url
+    model = new Model();
+    model->init();
+    model->setURL(QUrl(newUrl));
+        
+    return model;
+}
+
+void EntityTreeRenderer::releaseModel(Model* model) {
+    // If we're not on the renderer's thread, then remember this model to be deleted later
+    if (QThread::currentThread() != thread()) {
+        _releasedModels << model;
+    } else { // otherwise just delete it right away
+        delete model;
+    }
+}
+
+void EntityTreeRenderer::deleteReleasedModels() {
+    if (_releasedModels.size() > 0) {
+        foreach(Model* model, _releasedModels) {
+            delete model;
+        }
+        _releasedModels.clear();
+    }
+}
 
 

interface/src/entities/EntityTreeRenderer.h

@@ -63,6 +63,19 @@ public:
     void renderEntityTypeModel(EntityItem* entity, RenderArgs* args);
     
     static QThread* getMainThread();
+    
+    /// if a renderable entity item needs a model, we will allocate it for them
+    Q_INVOKABLE Model* allocateModel(const QString& url);
+    
+    /// if a renderable entity item needs to update the URL of a model, we will handle that for the entity
+    Q_INVOKABLE Model* updateModel(Model* original, const QString& newUrl); 
+
+    /// if a renderable entity item is done with a model, it should return it to us
+    void releaseModel(Model* model);
+    
+    void deleteReleasedModels();
+private:
+    QList<Model*> _releasedModels;
 };
 
 #endif // hifi_EntityTreeRenderer_h

interface/src/entities/RenderableModelEntityItem.cpp

@@ -29,10 +29,11 @@ EntityItem* RenderableModelEntityItem::factory(const EntityItemID& entityID, con
 }
 
 RenderableModelEntityItem::~RenderableModelEntityItem() {
-
-    // TODO: we can't just delete this because we may be on the wrong thread.
-    //delete _model;
-    _model = NULL;
+    assert(_myRenderer || !_model); // if we have a model, we need to know our renderer
+    if (_myRenderer && _model) {
+        _myRenderer->releaseModel(_model);
+        _model = NULL;
+    }
 };
 
 bool RenderableModelEntityItem::setProperties(const EntityItemProperties& properties, bool forceCopy) {
@@ -75,7 +76,8 @@ void RenderableModelEntityItem::render(RenderArgs* args) {
             if (!_model || _needsModelReload) {
                 // TODO: this getModel() appears to be about 3% of model render time. We should optimize
                 PerformanceTimer perfTimer("getModel");
-                getModel();
+                EntityTreeRenderer* renderer = static_cast<EntityTreeRenderer*>(args->_renderer);
+                getModel(renderer);
             }
             
             if (_model) {
@@ -191,47 +193,40 @@ void RenderableModelEntityItem::render(RenderArgs* args) {
     }
 }
 
-Model* RenderableModelEntityItem::getModel() {
+Model* RenderableModelEntityItem::getModel(EntityTreeRenderer* renderer) {
+    Model* result = NULL;
+
+    // make sure our renderer is setup
+    if (!_myRenderer) {
+        _myRenderer = renderer;
+    }
+    assert(_myRenderer == renderer); // you should only ever render on one renderer
+    
     _needsModelReload = false; // this is the reload
+
+    // if we have a URL, then we will want to end up returning a model...
     if (!getModelURL().isEmpty()) {
-        // double check our URLS match...
+    
+        // if we have a previously allocated model, but it's URL doesn't match
+        // then we need to let our renderer update our model for us.
         if (_model && QUrl(getModelURL()) != _model->getURL()) {
-            delete _model; // delete the old model...
-            _model = NULL;
+            result = _model = _myRenderer->updateModel(_model, getModelURL());
             _needsSimulation = true;
-        }
-
-        // if we don't have a model... but our item does have a model URL
-        if (!_model) {
-            // Make sure we only create new models on the thread that owns the EntityTreeRenderer
-            if (QThread::currentThread() != EntityTreeRenderer::getMainThread()) {
-            
-                // TODO: how do we better handle this??? we may need this for scripting...
-                // possible go back to a getModel() service on the TreeRenderer, but have it take a URL
-                // this would allow the entity items to use that service for loading, and since the
-                // EntityTreeRenderer is a Q_OBJECT it can call invokeMethod()
-                
-                qDebug() << "can't call getModel() on thread other than rendering thread...";
-
-                //QMetaObject::invokeMethod(this, "getModel", Qt::BlockingQueuedConnection, Q_RETURN_ARG(Model*, _model));
-                //qDebug() << "got it... _model=" << _model;
-                return _model;
-            }
-
-            _model = new Model();
-            _model->init();
-            _model->setURL(QUrl(getModelURL()));
+        } else if (!_model) { // if we don't yet have a model, then we want our renderer to allocate one
+            result = _model = _myRenderer->allocateModel(getModelURL());
             _needsSimulation = true;
+        } else { // we already have the model we want...
+            result = _model;
         }
-    } else {
-        // our URL is empty, we should clean up our old model
+    } else { // if our desired URL is empty, we may need to delete our existing model
         if (_model) {
-            delete _model; // delete the old model...
-            _model = NULL;
+            _myRenderer->releaseModel(_model);
+            result = _model = NULL;
             _needsSimulation = true;
         }
     }
-    return _model;
+    
+    return result;
 }
 
 

interface/src/entities/RenderableModelEntityItem.h

@@ -36,7 +36,8 @@ public:
         ModelEntityItem(entityItemID, properties),
         _model(NULL),
         _needsSimulation(true),
-        _needsModelReload(true) { };
+        _needsModelReload(true),
+        _myRenderer(NULL) { };
 
     virtual ~RenderableModelEntityItem();
 
@@ -48,11 +49,12 @@ public:
     virtual void somethingChangedNotification() { _needsSimulation = true; }
 
     virtual void render(RenderArgs* args);
-    Model* getModel();
+    Model* getModel(EntityTreeRenderer* renderer);
 private:
     Model* _model;
     bool _needsSimulation;
     bool _needsModelReload;
+    EntityTreeRenderer* _myRenderer;
 };
 
 #endif // hifi_RenderableModelEntityItem_h