State Machine for Character

Question

I come today with a state machine I'm currently working on.

Problem:

Given any State that the Character is in, a button press or combination of button presses can modify that state in a different way than how the initial state was entered.

I've looked at several different methods of creating a solution, but I feel that my current solution is lacking in its ability to be extendable. Would a Behavior Tree be more suitable for this kind of problem?

public class StateManager<T> where T : Engine.Object
{
    //See notes in the Constructor.
    IState _currentState,_lastState,_nextState,_defaultState;

T _objectReference;

StateCompletion _onCompletion,_nextOnComplete; 

public StateManager(T objectRef)
{


    //Creating a "NullState" removes any and all possibilities of a NRE in the event that the next state is considered Null.
    //It can also be used for error handling or testing deadlock cases. 
    _defaultState = new NullState();
    _onCompletion = OnComplete;
    _nextOnComplete = OnComplete;

    //This is to store the object that we are currently manipulating with the state machine itself.
    _objectReference = objectRef;

    //The various states of the state machine shift and store the various states it passes through. 
    _currentState = _defaultState;
    _nextState = _defaultState;
    _lastState = _currentState;

    //Enter the default "NullState"
    _currentState.Enter(_objectReference,_lastState,_onCompletion);
}

//Used for the default NullState
private void OnComplete(object isComplete)
{
    Debug.LogWarning("Warning! You are acting on a Null State");
}

public void ChangeState<TU>(StateCompletion onComplete) where TU : IState, new()
{
    //Assign next OnComplete Delegate
    _nextOnComplete = onComplete;

    //Assign the appropriate state. 
    _nextState = new TU();
    if(_lastState != _currentState) //Set previous state if applicable. 
    {
        _lastState = _currentState;
    }
}

public void Execute()
{
    //This is essentially wrapping the inner state machines update loop.
    _currentState.Execute();

    //Check Completion of the current State to catch when the state is finished calculating it's information. 
    if(_currentState.IsComplete()) //States can be 
    {
        SwitchState();
    }
}

public void CheckModifier(KeyCode key)
{
    //This would pass the keycode into the state and check to see if it could modify its current actions.
    _currentState.CheckStateModifiers();
    //Something about this screams that this is going to lead to loads of repeated codes.
}

public void PhysicsExecute()
{
    _currentState.PhysicsExcecute();
}

private void SwitchState()
{
    //Call the current states OnComplete delegate.
    _onCompletion(_objectReference);
    _onCompletion = _nextOnComplete; //Assign the next delegate based on the last state change request.

    //Begins calculating the next states information in Excecute. 
    _currentState = _nextState;
    _currentState.Enter(_objectReference,_lastState,_onCompletion);

    //Set next OnComplete to the Null completion method.
    _nextState = _defaultState;
    _nextOnComplete = OnComplete;
}   
}

Answer 1

I think that you have an issue with general approach you have chosen. StateManager can still be improved tho.

1. Use better and consistent naming. Why _onCompletion but _nextOnComplete? Why _nextState, but _lastState (shouldn't it be "previous"?)? Why ChangeState() when it doesnt change the current state, but sets the next state instead (i'm not sure i understand why it changes last state as well)? What IsCompleted() does? Does it actually checks for completion (then the name is fine) or does it retrieves the result of operation (then it could use a better naming)? Looks like it is the latter. It should be a property either way. Minor stuff like that improves code readability by a lot.

2. Delegates are clearly coupled with states, so, if for some reason you can not refactor them into events of IState, you can implemet some aggregator-object:

   private class StateInfo
   {
       public IState State { get; set; }
       public StateCompletion OnExecuted { get; set; }
   }
   

   and then instead of

   _onCompletion = _nextOnComplete;
   _currentState = _nextState;
   

   use _currentStateInfo = _nextStateInfo. This will reduce the number of fields and assignments.

3. I dont see an issue with _currentState.CheckStateModifiers();. Repeated code should be dealt with by extracting common logic to the base class (StateBase?).

4. I'm not sure _currentState.Enter (state entering object) makes much sense. Shouldn't object enter state and not vice versa?

It's hard to tell more without knowing implementation details and the logic you need to implement.