Merge pull request #3372 from ZappoMan/entitiesCrashes

Fixes the several entity crashes

libraries/entities/src/DeleteEntityOperator.cpp

@@ -125,6 +125,12 @@ bool DeleteEntityOperator::postRecursion(OctreeElement* element) {
     if ((subTreeContainsSomeEntitiesToDelete(element))) {
         element->markWithChangedTime();
     }
+    
+    // It should always be ok to prune children. Because we are only in this PostRecursion function if
+    // we've already finished processing all of the children of this current element. If any of those
+    // children are the containing element for any entity in our lists of entities to delete, then they
+    // must have already deleted the entity, and they are safe to prune. Since this operation doesn't
+    // ever add any elements we don't have to worry about memory being reused within this recursion pass.
     EntityTreeElement* entityTreeElement = static_cast<EntityTreeElement*>(element);
     entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves
     return keepSearching; // if we haven't yet found it, keep looking

libraries/entities/src/EntityTree.cpp

@@ -39,6 +39,9 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
     }
     _entityToElementMap.clear();
     Octree::eraseAllOctreeElements(createNewRoot);
+    _movingEntities.clear();
+    _changingEntities.clear();
+    _mortalEntities.clear();
 }
 
 bool EntityTree::handlesEditPacketType(PacketType packetType) const {
@@ -859,6 +862,7 @@ void EntityTree::forgetEntitiesDeletedBefore(quint64 sinceTime) {
 
 // TODO: consider consolidating processEraseMessageDetails() and processEraseMessage()
 int EntityTree::processEraseMessage(const QByteArray& dataByteArray, const SharedNodePointer& sourceNode) {
+    lockForWrite();
     const unsigned char* packetData = (const unsigned char*)dataByteArray.constData();
     const unsigned char* dataAt = packetData;
     size_t packetLength = dataByteArray.size();
@@ -901,10 +905,13 @@ int EntityTree::processEraseMessage(const QByteArray& dataByteArray, const Share
         }
         deleteEntities(entityItemIDsToDelete);
     }
+    unlock();
+    
     return processedBytes;
 }
 
 // This version skips over the header
+// NOTE: Caller must lock the tree before calling this.
 // TODO: consider consolidating processEraseMessageDetails() and processEraseMessage()
 int EntityTree::processEraseMessageDetails(const QByteArray& dataByteArray, const SharedNodePointer& sourceNode) {
     const unsigned char* packetData = (const unsigned char*)dataByteArray.constData();
@@ -938,7 +945,6 @@ int EntityTree::processEraseMessageDetails(const QByteArray& dataByteArray, cons
         }
         deleteEntities(entityItemIDsToDelete);
     }
-    
     return processedBytes;
 }
 

libraries/entities/src/MovingEntitiesOperator.cpp

@@ -38,6 +38,7 @@ void MovingEntitiesOperator::addEntityToMoveList(EntityItem* entity, const AACub
         // check our tree, to determine if this entity is known
         EntityToMoveDetails details;
         details.oldContainingElement = oldContainingElement;
+        details.oldContainingElementCube = oldContainingElement->getAACube();
         details.entity = entity;
         details.oldFound = false;
         details.newFound = false;
@@ -123,8 +124,30 @@ bool MovingEntitiesOperator::postRecursion(OctreeElement* element) {
         element->markWithChangedTime();
     }
     
-    EntityTreeElement* entityTreeElement = static_cast<EntityTreeElement*>(element);
-    entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves
+
+
+    // It's not OK to prune if we have the potential of deleting the original containig element.
+    // because if we prune the containing element then new might end up reallocating the same memory later 
+    // and that will confuse our logic.
+    // 
+    // it's ok to prune if:
+    // 2) this subtree doesn't contain any old elements
+    // 3) this subtree contains an old element, but this element isn't a direct parent of any old containing element
+
+    bool elementSubTreeContainsOldElements = false;
+    bool elementIsDirectParentOfOldElment = false;
+    foreach(const EntityToMoveDetails& details, _entitiesToMove) {
+        if (element->getAACube().contains(details.oldContainingElementCube)) {
+            elementSubTreeContainsOldElements = true;
+        }
+        if (element->isParentOf(details.oldContainingElement)) {
+            elementIsDirectParentOfOldElment = true;
+        }
+    }
+    if (!elementSubTreeContainsOldElements || !elementIsDirectParentOfOldElment) {
+        EntityTreeElement* entityTreeElement = static_cast<EntityTreeElement*>(element);
+        entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves
+    }
 
     return keepSearching; // if we haven't yet found it, keep looking
 }

libraries/entities/src/MovingEntitiesOperator.h

@@ -19,6 +19,7 @@ public:
     AACube newCube;
     AABox newBox;
     EntityTreeElement* oldContainingElement;
+    AACube oldContainingElementCube;
     bool oldFound;
     bool newFound;
 };