Skip to content

Commit

Permalink
Don't allow disabling SVS from substates
Browse files Browse the repository at this point in the history
SVS keeps track of a stack of states which need to correspond to the current
goal state. `state_creation_callback` and `state_deletion_callback` are called
when goal states are created/deleted, and SVS updates its stack.
`state_deletion_callback` has an `assert` to check that the state passed to it
is the same one that is being popped; the two stacks must always stay in sync,
and the assert checks for this invariant.

If disabling SVS is allowed in substates, we can break the invariant like this:

* Start with SVS enabled (which is the default). Let's call the top state S1.
* Go two subgoals deep (S3)
* After S3 returns to S2, enable SVS again
* When S2 returns, `svs::state_deletion_callback` will be called. `state_stack`
will have S3 on top, since we skipped `svs::state_deletion_callback` while SVS
was disabled.
* Therefore, `assert(state == s->get_state());` will fail

To prevent this case, we fail attempts to disable SVS while within a subgoal.
This is probably a corner-case, but it ensures that we can maintain the
invariant and not throw an `assert` exception (or hit undefined behavior when
we call `pop_back` on an empty `std::vector`!).

See #475.
  • Loading branch information
garfieldnate committed Jun 26, 2024
1 parent b8cf2be commit 448c42b
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 0 deletions.
8 changes: 8 additions & 0 deletions Core/CLI/src/cli_svs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ bool CommandLineInterface::DoSVS(const std::vector<std::string>& args)
}
else
{
if (thisAgent->svs->is_in_substate()) {
m_Result << "Cannot disable Spatial Visual System while in a substate.";
return false;
}
thisAgent->svs->set_enabled(false);
m_Result << "Spatial Visual System disabled.";
}
Expand All @@ -72,6 +76,10 @@ bool CommandLineInterface::DoSVS(const std::vector<std::string>& args)
}
else
{
if (thisAgent->svs->is_in_substate()) {
m_Result << "Cannot disable Spatial Visual System in substates while in a substate.";
return false;
}
thisAgent->svs->set_enabled_in_substates(false);
m_Result << "Spatial Visual System disabled in substates.";
}
Expand Down
5 changes: 5 additions & 0 deletions Core/SVS/src/svs.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,11 @@ class svs : public svs_interface, public cliproxy
return "";
}

bool is_in_substate()
{
return state_stack.size() > 1;
}

// dirty bit is true only if there has been a new command
// from soar or from SendSVSInput
// (no need to recheck filters)
Expand Down
4 changes: 4 additions & 0 deletions Core/SVS/src/svs_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ class svs_interface
virtual void set_enabled(bool newSetting) = 0;
virtual bool is_enabled_in_substates() = 0;
virtual void set_enabled_in_substates(bool newSetting) = 0;
/**
* Indicates that the current top of the state stack is for a subgoal/substate
*/
virtual bool is_in_substate() = 0;
};

svs_interface* make_svs(agent* a);
Expand Down

0 comments on commit 448c42b

Please sign in to comment.