Merge pull request #6500 from Atlante45/octree_crash

Octree crash fix

assignment-client/src/octree/OctreeQueryNode.cpp

@@ -59,7 +59,6 @@ OctreeQueryNode::~OctreeQueryNode() {
 
 void OctreeQueryNode::nodeKilled() {
     _isShuttingDown = true;
-    elementBag.unhookNotifications(); // if our node is shutting down, then we no longer need octree element notifications
     if (_octreeSendThread) {
         // just tell our thread we want to shutdown, this is asynchronous, and fast, we don't need or want it to block
         // while the thread actually shuts down
@@ -69,7 +68,6 @@ void OctreeQueryNode::nodeKilled() {
 
 void OctreeQueryNode::forceNodeShutdown() {
     _isShuttingDown = true;
-    elementBag.unhookNotifications(); // if our node is shutting down, then we no longer need octree element notifications
     if (_octreeSendThread) {
         // we really need to force our thread to shutdown, this is synchronous, we will block while the thread actually
         // shuts down because we really need it to shutdown, and it's ok if we wait for it to complete
@@ -338,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++;
@@ -348,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

@@ -424,8 +424,11 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus
                     quint64 lockWaitEnd = usecTimestampNow();
                     lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart);
                     quint64 encodeStart = usecTimestampNow();
-
+                    
                     OctreeElementPointer subTree = nodeData->elementBag.extract();
+                    if (!subTree) {
+                        return;
+                    }
 
                     /* TODO: Looking for a way to prevent locking and encoding a tree that is not
                     // going to result in any packets being sent...

assignment-client/src/octree/OctreeSendThread.h

@@ -17,7 +17,6 @@
 #include <atomic>
 
 #include <GenericThread.h>
-#include <OctreeElementBag.h>
 
 #include "OctreeQueryNode.h"
 

libraries/entities/src/EntityTree.cpp

@@ -44,12 +44,7 @@ void EntityTree::createRootElement() {
 }
 
 OctreeElementPointer EntityTree::createNewElement(unsigned char* octalCode) {
-    EntityTreeElementPointer newElement = EntityTreeElementPointer(new EntityTreeElement(octalCode),
+    auto newElement = EntityTreeElementPointer(new EntityTreeElement(octalCode));
-                                                                   // see comment int EntityTreeElement::createNewElement
-                                                                   [=](EntityTreeElement* dyingElement) {
-                                                                       EntityTreeElementPointer tmpSharedPointer(dyingElement);
-                                                                       dyingElement->notifyDeleteHooks();
-                                                                   });
     newElement->setTree(std::static_pointer_cast<EntityTree>(shared_from_this()));
     return std::static_pointer_cast<OctreeElement>(newElement);
 }

libraries/entities/src/EntityTreeElement.cpp

@@ -28,29 +28,8 @@ EntityTreeElement::~EntityTreeElement() {
     _octreeMemoryUsage -= sizeof(EntityTreeElement);
 }
 
-// This will be called primarily on addChildAt(), which means we're adding a child of our
-// own type to our own tree. This means we should initialize that child with any tree and type
-// specific settings that our children must have.
 OctreeElementPointer EntityTreeElement::createNewElement(unsigned char* octalCode) {
-    EntityTreeElementPointer newChild =
+    auto newChild = EntityTreeElementPointer(new EntityTreeElement(octalCode));
-        EntityTreeElementPointer(new EntityTreeElement(octalCode),
-                                 // This is a little bit horrible, but I haven't found a better way.  The OctreeElement
-                                 // destructor used to call notifyDeleteHooks(), which calls zero or more of
-                                 //   OctreeElementDeleteHook::elementDeleted
-                                 // which (now) expects an OctreeElementPointer argument.  The destructor doesn't have
-                                 // access to the shared pointer (which has had its reference count drop to zero,
-                                 // or the destructor wouldn't have been called).  The destructor also can't
-                                 // make a new shared pointer -- shared_from_this() is forbidden in a destructor, and
-                                 // using OctreeElementPointer(this) also fails.  So, I've installed a custom deleter:
-                                 [=](EntityTreeElement* dyingElement) {
-                                     // make a new shared pointer with a reference count of 1 (and no custom deleter)
-                                     EntityTreeElementPointer tmpSharedPointer(dyingElement);
-                                     // call notifyDeleteHooks which will use shared_from_this() to get this same
-                                     // shared pointer, for use with the elementDeleted calls.
-                                     dyingElement->notifyDeleteHooks();
-                                     // And now tmpSharedPointer's reference count drops to zero and the
-                                     // normal destructors are called.
-                                 });
     newChild->setTree(_myTree);
     return newChild;
 }
@@ -174,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.cpp

@@ -95,10 +95,6 @@ void OctreeElement::init(unsigned char * octalCode) {
 }
 
 OctreeElement::~OctreeElement() {
-    // We can't call notifyDeleteHooks from here:
-    //   notifyDeleteHooks();
-    // see comment in EntityTreeElement::createNewElement.
-    assert(_deleteHooksNotified);
     _voxelNodeCount--;
     if (isLeaf()) {
         _voxelNodeLeafCount--;
@@ -115,7 +111,6 @@ OctreeElement::~OctreeElement() {
 
 void OctreeElement::markWithChangedTime() {
     _lastChanged = usecTimestampNow();
-    notifyUpdateHooks(); // if the node has changed, notify our hooks
 }
 
 // This method is called by Octree when the subtree below this node
@@ -522,57 +517,6 @@ float OctreeElement::distanceToPoint(const glm::vec3& point) const {
     return distance;
 }
 
-QReadWriteLock OctreeElement::_deleteHooksLock;
-std::vector<OctreeElementDeleteHook*> OctreeElement::_deleteHooks;
-
-void OctreeElement::addDeleteHook(OctreeElementDeleteHook* hook) {
-    _deleteHooksLock.lockForWrite();
-    _deleteHooks.push_back(hook);
-    _deleteHooksLock.unlock();
-}
-
-void OctreeElement::removeDeleteHook(OctreeElementDeleteHook* hook) {
-    _deleteHooksLock.lockForWrite();
-    for (unsigned int i = 0; i < _deleteHooks.size(); i++) {
-        if (_deleteHooks[i] == hook) {
-            _deleteHooks.erase(_deleteHooks.begin() + i);
-            break;
-        }
-    }
-    _deleteHooksLock.unlock();
-}
-
-void OctreeElement::notifyDeleteHooks() {
-    _deleteHooksLock.lockForRead();
-    for (unsigned int i = 0; i < _deleteHooks.size(); i++) {
-        _deleteHooks[i]->elementDeleted(shared_from_this());
-    }
-    _deleteHooksLock.unlock();
-    _deleteHooksNotified = true;
-}
-
-std::vector<OctreeElementUpdateHook*> OctreeElement::_updateHooks;
-
-void OctreeElement::addUpdateHook(OctreeElementUpdateHook* hook) {
-    _updateHooks.push_back(hook);
-}
-
-void OctreeElement::removeUpdateHook(OctreeElementUpdateHook* hook) {
-    for (unsigned int i = 0; i < _updateHooks.size(); i++) {
-        if (_updateHooks[i] == hook) {
-            _updateHooks.erase(_updateHooks.begin() + i);
-            return;
-        }
-    }
-}
-
-void OctreeElement::notifyUpdateHooks() {
-    for (unsigned int i = 0; i < _updateHooks.size(); i++) {
-        _updateHooks[i]->elementUpdated(shared_from_this());
-    }
-}
-
-
 bool OctreeElement::findSpherePenetration(const glm::vec3& center, float radius,
                         glm::vec3& penetration, void** penetratedObject) const {
     // center and radius are in meters, so we have to scale the _cube into world-frame

libraries/octree/src/OctreeElement.h

@@ -32,28 +32,15 @@ using AtomicUIntStat = std::atomic<uintmax_t>;
 class EncodeBitstreamParams;
 class Octree;
 class OctreeElement;
-class OctreeElementBag;
-class OctreeElementDeleteHook;
 class OctreePacketData;
 class ReadBitstreamToTreeParams;
 class Shape;
 class VoxelSystem;
-typedef std::shared_ptr<OctreeElement> OctreeElementPointer;
-typedef std::shared_ptr<const OctreeElement> ConstOctreeElementPointer;
-typedef std::shared_ptr<Octree> OctreePointer;
-
-// Callers who want delete hook callbacks should implement this class
-class OctreeElementDeleteHook {
-public:
-    virtual void elementDeleted(OctreeElementPointer element) = 0;
-};
-
-// Callers who want update hook callbacks should implement this class
-class OctreeElementUpdateHook {
-public:
-    virtual void elementUpdated(OctreeElementPointer element) = 0;
-};
 
+using OctreeElementPointer = std::shared_ptr<OctreeElement>;
+using OctreeElementWeakPointer = std::weak_ptr<OctreeElement>;
+using ConstOctreeElementPointer = std::shared_ptr<const OctreeElement>;
+using OctreePointer = std::shared_ptr<Octree>;
 
 class OctreeElement: public std::enable_shared_from_this<OctreeElement> {
 
@@ -103,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 
@@ -181,12 +168,6 @@ public:
     bool matchesSourceUUID(const QUuid& sourceUUID) const;
     static uint16_t getSourceNodeUUIDKey(const QUuid& sourceUUID);
 
-    static void addDeleteHook(OctreeElementDeleteHook* hook);
-    static void removeDeleteHook(OctreeElementDeleteHook* hook);
-
-    static void addUpdateHook(OctreeElementUpdateHook* hook);
-    static void removeUpdateHook(OctreeElementUpdateHook* hook);
-
     static void resetPopulationStatistics();
     static unsigned long getNodeCount() { return _voxelNodeCount; }
     static unsigned long getInternalNodeCount() { return _voxelNodeCount - _voxelNodeLeafCount; }
@@ -244,8 +225,6 @@ protected:
     void setChildAtIndex(int childIndex, OctreeElementPointer child);
 
     void calculateAACube();
-    void notifyDeleteHooks();
-    void notifyUpdateHooks();
 
     AACube _cube; /// Client and server, axis aligned box for bounds of this voxel, 48 bytes
 
@@ -287,14 +266,6 @@ protected:
          _unknownBufferIndex : 1,
          _childrenExternal : 1; /// Client only, is this voxel's VBO buffer the unknown buffer index, 1 bit
 
-    bool _deleteHooksNotified = false;
-
-    static QReadWriteLock _deleteHooksLock;
-    static std::vector<OctreeElementDeleteHook*> _deleteHooks;
-
-    //static QReadWriteLock _updateHooksLock;
-    static std::vector<OctreeElementUpdateHook*> _updateHooks;
-
     static AtomicUIntStat _voxelNodeCount;
     static AtomicUIntStat _voxelNodeLeafCount;
 

libraries/octree/src/OctreeElementBag.cpp

@@ -12,54 +12,30 @@
 #include "OctreeElementBag.h"
 #include <OctalCode.h>
 
-OctreeElementBag::OctreeElementBag() : 
-    _bagElements()
-{
-    OctreeElement::addDeleteHook(this);
-    _hooked = true;
-}
-
-OctreeElementBag::~OctreeElementBag() {
-    unhookNotifications();
-    deleteAll();
-}
-
-void OctreeElementBag::unhookNotifications() {
-    if (_hooked) {
-        OctreeElement::removeDeleteHook(this);
-        _hooked = false;
-    }
-}
-
-void OctreeElementBag::elementDeleted(OctreeElementPointer element) {
-    remove(element); // note: remove can safely handle nodes that aren't in it, so we don't need to check contains()
-}
-
-
 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);
+    _bagElements.push(element);
 }
 
 OctreeElementPointer OctreeElementBag::extract() {
-    OctreeElementPointer result = NULL;
+    OctreeElementPointer result;
 
-    if (_bagElements.size() > 0) {
+    // Find the first element still alive
-        QSet<OctreeElementPointer>::iterator front = _bagElements.begin();
+    while (!result && !_bagElements.empty()) {
-        result = *front;
+        result = _bagElements.front().lock(); // Grab head's shared_ptr
-        _bagElements.erase(front);
+        _bagElements.pop();
     }
     return result;
 }
-
-bool OctreeElementBag::contains(OctreeElementPointer element) {
-    return _bagElements.contains(element);
-}
-
-void OctreeElementBag::remove(OctreeElementPointer element) {
-    _bagElements.remove(element);
-}

libraries/octree/src/OctreeElementBag.h

@@ -16,31 +16,24 @@
 #ifndef hifi_OctreeElementBag_h
 #define hifi_OctreeElementBag_h
 
+#include <queue>
+
 #include "OctreeElement.h"
 
-class OctreeElementBag : public OctreeElementDeleteHook {
+class OctreeElementBag {
-
+    using Bag = std::queue<OctreeElementWeakPointer>;
+    
 public:
-    OctreeElementBag();
-    ~OctreeElementBag();
-
     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();
-    virtual void elementDeleted(OctreeElementPointer element);
-
-    void unhookNotifications();
 
 private:
-    QSet<OctreeElementPointer> _bagElements;
+    Bag _bagElements;
-    bool _hooked;
 };
 
-typedef QMap<const OctreeElement*, void*> OctreeElementExtraEncodeData;
+using OctreeElementExtraEncodeData = QMap<const OctreeElement*, void*>;
 
 #endif // hifi_OctreeElementBag_h