Cleaning up of the octree as items are removed

libraries/render/src/render/Octree.cpp

@@ -95,7 +95,7 @@ Octree::Indices Octree::indexConcreteCellPath(const Locations& path) const {
     for (int l = 1; l < path.size(); l++) {
         auto& location = path[l];
         auto nextIndex = getConcreteCell(currentIndex).child(location.octant());
-        if (nextIndex == INVALID) {
+        if (nextIndex == INVALID_CELL) {
             break;
         }
 
@@ -110,7 +110,7 @@ Octree::Indices Octree::indexConcreteCellPath(const Locations& path) const {
 Octree::Index Octree::allocateCell(Index parent, const Location& location) {
 
     if (_cells[parent].hasChild(location.octant())) {
-        assert(_cells[parent].child(location.octant()) == INVALID);
+        assert(_cells[parent].child(location.octant()) == INVALID_CELL);
         return _cells[parent].child(location.octant());
     }
 
@@ -119,6 +119,10 @@ Octree::Index Octree::allocateCell(Index parent, const Location& location) {
     Index newIndex;
     if (_freeCells.empty()) {
         newIndex = (Index)_cells.size();
+        if (newIndex >= MAXIMUM_INDEX) {
+            // abort! we are trying to go overboard with the total number of allocated bricks
+            return INVALID_CELL;
+        }
         _cells.push_back(Cell(parent, location));
     } else {
         newIndex = _freeCells.back();
@@ -139,15 +143,21 @@ void Octree::freeCell(Index index) {
     }
 }
 
-void Octree::clearCell(Index index) {
+void Octree::cleanCellBranch(Index index) {
     auto& cell = editCell(index);
     
     // Free the brick
-    if (cell.hasBrick()) {
+    if (cell.isBrickEmpty()) {
-        freeBrick(cell.brick());
+        if (cell.hasBrick()) {
-        cell.setBrick(INVALID);
+            freeBrick(cell.brick());
+            cell.setBrick(INVALID_CELL);
+        }
+    } else {
+        // If the brick is still filled, stop clearing
+        return;
     }
 
+
     // Free the cell ?
     Index parentIdx = cell.parent();
     if (!cell.hasParent()) {
@@ -155,7 +165,7 @@ void Octree::clearCell(Index index) {
             // Stop here, this is the root cell!
             return;
         } else {
-            // THis is not expected
+            // This is not expected
             assert(false);
             return;
         }
@@ -163,13 +173,13 @@ void Octree::clearCell(Index index) {
 
     bool traverseParent = false;
     if (!cell.hasChildren()) {
-        editCell(parentIdx).setChild(cell.getlocation().octant(), INVALID);
+        editCell(parentIdx).setChild(cell.getlocation().octant(), INVALID_CELL);
         freeCell(index);
         traverseParent = true;
     }
 
     if (traverseParent) {
-        clearCell(parentIdx);
+        cleanCellBranch(parentIdx);
     }
 }
 
@@ -189,15 +199,28 @@ Octree::Indices Octree::indexCellPath(const Locations& path) {
         // One more cell index on the path, moving on
         currentIndex = newIndex;
         cellPath.push_back(currentIndex);
+
+        // Except !!! if we actually couldn't allocate anymore
+        if (newIndex == INVALID_CELL) {
+            // no more cellID available, stop allocating
+            // THe last index added is INVALID_CELL so the caller will know we failed allocating everything
+            break;
+        }
     }
 
     return cellPath;
 }
 
-
 Octree::Index Octree::allocateBrick() {
     if (_freeBricks.empty()) {
         Index brickIdx = (int)_bricks.size();
+        if (brickIdx >= MAXIMUM_INDEX) {
+            // abort! we are trying to go overboard with the total number of allocated bricks
+            assert(false);
+            // This should never happen because Bricks are allocated along with the cells and there
+            // is already a cap on the cells allocation
+            return INVALID_CELL;
+        }
         _bricks.push_back(Brick());
         return brickIdx;
     } else {
@@ -208,7 +231,7 @@ Octree::Index Octree::allocateBrick() {
 }
 
 void Octree::freeBrick(Index index) {
-    if (checkCellIndex(index)) {
+    if (checkBrickIndex(index)) {
         auto & brick = _bricks[index];
         //  brick.free();
         _freeBricks.push_back(index);
@@ -218,18 +241,25 @@ void Octree::freeBrick(Index index) {
 Octree::Index Octree::accessCellBrick(Index cellID, const CellBrickAccessor& accessor, bool createBrick) {
     assert(checkCellIndex(cellID));
     auto& cell = editCell(cellID);
+
+    // Allocate a brick if needed
     if (!cell.hasBrick()) {
         if (!createBrick) {
-            return INVALID;
+            return INVALID_CELL;
         }
-        cell.setBrick(allocateBrick());
+        auto newBrick = allocateBrick();
+        if (newBrick == INVALID_CELL) {
+            // This should never happen but just in case...
+            return INVALID_CELL;
+        }
+        cell.setBrick(newBrick);
     }
 
-    // access the brick
+    // Access the brick
     auto brickID = cell.brick();
     auto& brick = _bricks[brickID];
 
-    // execute the accessor
+    // Execute the accessor
     accessor(cell, brick, brickID);
 
     return brickID;
@@ -255,13 +285,17 @@ ItemSpatialTree::Index ItemSpatialTree::insertItem(Index cellIdx, const ItemKey&
 
         itemIn.push_back(item);
 
-        cell.signalBrickFilled();
+        cell.setBrickFilled();
     }, true);
 
     return cellIdx;
 }
 
 bool ItemSpatialTree::updateItem(Index cellIdx, const ItemKey& oldKey, const ItemKey& key, const ItemID& item) {
+    // In case we missed that one, nothing to do
+    if (cellIdx == INVALID_CELL) {
+        return true;
+    }
     auto success = false;
 
     // only if key changed
@@ -280,6 +314,10 @@ bool ItemSpatialTree::updateItem(Index cellIdx, const ItemKey& oldKey, const Ite
 }
 
 bool ItemSpatialTree::removeItem(Index cellIdx, const ItemKey& key, const ItemID& item) {
+    // In case we missed that one, nothing to do
+    if (cellIdx == INVALID_CELL) {
+        return true;
+    }
     auto success = false;
 
     // Remove the item from the brick
@@ -290,15 +328,15 @@ bool ItemSpatialTree::removeItem(Index cellIdx, const ItemKey& key, const ItemID
         itemList.erase(std::find(itemList.begin(), itemList.end(), item));
 
         if (brick.items.empty() && brick.subcellItems.empty()) {
-            cell.signalBrickEmpty();
+            cell.setBrickEmpty();
             emptyCell = true;
         }
         success = true;
     }, false); // do not create brick!
 
-
+    // Because we know the cell is now empty, lets try to clean the octree here
     if (emptyCell) {
-       clearCell(cellIdx);
+        cleanCellBranch(cellIdx);
     }
 
     return success;
@@ -323,8 +361,16 @@ ItemSpatialTree::Index ItemSpatialTree::resetItem(Index oldCell, const ItemKey&
 
     auto newCell = indexCell(location);
 
+    // Did we fail finding a cell for the item?
+    if (newCell == INVALID_CELL) {
+        // Remove the item where it was
+        if (oldCell != INVALID_CELL) {
+            removeItem(oldCell, oldKey, item);
+        }
+        return newCell;
+    }
     // Staying in the same cell
-    if (newCell == oldCell) {
+    else if (newCell == oldCell) {
         // Did the key changed, if yes update
         if (newKey._flags != oldKey._flags) {
             updateItem(newCell, oldKey, newKey, item);
@@ -333,7 +379,7 @@ ItemSpatialTree::Index ItemSpatialTree::resetItem(Index oldCell, const ItemKey&
         return newCell;
     }
     // do we know about this item ?
-    else if (oldCell == Item::INVALID_CELL) {
+    else if (oldCell == INVALID_CELL) {
         insertItem(newCell, newKey, item);
         return newCell;
     }
@@ -449,7 +495,7 @@ int Octree::selectTraverse(Index cellID, CellSelection& selection, const Frustum
             // then traverse deeper
             for (int i = 0; i < NUM_OCTANTS; i++) {
                 Index subCellID = cell.child((Link)i);
-                if (subCellID != INVALID) {
+                if (subCellID != INVALID_CELL) {
                     selectTraverse(subCellID, selection, selector);
                 }
             }
@@ -476,7 +522,7 @@ int  Octree::selectBranch(Index cellID, CellSelection& selection, const FrustumS
     // then traverse deeper
     for (int i = 0; i < NUM_OCTANTS; i++) {
         Index subCellID = cell.child((Link)i);
-        if (subCellID != INVALID) {
+        if (subCellID != INVALID_CELL) {
             selectBranch(subCellID, selection, selector);
         }
     }

libraries/render/src/render/Octree.h

@@ -17,6 +17,7 @@
 #include <cassert>
 #include <array>
 #include <functional>
+#include <limits>
 #include <glm/glm.hpp>
 #include <glm/gtx/bit.hpp>
 #include <AABox.h>
@@ -165,23 +166,66 @@ namespace render {
 
         // Cell or Brick Indexing
         using Index = ItemCell; // int32_t
-        static const Index INVALID = -1;
+        static const Index INVALID_CELL = -1;
         static const Index ROOT_CELL = 0;
+
+        // With a maximum of INT_MAX(2 ^ 31) cells in our octree
+        static const Index MAXIMUM_INDEX = INT_MAX; // For fun, test setting this to 100 and this should still works with only the allocated maximum number of cells
+
         using Indices = std::vector<Index>;
 
         // the cell description
         class Cell {
         public:
-            void free() { _location = Location(); for (auto& link : _links) { link = INVALID; } }
+
+            enum BitFlags : uint8_t {
+                HasChildren = 0x01,
+                BrickFilled = 0x02,
+            };
+
+            void free() { _location = Location(); for (auto& link : _links) { link = INVALID_CELL; } }
 
             const Location& getlocation() const { return _location; }
 
             Index parent() const { return _links[Parent]; }
-            bool hasParent() const { return parent() != INVALID; }
+            bool hasParent() const { return parent() != INVALID_CELL; }
 
             Index child(Link octant) const { return _links[octant]; }
-            bool hasChild(Link octant) const { return child(octant) != INVALID; }
+            bool hasChild(Link octant) const { return child(octant) != INVALID_CELL; }
-            bool hasChildren() const {
+            bool hasChildren() const { return (_location.spare & HasChildren); }
+            void setChild(Link octant, Index child) {
+                _links[octant] = child;
+                if (child != INVALID_CELL) {
+                    _location.spare |= HasChildren;
+                } else {
+                    if (!checkHasChildren()) {
+                        _location.spare &= ~HasChildren;
+                    }
+                }
+            }
+
+            Index brick() const { return _links[BrickLink]; }
+            bool hasBrick() const { return _links[BrickLink] != INVALID_CELL; }
+            void setBrick(Index brick) { _links[BrickLink] = brick; }
+
+            void setBrickFilled() { _location.spare |= BrickFilled; }
+            void setBrickEmpty() { _location.spare &= ~BrickFilled; }
+            bool isBrickEmpty() const { return !(_location.spare & BrickFilled); }
+
+            Cell() :
+                _links({ { INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL } })
+            {}
+
+            Cell(Index parent, Location loc) :
+                _location(loc),
+                _links({ { INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, INVALID_CELL, parent, INVALID_CELL } })
+            {}
+
+        private:
+            std::array<Index, NUM_LINKS> _links;
+            Location _location;
+
+            bool checkHasChildren() const {
                 for (LinkStorage octant = Octant_L_B_N; octant < NUM_OCTANTS; octant++) {
                     if (hasChild((Link)octant)) {
                         return true;
@@ -189,42 +233,21 @@ namespace render {
                 }
                 return false;
             }
-            void setChild(Link octant, Index child) { _links[octant] = child; }
-
-            Index brick() const { return _links[BrickLink]; }
-            bool hasBrick() const { return _links[BrickLink] != INVALID; }
-            void setBrick(Index brick) { _links[BrickLink] = brick; }
-            void signalBrickFilled() { _location.spare = 1; }
-            void signalBrickEmpty() { _location.spare = 0; }
-            bool isBrickEmpty() const { return _location.spare == 0; }
-
-            Cell() :
-                _links({ { INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID } })
-            {}
-
-            Cell(Index parent, Location loc) :
-                _location(loc),
-                _links({ { INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, parent, INVALID } })
-            {}
-
-        private:
-            std::array<Index, NUM_LINKS> _links;
-            Location _location;
         };
         using Cells = std::vector< Cell >;
 
         using Bricks = std::vector< Brick >;
 
         bool checkCellIndex(Index index) const { return (index >= 0) && (index < _cells.size()); }
-        bool checkBrickIndex(Index index) const { return (index >= 0) && (index < _bricks.size()); }
+        bool checkBrickIndex(Index index) const { return ((index >= 0) && (index < _bricks.size())); }
 
         Octree() {};
 
-        // Clear a cell: 
+        // Clean a cell branch starting from the leave: 
-        // Check that the cell brick is empty, if so free it
+        // Check that the cell brick is empty, if so free it else stop
-        // CHeck that the cell has no children, if so free itself
+        // Check that the cell has no children, if so free itself else stop
         // Apply the same logic to the parent cell
-        void clearCell(Index index);
+        void cleanCellBranch(Index index);
 
         // Indexing/Allocating the cells as the tree gets populated
         // Return the cell Index/Indices at the specified location/path, allocate all the cells on the path from the root if needed

libraries/render/src/render/drawCellBounds.slv

@@ -63,5 +63,5 @@ void main(void) {
     TransformObject obj = getTransformObject();
     <$transformModelToClipPos(cam, obj, pos, gl_Position)$>
 
-    varColor = vec4(colorWheel(fract(float(inCellLocation.w) / 5.0)), 0.5 + 0.4 * cellIsEmpty);
+    varColor = vec4(colorWheel(fract(float(inCellLocation.w) / 5.0)), 0.8 + 0.2 * cellIsEmpty);
 }