From 554adfc8e670adfc5846b13ad802c1de84c533ff Mon Sep 17 00:00:00 2001 From: Robert Osfield Date: Fri, 14 May 2010 12:24:13 +0000 Subject: [PATCH] Refactored Observer/ObserverNodePath and DatabasePager to improve their robustness. --- include/osg/Observer | 11 ++-- include/osg/ObserverNodePath | 23 +++---- include/osgDB/DatabasePager | 21 +----- src/osg/ObserverNodePath.cpp | 123 ++++++++++++----------------------- src/osgDB/DatabasePager.cpp | 61 +++++++++-------- 5 files changed, 88 insertions(+), 151 deletions(-) diff --git a/include/osg/Observer b/include/osg/Observer index 6e9fbb0b2..a5be3fa6b 100644 --- a/include/osg/Observer +++ b/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 Observers; @@ -68,7 +64,8 @@ class OSG_EXPORT ObserverSet protected: - Observers _observers; + mutable OpenThreads::Mutex _mutex; + Observers _observers; }; } diff --git a/include/osg/ObserverNodePath b/include/osg/ObserverNodePath index 5dfa0163c..a9a06bc14 100644 --- a/include/osg/ObserverNodePath +++ b/include/osg/ObserverNodePath @@ -15,17 +15,16 @@ #define OSG_OBSERVERNODEPATH 1 #include -#include -#include -#include +#include +#include namespace osg { -typedef std::list< osg::ref_ptr > RefNodePath; +typedef std::vector< osg::ref_ptr > 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 lock(*getObserverMutex()); + OpenThreads::ScopedLock lock(_mutex); return _nodePath.empty(); } protected: void _setNodePath(const osg::NodePath& nodePath); - void _clearNodePath(); - virtual bool objectUnreferenced(void* ptr); - - osg::NodePath _nodePath; - bool _valid; + typedef std::vector< osg::observer_ptr > ObsNodePath; + mutable OpenThreads::Mutex _mutex; + ObsNodePath _nodePath; }; } diff --git a/include/osgDB/DatabasePager b/include/osgDB/DatabasePager index 85db79d18..a5bc4d323 100644 --- a/include/osgDB/DatabasePager +++ b/include/osgDB/DatabasePager @@ -345,28 +345,9 @@ class OSGDB_EXPORT DatabasePager : public osg::NodeVisitor::DatabaseRequestHandl struct RequestQueue; - struct PagedLODObserver : public osg::observer_ptr - { - typedef osg::observer_ptr BaseClass; - - PagedLODObserver(osg::PagedLOD* plod):BaseClass(plod) {} - PagedLODObserver(const PagedLODObserver& plodObserver):BaseClass(plodObserver) {} - - PagedLODObserver& operator = (const PagedLODObserver& rhs) - { - (*static_cast(this)) = rhs; - return *this; - } - - virtual void objectDeleted(void* ptr) - { - BaseClass::objectDeleted(ptr); - } - }; - + typedef osg::observer_ptr PagedLODObserver; typedef std::list< PagedLODObserver > PagedLODList; - struct DatabaseRequest : public osg::Referenced { DatabaseRequest(): diff --git a/src/osg/ObserverNodePath.cpp b/src/osg/ObserverNodePath.cpp index 2120c1ddd..8af7903be 100644 --- a/src/osg/ObserverNodePath.cpp +++ b/src/osg/ObserverNodePath.cpp @@ -16,23 +16,17 @@ using namespace osg; -ObserverNodePath::ObserverNodePath(): - _valid(false) +ObserverNodePath::ObserverNodePath() { } -ObserverNodePath::ObserverNodePath(const ObserverNodePath& rhs): - _valid(false) +ObserverNodePath::ObserverNodePath(const ObserverNodePath& rhs) { - RefNodePath refNodePath; - if (rhs.getRefNodePath(refNodePath)) - { - setNodePath(refNodePath); - } + OpenThreads::ScopedLock lock_rhs(_mutex); + _nodePath = rhs._nodePath; } -ObserverNodePath::ObserverNodePath(const osg::NodePath& nodePath): - _valid(false) +ObserverNodePath::ObserverNodePath(const osg::NodePath& nodePath) { setNodePath(nodePath); } @@ -46,11 +40,9 @@ ObserverNodePath& ObserverNodePath::operator = (const ObserverNodePath& rhs) { if (&rhs==this) return *this; - RefNodePath refNodePath; - if (rhs.getRefNodePath(refNodePath)) - { - setNodePath(refNodePath); - } + OpenThreads::ScopedLock lock_rhs(rhs._mutex); + OpenThreads::ScopedLock lock_lhs(_mutex); + _nodePath = rhs._nodePath; return *this; } @@ -82,7 +74,7 @@ void ObserverNodePath::setNodePathTo(osg::Node* node) void ObserverNodePath::setNodePath(const osg::NodePath& nodePath) { - OpenThreads::ScopedLock lock(*getObserverMutex()); + OpenThreads::ScopedLock lock(_mutex); _setNodePath(nodePath); } @@ -98,97 +90,62 @@ void ObserverNodePath::setNodePath(const osg::RefNodePath& refNodePath) void ObserverNodePath::clearNodePath() { - OpenThreads::ScopedLock lock(*getObserverMutex()); + OpenThreads::ScopedLock lock(_mutex); _clearNodePath(); } bool ObserverNodePath::getRefNodePath(RefNodePath& refNodePath) const { - refNodePath.clear(); - - OpenThreads::ScopedLock lock(*getObserverMutex()); - if (!_valid) return false; - - for(osg::NodePath::const_iterator itr = _nodePath.begin(); - itr != _nodePath.end(); - ++itr) + OpenThreads::ScopedLock lock(_mutex); + refNodePath.resize(_nodePath.size()); + for(unsigned int i=0; i<_nodePath.size(); ++i) { - refNodePath.push_back(*itr); + refNodePath[i] = _nodePath[i].lock(); + if (!refNodePath[i].valid()) + { + OSG_NOTICE<<"ObserverNodePath::getRefNodePath() node has been invalidated"< lock(*getObserverMutex()); - if (!_valid) return false; - nodePath = _nodePath; + OpenThreads::ScopedLock 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"<addObserver(this); + _nodePath[i] = nodePath[i]; } - - _valid = true; } void ObserverNodePath::_clearNodePath() { - //OSG_NOTICE<<"ObserverNodePath["<removeObserver(this); - } + // OSG_NOTICE<<"ObserverNodePath["<= osg::DEBUG_INFO) { - OSG_NOTIFY(osg::DEBUG_INFO) - <<"Found compilable texture " << texture << " "; + OSG_INFO <<"Found compilable texture " << texture << " "; osg::Image* image = texture->getImage(0); - if (image) OSG_NOTIFY(osg::DEBUG_INFO) << image->getFileName(); - OSG_NOTIFY(osg::DEBUG_INFO) << std:: endl; + if (image) OSG_INFO << image->getFileName(); + 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. - // OSG_NOTIFY(osg::DEBUG_INFO)<<"Waiting for DatabasePager to cancel"<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 "<_fileName<<" : "<_fileName<<" : "<_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."<_loadedModel = 0; } @@ -1116,7 +1115,7 @@ unsigned int DatabasePager::addDatabaseThread(DatabaseThread::Mode mode, const s if (_startThreadCalled) { - OSG_NOTIFY(osg::DEBUG_INFO)<<"DatabasePager::startThread()"<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()"<_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 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"<get(); - if (plod) + osg::ref_ptr plod = itr->lock(); + 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"<get(); - if (plod) + osg::ref_ptr plod = itr->lock(); + 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"<get(); - if (plod) + osg::ref_ptr plod = itr->lock(); + 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"<get(); - if (plod) + osg::ref_ptr plod = itr->lock(); + 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"<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 " - << numObjectsCompiled - objTemp - << " Drawables" << std::endl; + { + OSG_INFO<< _frameNumber << " compiled " + << numObjectsCompiled - objTemp + << " Drawables" << std::endl; + } // remove the compiled drawables from the list. dwlist.erase(dwlist.begin(),itr); }