From 2f90df04eee532e55da8dcefc913c28e5f559941 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Wed, 27 Aug 2014 15:45:45 -0700 Subject: [PATCH] correct cleanup of Models/geometry and all EntityItems for various cases --- examples/editModels.js | 9 +++++ interface/src/entities/EntityTreeRenderer.cpp | 7 ---- interface/src/entities/EntityTreeRenderer.h | 3 -- .../entities/RenderableModelEntityItem.cpp | 5 +++ .../src/entities/RenderableModelEntityItem.h | 10 ++---- .../entities/src/DeleteEntityOperator.cpp | 15 +-------- libraries/entities/src/EntityItem.cpp | 9 ----- libraries/entities/src/EntityItem.h | 3 +- libraries/entities/src/EntityTree.cpp | 12 +++++-- libraries/entities/src/EntityTree.h | 3 +- libraries/entities/src/EntityTreeElement.cpp | 9 +++++ libraries/entities/src/EntityTreeElement.h | 1 + libraries/entities/src/ModelEntityItem.cpp | 3 -- libraries/entities/src/ModelEntityItem.h | 4 --- libraries/entities/src/todo.txt | 33 +++++++++---------- libraries/octree/src/Octree.cpp | 13 +++++--- libraries/octree/src/Octree.h | 4 +-- 17 files changed, 65 insertions(+), 78 deletions(-) diff --git a/examples/editModels.js b/examples/editModels.js index b3eb60856b..c68fcc5fad 100644 --- a/examples/editModels.js +++ b/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..."); } diff --git a/interface/src/entities/EntityTreeRenderer.cpp b/interface/src/entities/EntityTreeRenderer.cpp index 7d322590db..c1a3d2bf82 100644 --- a/interface/src/entities/EntityTreeRenderer.cpp +++ b/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() { diff --git a/interface/src/entities/EntityTreeRenderer.h b/interface/src/entities/EntityTreeRenderer.h index 9e61899a00..2ec9cf2f10 100644 --- a/interface/src/entities/EntityTreeRenderer.h +++ b/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 diff --git a/interface/src/entities/RenderableModelEntityItem.cpp b/interface/src/entities/RenderableModelEntityItem.cpp index 7c866ffbb6..b24df63fab 100644 --- a/interface/src/entities/RenderableModelEntityItem.cpp +++ b/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); diff --git a/interface/src/entities/RenderableModelEntityItem.h b/interface/src/entities/RenderableModelEntityItem.h index 167fba1b98..60714fec15 100644 --- a/interface/src/entities/RenderableModelEntityItem.h +++ b/interface/src/entities/RenderableModelEntityItem.h @@ -36,15 +36,9 @@ public: ModelEntityItem(entityItemID, properties), _model(NULL), _needsSimulation(true), - _needsModelReload(true) { - -qDebug() << "*********** RenderableModelEntityItem -- ENTITY ITEM BEING CREATED ************* this=" << this; - - }; + _needsModelReload(true) { }; - virtual ~RenderableModelEntityItem() { -qDebug() << "*********** RenderableModelEntityItem -- ENTITY ITEM BEING DELETED ************* this=" << this; - }; + virtual ~RenderableModelEntityItem(); virtual bool setProperties(const EntityItemProperties& properties, bool forceCopy); virtual int readEntitySubclassDataFromBuffer(const unsigned char* data, int bytesLeftToRead, diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index 8dcf4f022b..c3132c9909 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/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; - qDebug() << "DeleteEntityOperator::PreRecursion().... calling... removeEntityItem(theEntity)"; - bool removed = entityTreeElement->removeEntityItem(theEntity); - qDebug() << " removed=" << removed; - - qDebug() << "DeleteEntityOperator::PreRecursion().... calling... _tree->setContainingElement(entityItemID, NULL);"; + entityTreeElement->removeEntityItem(theEntity); _tree->setContainingElement(entityItemID, NULL); - - qDebug() << "DeleteEntityOperator::PreRecursion().... calling... delete theEntity"; delete theEntity; // now actually delete it! - _foundCount++; } } diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index ceaeb8c8bf..42b08f373b 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/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 { diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 9236a1d4f4..91300ee506 100644 --- a/libraries/entities/src/EntityItem.h +++ b/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; } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 181c8bd596..833b74d768 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/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 { diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 5d7d403611..1fdeaa2b0d 100644 --- a/libraries/entities/src/EntityTree.h +++ b/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(_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 diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp index e0e288fe75..1b40015f08 100644 --- a/libraries/entities/src/EntityTreeElement.cpp +++ b/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(); diff --git a/libraries/entities/src/EntityTreeElement.h b/libraries/entities/src/EntityTreeElement.h index 00546ea912..f64b972ae1 100644 --- a/libraries/entities/src/EntityTreeElement.h +++ b/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); diff --git a/libraries/entities/src/ModelEntityItem.cpp b/libraries/entities/src/ModelEntityItem.cpp index 3fcae7fa31..c549b6b341 100644 --- a/libraries/entities/src/ModelEntityItem.cpp +++ b/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 { diff --git a/libraries/entities/src/ModelEntityItem.h b/libraries/entities/src/ModelEntityItem.h index 9aa5b1787b..00061f8b06 100644 --- a/libraries/entities/src/ModelEntityItem.h +++ b/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 diff --git a/libraries/entities/src/todo.txt b/libraries/entities/src/todo.txt index f4fa458e02..767a9317a5 100644 --- a/libraries/entities/src/todo.txt +++ b/libraries/entities/src/todo.txt @@ -1,23 +1,7 @@ // REQUIRED: - - 8) Make sure EntityItems are deleted... - 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() + 10) Lifetime?? 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 @@ -231,7 +215,7 @@ // SOLVED -- 2) verify shadows work // SOLVED -- 9) Handle the ID -> UUID swap in old files to new files - verify old files read correctly // SOLVED -- 2) Test models -> attachments logic --- TESTED/WORKS -// SOLVED -- 1) Import/Export Models - verify it works. /copy/paste?? +// SOLVED -- 1) Import/Export Models - verify it works. /copy/paste?? // DONE -- 22d) void ModelTree::findModelsInCube(const AACube& cube, QVector& foundModels)... // DONE -- 22e) void ModelTreeElement::getModelsInside(const AACube& box, QVector& foundModels)... // DONE -- 22f) Application::exportEntities() tested/works @@ -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 diff --git a/libraries/octree/src/Octree.cpp b/libraries/octree/src/Octree.cpp index a9b9b0d3ea..05539ef767 100644 --- a/libraries/octree/src/Octree.cpp +++ b/libraries/octree/src/Octree.cpp @@ -51,9 +51,8 @@ Octree::Octree(bool shouldReaverage) : } Octree::~Octree() { - // delete the children of the root element - // this recursively deletes the tree - delete _rootElement; + // This will delete all children, don't create a new root in this case. + eraseAllOctreeElements(false); } // 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 - _rootElement = createNewElement(); + if (createNewRoot) { + _rootElement = createNewElement(); + } else { + _rootElement = NULL; + } _isDirty = true; } diff --git a/libraries/octree/src/Octree.h b/libraries/octree/src/Octree.h index e31ab986c3..5901896edb 100644 --- a/libraries/octree/src/Octree.h +++ b/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);