From ab956f15c333495f782e637438aedfa021ef1e65 Mon Sep 17 00:00:00 2001 From: Torsten Dreyer Date: Sat, 27 Sep 2014 21:48:36 +0200 Subject: [PATCH] A better fix for crash in the Effect System Stuart has improved the UniformCache approach, here are his changes: - We have a UniformCache so that each unique Uniform is only created once - As part of the UniformFactory we also have a queue of listeners that are still to be added - When the main thread sends an Update node visitor across the Effects, all queued listeners are de-queued and added. --- simgear/scene/material/Effect.cxx | 101 ++++++++++++++++++---- simgear/scene/material/Effect.hxx | 7 ++ simgear/scene/material/EffectBuilder.hxx | 41 ++++----- simgear/scene/material/TextureBuilder.cxx | 8 +- 4 files changed, 120 insertions(+), 37 deletions(-) diff --git a/simgear/scene/material/Effect.cxx b/simgear/scene/material/Effect.cxx index 6fbc55bd..eaf65d0c 100644 --- a/simgear/scene/material/Effect.cxx +++ b/simgear/scene/material/Effect.cxx @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -77,7 +78,8 @@ #include #include #include - +#include +#include namespace simgear { @@ -94,12 +96,22 @@ public: Uniform::Type uniformType, SGConstPropertyNode_ptr valProp, const SGReaderWriterOptions* options ); + void updateListeners( SGPropertyNode* propRoot ); + void addListener(DeferredPropertyListener* listener); private: // Default names for vector property components static const char* vec3Names[]; static const char* vec4Names[]; - std::map > uniformCache; + SGMutex _mutex; + + typedef boost::tuple UniformCacheKey; + typedef boost::tuple, SGPropertyChangeListener*> UniformCacheValue; + //std::map uniformCache; + std::map > uniformCache; + + typedef std::queue DeferredListenerList; + DeferredListenerList deferredListenerList; }; const char* UniformFactoryImpl::vec3Names[] = {"x", "y", "z"}; @@ -111,36 +123,63 @@ ref_ptr UniformFactoryImpl::getUniform( Effect * effect, SGConstPropertyNode_ptr valProp, const SGReaderWriterOptions* options ) { - - ref_ptr uniform = uniformCache[name]; - if( !uniform.valid() ) { - SG_LOG(SG_ALL,SG_ALERT,"new uniform '" << name << "'"); - uniformCache[name] = uniform = new Uniform; - } else { - SG_LOG(SG_ALL,SG_ALERT,"reusing uniform '" << name << "'"); - return uniform; + SGGuard scopeLock(_mutex); + std::string val = "0"; + + if (valProp->nChildren() == 0) { + // Completely static value + val = valProp->getStringValue(); + } else { + // Value references section of Effect + const SGPropertyNode* prop = getEffectPropertyNode(effect, valProp); + + if (prop) { + if (prop->nChildren() == 0) { + // Static value in parameters section + val = prop->getStringValue(); + } else { + // Dynamic property value in parameters section + val = getGlobalProperty(prop, options); + } + } else { + SG_LOG(SG_GL,SG_DEBUG,"Invalid parameter " << valProp->getName() << " for uniform " << name << " in Effect "); + } + } + + UniformCacheKey key = boost::make_tuple(name, uniformType, val); + //UniformCacheValue value = uniformCache[key]; + //ref_ptr uniform = value.get_head(); + ref_ptr uniform = uniformCache[key]; + + if (uniform.valid()) { + // We've got a hit to cache - simply return it + return uniform; } + SG_LOG(SG_GL,SG_DEBUG,"new uniform " << name << " value " << uniformCache.size()); + uniformCache[key] = uniform = new Uniform; + DeferredPropertyListener* updater = 0; + uniform->setName(name); uniform->setType(uniformType); switch (uniformType) { case Uniform::BOOL: - initFromParameters(effect, valProp, uniform.get(), + updater = initFromParameters(effect, valProp, uniform.get(), static_cast(&Uniform::set), options); break; case Uniform::FLOAT: - initFromParameters(effect, valProp, uniform.get(), + updater = initFromParameters(effect, valProp, uniform.get(), static_cast(&Uniform::set), options); break; case Uniform::FLOAT_VEC3: - initFromParameters(effect, valProp, uniform.get(), + updater = initFromParameters(effect, valProp, uniform.get(), static_cast(&Uniform::set), vec3Names, options); break; case Uniform::FLOAT_VEC4: - initFromParameters(effect, valProp, uniform.get(), + updater = initFromParameters(effect, valProp, uniform.get(), static_cast(&Uniform::set), vec4Names, options); break; @@ -151,17 +190,43 @@ ref_ptr UniformFactoryImpl::getUniform( Effect * effect, case Uniform::SAMPLER_1D_SHADOW: case Uniform::SAMPLER_2D_SHADOW: case Uniform::SAMPLER_CUBE: - initFromParameters(effect, valProp, uniform.get(), + updater = initFromParameters(effect, valProp, uniform.get(), static_cast(&Uniform::set), options); break; default: // avoid compiler warning + SG_LOG(SG_ALL,SG_ALERT,"UNKNOWN Uniform type '" << uniformType << "'"); break; } + addListener(updater); return uniform; } +void UniformFactoryImpl::addListener(DeferredPropertyListener* listener) +{ + if (listener != 0) { + // Uniform requires a property listener. Add it to the list to be + // created when the main thread gets to it. + deferredListenerList.push(listener); + } +} + +void UniformFactoryImpl::updateListeners( SGPropertyNode* propRoot ) +{ + SGGuard scopeLock(_mutex); + + if (deferredListenerList.empty()) return; + + SG_LOG(SG_GL,SG_DEBUG,"Adding " << deferredListenerList.size() << " listeners for effects."); + + // Instantiate all queued listeners + while (! deferredListenerList.empty()) { + DeferredPropertyListener* listener = deferredListenerList.front(); + listener->activate(propRoot); + deferredListenerList.pop(); + } +} typedef Singleton UniformFactory; @@ -183,6 +248,8 @@ Effect::Effect(const Effect& rhs, const CopyOp& copyop) techniques.push_back(static_cast(copyop(itr->get()))); generator = rhs.generator; + _name = rhs._name; + _name += " clone"; } // Assume that the last technique is always valid. @@ -1385,6 +1452,10 @@ void Effect::InitializeCallback::doUpdate(osg::Node* node, osg::NodeVisitor* nv) if (!effect) return; SGPropertyNode* root = getPropertyRoot(); + + // Initialize all queued listeners + UniformFactory::instance()->updateListeners(root); + for (vector >::iterator itr = effect->_extraData.begin(), end = effect->_extraData.end(); itr != end; diff --git a/simgear/scene/material/Effect.hxx b/simgear/scene/material/Effect.hxx index 0966dc1a..a1dcfc97 100644 --- a/simgear/scene/material/Effect.hxx +++ b/simgear/scene/material/Effect.hxx @@ -73,6 +73,13 @@ private: bool _initialized; }; +class DeferredPropertyListener { +public: + virtual void activate(SGPropertyNode* propRoot) {}; + virtual ~DeferredPropertyListener() {}; +}; + + class Effect : public osg::Object { public: diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx index 3c9c03d1..269909da 100644 --- a/simgear/scene/material/EffectBuilder.hxx +++ b/simgear/scene/material/EffectBuilder.hxx @@ -495,8 +495,7 @@ make_OSGFunctor(Obj* obj, void (Obj::*const func)(const OSGParam&)) template class ScalarChangeListener - : public SGPropertyChangeListener, public InitializeWhenAdded, - public Effect::Updater + : public SGPropertyChangeListener, public DeferredPropertyListener { public: ScalarChangeListener(ObjType* obj, const F& setter, @@ -504,6 +503,7 @@ public: : _obj(obj), _setter(setter) { _propName = new std::string(propName); + SG_LOG(SG_GL,SG_DEBUG,"Creating ScalarChangeListener for " << *_propName ); } virtual ~ScalarChangeListener() { @@ -514,9 +514,9 @@ public: { _setter(_obj.get(), node->getValue()); } - void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot) + void activate(SGPropertyNode* propRoot) { - SG_LOG(SG_ALL,SG_ALERT,"Adding change listener to " << *_propName ); + SG_LOG(SG_GL,SG_DEBUG,"Adding change listener to " << *_propName ); SGPropertyNode* listenProp = makeNode(propRoot, *_propName); delete _propName; _propName = 0; @@ -530,8 +530,7 @@ private: }; template -class EffectExtendedPropListener : public InitializeWhenAdded, - public Effect::Updater +class EffectExtendedPropListener : public DeferredPropertyListener { public: template @@ -550,7 +549,7 @@ public: delete _propName; delete _childNames; } - void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot) + void activate(SGPropertyNode* propRoot) { SGPropertyNode* parent = 0; if (_propName) @@ -574,7 +573,7 @@ private: }; template -Effect::Updater* +DeferredPropertyListener* new_EEPropListener(const Func& func, const std::string* propName, const Itr& namesBegin, const Itr& namesEnd) { @@ -608,32 +607,33 @@ inline void setDynamicVariance(osg::Object* obj) * used. */ template -void +DeferredPropertyListener* initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, const F& setter, const SGReaderWriterOptions* options) { const SGPropertyNode* valProp = getEffectPropertyNode(effect, prop); + ScalarChangeListener* listener = 0; if (!valProp) - return; + return listener; if (valProp->nChildren() == 0) { setter(obj, valProp->getValue()); } else { setDynamicVariance(obj); std::string propName = getGlobalProperty(valProp, options); - ScalarChangeListener* listener + listener = new ScalarChangeListener(obj, setter, propName); - effect->addUpdater(listener); } + return listener; } template -inline void +inline DeferredPropertyListener* initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, SetterReturn (ObjType::*setter)(const OSGParamType), const SGReaderWriterOptions* options) { - initFromParameters(effect, prop, obj, + return initFromParameters(effect, prop, obj, boost::bind(setter, _1, _2), options); } @@ -658,16 +658,17 @@ initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, */ template -void +DeferredPropertyListener* initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, const F& setter, NameItrType nameItr, const SGReaderWriterOptions* options) { typedef typename Bridge::sg_type sg_type; + DeferredPropertyListener* listener = 0; const int numComponents = props::NumComponents::num_components; const SGPropertyNode* valProp = getEffectPropertyNode(effect, prop); if (!valProp) - return; + return listener; if (valProp->nChildren() == 0) { // Has ? setter(obj, Bridge::get(valProp->getValue())); } else { @@ -677,22 +678,22 @@ initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, if (paramNames.empty()) throw BuilderException(); std::vector::const_iterator pitr = paramNames.begin(); - Effect::Updater* listener + listener = new_EEPropListener(make_OSGFunctor (obj, setter), 0, pitr, pitr + numComponents); - effect->addUpdater(listener); } + return listener; } template -inline void +inline DeferredPropertyListener* initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj, SetterReturn (ObjType::*setter)(const OSGParamType&), NameItrType nameItr, const SGReaderWriterOptions* options) { - initFromParameters(effect, prop, obj, + return initFromParameters(effect, prop, obj, boost::bind(setter, _1, _2), nameItr, options); } diff --git a/simgear/scene/material/TextureBuilder.cxx b/simgear/scene/material/TextureBuilder.cxx index 0c859a81..d0d3b267 100644 --- a/simgear/scene/material/TextureBuilder.cxx +++ b/simgear/scene/material/TextureBuilder.cxx @@ -858,10 +858,14 @@ TexEnvCombine* buildTexEnvCombine(Effect* effect, const SGPropertyNode* envProp, } #endif const SGPropertyNode* colorNode = envProp->getChild("constant-color"); - if (colorNode) - initFromParameters(effect, colorNode, result, + if (colorNode) { + DeferredPropertyListener* listener = initFromParameters(effect, colorNode, result, &TexEnvCombine::setConstantColor, colorFields, options); + if (listener != 0) { + SG_LOG(SG_ALL,SG_ALERT,"Texture with property defined parameter"); + } + } return result; } -- 2.39.5