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

@@ -30,6 +30,7 @@
 #include <functional>
 #include <iterator>
 #include <map>
+#include <queue>
 #include <utility>
 #include <boost/tr1/unordered_map.hpp>
 
@@ -77,7 +78,8 @@
 #include <simgear/structure/OSGUtils.hxx>
 #include <simgear/structure/SGExpression.hxx>
 #include <simgear/props/vectorPropTemplates.hxx>
-
+#include <simgear/threads/SGThread.hxx>
+#include <simgear/threads/SGGuard.hxx>
 
 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<std::string,ref_ptr<Uniform> > uniformCache;
+    SGMutex _mutex;
+
+    typedef boost::tuple<std::string, Uniform::Type, std::string> UniformCacheKey;
+    typedef boost::tuple<ref_ptr<Uniform>, SGPropertyChangeListener*> UniformCacheValue;
+    //std::map<UniformCacheKey,UniformCacheValue > uniformCache;
+    std::map<UniformCacheKey,ref_ptr<Uniform> > uniformCache;
+
+    typedef std::queue<DeferredPropertyListener*> DeferredListenerList;
+    DeferredListenerList deferredListenerList;
 };
 
 const char* UniformFactoryImpl::vec3Names[] = {"x", "y", "z"};
@@ -111,36 +123,63 @@ ref_ptr<Uniform> UniformFactoryImpl::getUniform( Effect * effect,
                                  SGConstPropertyNode_ptr valProp,
                                  const SGReaderWriterOptions* options )
 {
+	SGGuard<SGMutex> scopeLock(_mutex);
+	std::string val = "0";
 
-    ref_ptr<Uniform> uniform = uniformCache[name];
+	if (valProp->nChildren() == 0) {
-    if( !uniform.valid() ) {
+		// Completely static value
-       SG_LOG(SG_ALL,SG_ALERT,"new uniform '" << name << "'");
+		val = valProp->getStringValue();
-       uniformCache[name] = uniform = new Uniform;
+	} else {
-    } else {
+		// Value references <parameters> section of Effect
-      SG_LOG(SG_ALL,SG_ALERT,"reusing uniform '" << name << "'");
+		const SGPropertyNode* prop = getEffectPropertyNode(effect, valProp);
-      return uniform;
+
+		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> uniform = value.get_head();
+	ref_ptr<Uniform> 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<bool (Uniform::*)(bool)>(&Uniform::set),
                            options);
         break;
     case Uniform::FLOAT:
-        initFromParameters(effect, valProp, uniform.get(),
+    	updater = initFromParameters(effect, valProp, uniform.get(),
                            static_cast<bool (Uniform::*)(float)>(&Uniform::set),
                            options);
         break;
     case Uniform::FLOAT_VEC3:
-        initFromParameters(effect, valProp, uniform.get(),
+    	updater = initFromParameters(effect, valProp, uniform.get(),
                            static_cast<bool (Uniform::*)(const Vec3&)>(&Uniform::set),
                            vec3Names, options);
         break;
     case Uniform::FLOAT_VEC4:
-        initFromParameters(effect, valProp, uniform.get(),
+    	updater = initFromParameters(effect, valProp, uniform.get(),
                            static_cast<bool (Uniform::*)(const Vec4&)>(&Uniform::set),
                            vec4Names, options);
         break;
@@ -151,17 +190,43 @@ ref_ptr<Uniform> 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<bool (Uniform::*)(int)>(&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<SGMutex> 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<UniformFactoryImpl> UniformFactory;
 
@@ -183,6 +248,8 @@ Effect::Effect(const Effect& rhs, const CopyOp& copyop)
         techniques.push_back(static_cast<Technique*>(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<SGSharedPtr<Updater> >::iterator itr = effect->_extraData.begin(),
              end = effect->_extraData.end();
          itr != end;

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:

simgear/scene/material/EffectBuilder.hxx

@@ -495,8 +495,7 @@ make_OSGFunctor(Obj* obj, void (Obj::*const func)(const OSGParam&))
 
 template<typename OSGParamType, typename ObjType, typename F>
 class ScalarChangeListener
-    : public SGPropertyChangeListener, public InitializeWhenAdded,
+    : public SGPropertyChangeListener, public DeferredPropertyListener
-      public Effect::Updater
 {
 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<OSGParamType>());
     }
-    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<typename T, typename Func>
-class EffectExtendedPropListener : public InitializeWhenAdded,
+class EffectExtendedPropListener : public DeferredPropertyListener
-                                   public Effect::Updater
 {
 public:
     template<typename Itr>
@@ -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<typename T, typename Func, typename Itr>
-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<typename OSGParamType, typename ObjType, typename F>
-void
+DeferredPropertyListener*
 initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
                    const F& setter, const SGReaderWriterOptions* options)
 {
     const SGPropertyNode* valProp = getEffectPropertyNode(effect, prop);
+    ScalarChangeListener<OSGParamType, ObjType, F>* listener = 0;
     if (!valProp)
-        return;
+        return listener;
     if (valProp->nChildren() == 0) {
         setter(obj, valProp->getValue<OSGParamType>());
     } else {
         setDynamicVariance(obj);
         std::string propName = getGlobalProperty(valProp, options);
-        ScalarChangeListener<OSGParamType, ObjType, F>* listener
+        listener
             = new ScalarChangeListener<OSGParamType, ObjType, F>(obj, setter,
                                                                  propName);
-        effect->addUpdater(listener);
     }
+    return listener;
 }
 
 template<typename OSGParamType, typename ObjType, typename SetterReturn>
-inline void
+inline DeferredPropertyListener*
 initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
                    SetterReturn (ObjType::*setter)(const OSGParamType),
                    const SGReaderWriterOptions* options)
 {
-    initFromParameters<OSGParamType>(effect, prop, obj,
+    return initFromParameters<OSGParamType>(effect, prop, obj,
                                      boost::bind(setter, _1, _2), options);
 }
 
@@ -658,16 +658,17 @@ initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
  */
 template<typename OSGParamType, typename ObjType, typename NameItrType,
          typename F>
-void
+DeferredPropertyListener*
 initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
                    const F& setter,
                    NameItrType nameItr, const SGReaderWriterOptions* options)
 {
     typedef typename Bridge<OSGParamType>::sg_type sg_type;
+    DeferredPropertyListener* listener = 0;
     const int numComponents = props::NumComponents<sg_type>::num_components;
     const SGPropertyNode* valProp = getEffectPropertyNode(effect, prop);
     if (!valProp)
-        return;
+        return listener;
     if (valProp->nChildren() == 0) { // Has <use>?
         setter(obj, Bridge<OSGParamType>::get(valProp->getValue<sg_type>()));
     } else {
@@ -677,22 +678,22 @@ initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
         if (paramNames.empty())
             throw BuilderException();
         std::vector<std::string>::const_iterator pitr = paramNames.begin();
-        Effect::Updater* listener
+        listener
             =  new_EEPropListener<sg_type>(make_OSGFunctor<OSGParamType>
                                            (obj, setter),
                                            0, pitr, pitr + numComponents);
-        effect->addUpdater(listener);
     }
+    return listener;
 }
 
 template<typename OSGParamType, typename ObjType, typename NameItrType,
          typename SetterReturn>
-inline void
+inline DeferredPropertyListener*
 initFromParameters(Effect* effect, const SGPropertyNode* prop, ObjType* obj,
                    SetterReturn (ObjType::*setter)(const OSGParamType&),
                    NameItrType nameItr, const SGReaderWriterOptions* options)
 {
-    initFromParameters<OSGParamType>(effect, prop, obj,
+    return initFromParameters<OSGParamType>(effect, prop, obj,
                                      boost::bind(setter, _1, _2), nameItr,
                                      options);
 }

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)
+    if (colorNode) {
-        initFromParameters(effect, colorNode, result,
+    	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;
 }