Better error reporting for effects

Throw an exception when an undefine attribute value is found in an effect.

Also, fix a typo in TexEnvCombine operand attributes.

simgear/scene/material/EffectBuilder.cxx

@@ -51,6 +51,8 @@ string getGlobalProperty(const SGPropertyNode* prop)
     return useProp->getStringValue();
 }
 
+namespace effect
+{
 BuilderException::BuilderException()
 {
 }
@@ -69,6 +71,7 @@ BuilderException::BuilderException(const std::string& message,
 BuilderException::~BuilderException() throw()
 {
 
+}
 }
 
 bool isAttributeActive(Effect* effect, const SGPropertyNode* prop)

simgear/scene/material/EffectBuilder.hxx

@@ -154,10 +154,18 @@ EffectPropertyMap<T>::EffectPropertyMap(const EffectNameValue<T> (&attrs)[N])
         _map.insert(typename BMap::value_type(attrs[i].first, attrs[i].second));
 }
 
+class BuilderException : public sg_exception
+{
+public:
+    BuilderException();
+    BuilderException(const char* message, const char* origin = 0);
+    BuilderException(const std::string& message, const std::string& = "");
+    virtual ~BuilderException() throw();
+};
 }
 
 template<typename T>
-bool findAttr(const effect::EffectPropertyMap<T>& pMap,
+void findAttr(const effect::EffectPropertyMap<T>& pMap,
               const char* name,
               T& result)
 {
@@ -165,32 +173,32 @@ bool findAttr(const effect::EffectPropertyMap<T>& pMap,
     typename EffectPropertyMap<T>::BMap::iterator itr
         = pMap._map.get<from>().find(name);
     if (itr == pMap._map.end()) {
-        return false;
+        throw effect::BuilderException(string("findAttr: could not find attribute ")
+                               + string(name));
     } else {
         result = itr->second;
-        return true;
     }
 }
 
 template<typename T>
-inline bool findAttr(const effect::EffectPropertyMap<T>& pMap,
+inline void findAttr(const effect::EffectPropertyMap<T>& pMap,
                      const std::string& name,
                      T& result)
 {
-    return findAttr(pMap, name.c_str(), result);
+    findAttr(pMap, name.c_str(), result);
 }
 
 template<typename T>
-bool findAttr(const effect::EffectPropertyMap<T>& pMap,
+void findAttr(const effect::EffectPropertyMap<T>& pMap,
               const SGPropertyNode* prop,
               T& result)
 {
     if (!prop)
-        return false;
+        throw effect::BuilderException("findAttr: empty property");
     const char* name = prop->getStringValue();
     if (!name)
-        return false;
+        throw effect::BuilderException("findAttr: no name for lookup");
-    return findAttr(pMap, name, result);
+    findAttr(pMap, name, result);
 }
 
 template<typename T>
@@ -234,15 +242,6 @@ const SGPropertyNode* getEffectPropertyChild(Effect* effect,
  */
 std::string getGlobalProperty(const SGPropertyNode* prop);
 
-class BuilderException : public sg_exception
-{
-public:
-    BuilderException();
-    BuilderException(const char* message, const char* origin = 0);
-    BuilderException(const std::string& message, const std::string& = "");
-    virtual ~BuilderException() throw();
-};
-
 class PassAttributeBuilder : public SGReferenced
 {
 protected:

simgear/scene/material/TextureBuilder.cxx

@@ -181,24 +181,27 @@ TexTuple makeTexTuple(Effect* effect, const SGPropertyNode* props,
                       const string& texType)
 {
     Texture::FilterMode minFilter = Texture::LINEAR_MIPMAP_LINEAR;
-    findAttr(filterModes, getEffectPropertyChild(effect, props, "filter"),
+    const SGPropertyNode* ep = 0;
-             minFilter);
+    if ((ep = getEffectPropertyChild(effect, props, "filter")))
+        findAttr(filterModes, ep, minFilter);
     Texture::FilterMode magFilter = Texture::LINEAR;
-    findAttr(filterModes, getEffectPropertyChild(effect, props,
+    if ((ep = getEffectPropertyChild(effect, props, "mag-filter")))
-                                                 "mag-filter"),
+        findAttr(filterModes, ep, magFilter);
-             magFilter);
     const SGPropertyNode* pWrapS
         = getEffectPropertyChild(effect, props, "wrap-s");
     Texture::WrapMode sWrap = Texture::CLAMP;
-    findAttr(wrapModes, pWrapS, sWrap);
+    if (pWrapS)
+        findAttr(wrapModes, pWrapS, sWrap);
     const SGPropertyNode* pWrapT
         = getEffectPropertyChild(effect, props, "wrap-t");
     Texture::WrapMode tWrap = Texture::CLAMP;
-    findAttr(wrapModes, pWrapT, tWrap);
+    if (pWrapT)
+        findAttr(wrapModes, pWrapT, tWrap);
     const SGPropertyNode* pWrapR
         = getEffectPropertyChild(effect, props, "wrap-r");
     Texture::WrapMode rWrap = Texture::CLAMP;
-    findAttr(wrapModes, pWrapR, rWrap);
+    if (pWrapR)
+        findAttr(wrapModes, pWrapR, rWrap);
     const SGPropertyNode* pImage
         = getEffectPropertyChild(effect, props, "image");
     string imageName;
@@ -432,10 +435,10 @@ EffectPropertyMap<TexEnvCombine::SourceParam> sourceParams(sourceParamInit);
 
 EffectNameValue<TexEnvCombine::OperandParam> opParamInit[] =
 {
-    {"src_color", TexEnvCombine::SRC_COLOR},
+    {"src-color", TexEnvCombine::SRC_COLOR},
-    {"one_minus_src_color", TexEnvCombine::ONE_MINUS_SRC_COLOR},
+    {"one-minus-src-color", TexEnvCombine::ONE_MINUS_SRC_COLOR},
-    {"src_alpha", TexEnvCombine::SRC_ALPHA},
+    {"src-alpha", TexEnvCombine::SRC_ALPHA},
-    {"one_minus_src_alpha", TexEnvCombine::ONE_MINUS_SRC_ALPHA}
+    {"one-minus-src-alpha", TexEnvCombine::ONE_MINUS_SRC_ALPHA}
 };
 
 EffectPropertyMap<TexEnvCombine::OperandParam> operandParams(opParamInit);
@@ -567,23 +570,18 @@ TexGen* buildTexGen(Effect* effect, const SGPropertyNode* tgenProp)
     TexGen* result = new TexGen;
     const SGPropertyNode* p = 0;
     TexGen::Mode mode = TexGen::OBJECT_LINEAR;
-    if (findAttr(tgenModes, getEffectPropertyChild(effect, tgenProp, "mode"),
+    findAttr(tgenModes, getEffectPropertyChild(effect, tgenProp, "mode"), mode);
-                 mode))
+    result->setMode(mode);
-        result->setMode(mode);
     const SGPropertyNode* planesNode = tgenProp->getChild("planes");
     if (planesNode) {
         for (int i = 0; i < planesNode->nChildren(); ++i) {
             const SGPropertyNode* planeNode = planesNode->getChild(i);
             TexGen::Coord coord;
-            if (!findAttr(tgenCoords, planeNode->getName(), coord)) {
+            findAttr(tgenCoords, planeNode->getName(), coord);
-                SG_LOG(SG_INPUT, SG_ALERT, "Unknown TexGen plane "
+            const SGPropertyNode* realNode
-                       << planeNode->getName());
+                = getEffectPropertyNode(effect, planeNode);
-            } else {
+            SGVec4d plane = realNode->getValue<SGVec4d>();
-                const SGPropertyNode* realNode
+            result->setPlane(coord, toOsg(plane));
-                    = getEffectPropertyNode(effect, planeNode);
-                SGVec4d plane = realNode->getValue<SGVec4d>();
-                result->setPlane(coord, toOsg(plane));
-            }
         }
     }
     return result;
@@ -611,10 +609,16 @@ bool makeTextureParameters(SGPropertyNode* paramRoot, const StateSet* ss)
     }
     makeChild(texUnit, "active")->setValue(true);
     makeChild(texUnit, "type")->setValue("2d");
+    string filter = findName(filterModes,
+                             texture->getFilter(Texture::MIN_FILTER));
+    string magFilter = findName(filterModes,
+                             texture->getFilter(Texture::MAG_FILTER));
     string wrapS = findName(wrapModes, texture->getWrap(Texture::WRAP_S));
     string wrapT = findName(wrapModes, texture->getWrap(Texture::WRAP_T));
     string wrapR = findName(wrapModes, texture->getWrap(Texture::WRAP_R));
     makeChild(texUnit, "image")->setStringValue(imageName);
+    makeChild(texUnit, "filter")->setStringValue(filter);
+    makeChild(texUnit, "mag-filter")->setStringValue(magFilter);
     makeChild(texUnit, "wrap-s")->setStringValue(wrapS);
     makeChild(texUnit, "wrap-t")->setStringValue(wrapT);
     makeChild(texUnit, "wrap-r")->setStringValue(wrapR);

simgear/scene/material/makeEffect.cxx

@@ -4,6 +4,7 @@
 #endif
 
 #include "Effect.hxx"
+#include "EffectBuilder.hxx"
 #include "Technique.hxx"
 #include "Pass.hxx"
 
@@ -117,24 +118,42 @@ Effect* makeEffect(const string& name,
                    bool realizeTechniques,
                    const osgDB::ReaderWriter::Options* options)
 {
-    OpenThreads::ScopedLock<OpenThreads::ReentrantMutex> lock(effectMutex);
+    {
-    EffectMap::iterator itr = effectMap.find(name);
+        OpenThreads::ScopedLock<OpenThreads::ReentrantMutex> lock(effectMutex);
-    if (itr != effectMap.end())
+        EffectMap::iterator itr = effectMap.find(name);
-        return itr->second.get();
+        if (itr != effectMap.end())
+            return itr->second.get();
+    }
     string effectFileName(name);
     effectFileName += ".eff";
     string absFileName
         = osgDB::findDataFile(effectFileName, options);
     if (absFileName.empty()) {
-        SG_LOG(SG_INPUT, SG_WARN, "can't find \"" << effectFileName << "\"");
+        SG_LOG(SG_INPUT, SG_ALERT, "can't find \"" << effectFileName << "\"");
         return 0;
     }
     SGPropertyNode_ptr effectProps = new SGPropertyNode();
-    readProperties(absFileName, effectProps.ptr(), 0, true);
+    try {
-    Effect* result = makeEffect(effectProps.ptr(), realizeTechniques, options);
+        readProperties(absFileName, effectProps.ptr(), 0, true);
-    if (result)
+    }
-        effectMap.insert(make_pair(name, result));
+    catch (sg_io_exception& e) {
-    return result;
+        SG_LOG(SG_INPUT, SG_ALERT, "error reading \"" << absFileName << "\": "
+               << e.getFormattedMessage());
+        return 0;
+    }
+    ref_ptr<Effect> result = makeEffect(effectProps.ptr(), realizeTechniques,
+                                        options);
+    if (result.valid()) {
+        OpenThreads::ScopedLock<OpenThreads::ReentrantMutex> lock(effectMutex);
+        pair<EffectMap::iterator, bool> irslt
+            = effectMap.insert(make_pair(name, result));
+        if (!irslt.second) {
+            // Another thread beat us to it!. Discard our newly
+            // constructed Effect and use the one in the cache.
+            result = irslt.first->second;
+        }
+    }
+    return result.release();
 }
 
 
@@ -165,7 +184,7 @@ Effect* makeEffect(SGPropertyNode* prop,
             }
         }
     }
-    Effect* effect = 0;
+    ref_ptr<Effect> effect;
     // Merge with the parent effect, if any
     SGPropertyNode_ptr inheritProp = prop->getChild("inherits-from");
     Effect* parent = 0;
@@ -173,36 +192,56 @@ Effect* makeEffect(SGPropertyNode* prop,
         //prop->removeChild("inherits-from");
         parent = makeEffect(inheritProp->getStringValue(), false, options);
         if (parent) {
-            Effect::Cache* cache = parent->getCache();
             Effect::Key key;
+            key.unmerged = prop;
             if (options) {
-                key = Effect::Key(prop, options->getDatabasePathList());
+                key.paths = options->getDatabasePathList();
-            } else {
-                osgDB::FilePathList dummy;
-                key = Effect::Key(prop, dummy);
             }
-            Effect::Cache::iterator itr = cache->find(key);
+            Effect::Cache* cache = 0;
-            if (itr != cache->end()) {
+            Effect::Cache::iterator itr;
-                effect = itr->second.get();
+            {
-            } else {
+                OpenThreads::ScopedLock<OpenThreads::ReentrantMutex>
+                    lock(effectMutex);
+                cache = parent->getCache();
+                itr = cache->find(key);
+                if (itr != cache->end()) 
+                    effect = itr->second.get();
+            }
+            if (!effect.valid()) {
                 effect = new Effect;
                 effect->root = new SGPropertyNode;
                 mergePropertyTrees(effect->root, prop, parent->root);
-                cache->insert(make_pair(key, effect));
+                effect->parametersProp = effect->root->getChild("parameters");
+                OpenThreads::ScopedLock<OpenThreads::ReentrantMutex>
+                    lock(effectMutex);
+                pair<Effect::Cache::iterator, bool> irslt
+                    = cache->insert(make_pair(key, effect));
+                if (!irslt.second)
+                    effect = irslt.first->second;
             }
         } else {
-            SG_LOG(SG_INPUT, SG_WARN, "can't find base effect " <<
+            SG_LOG(SG_INPUT, SG_ALERT, "can't find base effect " <<
                    inheritProp->getStringValue());
+            return 0;
         }
-    }
+    } else {
-    if (!effect) {
         effect = new Effect;
         effect->root = prop;
+        effect->parametersProp = effect->root->getChild("parameters");
     }
-    effect->parametersProp = effect->root->getChild("parameters");
+    if (realizeTechniques) {
-    if (realizeTechniques)
+        try {
-        effect->realizeTechniques(options);
+            OpenThreads::ScopedLock<OpenThreads::ReentrantMutex>
-    return effect;
+                lock(effectMutex);
+            effect->realizeTechniques(options);
+        }
+        catch (BuilderException& e) {
+            SG_LOG(SG_INPUT, SG_ALERT, "Error building technique: "
+                   << e.getFormattedMessage());
+            return 0;
+        }
+    }
+    return effect.release();
 }
 
 }