fixed a couple crashes in editing entities

libraries/entities/src/UpdateEntityOperator.cpp

@@ -22,6 +22,7 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree,
     _tree(tree),
     _existingEntity(existingEntity),
     _containingElement(containingElement),
+    _containingElementCube(containingElement->getAACube()), 
     _properties(properties),
     _entityItemID(existingEntity->getEntityItemID()),
     _foundOld(false),
@@ -85,7 +86,7 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree,
     }
 
     _newEntityBox = _newEntityCube.clamp(0.0f, 1.0f); // clamp to domain bounds
-
+    
     if (wantDebug) {
         qDebug() << "    _properties.getPosition()=" << _properties.getPosition() << "in meters";
         qDebug() << "    _properties.getRadius()=" << _properties.getRadius() << "in meters";
@@ -151,13 +152,16 @@ bool UpdateEntityOperator::PreRecursion(OctreeElement* element) {
 
     if (wantDebug) {
         qDebug() << "UpdateEntityOperator::PreRecursion()....";    
-        qDebug() << "    _containingElement=" << _containingElement;
-        qDebug() << "    _containingElement->getAACube()=" << _containingElement->getAACube();
-        qDebug() << "    _foundOld=" << _foundOld;    
-        qDebug() << "    _foundNew=" << _foundNew;
-
         qDebug() << "    entityTreeElement=" << entityTreeElement;
         qDebug() << "    entityTreeElement->getAACube()=" << entityTreeElement->getAACube();
+
+        qDebug() << "    _foundOld=" << _foundOld;    
+        qDebug() << "    _foundNew=" << _foundNew;
+        qDebug() << "    _removeOld=" << _removeOld;
+
+        qDebug() << "    _containingElement=" << _containingElement;
+        qDebug() << "    _containingElementCube=" << _containingElementCube;
+
         qDebug() << "    _oldEntityCube=" << _oldEntityCube;
         qDebug() << "    _oldEntityBox=" << _oldEntityBox;
         qDebug() << "    subtreeContainsOld=" << subtreeContainsOld;
@@ -180,9 +184,9 @@ bool UpdateEntityOperator::PreRecursion(OctreeElement* element) {
         // and we can stop searching.
         if (entityTreeElement == _containingElement) {
 
-        if (wantDebug) {
+            if (wantDebug) {
-            qDebug() << "    OLD BRANCH.... CONTAINING ELEMENT";
+                qDebug() << "    OLD BRANCH.... CONTAINING ELEMENT";
-        }
+            }
         
             // If the containgElement IS NOT the best fit for the new entity properties
             // then we need to remove it, and the updateEntity below will store it in the
@@ -223,7 +227,7 @@ bool UpdateEntityOperator::PreRecursion(OctreeElement* element) {
     
 
         // If this element is the best fit for the new entity properties, then add/or update it
-        if (entityTreeElement->bestFitBounds(_newEntityCube)) {
+        if (entityTreeElement->bestFitBounds(_newEntityBox)) {
 
             if (wantDebug) {
                 qDebug() << "    NEW BRANCH.... BEST FIT";
@@ -266,37 +270,125 @@ bool UpdateEntityOperator::PostRecursion(OctreeElement* element) {
     // We might have two paths, one for the old entity and one for the new entity.
     bool keepSearching = !_foundOld || !_foundNew;
 
+    bool subtreeContainsOld = subTreeContainsOldEntity(element);
+    bool subtreeContainsNew = subTreeContainsNewEntity(element);
+
+    bool wantDebug = false;
+    if (wantDebug) {
+        qDebug() << "UpdateEntityOperator::PostRecursion()...";
+        qDebug() << "    element=" << element;
+        qDebug() << "    element->getAACube()=" << element->getAACube();
+
+        qDebug() << "    subtreeContainsOld=" << subtreeContainsOld;
+        qDebug() << "    subtreeContainsNew=" << subtreeContainsNew;
+
+        qDebug() << "    _foundOld=" << _foundOld;    
+        qDebug() << "    _foundNew=" << _foundNew;
+        qDebug() << "    _removeOld=" << _removeOld;
+        qDebug() << "    keepSearching=" << keepSearching;
+
+        qDebug() << "    _containingElement=" << _containingElement;
+        qDebug() << "    _containingElementCube=" << _containingElementCube;
+
+        qDebug() << "    _properties.getPosition()=" << _properties.getPosition() << "in meters";
+        qDebug() << "    _properties.getRadius()=" << _properties.getRadius() << "in meters";
+        qDebug() << "    _newEntityCube=" << _newEntityCube;
+        qDebug() << "    _newEntityBox=" << _newEntityBox;
+    }
+
+
     // As we unwind, if we're in either of these two paths, we mark our element
     // as dirty.
-    if ((_foundOld && subTreeContainsOldEntity(element)) ||
+    if ((_foundOld && subtreeContainsOld) ||
-            (_foundNew && subTreeContainsNewEntity(element))) {
+            (_foundNew && subtreeContainsNew)) {
         element->markWithChangedTime();
     }
 
-    EntityTreeElement* entityTreeElement = static_cast<EntityTreeElement*>(element);
+    // It's not OK to prune if we have the potential of deleting the original containig element.
-    bool somethingPruned = entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves
+    // because if we prune the containing element then new might end up reallocating the same memory later 
-    bool wantDebug = false;
+    // and that will confuse our logic.
-    if (somethingPruned && wantDebug) {
+    // 
-        qDebug() << "UpdateEntityOperator::PostRecursion() something pruned!!!";
+    // it's ok to prune if:
+    // 1) we're not removing the old
+    // 2) we are removing the old, but this subtree doesn't contain the old
+    // 3) we are removing the old, this subtree contains the old, but this element isn't a direct parent of _containingElement
+    if (!_removeOld || !subtreeContainsOld || !element->isParentOf(_containingElement)) {
+        EntityTreeElement* entityTreeElement = static_cast<EntityTreeElement*>(element);
+        bool somethingPruned = entityTreeElement->pruneChildren(); // take this opportunity to prune any empty leaves
+        if (somethingPruned && wantDebug) {
+            qDebug() << "UpdateEntityOperator::PostRecursion() something pruned!!!";
+        }
     }
     
     return keepSearching; // if we haven't yet found it, keep looking
 }
 
 OctreeElement* UpdateEntityOperator::PossiblyCreateChildAt(OctreeElement* element, int childIndex) { 
+
+    bool wantDebug = false;
+    if (wantDebug) {
+        qDebug() << "UpdateEntityOperator::PossiblyCreateChildAt()...";
+        qDebug() << "    element=" << element;
+        qDebug() << "    element->getAACube()=" << element->getAACube();
+        qDebug() << "    childIndex=" << childIndex;
+
+        bool subtreeContainsOld = subTreeContainsOldEntity(element);
+        bool subtreeContainsNew = subTreeContainsNewEntity(element);
+
+        qDebug() << "    subtreeContainsOld=" << subtreeContainsOld;
+        qDebug() << "    subtreeContainsNew=" << subtreeContainsNew;
+
+        qDebug() << "    _foundOld=" << _foundOld;    
+        qDebug() << "    _foundNew=" << _foundNew;
+        qDebug() << "    _removeOld=" << _removeOld;
+
+        qDebug() << "    _containingElement=" << _containingElement;
+        qDebug() << "    _containingElementCube=" << _containingElementCube;
+
+        qDebug() << "    _properties.getPosition()=" << _properties.getPosition() << "in meters";
+        qDebug() << "    _properties.getRadius()=" << _properties.getRadius() << "in meters";
+        qDebug() << "    _newEntityCube=" << _newEntityCube;
+        qDebug() << "    _newEntityBox=" << _newEntityBox;
+    }
+
     // If we're getting called, it's because there was no child element at this index while recursing.
     // We only care if this happens while still searching for the new entity location.
     // Check to see if 
     if (!_foundNew) {
-    
+        float childElementScale = element->getScale() / 2.0f; // all of our children will be half our scale
-        float childElementScale = element->getAACube().getScale() / 2.0f; // all of our children will be half our scale
-        // if the scale of our desired cube is smaller than our children, then consider making a child
-        if (_newEntityCube.getScale() <= childElementScale) {
-            //qDebug() << "UpdateEntityOperator::PossiblyCreateChildAt().... calling element->getMyChildContaining(_newEntityCube);";
-            int indexOfChildContainingNewEntity = element->getMyChildContaining(_newEntityCube);
         
+        // Note: because the entity's bounds might have been clamped to the domain. We want to check if the
+        // bounds of the clamped box would fit in our child elements. It may be the case that the actual
+        // bounds of the element would hang outside of the child elements cells.
+        bool entityWouldFitInChild = _newEntityBox.getLargestDimension() <= childElementScale;
+
+        if (wantDebug) {
+            qDebug() << "    childElementScale=" << childElementScale;
+            qDebug() << "    _newEntityBox.getLargestDimension()=" << _newEntityBox.getLargestDimension();
+            qDebug() << "    entityWouldFitInChild=" << entityWouldFitInChild;
+        }
+
+        
+        // if the scale of our desired cube is smaller than our children, then consider making a child
+        if (entityWouldFitInChild) {
+            int indexOfChildContainingNewEntity = element->getMyChildContaining(_newEntityBox);
+
+            if (wantDebug) {
+                qDebug() << "    indexOfChildContainingNewEntity=" << indexOfChildContainingNewEntity;
+            }
+
             if (childIndex == indexOfChildContainingNewEntity) {
-                return element->addChildAtIndex(childIndex);
+                return element->addChildAtIndex(childIndex);;
+            }
+        } else {
+            // NOTE: This can happen for good reason. Consider the case where we are recursing BACK UP
+            // from the old location of clamped placement and the new location is "less clamped".
+            // Say the old location was clamped in all three dimensions so its largest clamped dimension was
+            // half the actual entity dimension. But the NEW clamped dimension is only clamped on 
+            // one or two axis. In this case we'll travel back up through cells that are smaller
+            // than our desired cell size.
+            if (wantDebug) {
+                qDebug() << "    cell too small to be interesting";
             }
         }
     }

libraries/entities/src/UpdateEntityOperator.h

@@ -24,6 +24,7 @@ private:
     EntityTree* _tree;
     EntityItem* _existingEntity;
     EntityTreeElement* _containingElement;
+    AACube _containingElementCube; // we temporarily store our cube here in case we need to delete the containing element
     EntityItemProperties _properties;
     EntityItemID _entityItemID;
     bool _foundOld;
@@ -34,8 +35,8 @@ private:
     AACube _oldEntityCube;
     AACube _newEntityCube;
 
-    AABox _oldEntityBox;
+    AABox _oldEntityBox; // clamped to domain
-    AABox _newEntityBox;
+    AABox _newEntityBox; // clamped to domain
 
     bool subTreeContainsOldEntity(OctreeElement* element);
     bool subTreeContainsNewEntity(OctreeElement* element);

libraries/entities/src/todo.txt

@@ -1,7 +1,6 @@
 // REQUIRED:
 
-    1) random crashes on moving (I think things going out of bounds???)
+    1) crash on rendering of element that flies off the domain???
-        -- event with edit clamping, we seem to crash if we move an entity to the Y=0 plane
 
     2) Test file save load for case where two siblings have more than MTU amount of data. I wonder if the fact that file save
        doesn't include the extra exists bits will break something.
@@ -191,7 +190,16 @@
 // SOLVED -- 43) Moving models don't always move.. I think this relates to the render optimization... 
 //               need to make sure we re-simulate when a model is moving...
 // SOLVED -- 44) if velocity sends a model out of the domain - delete it
-// SOLVED -- 6) Don't allow models to be "edited" to be outside of domain. clamp them to the domain. - clamp it baby!!
+// SOLVED -- 45) Don't allow models to be "edited" to be outside of domain. clamp them to the domain. - clamp it baby!!
+// SOLVED -- 46) random crashes on moving -- 
+//        FIXED - bug in PossiblyCreateChildAt for clamped entities - new entity not added.
+//        FIXED - Assertion failed: (!_removeOld), function PreRecursion, file /Users/zappoman/Development/HiFi/hifi/libraries/entities/src/UpdateEntityOperator.cpp, line 234
+//            -- what appears to be happening is that the containing elements memory location is getting reused
+//            -- 1) the OLD containing element was found first,
+//            -- 2) the entity was removed
+//            -- 3) the containing element got pruned
+//            -- 4) the NEW branch is traversed and PossiblyCreateChildAt creates a new element -- THAT HAS SAME POINTER AS CONTAINING ELEMENT!!!!
+//            -- 5) the logic for recursedelement == _containingElement gets confused
 
 ---------------- properties -----------------
 Base properties...

libraries/octree/src/OctreeElement.cpp

@@ -240,6 +240,22 @@ OctreeElement* OctreeElement::removeChildAtIndex(int childIndex) {
     return returnedChild;
 }
 
+bool OctreeElement::isParentOf(OctreeElement* possibleChild) const {
+    bool isParentOf = false;
+    if (possibleChild) {
+        for (int childIndex = 0; childIndex < NUMBER_OF_CHILDREN; childIndex++) {
+            OctreeElement* childAt = getChildAtIndex(childIndex);
+
+            if (childAt == possibleChild) {
+                isParentOf = true;
+                break;
+            }
+        }
+    }
+    return isParentOf;
+}
+
+
 #ifdef HAS_AUDIT_CHILDREN
 void OctreeElement::auditChildren(const char* label) const {
     bool auditFailed = false;

libraries/octree/src/OctreeElement.h

@@ -124,6 +124,7 @@ public:
     OctreeElement* getChildAtIndex(int childIndex) const;
     void deleteChildAtIndex(int childIndex);
     OctreeElement* removeChildAtIndex(int childIndex);
+    bool isParentOf(OctreeElement* possibleChild) const;
 
     /// handles deletion of all descendants, returns false if delete not approved
     bool safeDeepDeleteChildAtIndex(int childIndex, int recursionCount = 0);