correct cleanup of Models/geometry and all EntityItems for various cases

examples/editModels.js

@@ -1619,14 +1619,23 @@ function handeMenuEvent(menuItem){
             print("  Delete Entity.... leftController.entityID="+ leftController.entityID);
             Entities.deleteEntity(leftController.entityID);
             leftController.grabbing = false;
+            if (glowedEntityID.id == leftController.entityID.id) {
+                glowedEntityID = { id: -1, isKnownID: false };
+            }
         } else if (rightController.grabbing) {
             print("  Delete Entity.... rightController.entityID="+ rightController.entityID);
             Entities.deleteEntity(rightController.entityID);
             rightController.grabbing = false;
+            if (glowedEntityID.id == rightController.entityID.id) {
+                glowedEntityID = { id: -1, isKnownID: false };
+            }
         } else if (entitySelected) {
             print("  Delete Entity.... selectedEntityID="+ selectedEntityID);
             Entities.deleteEntity(selectedEntityID);
             entitySelected = false;
+            if (glowedEntityID.id == selectedEntityID.id) {
+                glowedEntityID = { id: -1, isKnownID: false };
+            }
         } else {
             print("  Delete Entity.... not holding...");
         }

interface/src/entities/EntityTreeRenderer.cpp

@@ -42,17 +42,10 @@ EntityTreeRenderer::EntityTreeRenderer() :
 }
 
 EntityTreeRenderer::~EntityTreeRenderer() {
-    clearModelsCache();
 }
 
 void EntityTreeRenderer::clear() {
     OctreeRenderer::clear();
-    clearModelsCache();
-}
-
-void EntityTreeRenderer::clearModelsCache() {
-    // TODO: we need to implement this.... how do we want to handle this?
-    qDebug() << "EntityTreeRenderer::clearModelsCache()...";
 }
 
 void EntityTreeRenderer::init() {

interface/src/entities/EntityTreeRenderer.h

@@ -63,9 +63,6 @@ public:
     void renderEntityTypeModel(EntityItem* entity, RenderArgs* args);
     
     static QThread* getMainThread();
-
-protected:
-    void clearModelsCache();
 };
 
 #endif // hifi_EntityTreeRenderer_h

interface/src/entities/RenderableModelEntityItem.cpp

@@ -28,6 +28,11 @@ EntityItem* RenderableModelEntityItem::factory(const EntityItemID& entityID, con
     return new RenderableModelEntityItem(entityID, properties);
 }
 
+RenderableModelEntityItem::~RenderableModelEntityItem() {
+    delete _model;
+    _model = NULL;
+};
+
 bool RenderableModelEntityItem::setProperties(const EntityItemProperties& properties, bool forceCopy) {
     QString oldModelURL = getModelURL();
     bool somethingChanged = ModelEntityItem::setProperties(properties, forceCopy);

interface/src/entities/RenderableModelEntityItem.h

@@ -36,15 +36,9 @@ public:
         ModelEntityItem(entityItemID, properties),
         _model(NULL),
         _needsSimulation(true),
-        _needsModelReload(true) { 
+        _needsModelReload(true) { };
 
-qDebug() << "*********** RenderableModelEntityItem -- ENTITY ITEM BEING CREATED ************* this=" << this;
+    virtual ~RenderableModelEntityItem();
-        
-        };
-
-    virtual ~RenderableModelEntityItem() { 
-qDebug() << "*********** RenderableModelEntityItem -- ENTITY ITEM BEING DELETED ************* this=" << this;
-        };
 
     virtual bool setProperties(const EntityItemProperties& properties, bool forceCopy);
     virtual int readEntitySubclassDataFromBuffer(const unsigned char* data, int bytesLeftToRead, 

libraries/entities/src/DeleteEntityOperator.cpp

@@ -95,27 +95,14 @@ bool DeleteEntityOperator::PreRecursion(OctreeElement* element) {
             // and we can stop searching.
             if (entityTreeElement == details.containingElement) {
 
-                qDebug() << "DeleteEntityOperator::PreRecursion().... details.entity->getEntityItemID()=" << details.entity->getEntityItemID();
                 EntityTreeElement* containingElement = _tree->getContainingElement(details.entity->getEntityItemID());
-                qDebug() << "DeleteEntityOperator::PreRecursion().... BEFORE delete... containingElement=" << containingElement;
-                qDebug() << "DeleteEntityOperator::PreRecursion().... BEFORE delete... details.containingElement=" << details.containingElement;
 
                 // This is a good place to delete it!!!
                 EntityItemID entityItemID = details.entity->getEntityItemID();
-
-                qDebug() << "DeleteEntityOperator::PreRecursion().... calling... entityTreeElement->getEntityWithEntityItemID(entityItemID);";
                 EntityItem* theEntity = entityTreeElement->getEntityWithEntityItemID(entityItemID);
-                qDebug() << "     theEntity=" << theEntity;
+                entityTreeElement->removeEntityItem(theEntity);
-                qDebug() << "DeleteEntityOperator::PreRecursion().... calling... removeEntityItem(theEntity)";
-                bool removed = entityTreeElement->removeEntityItem(theEntity);
-                qDebug() << "     removed=" << removed;
-
-                qDebug() << "DeleteEntityOperator::PreRecursion().... calling... _tree->setContainingElement(entityItemID, NULL);";
                 _tree->setContainingElement(entityItemID, NULL);
-
-                qDebug() << "DeleteEntityOperator::PreRecursion().... calling... delete theEntity";
                 delete theEntity; // now actually delete it!
-
                 _foundCount++;
             }
         }

libraries/entities/src/EntityItem.cpp

@@ -57,21 +57,12 @@ void EntityItem::initFromEntityItemID(const EntityItemID& entityItemID) {
     _lifetime = DEFAULT_LIFETIME;
 }
 
-static int totalLiveEntities = 0;
 EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemProperties& properties) {
     _type = EntityTypes::Unknown;
     _lastEdited = 0;
     _lastUpdated = 0;
     initFromEntityItemID(entityItemID);
     setProperties(properties, true); // force copy
-    
-    totalLiveEntities++;
-    qDebug() << "*********** ENTITY ITEM BEING CREATED ************* this=" << this << " totalLiveEntities=" << totalLiveEntities;
-}
-
-EntityItem::~EntityItem() {
-    totalLiveEntities--;
-    qDebug() << "*********** ENTITY ITEM BEING DELETED ************* this=" << this << " totalLiveEntities=" << totalLiveEntities;
 }
 
 EntityPropertyFlags EntityItem::getEntityProperties(EncodeBitstreamParams& params) const {

libraries/entities/src/EntityItem.h

@@ -40,8 +40,7 @@ public:
     DONT_ALLOW_INSTANTIATION // This class can not be instantiated directly
     
     EntityItem(const EntityItemID& entityItemID, const EntityItemProperties& properties);
-    
+    virtual ~EntityItem() { }
-    virtual ~EntityItem();
 
     // ID and EntityItemID related methods
     QUuid getID() const { return _id; }

libraries/entities/src/EntityTree.cpp

@@ -20,15 +20,23 @@ EntityTree::EntityTree(bool shouldReaverage) : Octree(shouldReaverage) {
     _rootElement = createNewElement();
 }
 
+EntityTree::~EntityTree() {
+    eraseAllOctreeElements(false);
+}
+
 EntityTreeElement* EntityTree::createNewElement(unsigned char * octalCode) {
     EntityTreeElement* newElement = new EntityTreeElement(octalCode);
     newElement->setTree(this);
     return newElement;
 }
 
-void EntityTree::eraseAllOctreeElements() {
+void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
+    // this would be a good place to clean up our entities...
+    foreach (EntityTreeElement* element, _entityToElementMap) {
+        element->cleanupEntities();
+    }
     _entityToElementMap.clear();
-    Octree::eraseAllOctreeElements();
+    Octree::eraseAllOctreeElements(createNewRoot);
 }
 
 bool EntityTree::handlesEditPacketType(PacketType packetType) const {

libraries/entities/src/EntityTree.h

@@ -42,6 +42,7 @@ class EntityTree : public Octree {
     Q_OBJECT
 public:
     EntityTree(bool shouldReaverage = false);
+    virtual ~EntityTree();
 
     /// Implements our type specific root element factory
     virtual EntityTreeElement* createNewElement(unsigned char * octalCode = NULL);
@@ -49,7 +50,7 @@ public:
     /// Type safe version of getRoot()
     EntityTreeElement* getRoot() { return static_cast<EntityTreeElement*>(_rootElement); }
 
-    virtual void eraseAllOctreeElements();
+    virtual void eraseAllOctreeElements(bool createNewRoot = true);
 
     // These methods will allow the OctreeServer to send your tree inbound edit packets of your
     // own definition. Implement these to allow your octree based server to support editing

libraries/entities/src/EntityTreeElement.cpp

@@ -578,6 +578,15 @@ EntityItem* EntityTreeElement::getEntityWithEntityItemID(const EntityItemID& id)
     return foundEntity;
 }
 
+void EntityTreeElement::cleanupEntities() {
+    uint16_t numberOfEntities = _entityItems->size();
+    for (uint16_t i = 0; i < numberOfEntities; i++) {
+        EntityItem* entity = (*_entityItems)[i];
+        delete entity;
+    }
+    _entityItems->clear();
+}
+
 bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) {
     bool foundEntity = false;
     uint16_t numberOfEntities = _entityItems->size();

libraries/entities/src/EntityTreeElement.h

@@ -141,6 +141,7 @@ public:
 
     EntityItem* getEntityWithEntityItemID(const EntityItemID& id);
 
+    void cleanupEntities(); /// called by EntityTree on cleanup this will free all entities
     bool removeEntityWithEntityItemID(const EntityItemID& id);
     bool removeEntityItem(const EntityItem* entity);
 

libraries/entities/src/ModelEntityItem.cpp

@@ -28,9 +28,6 @@ ModelEntityItem::ModelEntityItem(const EntityItemID& entityItemID, const EntityI
     _type = EntityTypes::Model;     
     setProperties(properties, true);
     _animationFrameIndex = 0.0f;
-
-qDebug() << "*********** ModelEntityItem -- ENTITY ITEM BEING CREATED ************* this=" << this;
-
 }
 
 EntityItemProperties ModelEntityItem::getProperties() const {

libraries/entities/src/ModelEntityItem.h

@@ -19,10 +19,6 @@ public:
     static EntityItem* factory(const EntityItemID& entityID, const EntityItemProperties& properties);
 
     ModelEntityItem(const EntityItemID& entityItemID, const EntityItemProperties& properties);
-    virtual ~ModelEntityItem() {
-qDebug() << "*********** ModelEntityItem -- ENTITY ITEM BEING DELETED ************* this=" << this;
-    };
-
 
     ALLOW_INSTANTIATION // This class can be instantiated
 

libraries/entities/src/todo.txt

@@ -1,23 +1,7 @@
 // REQUIRED:
 
     
-    8) Make sure EntityItems are deleted...
+    10) Lifetime??
-            delete all owned entity items on the deletion of the EntityTreeElement?
-            delete all entity items on deletion of the tree?
-            
-            cases:
-                PASSED -- 1) test UI delete entity (on client)
-                PASSED -- 2) test UI delete entity (on server)
-                PASSED --- 7) test UI delete on other client
-                
-                3) test client shutdown
-                4) test server shutdown (on server) - aka domain restart
-                5) test server shutdown (on client)
-                6) test change domains
-
-
-    
-    9) EntityTreeRenderer::clearModelsCache()
 
 
     7) Test file save load for case where two siblings have more than MTU amount of data. I wonder if the fact that file save
@@ -243,6 +227,19 @@
 //                          Note: there's a bug in production related to the overlay correctly rendering when
 //                          the original (non-translated positions) of the models are out of view. This is a bug
 //                          not introduced byt this PR.
+// SOLVED -- 8) Make sure EntityItems are deleted...
+//            delete all entity items on deletion of the tree?
+//            cases:
+//                PASSED -- 1) test UI delete entity (on client)
+//                PASSED -- 2) test UI delete entity (on server)
+//                PASSED --- 7) test UI delete on other client
+//                PASSED --- 3) test client shutdown
+//                PASSED --- 4) test server shutdown (on server)
+//                PASSED ---   a) click X on domain page
+//                PASSED ---   b) shutdown domain server
+//                PASSED --- 6) test change domains
+//                SAME AS PRODUCTION - 5) test server shutdown (on client) - models aren't deleted when model server goes away
+// SOLVED -- 9) EntityTreeRenderer::clearModelsCache() - Model instance cleanup handled in ~RenderableModelEntityItem
 
 
 

libraries/octree/src/Octree.cpp

@@ -51,9 +51,8 @@ Octree::Octree(bool shouldReaverage) :
 }
 
 Octree::~Octree() {
-    // delete the children of the root element
+    // This will delete all children, don't create a new root in this case.
-    // this recursively deletes the tree
+    eraseAllOctreeElements(false);
-    delete _rootElement;
 }
 
 // Recurses voxel tree calling the RecurseOctreeOperation function for each element.
@@ -488,9 +487,13 @@ void Octree::deleteOctalCodeFromTreeRecursion(OctreeElement* element, void* extr
     }
 }
 
-void Octree::eraseAllOctreeElements() {
+void Octree::eraseAllOctreeElements(bool createNewRoot) {
     delete _rootElement; // this will recurse and delete all children
+    if (createNewRoot) {
         _rootElement = createNewElement();
+    } else {
+        _rootElement = NULL;
+    }
     _isDirty = true;
 }
 

libraries/octree/src/Octree.h

@@ -215,7 +215,7 @@ class Octree : public QObject {
     Q_OBJECT
 public:
     Octree(bool shouldReaverage = false);
-    ~Octree();
+    virtual ~Octree();
 
     /// Your tree class must implement this to create the correct element type
     virtual OctreeElement* createNewElement(unsigned char * octalCode = NULL) = 0;
@@ -243,7 +243,7 @@ public:
 
     OctreeElement* getRoot() { return _rootElement; }
 
-    virtual void eraseAllOctreeElements();
+    virtual void eraseAllOctreeElements(bool createNewRoot = true);
 
     void processRemoveOctreeElementsBitstream(const unsigned char* bitstream, int bufferSizeBytes);
     void readBitstreamToTree(const unsigned char* bitstream,  unsigned long int bufferSizeBytes, ReadBitstreamToTreeParams& args);