From becbad7ea4719e5001f88cdc25328133b44b5ea2 Mon Sep 17 00:00:00 2001 From: James Turner Date: Mon, 6 Jan 2014 08:26:42 +0000 Subject: [PATCH] Crash fix: use cache to find materials. Add a thread-safe cache of the currently valid materials, to avoid the osgDB thread making repeated SGCondition evaluations in conflict with the main thread. (Requires an accompanying FG change) --- simgear/scene/material/matlib.cxx | 45 +++++++++++++++++++++++++++--- simgear/scene/material/matlib.hxx | 31 +++++++++++++++++--- simgear/scene/tgdb/SGOceanTile.cxx | 2 +- simgear/scene/tgdb/obj.cxx | 16 +++++------ 4 files changed, 77 insertions(+), 17 deletions(-) diff --git a/simgear/scene/material/matlib.cxx b/simgear/scene/material/matlib.cxx index 279a6cfe..a7b017fa 100644 --- a/simgear/scene/material/matlib.cxx +++ b/simgear/scene/material/matlib.cxx @@ -40,6 +40,8 @@ #include #include #include +#include +#include #include "mat.hxx" @@ -49,8 +51,17 @@ using std::string; + +class SGMaterialLib::MatLibPrivate +{ +public: + SGMutex mutex; +}; + // Constructor -SGMaterialLib::SGMaterialLib ( void ) { +SGMaterialLib::SGMaterialLib ( void ) : + d(new MatLibPrivate) +{ } // Load a library of material properties @@ -96,15 +107,16 @@ bool SGMaterialLib::load( const string &fg_root, const string& mpath, } // find a material record by material name -SGMaterial *SGMaterialLib::find( const string& material ) { +SGMaterial *SGMaterialLib::find( const string& material ) const +{ SGMaterial *result = NULL; - material_map_iterator it = matlib.find( material ); + const_material_map_iterator it = matlib.find( material ); if ( it != end() ) { // We now have a list of materials that match this // name. Find the first one that either doesn't have // a condition, or has a condition that evaluates // to true. - material_list_iterator iter = it->second.begin(); + material_list::const_iterator iter = it->second.begin(); while (iter != it->second.end()) { result = *iter; if (result->valid()) { @@ -117,6 +129,31 @@ SGMaterial *SGMaterialLib::find( const string& material ) { return NULL; } +void SGMaterialLib::refreshActiveMaterials() +{ + active_material_cache newCache; + material_map_iterator it = matlib.begin(); + for (; it != matlib.end(); ++it) { + newCache[it->first] = find(it->first); + } + + // use this approach to minimise the time we're holding the lock + // lock on the mutex (and hence, would block findCached calls) + SGGuard g(d->mutex); + active_cache = newCache; +} + +SGMaterial *SGMaterialLib::findCached( const string& material ) const +{ + SGGuard g(d->mutex); + + active_material_cache::const_iterator it = active_cache.find(material); + if (it == active_cache.end()) + return NULL; + + return it->second; +} + // Destructor SGMaterialLib::~SGMaterialLib ( void ) { SG_LOG( SG_GENERAL, SG_INFO, "SGMaterialLib::~SGMaterialLib() size=" << matlib.size()); diff --git a/simgear/scene/material/matlib.hxx b/simgear/scene/material/matlib.hxx index 8033ea3c..45db7734 100644 --- a/simgear/scene/material/matlib.hxx +++ b/simgear/scene/material/matlib.hxx @@ -29,6 +29,7 @@ #include +#include #include // Standard C++ string library #include // STL associative "array" #include // STL "array" @@ -43,16 +44,22 @@ namespace osg { class Geode; } class SGMaterialLib { private: - + class MatLibPrivate; + std::auto_ptr d; + // associative array of materials typedef std::vector< SGSharedPtr > material_list; typedef material_list::iterator material_list_iterator; + typedef std::map < std::string, material_list> material_map; typedef material_map::iterator material_map_iterator; typedef material_map::const_iterator const_material_map_iterator; material_map matlib; - + + typedef std::map < std::string, SGSharedPtr > active_material_cache; + active_material_cache active_cache; + public: // Constructor @@ -62,8 +69,24 @@ public: bool load( const std::string &fg_root, const std::string& mpath, SGPropertyNode *prop_root ); // find a material record by material name - SGMaterial *find( const std::string& material ); - + SGMaterial *find( const std::string& material ) const; + + /** + * Material lookup involves evaluation of SGConditions to determine which + * possible material (by season, region, etc) is valid. This involves + * vproperty tree queries, so repeated calls to find() can cause + * race conditions when called from the osgDB pager thread. (especially + * during startup) + * + * To fix this, and also avoid repeated re-evaluation of the material + * conditions, we provide a version which uses a cached, threadsafe table + * of the currently valid materials. The main thread calls the refresh + * method below to evaluate the valid materials, and findCached can be + * safely called from other threads with no access to unprotected state. + */ + SGMaterial *findCached( const std::string& material ) const; + void refreshActiveMaterials(); + material_map_iterator begin() { return matlib.begin(); } const_material_map_iterator begin() const { return matlib.begin(); } diff --git a/simgear/scene/tgdb/SGOceanTile.cxx b/simgear/scene/tgdb/SGOceanTile.cxx index c5efc625..f990f946 100644 --- a/simgear/scene/tgdb/SGOceanTile.cxx +++ b/simgear/scene/tgdb/SGOceanTile.cxx @@ -282,7 +282,7 @@ osg::Node* SGOceanTile(const SGBucket& b, SGMaterialLib *matlib, int latPoints, double tex_width = 1000.0; // find Ocean material in the properties list - SGMaterial *mat = matlib->find( "Ocean" ); + SGMaterial *mat = matlib->findCached( "Ocean" ); if ( mat != NULL ) { // set the texture width and height values for this // material diff --git a/simgear/scene/tgdb/obj.cxx b/simgear/scene/tgdb/obj.cxx index 771ff2c4..b5d6dd71 100644 --- a/simgear/scene/tgdb/obj.cxx +++ b/simgear/scene/tgdb/obj.cxx @@ -146,7 +146,7 @@ public: std::string materialName = obj.get_pt_materials()[grp]; SGMaterial* material = 0; if (matlib) - material = matlib->find(materialName); + material = matlib->findCached(materialName); SGVec4f color = getMaterialLightColor(material); if (3 <= materialName.size() && materialName.substr(0, 3) != "RWY") { @@ -326,7 +326,7 @@ public: { if (!matlib) return SGVec2f(1, 1); - SGMaterial* material = matlib->find(name); + SGMaterial* material = matlib->findCached(name); if (!material) return SGVec2f(1, 1); @@ -400,7 +400,7 @@ public: osg::Geometry* geometry = i->second.buildGeometry(); SGMaterial *mat = 0; if (matlib) - mat = matlib->find(i->first); + mat = matlib->findCached(i->first); eg = new EffectGeode; eg->setName("EffectGeode"); if (mat) @@ -427,7 +427,7 @@ public: mt_init(&seed, unsigned(123)); for (i = materialTriangleMap.begin(); i != materialTriangleMap.end(); ++i) { - SGMaterial *mat = matlib->find(i->first); + SGMaterial *mat = matlib->findCached(i->first); if (!mat) continue; @@ -482,7 +482,7 @@ public: mt_init(&seed, unsigned(123)); for (i = materialTriangleMap.begin(); i != materialTriangleMap.end(); ++i) { - SGMaterial *mat = matlib->find(i->first); + SGMaterial *mat = matlib->findCached(i->first); SGTexturedTriangleBin triangleBin = i->second; if (!mat) @@ -799,7 +799,7 @@ public: mt_init(&seed, unsigned(586)); for (i = materialTriangleMap.begin(); i != materialTriangleMap.end(); ++i) { - SGMaterial *mat = matlib->find(i->first); + SGMaterial *mat = matlib->findCached(i->first); if (!mat) continue; @@ -955,13 +955,13 @@ public: SGVec4f red(1, 0, 0, 1); SGMaterial* mat = 0; if (matlib) - mat = matlib->find("RWY_RED_LIGHTS"); + mat = matlib->findCached("RWY_RED_LIGHTS"); if (mat) red = mat->get_light_color(); SGVec4f white(1, 1, 1, 1); mat = 0; if (matlib) - mat = matlib->find("RWY_WHITE_LIGHTS"); + mat = matlib->findCached("RWY_WHITE_LIGHTS"); if (mat) white = mat->get_light_color(); SGDirectionalLightListBin::const_iterator i; -- 2.39.5