From 5b9af0c0aaa3dc9caa43344b49e9626732694d2c Mon Sep 17 00:00:00 2001 From: Torsten Dreyer Date: Fri, 5 Sep 2014 11:28:28 +0200 Subject: [PATCH 1/3] Revert "Partial fix for crash in SGPropertyNode::fireValueChanged" This reverts commit f33ad357e928b5210c87cb8977d3cc88deba811b. --- simgear/scene/material/Effect.cxx | 22 ---------------------- simgear/scene/material/Effect.hxx | 22 ---------------------- simgear/scene/material/EffectBuilder.hxx | 22 ++++++++++------------ simgear/scene/material/EffectGeode.cxx | 2 -- 4 files changed, 10 insertions(+), 58 deletions(-) diff --git a/simgear/scene/material/Effect.cxx b/simgear/scene/material/Effect.cxx index f94be67a..7d961af4 100644 --- a/simgear/scene/material/Effect.cxx +++ b/simgear/scene/material/Effect.cxx @@ -1356,28 +1356,6 @@ void Effect::InitializeCallback::doUpdate(osg::Node* node, osg::NodeVisitor* nv) } } -void Effect::UpdateCallback::operator()(osg::Node* node, osg::NodeVisitor* nv) -{ - EffectGeode* eg = dynamic_cast(node); - if (!eg) - return; - Effect* effect = eg->getEffect(); - if (!effect) - return; - - for (vector >::iterator itr = effect->_extraData.begin(), - end = effect->_extraData.end(); - itr != end; - ++itr) { - PropertyPoller * poller - = dynamic_cast(itr->ptr()); - if (poller) - poller->pollProperties(effect); - } - - traverse(node, nv); -} - bool Effect::Key::EqualTo::operator()(const Effect::Key& lhs, const Effect::Key& rhs) const { diff --git a/simgear/scene/material/Effect.hxx b/simgear/scene/material/Effect.hxx index 346c52c2..0966dc1a 100644 --- a/simgear/scene/material/Effect.hxx +++ b/simgear/scene/material/Effect.hxx @@ -73,17 +73,6 @@ private: bool _initialized; }; -class PropertyPoller -{ -public: - PropertyPoller() {}; - virtual ~PropertyPoller() {}; - virtual void pollProperties(Effect* effect) - { - } -private: -}; - class Effect : public osg::Object { public: @@ -132,17 +121,6 @@ public: { void doUpdate(osg::Node* node, osg::NodeVisitor* nv); }; - friend struct UpdateCallback; - struct UpdateCallback : public osg::NodeCallback - { - UpdateCallback() {} - UpdateCallback(const UpdateCallback& nc, const osg::CopyOp& copyop) - : osg::NodeCallback(nc, copyop) - { - } - - virtual void operator()(osg::Node* node, osg::NodeVisitor* nv); - }; protected: std::vector > _extraData; ~Effect(); diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx index 52e2cf1c..a919f10b 100644 --- a/simgear/scene/material/EffectBuilder.hxx +++ b/simgear/scene/material/EffectBuilder.hxx @@ -496,17 +496,19 @@ make_OSGFunctor(Obj* obj, void (Obj::*const func)(const OSGParam&)) template class ScalarChangeListener : public SGPropertyChangeListener, public InitializeWhenAdded, - public PropertyPoller, public Effect::Updater { public: ScalarChangeListener(ObjType* obj, const F& setter, const std::string& propName) - : _obj(obj), _setter(setter), _propName(propName) + : _obj(obj), _setter(setter) { + _propName = new std::string(propName); } virtual ~ScalarChangeListener() { + delete _propName; + _propName = 0; } void valueChanged(SGPropertyNode* node) { @@ -514,20 +516,16 @@ public: } void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot) { - _listenProp = makeNode(propRoot, _propName); -// if ( _listenProp.valid() ) -// _listenProp->addChangeListener(this, true); - } - void pollProperties(Effect* effect) - { - if( false == _listenProp.valid() ) return; - valueChanged(_listenProp); + SGPropertyNode* listenProp = makeNode(propRoot, *_propName); + delete _propName; + _propName = 0; + if (listenProp) + listenProp->addChangeListener(this, true); } private: - SGPropertyNode_ptr _listenProp; osg::ref_ptr _obj; F _setter; - std::string _propName; + std::string* _propName; }; template diff --git a/simgear/scene/material/EffectGeode.cxx b/simgear/scene/material/EffectGeode.cxx index 2355bb67..c2967639 100644 --- a/simgear/scene/material/EffectGeode.cxx +++ b/simgear/scene/material/EffectGeode.cxx @@ -52,9 +52,7 @@ void EffectGeode::setEffect(Effect* effect) _effect = effect; if (!_effect) return; - //TODO: do we leak the callbacks or does the geode own pointer afterwards? addUpdateCallback(new Effect::InitializeCallback); - addUpdateCallback(new Effect::UpdateCallback); } void EffectGeode::resizeGLObjectBuffers(unsigned int maxSize) From 46bed67cceb0c80d674729678981dfaf306aa5b8 Mon Sep 17 00:00:00 2001 From: Torsten Dreyer Date: Fri, 5 Sep 2014 11:16:28 +0200 Subject: [PATCH 2/3] first stab at UniformFactory --- simgear/scene/material/Effect.cxx | 132 +++++++++++++++-------- simgear/scene/material/EffectBuilder.hxx | 1 + 2 files changed, 87 insertions(+), 46 deletions(-) diff --git a/simgear/scene/material/Effect.cxx b/simgear/scene/material/Effect.cxx index 7d961af4..6fbc55bd 100644 --- a/simgear/scene/material/Effect.cxx +++ b/simgear/scene/material/Effect.cxx @@ -87,6 +87,85 @@ using namespace osgUtil; using namespace effect; +class UniformFactoryImpl { +public: + ref_ptr getUniform( Effect * effect, + const string & name, + Uniform::Type uniformType, + SGConstPropertyNode_ptr valProp, + const SGReaderWriterOptions* options ); +private: + // Default names for vector property components + static const char* vec3Names[]; + static const char* vec4Names[]; + + std::map > uniformCache; +}; + +const char* UniformFactoryImpl::vec3Names[] = {"x", "y", "z"}; +const char* UniformFactoryImpl::vec4Names[] = {"x", "y", "z", "w"}; + +ref_ptr UniformFactoryImpl::getUniform( Effect * effect, + const string & name, + Uniform::Type uniformType, + 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; + } + + uniform->setName(name); + uniform->setType(uniformType); + switch (uniformType) { + case Uniform::BOOL: + initFromParameters(effect, valProp, uniform.get(), + static_cast(&Uniform::set), + options); + break; + case Uniform::FLOAT: + initFromParameters(effect, valProp, uniform.get(), + static_cast(&Uniform::set), + options); + break; + case Uniform::FLOAT_VEC3: + initFromParameters(effect, valProp, uniform.get(), + static_cast(&Uniform::set), + vec3Names, options); + break; + case Uniform::FLOAT_VEC4: + initFromParameters(effect, valProp, uniform.get(), + static_cast(&Uniform::set), + vec4Names, options); + break; + case Uniform::INT: + case Uniform::SAMPLER_1D: + case Uniform::SAMPLER_2D: + case Uniform::SAMPLER_3D: + case Uniform::SAMPLER_1D_SHADOW: + case Uniform::SAMPLER_2D_SHADOW: + case Uniform::SAMPLER_CUBE: + initFromParameters(effect, valProp, uniform.get(), + static_cast(&Uniform::set), + options); + break; + default: // avoid compiler warning + break; + } + + return uniform; +} + + +typedef Singleton UniformFactory; + + Effect::Effect() : _cache(0), _isRealized(false) { @@ -173,10 +252,6 @@ void buildPass(Effect* effect, Technique* tniq, const SGPropertyNode* prop, } } -// Default names for vector property components -const char* vec3Names[] = {"x", "y", "z"}; -const char* vec4Names[] = {"x", "y", "z", "w"}; - osg::Vec4f getColor(const SGPropertyNode* prop) { if (prop->nChildren() == 0) { @@ -898,10 +973,10 @@ struct UniformBuilder :public PassAttributeBuilder } if (!isAttributeActive(effect, prop)) return; - const SGPropertyNode* nameProp = prop->getChild("name"); - const SGPropertyNode* typeProp = prop->getChild("type"); - const SGPropertyNode* positionedProp = prop->getChild("positioned"); - const SGPropertyNode* valProp = prop->getChild("value"); + SGConstPropertyNode_ptr nameProp = prop->getChild("name"); + SGConstPropertyNode_ptr typeProp = prop->getChild("type"); + SGConstPropertyNode_ptr positionedProp = prop->getChild("positioned"); + SGConstPropertyNode_ptr valProp = prop->getChild("value"); string name; Uniform::Type uniformType = Uniform::FLOAT; if (nameProp) { @@ -941,44 +1016,9 @@ struct UniformBuilder :public PassAttributeBuilder } else { findAttr(uniformTypes, typeProp, uniformType); } - ref_ptr uniform = new Uniform; - uniform->setName(name); - uniform->setType(uniformType); - switch (uniformType) { - case Uniform::BOOL: - initFromParameters(effect, valProp, uniform.get(), - static_cast(&Uniform::set), - options); - break; - case Uniform::FLOAT: - initFromParameters(effect, valProp, uniform.get(), - static_cast(&Uniform::set), - options); - break; - case Uniform::FLOAT_VEC3: - initFromParameters(effect, valProp, uniform.get(), - static_cast(&Uniform::set), - vec3Names, options); - break; - case Uniform::FLOAT_VEC4: - initFromParameters(effect, valProp, uniform.get(), - static_cast(&Uniform::set), - vec4Names, options); - break; - case Uniform::INT: - case Uniform::SAMPLER_1D: - case Uniform::SAMPLER_2D: - case Uniform::SAMPLER_3D: - case Uniform::SAMPLER_1D_SHADOW: - case Uniform::SAMPLER_2D_SHADOW: - case Uniform::SAMPLER_CUBE: - initFromParameters(effect, valProp, uniform.get(), - static_cast(&Uniform::set), - options); - break; - default: // avoid compiler warning - break; - } + ref_ptr uniform = UniformFactory::instance()-> + getUniform( effect, name, uniformType, valProp, options ); + // optimize common uniforms if (uniformType == Uniform::SAMPLER_2D || uniformType == Uniform::INT) { diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx index a919f10b..3c9c03d1 100644 --- a/simgear/scene/material/EffectBuilder.hxx +++ b/simgear/scene/material/EffectBuilder.hxx @@ -516,6 +516,7 @@ public: } void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot) { + SG_LOG(SG_ALL,SG_ALERT,"Adding change listener to " << *_propName ); SGPropertyNode* listenProp = makeNode(propRoot, *_propName); delete _propName; _propName = 0; From ab956f15c333495f782e637438aedfa021ef1e65 Mon Sep 17 00:00:00 2001 From: Torsten Dreyer Date: Sat, 27 Sep 2014 21:48:36 +0200 Subject: [PATCH 3/3] 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 | 99 +++++++++++++++++++---- simgear/scene/material/Effect.hxx | 7 ++ simgear/scene/material/EffectBuilder.hxx | 41 +++++----- simgear/scene/material/TextureBuilder.cxx | 8 +- 4 files changed, 119 insertions(+), 36 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 ) { + SGGuard scopeLock(_mutex); + std::string val = "0"; - 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; + 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; }