Refactored Observer/ObserverNodePath and DatabasePager to improve their robustness.

include/osg/Observer

@@ -27,11 +27,6 @@ class OSG_EXPORT Observer
         Observer();
         virtual ~Observer();
 
-        /** Get the optional global observer mutex, this can be shared between all osg::Observer.*/
-        static OpenThreads::Mutex* getGlobalObserverMutex();
-
-        inline OpenThreads::Mutex* getObserverMutex() const { return osg::Observer::getGlobalObserverMutex(); }
-
         /** objectUnreferenced(void*) is called when the observed object's referenced count goes to zero, indicating that
         * the object will be deleted unless a new reference is made to it.  If you wish to prevent deletion of the object
         * then it's reference count should be incremented such as via taking a ref_ptr<> to it, if no refernce is taken
@@ -54,12 +49,13 @@ class OSG_EXPORT ObserverSet
         ObserverSet();
         ~ObserverSet();
 
-        inline OpenThreads::Mutex* getObserverSetMutex() const { return osg::Observer::getGlobalObserverMutex(); }
+        inline OpenThreads::Mutex* getObserverSetMutex() const { return &_mutex; }
 
         void addObserver(Observer* observer);
         void removeObserver(Observer* observer);
 
         void signalObjectUnreferenced(void* ptr);
+
         void signalObjectDeleted(void* ptr);
 
         typedef std::set<Observer*> Observers;
@@ -68,6 +64,7 @@ class OSG_EXPORT ObserverSet
 
     protected:
 
+        mutable OpenThreads::Mutex      _mutex;
         Observers                       _observers;
 };
 

include/osg/ObserverNodePath

@@ -15,17 +15,16 @@
 #define OSG_OBSERVERNODEPATH 1
 
 #include <osg/Node>
-#include <osg/Observer>
+#include <osg/observer_ptr>
-#include <osg/ref_ptr>
+#include <vector>
-#include <list>
 
 namespace osg {
 
-typedef std::list< osg::ref_ptr<osg::Node> > RefNodePath;
+typedef std::vector< osg::ref_ptr<osg::Node> > RefNodePath;
 
 /** ObserverNodePath is an observer class for tracking changes to a NodePath,
   * that automatically invalidates it when nodes are deleted.*/
-class OSG_EXPORT ObserverNodePath : public osg::Observer
+class OSG_EXPORT ObserverNodePath
 {
     public:
         ObserverNodePath();
@@ -38,9 +37,6 @@ class OSG_EXPORT ObserverNodePath : public osg::Observer
 
         ObserverNodePath& operator = (const ObserverNodePath& rhs);
 
-        bool valid() const { return _valid; }
-
-
         /** get the NodePath from the first parental chain back to root, plus the specified node.*/
         void setNodePathTo(osg::Node* node);
 
@@ -61,21 +57,18 @@ class OSG_EXPORT ObserverNodePath : public osg::Observer
 
         bool empty() const
         {
-            if (!_valid) return true;
+            OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_mutex);
-            OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
             return _nodePath.empty();
         }
 
     protected:
 
         void _setNodePath(const osg::NodePath& nodePath);
-
         void _clearNodePath();
 
-        virtual bool objectUnreferenced(void* ptr);
+        typedef std::vector< osg::observer_ptr<osg::Node> > ObsNodePath;
-
+        mutable OpenThreads::Mutex      _mutex;
-        osg::NodePath                   _nodePath;
+        ObsNodePath                     _nodePath;
-        bool                            _valid;
 };
 
 }

include/osgDB/DatabasePager

@@ -345,28 +345,9 @@ class OSGDB_EXPORT DatabasePager : public osg::NodeVisitor::DatabaseRequestHandl
 
         struct RequestQueue;
 
-        struct PagedLODObserver : public osg::observer_ptr<osg::PagedLOD>
+        typedef osg::observer_ptr<osg::PagedLOD> PagedLODObserver;
-        {
-            typedef osg::observer_ptr<osg::PagedLOD> BaseClass;
-
-            PagedLODObserver(osg::PagedLOD* plod):BaseClass(plod) {}
-            PagedLODObserver(const PagedLODObserver& plodObserver):BaseClass(plodObserver) {}
-
-            PagedLODObserver& operator = (const PagedLODObserver& rhs)
-            {
-                (*static_cast<BaseClass*>(this)) = rhs;
-                return *this;
-            }
-
-            virtual void objectDeleted(void* ptr)
-            {
-                BaseClass::objectDeleted(ptr);
-            }
-        };
-
         typedef std::list< PagedLODObserver >                PagedLODList;
 
-
         struct DatabaseRequest : public osg::Referenced
         {
             DatabaseRequest():

src/osg/ObserverNodePath.cpp

@@ -16,23 +16,17 @@
 
 using namespace osg;
 
-ObserverNodePath::ObserverNodePath():
+ObserverNodePath::ObserverNodePath()
-    _valid(false)
 {
 }
 
-ObserverNodePath::ObserverNodePath(const ObserverNodePath& rhs):
+ObserverNodePath::ObserverNodePath(const ObserverNodePath& rhs)
-    _valid(false)
 {
-    RefNodePath refNodePath;
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock_rhs(_mutex);
-    if (rhs.getRefNodePath(refNodePath))
+    _nodePath = rhs._nodePath;
-    {
-        setNodePath(refNodePath);
-    }
 }
 
-ObserverNodePath::ObserverNodePath(const osg::NodePath& nodePath):
+ObserverNodePath::ObserverNodePath(const osg::NodePath& nodePath)
-    _valid(false)
 {
     setNodePath(nodePath);
 }
@@ -46,11 +40,9 @@ ObserverNodePath& ObserverNodePath::operator = (const ObserverNodePath& rhs)
 {
     if (&rhs==this) return *this;
 
-    RefNodePath refNodePath;
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock_rhs(rhs._mutex);
-    if (rhs.getRefNodePath(refNodePath))
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock_lhs(_mutex);
-    {
+    _nodePath = rhs._nodePath;
-        setNodePath(refNodePath);
-    }
     return *this;
 }
 
@@ -82,7 +74,7 @@ void ObserverNodePath::setNodePathTo(osg::Node* node)
 
 void ObserverNodePath::setNodePath(const osg::NodePath& nodePath)
 {
-    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_mutex);
     _setNodePath(nodePath);
 }
 
@@ -98,97 +90,62 @@ void ObserverNodePath::setNodePath(const osg::RefNodePath& refNodePath)
 
 void ObserverNodePath::clearNodePath()
 {
-    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_mutex);
     _clearNodePath();
 }
 
 bool ObserverNodePath::getRefNodePath(RefNodePath& refNodePath) const
 {
-    refNodePath.clear();
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_mutex);
-
+    refNodePath.resize(_nodePath.size());
-    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
+    for(unsigned int i=0; i<_nodePath.size(); ++i)
-    if (!_valid) return false;
-
-    for(osg::NodePath::const_iterator itr = _nodePath.begin();
-        itr != _nodePath.end();
-        ++itr)
     {
-        refNodePath.push_back(*itr);
+        refNodePath[i] = _nodePath[i].lock();
+        if (!refNodePath[i].valid())
+        {
+            OSG_NOTICE<<"ObserverNodePath::getRefNodePath() node has been invalidated"<<std::endl;
+            refNodePath.clear();
+            return false;
+        }
     }
-
     return true;
 }
 
 bool ObserverNodePath::getNodePath(NodePath& nodePath) const
 {
+    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(_mutex);
+    nodePath.resize(_nodePath.size());
+    for(unsigned int i=0; i<_nodePath.size(); ++i)
+    {
+        if (_nodePath[i].valid())
+        {
+            nodePath[i] = _nodePath[i].get();
+        }
+        else
+        {
+            OSG_NOTICE<<"ObserverNodePath::getNodePath() node has been invalidated"<<std::endl;
             nodePath.clear();
-
+            return false;
-    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
+        }
-    if (!_valid) return false;
+    }
-    nodePath = _nodePath;
     return true;
 }
 
 void ObserverNodePath::_setNodePath(const osg::NodePath& nodePath)
 {
-    if (nodePath==_nodePath) return;
-
     _clearNodePath();
 
-    //OSG_NOTICE<<"ObserverNodePath["<<this<<"]::_setNodePath() nodePath.size()="<<nodePath.size()<<std::endl;
+    // OSG_NOTICE<<"ObserverNodePath["<<this<<"]::_setNodePath() nodePath.size()="<<nodePath.size()<<std::endl;
 
-    _nodePath = nodePath;
+    _nodePath.resize(nodePath.size());
-
+    for(unsigned int i=0; i<nodePath.size(); ++i)
-    for(osg::NodePath::iterator itr = _nodePath.begin();
-        itr != _nodePath.end();
-        ++itr)
     {
-        //OSG_NOTICE<<"   addObserver("<<*itr<<")"<<std::endl;
+        _nodePath[i] = nodePath[i];
-        (*itr)->addObserver(this);
     }
-
-    _valid = true;
 }
 
 void ObserverNodePath::_clearNodePath()
 {
-    //OSG_NOTICE<<"ObserverNodePath["<<this<<"]::_clearNodePath() _nodePath.size()="<<_nodePath.size()<<std::endl;
+    // OSG_NOTICE<<"ObserverNodePath["<<this<<"]::_clearNodePath() _nodePath.size()="<<_nodePath.size()<<std::endl;
-    for(osg::NodePath::iterator itr = _nodePath.begin();
-        itr != _nodePath.end();
-        ++itr)
-    {
-        //OSG_NOTICE<<"   removeObserver("<<*itr<<")"<<std::endl;
-        (*itr)->removeObserver(this);
-    }
     _nodePath.clear();
-    _valid = false;
-}
-
-bool ObserverNodePath::objectUnreferenced(void* ptr)
-{
-    osg::Node* node = reinterpret_cast<osg::Node*>(ptr);
-
-    OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*getObserverMutex());
-
-    _valid = false;
-
-    for(osg::NodePath::iterator itr = _nodePath.begin();
-        itr != _nodePath.end();
-        ++itr)
-    {
-        if (*itr == node)
-        {
-            _nodePath.erase(itr);
-
-            //OSG_NOTICE<<"ObserverNodePath["<<this<<"]::objectUnreferenced("<<ptr<<") found pointer in node path."<<std::endl;
-
-            // return true as we wish calling method to remove self from observer set.
-            return true;
-        }
-    }
-
-    //OSG_NOTICE<<"Error: ObserverNodePath["<<this<<"]::::objectUnreferenced("<<ptr<<") could not find pointer in node path."<<std::endl;
-
-    // return true as we wish calling method to remove self from observer set.
-    return true;
 }

src/osgDB/DatabasePager.cpp

@@ -186,11 +186,10 @@ public:
                         compileStateSet = true;
                         if (osg::getNotifyLevel() >= osg::DEBUG_INFO)
                         {
-                            OSG_NOTIFY(osg::DEBUG_INFO)
+                            OSG_INFO <<"Found compilable texture " << texture << " ";
-                                <<"Found compilable texture " << texture << " ";
                             osg::Image* image = texture->getImage(0);
-                            if (image) OSG_NOTIFY(osg::DEBUG_INFO) << image->getFileName();
+                            if (image) OSG_INFO << image->getFileName();
-                            OSG_NOTIFY(osg::DEBUG_INFO) << std:: endl;
+                            OSG_INFO << std:: endl;
                         }
                         break;
                     }
@@ -494,8 +493,8 @@ int DatabasePager::DatabaseThread::cancel()
         while(isRunning())
         {
             // commenting out debug info as it was cashing crash on exit, presumable
-            // due to OSG_NOTIFY or std::cout destructing earlier than this destructor.
+            // due to OSG_NOTICE or std::cout destructing earlier than this destructor.
-            // OSG_NOTIFY(osg::DEBUG_INFO)<<"Waiting for DatabasePager to cancel"<<std::endl;
+            // OSG_INFO<<"Waiting for DatabasePager to cancel"<<std::endl;
             OpenThreads::Thread::YieldCurrentThread();
         }
         
@@ -669,7 +668,7 @@ void DatabasePager::DatabaseThread::run()
                         Registry::instance()->readNode(databaseRequest->_fileName, databaseRequest->_loadOptions.get(), false);
 
             if (rr.validNode()) databaseRequest->_loadedModel = rr.getNode();
-            if (rr.error()) OSG_NOTIFY(osg::WARN)<<"Error in reading file "<<databaseRequest->_fileName<<" : "<<rr.message() << std::endl;
+            if (rr.error()) OSG_WARN<<"Error in reading file "<<databaseRequest->_fileName<<" : "<<rr.message() << std::endl;
             if (rr.notEnoughMemory()) OSG_INFO<<"Not enought memory to load file "<<databaseRequest->_fileName << std::endl;
 
             if (databaseRequest->_loadedModel.valid() &&
@@ -690,7 +689,7 @@ void DatabasePager::DatabaseThread::run()
             osg::RefNodePath refNodePath;
             if (!databaseRequest->_observerNodePath.getRefNodePath(refNodePath))
             {
-                OSG_INFO<<_name<<": Warning node in parental chain has been deleted, discarding load."<<std::endl;
+                OSG_NOTICE<<_name<<": Warning node in parental chain has been deleted, discarding load."<<std::endl;
                 databaseRequest->_loadedModel = 0;
             }
 
@@ -1116,7 +1115,7 @@ unsigned int DatabasePager::addDatabaseThread(DatabaseThread::Mode mode, const s
     
     if (_startThreadCalled)
     {
-        OSG_NOTIFY(osg::DEBUG_INFO)<<"DatabasePager::startThread()"<<std::endl;
+        OSG_INFO<<"DatabasePager::startThread()"<<std::endl;
         thread->startThread();
     }
     
@@ -1378,7 +1377,7 @@ void DatabasePager::requestNodeFile(const std::string& fileName,osg::Group* grou
         {
             _startThreadCalled = true;
             _done = false;
-            OSG_NOTIFY(osg::DEBUG_INFO)<<"DatabasePager::startThread()"<<std::endl;
+            OSG_INFO<<"DatabasePager::startThread()"<<std::endl;
             
             if (_databaseThreads.empty()) 
             {
@@ -1434,7 +1433,7 @@ bool DatabasePager::requiresUpdateSceneGraph() const
     return !(_dataToMergeList->_requestList.empty());
 }
 
-// #define UPDATE_TIMING 1
+//#define UPDATE_TIMING 1
 void DatabasePager::updateSceneGraph(const osg::FrameStamp& frameStamp)
 {
 #ifdef UPDATE_TIMING
@@ -1443,8 +1442,6 @@ void DatabasePager::updateSceneGraph(const osg::FrameStamp& frameStamp)
 #endif
 
     {
-        OpenThreads::ScopedLock<OpenThreads::Mutex> lock(*osg::Observer::getGlobalObserverMutex());
-
         removeExpiredSubgraphs(frameStamp);
 
 #ifdef UPDATE_TIMING
@@ -1550,7 +1547,7 @@ void DatabasePager::addLoadedDataToSceneGraph(const osg::FrameStamp &frameStamp)
 
     if (!localFileLoadedList.empty())
     {
-        OSG_NOTIFY(osg::DEBUG_INFO)<<"Done DatabasePager::addLoadedDataToSceneGraph"<<
+        OSG_INFO<<"Done DatabasePager::addLoadedDataToSceneGraph"<<
             osg::Timer::instance()->delta_m(before,mid)<<"ms,\t"<<
             osg::Timer::instance()->delta_m(mid,last)<<"ms"<<
             "  objects"<<localFileLoadedList.size()<<std::endl<<std::endl;
@@ -1600,8 +1597,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
         itr != _activePagedLODList.end();
         )
     {
-        osg::PagedLOD* plod = itr->get();
+        osg::ref_ptr<osg::PagedLOD> plod = itr->lock();
-        if (plod)
+        if (plod.valid())
         {
             int delta = frameStamp.getFrameNumber() - plod->getFrameNumberOfLastTraversal();
             if (delta>1)
@@ -1622,6 +1619,7 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
         }
         else
         {
+            OSG_NOTICE<<"DatabasePager::removeExpiredSubgraphs(), removing PagedLOD from _activePagedLODLists"<<std::endl;
             itr = _activePagedLODList.erase(itr);
         }
     }
@@ -1630,8 +1628,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
         itr != _inactivePagedLODList.end();
         )
     {
-        osg::PagedLOD* plod = itr->get();
+        osg::ref_ptr<osg::PagedLOD> plod = itr->lock();
-        if (plod)
+        if (plod.valid())
         {
             int delta = frameStamp.getFrameNumber() - plod->getFrameNumberOfLastTraversal();
             if (delta>1)
@@ -1647,6 +1645,7 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
         }
         else
         {
+            OSG_INFO<<"DatabasePager::removeExpiredSubgraphs(), removing PagedLOD from _inactivePagedLODLists"<<std::endl;
             itr = _inactivePagedLODList.erase(itr);
         }
     }
@@ -1687,8 +1686,8 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
         itr!=_inactivePagedLODList.end() && countPagedLODsVisitor._numPagedLODs<numToPrune;
         ++itr)
     {
-        osg::PagedLOD* plod = itr->get();
+        osg::ref_ptr<osg::PagedLOD> plod = itr->lock();
-        if (plod)
+        if (plod.valid())
         {
             osg::NodeList localChildrenRemoved;
             plod->removeExpiredChildren(expiryTime, expiryFrame, localChildrenRemoved);
@@ -1704,14 +1703,18 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
                 std::copy(localChildrenRemoved.begin(),localChildrenRemoved.end(),std::back_inserter(childrenRemoved));
             }
         }
+        else
+        {
+            OSG_NOTICE<<"DatabasePager::removeExpiredSubgraphs() _inactivePagedLOD has been invalidated, but ignored"<<std::endl;
+        }
     }
     
     for(PagedLODList::iterator itr = _activePagedLODList.begin();
         itr!=_activePagedLODList.end() && countPagedLODsVisitor._numPagedLODs<numToPrune;
         ++itr)
     {
-        osg::PagedLOD* plod = itr->get();
+        osg::ref_ptr<osg::PagedLOD> plod = itr->lock();
-        if (plod)
+        if (plod.valid())
         {
             osg::NodeList localChildrenRemoved;
             plod->removeExpiredChildren(expiryTime, expiryFrame, localChildrenRemoved);
@@ -1727,6 +1730,10 @@ void DatabasePager::removeExpiredSubgraphs(const osg::FrameStamp& frameStamp)
                 std::copy(localChildrenRemoved.begin(),localChildrenRemoved.end(),std::back_inserter(childrenRemoved));
             }
         }
+        else
+        {
+            OSG_NOTICE<<"DatabasePager::removeExpiredSubgraphs() _activePagedLOD has been invalidated, but ignored"<<std::endl;
+        }
     }
     
     osg::Timer_t end_b_Tick = osg::Timer::instance()->tick();
@@ -1961,7 +1968,7 @@ void DatabasePager::compileGLObjects(osg::State& state, double& availableTime)
                 }
                 if (osg::getNotifyLevel() >= osg::DEBUG_INFO
                     && numObjectsCompiled > objTemp)
-                    OSG_NOTIFY(osg::DEBUG_INFO)<< _frameNumber << " compiled "
+                    OSG_INFO<< _frameNumber << " compiled "
                                                 << numObjectsCompiled - objTemp
                                                 << " StateSets" << std::endl;
                 // remove the compiled statesets from the list.
@@ -1997,9 +2004,11 @@ void DatabasePager::compileGLObjects(osg::State& state, double& availableTime)
                 }
                 if (osg::getNotifyLevel() >= osg::DEBUG_INFO
                     && numObjectsCompiled > objTemp)
-                    OSG_NOTIFY(osg::DEBUG_INFO)<< _frameNumber << " compiled "
+                {
+                    OSG_INFO<< _frameNumber << " compiled "
                             << numObjectsCompiled - objTemp
                             << " Drawables" << std::endl;
+                }
                 // remove the compiled drawables from the list.
                 dwlist.erase(dwlist.begin(),itr);
             }