From 2e71b64de1d937e2b7c34dd4a2365be455d0b82a Mon Sep 17 00:00:00 2001 From: Tim Moore Date: Thu, 26 Nov 2009 11:32:00 +0100 Subject: [PATCH] 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 | 3 + simgear/scene/material/EffectBuilder.hxx | 35 ++++----- simgear/scene/material/TextureBuilder.cxx | 52 +++++++------ simgear/scene/material/makeEffect.cxx | 95 ++++++++++++++++------- 4 files changed, 115 insertions(+), 70 deletions(-) diff --git a/simgear/scene/material/EffectBuilder.cxx b/simgear/scene/material/EffectBuilder.cxx index 14a28061..34f14ac8 100644 --- a/simgear/scene/material/EffectBuilder.cxx +++ b/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) diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx index 69e926ae..833ff515 100644 --- a/simgear/scene/material/EffectBuilder.hxx +++ b/simgear/scene/material/EffectBuilder.hxx @@ -154,10 +154,18 @@ EffectPropertyMap::EffectPropertyMap(const EffectNameValue (&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 -bool findAttr(const effect::EffectPropertyMap& pMap, +void findAttr(const effect::EffectPropertyMap& pMap, const char* name, T& result) { @@ -165,32 +173,32 @@ bool findAttr(const effect::EffectPropertyMap& pMap, typename EffectPropertyMap::BMap::iterator itr = pMap._map.get().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 -inline bool findAttr(const effect::EffectPropertyMap& pMap, +inline void findAttr(const effect::EffectPropertyMap& pMap, const std::string& name, T& result) { - return findAttr(pMap, name.c_str(), result); + findAttr(pMap, name.c_str(), result); } template -bool findAttr(const effect::EffectPropertyMap& pMap, +void findAttr(const effect::EffectPropertyMap& 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; - return findAttr(pMap, name, result); + throw effect::BuilderException("findAttr: no name for lookup"); + findAttr(pMap, name, result); } template @@ -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: diff --git a/simgear/scene/material/TextureBuilder.cxx b/simgear/scene/material/TextureBuilder.cxx index c1f353ce..a924b955 100644 --- a/simgear/scene/material/TextureBuilder.cxx +++ b/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"), - minFilter); + const SGPropertyNode* ep = 0; + if ((ep = getEffectPropertyChild(effect, props, "filter"))) + findAttr(filterModes, ep, minFilter); Texture::FilterMode magFilter = Texture::LINEAR; - findAttr(filterModes, getEffectPropertyChild(effect, props, - "mag-filter"), - magFilter); + if ((ep = getEffectPropertyChild(effect, props, "mag-filter"))) + findAttr(filterModes, ep, 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 sourceParams(sourceParamInit); EffectNameValue opParamInit[] = { - {"src_color", TexEnvCombine::SRC_COLOR}, - {"one_minus_src_color", TexEnvCombine::ONE_MINUS_SRC_COLOR}, - {"src_alpha", TexEnvCombine::SRC_ALPHA}, - {"one_minus_src_alpha", TexEnvCombine::ONE_MINUS_SRC_ALPHA} + {"src-color", TexEnvCombine::SRC_COLOR}, + {"one-minus-src-color", TexEnvCombine::ONE_MINUS_SRC_COLOR}, + {"src-alpha", TexEnvCombine::SRC_ALPHA}, + {"one-minus-src-alpha", TexEnvCombine::ONE_MINUS_SRC_ALPHA} }; EffectPropertyMap 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"), - mode)) - result->setMode(mode); + findAttr(tgenModes, getEffectPropertyChild(effect, tgenProp, "mode"), 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)) { - SG_LOG(SG_INPUT, SG_ALERT, "Unknown TexGen plane " - << planeNode->getName()); - } else { - const SGPropertyNode* realNode - = getEffectPropertyNode(effect, planeNode); - SGVec4d plane = realNode->getValue(); - result->setPlane(coord, toOsg(plane)); - } + findAttr(tgenCoords, planeNode->getName(), coord); + const SGPropertyNode* realNode + = getEffectPropertyNode(effect, planeNode); + SGVec4d plane = realNode->getValue(); + 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); diff --git a/simgear/scene/material/makeEffect.cxx b/simgear/scene/material/makeEffect.cxx index 54f85664..1a8efcdb 100644 --- a/simgear/scene/material/makeEffect.cxx +++ b/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 lock(effectMutex); - EffectMap::iterator itr = effectMap.find(name); - if (itr != effectMap.end()) - return itr->second.get(); + { + OpenThreads::ScopedLock lock(effectMutex); + EffectMap::iterator itr = effectMap.find(name); + 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); - Effect* result = makeEffect(effectProps.ptr(), realizeTechniques, options); - if (result) - effectMap.insert(make_pair(name, result)); - return result; + try { + readProperties(absFileName, effectProps.ptr(), 0, true); + } + catch (sg_io_exception& e) { + SG_LOG(SG_INPUT, SG_ALERT, "error reading \"" << absFileName << "\": " + << e.getFormattedMessage()); + return 0; + } + ref_ptr result = makeEffect(effectProps.ptr(), realizeTechniques, + options); + if (result.valid()) { + OpenThreads::ScopedLock lock(effectMutex); + pair 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; // 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()); - } else { - osgDB::FilePathList dummy; - key = Effect::Key(prop, dummy); + key.paths = options->getDatabasePathList(); + } + Effect::Cache* cache = 0; + Effect::Cache::iterator itr; + { + OpenThreads::ScopedLock + lock(effectMutex); + cache = parent->getCache(); + itr = cache->find(key); + if (itr != cache->end()) + effect = itr->second.get(); } - Effect::Cache::iterator itr = cache->find(key); - if (itr != cache->end()) { - effect = itr->second.get(); - } else { + 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 + lock(effectMutex); + pair 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; } - } - if (!effect) { + } else { effect = new Effect; effect->root = prop; + effect->parametersProp = effect->root->getChild("parameters"); + } + if (realizeTechniques) { + try { + OpenThreads::ScopedLock + lock(effectMutex); + effect->realizeTechniques(options); + } + catch (BuilderException& e) { + SG_LOG(SG_INPUT, SG_ALERT, "Error building technique: " + << e.getFormattedMessage()); + return 0; + } } - effect->parametersProp = effect->root->getChild("parameters"); - if (realizeTechniques) - effect->realizeTechniques(options); - return effect; + return effect.release(); } } -- 2.39.5