Make OctreeElementBag safer to use

assignment-client/src/octree/OctreeQueryNode.cpp

@@ -336,8 +336,7 @@ void OctreeQueryNode::dumpOutOfView() {
     int stillInView = 0;
     int outOfView = 0;
     OctreeElementBag tempBag;
-    while (!elementBag.isEmpty()) {
+    while (OctreeElementPointer elementToCheck = elementBag.extract()) {
-        OctreeElementPointer elementToCheck = elementBag.extract();
         if (elementToCheck->isInView(_currentViewFrustum)) {
             tempBag.insert(elementToCheck);
             stillInView++;
@@ -346,8 +345,7 @@ void OctreeQueryNode::dumpOutOfView() {
         }
     }
     if (stillInView > 0) {
-        while (!tempBag.isEmpty()) {
+        while (OctreeElementPointer elementToKeepInBag = tempBag.extract()) {
-            OctreeElementPointer elementToKeepInBag = tempBag.extract();
             if (elementToKeepInBag->isInView(_currentViewFrustum)) {
                 elementBag.insert(elementToKeepInBag);
             }

assignment-client/src/octree/OctreeSendThread.cpp

@@ -417,7 +417,7 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus
             quint64 startInside = usecTimestampNow();
 
             bool lastNodeDidntFit = false; // assume each node fits
-            if (!nodeData->elementBag.isEmpty()) {
+            if (OctreeElementPointer subTree = nodeData->elementBag.extract()) {
 
                 quint64 lockWaitStart = usecTimestampNow();
                 _myServer->getOctree()->withReadLock([&]{
@@ -425,8 +425,6 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus
                     lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart);
                     quint64 encodeStart = usecTimestampNow();
 
-                    OctreeElementPointer subTree = nodeData->elementBag.extract();
-
                     /* TODO: Looking for a way to prevent locking and encoding a tree that is not
                     // going to result in any packets being sent...
                     //

libraries/entities/src/EntityTreeElement.cpp

@@ -153,7 +153,7 @@ void EntityTreeElement::updateEncodedData(int childIndex, AppendState childAppen
 
 
 
-void EntityTreeElement::elementEncodeComplete(EncodeBitstreamParams& params, OctreeElementBag* bag) const {
+void EntityTreeElement::elementEncodeComplete(EncodeBitstreamParams& params) const {
     const bool wantDebug = false;
     
     if (wantDebug) {

libraries/entities/src/EntityTreeElement.h

@@ -122,7 +122,7 @@ public:
     virtual bool shouldIncludeChildData(int childIndex, EncodeBitstreamParams& params) const;
     virtual bool shouldRecurseChildTree(int childIndex, EncodeBitstreamParams& params) const;
     virtual void updateEncodedData(int childIndex, AppendState childAppendState, EncodeBitstreamParams& params) const;
-    virtual void elementEncodeComplete(EncodeBitstreamParams& params, OctreeElementBag* bag) const;
+    virtual void elementEncodeComplete(EncodeBitstreamParams& params) const;
 
     bool alreadyFullyEncoded(EncodeBitstreamParams& params) const;
 

libraries/octree/src/Octree.cpp

@@ -1736,7 +1736,7 @@ int Octree::encodeTreeBitstreamRecursion(OctreeElementPointer element,
 
     // If our element is completed let the element know so it can do any cleanup it of extra  wants
     if (elementAppendState == OctreeElement::COMPLETED) {
-        element->elementEncodeComplete(params, &bag);
+        element->elementEncodeComplete(params);
     }
 
     return bytesAtThisLevel;
@@ -2104,9 +2104,7 @@ void Octree::writeToSVOFile(const char* fileName, OctreeElementPointer element)
         int bytesWritten = 0;
         bool lastPacketWritten = false;
 
-        while (!elementBag.isEmpty()) {
+        while (OctreeElementPointer subTree = elementBag.extract()) {
-            OctreeElementPointer subTree = elementBag.extract();
-
             EncodeBitstreamParams params(INT_MAX, IGNORE_VIEW_FRUSTUM, WANT_COLOR, NO_EXISTS_BITS);
             withReadLock([&] {
                 params.extraEncodeData = &extraEncodeData;

libraries/octree/src/Octree.h

@@ -18,16 +18,6 @@
 #include <QHash>
 #include <QObject>
 
-class CoverageMap;
-class ReadBitstreamToTreeParams;
-class Octree;
-class OctreeElement;
-class OctreeElementBag;
-class OctreePacketData;
-class Shape;
-typedef std::shared_ptr<Octree> OctreePointer;
-
-
 #include <shared/ReadWriteLockable.h>
 #include <SimpleMovingAverage.h>
 
@@ -38,6 +28,13 @@ typedef std::shared_ptr<Octree> OctreePointer;
 #include "OctreePacketData.h"
 #include "OctreeSceneStats.h"
 
+class CoverageMap;
+class ReadBitstreamToTreeParams;
+class Octree;
+class OctreeElement;
+class OctreePacketData;
+class Shape;
+using OctreePointer = std::shared_ptr<Octree>;
 
 extern QVector<QString> PERSIST_EXTENSIONS;
 

libraries/octree/src/OctreeElement.h

@@ -32,7 +32,6 @@ using AtomicUIntStat = std::atomic<uintmax_t>;
 class EncodeBitstreamParams;
 class Octree;
 class OctreeElement;
-class OctreeElementBag;
 class OctreePacketData;
 class ReadBitstreamToTreeParams;
 class Shape;
@@ -91,7 +90,7 @@ public:
     virtual bool shouldRecurseChildTree(int childIndex, EncodeBitstreamParams& params) const { return true; }
     
     virtual void updateEncodedData(int childIndex, AppendState childAppendState, EncodeBitstreamParams& params) const { }
-    virtual void elementEncodeComplete(EncodeBitstreamParams& params, OctreeElementBag* bag) const { }
+    virtual void elementEncodeComplete(EncodeBitstreamParams& params) const { }
 
     /// Override to serialize the state of this element. This is used for persistance and for transmission across the network.
     virtual AppendState appendElementData(OctreePacketData* packetData, EncodeBitstreamParams& params) const 

libraries/octree/src/OctreeElementBag.cpp

@@ -13,29 +13,29 @@
 #include <OctalCode.h>
 
 void OctreeElementBag::deleteAll() {
-    _bagElements.clear();
+    _bagElements = Bag();
+}
+
+bool OctreeElementBag::isEmpty() {
+    // Pop all expired front elements
+    while (!_bagElements.empty() && _bagElements.front().expired()) {
+        _bagElements.pop();
+    }
+    
+    return _bagElements.empty();
 }
 
 void OctreeElementBag::insert(OctreeElementPointer element) {
-    _bagElements.insert(element.get(), element);
+    _bagElements.push(element);
 }
 
 OctreeElementPointer OctreeElementBag::extract() {
     OctreeElementPointer result;
 
     // Find the first element still alive
-    while (!_bagElements.empty() && !result) {
+    while (!result && !_bagElements.empty()) {
-        auto it = _bagElements.begin();
+        result = _bagElements.front().lock(); // Grab head's shared_ptr
-        result = it->lock();
+        _bagElements.pop();
-        _bagElements.erase(it);
     }
     return result;
 }
-
-bool OctreeElementBag::contains(OctreeElementPointer element) {
-    return _bagElements.contains(element.get());
-}
-
-void OctreeElementBag::remove(OctreeElementPointer element) {
-    _bagElements.remove(element.get());
-}

libraries/octree/src/OctreeElementBag.h

@@ -16,25 +16,24 @@
 #ifndef hifi_OctreeElementBag_h
 #define hifi_OctreeElementBag_h
 
+#include <queue>
+
 #include "OctreeElement.h"
-#include <SharedUtil.h>
 
 class OctreeElementBag {
-public:
+    using Bag = std::queue<OctreeElementWeakPointer>;
     
+public:
     void insert(OctreeElementPointer element); // put a element into the bag
     OctreeElementPointer extract(); // pull a element out of the bag (could come in any order)
-    bool contains(OctreeElementPointer element); // is this element in the bag?
+    bool isEmpty();
-    void remove(OctreeElementPointer element); // remove a specific element from the bag
-    bool isEmpty() const { return _bagElements.isEmpty(); }
-    int count() const { return _bagElements.size(); }
     
     void deleteAll();
 
 private:
-    QHash<OctreeElement*, OctreeElementWeakPointer> _bagElements;
+    Bag _bagElements;
 };
 
-typedef QMap<const OctreeElement*, void*> OctreeElementExtraEncodeData;
+using OctreeElementExtraEncodeData = QMap<const OctreeElement*, void*>;
 
 #endif // hifi_OctreeElementBag_h