diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index dd22425f09..5b0ada4ec1 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/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(element); entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves return keepSearching; // if we haven't yet found it, keep looking diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index d2d87eb111..4380171df2 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -862,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(); @@ -904,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(); @@ -941,7 +945,6 @@ int EntityTree::processEraseMessageDetails(const QByteArray& dataByteArray, cons } deleteEntities(entityItemIDsToDelete); } - return processedBytes; } diff --git a/libraries/entities/src/MovingEntitiesOperator.cpp b/libraries/entities/src/MovingEntitiesOperator.cpp index 07242b0f63..d6ccb1aad4 100644 --- a/libraries/entities/src/MovingEntitiesOperator.cpp +++ b/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(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(element); + entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves + } return keepSearching; // if we haven't yet found it, keep looking } diff --git a/libraries/entities/src/MovingEntitiesOperator.h b/libraries/entities/src/MovingEntitiesOperator.h index 80096184cb..047cc0c52b 100644 --- a/libraries/entities/src/MovingEntitiesOperator.h +++ b/libraries/entities/src/MovingEntitiesOperator.h @@ -19,6 +19,7 @@ public: AACube newCube; AABox newBox; EntityTreeElement* oldContainingElement; + AACube oldContainingElementCube; bool oldFound; bool newFound; };