Add timeout conditions

[weavejester]

This closes #40, though there are some rough edges in the implementation.

First, timeout conditions don't have an associated parameter, so the name of the condition will just be the randomly assigned default.

Second, I added in a clause in TransitionLine to handle the label for timeout conditions, but it might be better to refactor this to make use of the display_string method on the Condition objects instead. As far as I can see, the label formatting in TransitionLine mimics the output from display_string, except for triggers.

Finally, I use a _time_in_state float to record the time that the state machine has spent in the current state, incrementing it at each update. I think this is reasonable compared to just taking a timestamp, as it gives a consistant time per update.

[imjp94]

Thank you!

Overall it looks really good, just a few things to resolve before merge, as after reviewing your code I thought TimeoutCondition can be treated just like FloatCondition:

• TimeoutCondition extends FloatCondition then simply override get_value_string()

  func get_value_string():
      return .get_value_string() + "s"
  

  so that it would benefits from the value comparation of ValueCondition and the first issue can also be solved effortlessly

  First, timeout conditions don't have an associated parameter, so the name of the condition will just be the randomly assigned default.

  As well as the second issue, since it is a ValueCondition like FloatCondition and there would be no need to edit TransitionLine(Yeah, I just realized that Condition.display_string() is never used, but let's just keep it that way for now)

  Second, I added in a clause in TransitionLine to handle the label for timeout conditions, but it might be better to refactor this to make use of the display_string method on the Condition objects instead. As far as I can see, the label formatting in TransitionLine mimics the output from display_string, except for triggers.

  

• Transition.transit() replacing if condition.has_method("has_timed_out"): by checking if condition is TimeoutCondition, then call compare() with time_in_state

• TimeoutConditionEditor extends from ValueConditionEditor, both script and packed scene

• Fix error

  res://addons/imjp94.yafsm/scenes/condition_editors/TimeoutConditionEditor.gd:35 - Invalid call. Nonexistent function 'create_action' in base 'Nil'.
  

  I haven't figure out why undo_redo is not properly passed to TimeoutConditionEditor

[weavejester]

I'll look into the error. I was getting it earlier, but I thought that I had fixed it. Sorry about that!

I don't understand your comments about ValueCondition, I'm afraid. Currently the timeout condition triggers after the FSM has been in a state for a certain amount of time, so the timeout has a value (e.g. 0.31 seconds), but no associated parameter.

Equally, it also has no associated comparator, because the only comparators that make sense in context are > or >=, which are effectively the same thing because it's comparing to a float.

So the reason I didn't make it a ValueCondition to begin with is that it doesn't seem to have much in common with one. It has a value, but that value isn't compared to a parameter, but checked against the time spent in a state.

[imjp94]

Ah sorry, my bad, I mistaken a TimeoutCondition with TimeCondition.

But there's still some other issues:

• TimeoutCondition will only be tested once because StateMachinePlayer._transit() will refuse to test again unless parameter edited or last transition was successful:

   # Only get called in 2 condition, _parameters edited or last transition was successful
  func _transit():
      if not active:
    	  return
      # Attempt to transit if parameter edited or last transition was successful
      if not _is_param_edited and not _was_transited:
    	  return
  

  I am not sure how we can solve it yet, but maybe TimeoutCondition should be treated specially like a Trigger?

• Support TimeoutCondition remote debug. Which can be easily done through editing StateMachineEditorLayer.debug_update()

  https://user-images.githubusercontent.com/11486079/152685342-47370430-f7bb-46e0-8206-41cc2a9e2d5b.mp4

• Add a public getter for _time_in_state, since we are counting the elapsed time so why not let user make use of it:

[weavejester]

Ah, I see. It only works in my test case because I have a parameter changing every update.

Give me a few days to think about this. My inclination is to put timeout conditions in an array, then have a separate method to check state timeouts. May I ask why conditions are currently stored as a dictionary, keyed on the parameter?

[imjp94]

The data structure of state machine was designed this way for the sake of intuitive, based on the relationship between data:

var state_machine = state_machine_player.state_machine
var state = state_machine.states[state_name] # keyed by state name
var transition = state_machine.transitions[from][to] # keyed by state name transition from/to
var condition = transition.conditions[condition_name] # keyed by condition name

The conditions are actually keyed by Condition.name. I guess the impression that conditions are keyed by parameter is mainly because of ValueCondition where parameter is used as the name of condition.

In fact, gd-YAFSM has no concept of "parameter", a parameter is nothing but a identifier for the value passed to the state machine.

[weavejester]

Hi, sorry for the delay. I've thought about this a little now, and I think the timeout transition might be the wrong approach.

The time_in_state parameter is probably still a good idea, if (as you suggested) we add a public getter. There's little cost to adding it, and it gives useful information. If it's okay with you, I'll open up a separate PR for that small addition.

Instead of timeout transitions, what if there was a mechanism where parameters could automatically pull their values from variables on other nodes. For example, what if a parameter named $Character:walk automatically added code for:

set_param("$Character:walk", $Character.walk)

Or more generally:

if param_name.begins_with("$"):
    var node_path = param_name.trim_prefix("$")
    match get_node_and_resource(node_path):
        [_, _, null]:
            pass   # node with no variable index
        [var node, null, var index]:
            set_param(param_name, node.get_indexed(index))
        [_, var resource, var index]:
            set_param(param_name, resource.get_indexed(index))

This way a timeout transition could be written as a float condition named $.:time_in_state or perhaps more simply :time_in_state. It would also allow other variables to be pulled from other nodes without needing to write code for them, as long as no transformations were needed.

[imjp94]

The idea sounds really cool, but I don't think it fits well as an alternative solution to timeout condition, I thinks it's more related to expression condition #31

The original idea of adding a timeout is really simple yet powerful, for example in the case of gd-YAFSM demo, developer can replace splash_anim_finished with timeout to make a working prototype in the early development :

I think we both somehow over-engineer the solution, I just realized we can just add a "delay" property to Transition, so that we don't have to manage condition name anymore and there's only one delay for each Transition(unlike Condition, where you can have multiple same type conditions)

Transition.gd

tool
extends Resource

signal condition_added(condition)
signal condition_removed(condition)

export(String) var from # Name of state transiting from
export(String) var to # Name of state transiting to
export(Dictionary) var conditions setget ,get_conditions # Conditions to transit successfuly, keyed by Condition.name
export(int) var priority = 0 # Higher the number, higher the priority
export(float) var delay = 0
...

[weavejester]

A delay property is an interesting idea, but how do you envision transitions working if the user only needs a timeout, and doesn't need the associated condition check?

[imjp94]

Here's a rough idea

[weavejester]

Okay, but what happens if I want a transition with just a delay, and no associated parameter comparison? For example, what if I want a "dash" state to always transition back to "idle" after 0.1 seconds?

[imjp94]

Just use a transition without any conditions and set delay to 0.1s

[weavejester]

Oh, I see! I had it in my head that a transition required at least one condition, but that was a dumb assumption for me to make :)

Adding a delay to the transition seems like a good idea. It would certainly solve my problem, and I can also see how it would be useful when combined with other conditions (like splash_anim_finished in your example).

[Nukiloco]

Just wondering what is the status of this PR? Thanks!