From f076c43b90d9615c8c94622d30de022a0a84ead5 Mon Sep 17 00:00:00 2001 From: Stuart Buchanan Date: Thu, 14 Feb 2019 22:15:38 +0000 Subject: [PATCH] Improve parsing of state machines State machines will now throw exceptions if errors are encountered when loading the XML file. This includes: - Unknown or states referenced by transitions - States and transitions without elements - Transitions without or elements - State machines with fewer than two states Previously the state machine would load and then crash when hitting such malformed states or transitions. Now it will refuse to load, while providing useful error messages as to the problem. --- simgear/structure/StateMachine.cxx | 174 ++++++++++++++++++----------- 1 file changed, 111 insertions(+), 63 deletions(-) diff --git a/simgear/structure/StateMachine.cxx b/simgear/structure/StateMachine.cxx index 2bff0f3f..e29fefb1 100644 --- a/simgear/structure/StateMachine.cxx +++ b/simgear/structure/StateMachine.cxx @@ -20,7 +20,7 @@ */ #include - + #include "StateMachine.hxx" #include @@ -32,13 +32,13 @@ #include #include #include - + namespace simgear { typedef std::vector StatePtrVec; -static void readBindingList(SGPropertyNode* desc, const std::string& name, +static void readBindingList(SGPropertyNode* desc, const std::string& name, SGPropertyNode* root, SGBindingList& result) { for (auto b : desc->getChildren(name)) { @@ -54,8 +54,8 @@ class StateMachine::State::StatePrivate { public: std::string _name; - SGBindingList _updateBindings, - _entryBindings, + SGBindingList _updateBindings, + _entryBindings, _exitBindings; }; @@ -71,14 +71,14 @@ public: bool _excludeTarget; SGSharedPtr _condition; }; - + /////////////////////////////////////////////////////////////////////////// class StateMachine::StateMachinePrivate : public SGPropertyChangeListener { public: StateMachinePrivate(StateMachine* p) : _p(p) { } - + void computeEligibleTransitions() { _eligible.clear(); @@ -88,7 +88,7 @@ public: } } } - + StateMachine* _p; bool _initialised; State_ptr _currentState; @@ -96,14 +96,14 @@ public: std::vector _transitions; std::vector _eligible; SGTimeStamp _timeInState; - + bool _listenerLockout; ///< block our listener when self-updating props virtual void valueChanged(SGPropertyNode* changed) { if (_listenerLockout) { return; } - + if (changed == _currentStateIndex) { State_ptr s = _p->stateByIndex(changed->getIntValue()); _p->changeToState(s); @@ -111,7 +111,7 @@ public: _p->changeToStateName(changed->getStringValue()); } } - + // exposed properties SGPropertyNode_ptr _root; SGPropertyNode_ptr _currentStateIndex; @@ -144,12 +144,12 @@ void StateMachine::State::update() void StateMachine::State::fireEntryBindings() { fireBindingList(d->_entryBindings); -} +} void StateMachine::State::fireExitBindings() { fireBindingList(d->_exitBindings); -} +} void StateMachine::State::addUpdateBinding(SGBinding* aBinding) { @@ -184,38 +184,38 @@ StateMachine::Transition::~Transition() StateMachine::State* StateMachine::Transition::target() const { return d->_target; -} +} void StateMachine::Transition::addSourceState(State* aSource) { if (aSource == d->_target) { // should this be disallowed outright? SG_LOG(SG_GENERAL, SG_WARN, d->_name << ": adding target state as source"); } - + d->_sourceStates.insert(aSource); -} +} bool StateMachine::Transition::applicableForState(State* aCurrent) const { if (d->_excludeTarget && (aCurrent == d->_target)) { return false; } - + if (d->_sourceStates.empty()) { return true; } return d->_sourceStates.count(aCurrent); -} - +} + bool StateMachine::Transition::evaluate() const { return d->_condition->test(); -} +} void StateMachine::Transition::fireBindings() { fireBindingList(d->_bindings); -} +} std::string StateMachine::Transition::name() const { @@ -231,12 +231,12 @@ void StateMachine::Transition::addBinding(SGBinding* aBinding) { d->_bindings.push_back(aBinding); } - + void StateMachine::Transition::setExcludeTarget(bool aExclude) { d->_excludeTarget = aExclude; } - + /////////////////////////////////////////////////////////////////////////// StateMachine::StateMachine() : @@ -249,7 +249,7 @@ StateMachine::StateMachine() : StateMachine::~StateMachine() { - + } void StateMachine::init() @@ -257,23 +257,23 @@ void StateMachine::init() if (d->_initialised) { return; } - + if (d->_states.empty()) { throw sg_range_exception("StateMachine::init: no states defined"); } - + d->_currentStateIndex = d->_root->getChild("current-index", 0, true); d->_currentStateIndex->setIntValue(0); - + d->_currentStateName = d->_root->getChild("current-name", 0, true); d->_currentStateName->setStringValue(""); - + d->_currentStateIndex->addChangeListener(d.get()); d->_currentStateName->addChangeListener(d.get()); - + d->_timeInStateProp = d->_root->getChild("elapsed-time-msec", 0, true); d->_timeInStateProp->setIntValue(0); - + // TODO go to default state if found innerChangeState(d->_states[0], NULL); d->_initialised = true; @@ -283,20 +283,23 @@ void StateMachine::shutdown() { d->_currentStateIndex->removeChangeListener(d.get()); d->_currentStateName->removeChangeListener(d.get()); - + } void StateMachine::innerChangeState(State_ptr aState, Transition_ptr aTrans) { if (d->_currentState) { d->_currentState->fireExitBindings(); + SG_LOG(SG_GENERAL, SG_INFO, "Changing from state " << d->_currentState->name() << " to state:" << aState->name()); + } else { + SG_LOG(SG_GENERAL, SG_INFO, "Initializing to state:" << aState->name()); } - -// fire bindings before we change the state, hmmmm + +// fire bindings before we change the state, hmmmm if (aTrans) { aTrans->fireBindings(); } - + // update our private state and properties d->_listenerLockout = true; d->_currentState = aState; @@ -305,11 +308,11 @@ void StateMachine::innerChangeState(State_ptr aState, Transition_ptr aTrans) d->_currentStateIndex->setIntValue(indexOfState(aState)); d->_timeInStateProp->setIntValue(0); d->_listenerLockout = false; - + // fire bindings d->_currentState->fireEntryBindings(); d->_currentState->update(); - + d->computeEligibleTransitions(); } @@ -319,11 +322,11 @@ void StateMachine::changeToState(State_ptr aState, bool aOnlyIfDifferent) if (std::find(d->_states.begin(), d->_states.end(), aState) == d->_states.end()) { throw sg_exception("Requested change to state not in machine"); } - + if (aOnlyIfDifferent && (aState == d->_currentState)) { return; } - + innerChangeState(aState, NULL); } @@ -333,7 +336,7 @@ void StateMachine::changeToStateName(const std::string& aName, bool aOnlyIfDiffe if (!st) { throw sg_range_exception("unknown state:" + aName); } - + changeToState(st, aOnlyIfDifferent); } @@ -341,7 +344,7 @@ StateMachine::State_ptr StateMachine::state() const { return d->_currentState; } - + SGPropertyNode* StateMachine::root() { return d->_root; @@ -352,27 +355,27 @@ void StateMachine::update(double aDt) // do this first, for triggers which depend on time in current state // (spring-loaded transitions) d->_timeInStateProp->setIntValue(d->_timeInState.elapsedMSec()); - + Transition_ptr trigger; - + for (auto trans : d->_eligible) { if (trans->evaluate()) { if (trigger != Transition_ptr()) { - SG_LOG(SG_GENERAL, SG_WARN, "ambiguous transitions! " + SG_LOG(SG_GENERAL, SG_WARN, "ambiguous transitions! " << trans->name() << " or " << trigger->name()); } - + trigger = trans; } } - + if (trigger != Transition_ptr()) { SG_LOG(SG_GENERAL, SG_DEBUG, "firing transition:" << trigger->name()); innerChangeState(trigger->target(), trigger); } - + d->_currentState->update(); -} +} StateMachine::State_ptr StateMachine::findStateByName(const std::string& aName) const { @@ -381,7 +384,7 @@ StateMachine::State_ptr StateMachine::findStateByName(const std::string& aName) return sp; } } - + SG_LOG(SG_GENERAL, SG_WARN, "unknown state:" << aName); return State_ptr(); } @@ -391,17 +394,17 @@ StateMachine::State_ptr StateMachine::stateByIndex(unsigned int aIndex) const if (aIndex >= d->_states.size()) { throw sg_range_exception("invalid state index, out of bounds"); } - + return d->_states[aIndex]; } - + int StateMachine::indexOfState(State_ptr aState) const { StatePtrVec::const_iterator it = std::find(d->_states.begin(), d->_states.end(), aState); if (it == d->_states.end()) { return -1; } - + return it - d->_states.begin(); } @@ -410,7 +413,7 @@ StateMachine::State_ptr StateMachine::createState(const std::string& aName) if (findStateByName(aName) != NULL) { throw sg_range_exception("duplicate state name"); } - + State_ptr st = new State(aName); addState(st); return st; @@ -431,38 +434,83 @@ void StateMachine::initFromPlist(SGPropertyNode* desc, SGPropertyNode* root) d->_root = root->getNode(path, 0, true); assert(d->_root); } - + + int stateCount = 0; for (auto stateDesc : desc->getChildren("state")) { + stateCount++; std::string nm = stateDesc->getStringValue("name"); + + if (nm.empty()) { + SG_LOG(SG_GENERAL, SG_ALERT, "No name found for state in branch " << path); + throw sg_exception("No name element in state"); + } + State_ptr st(new State(nm)); - + readBindingList(stateDesc, "enter", root, st->d->_entryBindings); readBindingList(stateDesc, "update", root, st->d->_updateBindings); readBindingList(stateDesc, "exit", root, st->d->_exitBindings); - + addState(st); } // of states iteration - + + if (stateCount < 2) { + SG_LOG(SG_GENERAL, SG_ALERT, "Fewer than two state elements found in branch " << path); + throw sg_exception("Fewer than two state elements found."); + } + for (auto tDesc : desc->getChildren("transition")) { std::string nm = tDesc->getStringValue("name"); - State_ptr target = findStateByName(tDesc->getStringValue("target")); - + std::string target_id = tDesc->getStringValue("target"); + + if (nm.empty()) { + SG_LOG(SG_GENERAL, SG_ALERT, "No name found for transition in branch " << path); + throw sg_exception("No name element in transition"); + } + + if (target_id.empty()) { + SG_LOG(SG_GENERAL, SG_ALERT, "No target element in transition " + << nm << " in state branch " << path); + throw sg_exception("No target element in transition"); + } + + State_ptr target = findStateByName(target_id); + + if (target == NULL) { + SG_LOG(SG_GENERAL, SG_ALERT, "Unknown target state " << target_id << " in transition " + << nm << " in state branch " << path); + throw sg_exception("No condition element in transition"); + } + + if (tDesc->getChild("condition") == NULL) { + SG_LOG(SG_GENERAL, SG_ALERT, "No condition element in transition " + << nm << " in state branch " << path); + throw sg_exception("No condition element in transition"); + } + SGCondition* cond = sgReadCondition(root, tDesc->getChild("condition")); - + Transition_ptr t(new Transition(nm, target)); t->setTriggerCondition(cond); - + t->setExcludeTarget(tDesc->getBoolValue("exclude-target", true)); for (auto src : tDesc->getChildren("source")) { State_ptr srcState = findStateByName(src->getStringValue()); + + if (srcState == NULL) { + SG_LOG(SG_GENERAL, SG_ALERT, "Unknown source state " << src->getStringValue() << " in transition " + << nm << " in state branch " << path); + throw sg_exception("No condition element in transition"); + } + t->addSourceState(srcState); } - + readBindingList(tDesc, "binding", root, t->d->_bindings); - + addTransition(t); } // of states iteration - + init(); }