Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FrayCombatStateMachine documentation doesn't adequately describe how to receive its data #44

Closed
20milliliter opened this issue Mar 23, 2023 · 18 comments
Labels
documentation Improvements or additions to documentation

Comments

@20milliliter
Copy link
Contributor

20milliliter commented Mar 23, 2023

The CombatStateMachine is intended to recieve inputs or sequences and automatically resolve the transition of states, but how is that data meant to be forwarded to other nodes, such as a fighter's AnimationPlayer?

FrayCombatStateMachine contains no signals.
FrayRootState has transitioned(from: StringName, to: StringName), but it is not referenced in any other classes and does not appear to be intended to be accessible from the state machine.

What is the intended method of allowing an AnimationPlayer (for instance) to change its animation in coordination with a state transition within a FrayCombatStateMachine?

@Pyxus
Copy link
Owner

Pyxus commented Mar 23, 2023

Thanks for bringing this to my attention. This is actually a left over from a much earlier design decision. I see no issue with accessing the public interface of the root state object for any reason. However, I did intend there to be a signal on the state machine node as that would be more intuitive; this is also something that should be mentioned in the 'getting started'.

@20milliliter 20milliliter changed the title FrayCombatStateMachine documentation doesn't adequately describe how to receive it's data FrayCombatStateMachine documentation doesn't adequately describe how to receive its data Mar 23, 2023
@20milliliter
Copy link
Contributor Author

Also, what's the intended means of updating the values of FrayConditions within a state machine?

And how do you define when a given state ends? (SwitchMode.AT_END)

@Pyxus
Copy link
Owner

Pyxus commented Mar 27, 2023

  1. Conditions are managed on the state object. Since the root state uses conditions for pre-reqs and such you'll likely only ever need to update conditions on it. To do so you call root.set_condition(condition_name, value)

  2. Just like how you can designate a start_state on the root state object, you can also designate a end_state. A root state is considered to have ended when it reaches that end state.

More things I definitely need to make clear through documentation.

@20milliliter
Copy link
Contributor Author

So FrayRootState._states are connected via FrayStateMachineTransitions, but its included switch_mode property is only usable when multiple RootStates are connected, also via a FrayStateMachineTransition?

@20milliliter
Copy link
Contributor Author

20milliliter commented Mar 27, 2023

The main conceptual roadblock is what purpose state machine encapsulation provides. What is intended to be within a situation, (assuming 'Situation' is synonymous for FrayRootStates) and what is intended to be outside?

The following is my best guess:
image
However, its not made clear how–if at all–that Situations are also intended to be transitioned betweened automatically in this way.

@Pyxus
Copy link
Owner

Pyxus commented Mar 27, 2023

Yeah, essentially. This is a hierarchical state machine, so it's designed to support states within states. The end switch allows you to control transitions based on the "state" of a sub-state. Small elaboration on my part though... there is a _is_done_processing_impl() virtual method present on the base state class, this returning true is what actually determines if a state is considered to have "ended". RootStates override this to return true when the current state is the end state, or if there is no end state set. So it's not that the switch_mode property is only usable when using child RootStates. Its more so Fray doesn't provide any states which "end" by default besides the root state. However, users can introduce new states if they so desired and implement the behavior they want.

@Pyxus
Copy link
Owner

Pyxus commented Mar 28, 2023

The main conceptual roadblock is what purpose state machine encapsulation provides. What is intended to be within a situation, (assuming 'Situation' is synonymous for FrayRootStates) and what is intended to be outside?

The following is my best guess: image However, its not made clear how–if at all–that Situations are also intended to be transitioned betweened automatically in this way.

You are correct a situation is just a named FrayRootState within a FrayCombatStateMachine. Conceptually it is meant to encapsulate any number of "situations" your fighter may find themself in. How you draw the line between those situations is entirely up to you.

Fray, currently, does not have any way of automatically transitioning between situations. It's intended to be handled externally by the user. This is something I had to think a lot about, and at one point considered representing situations with high-level states. However I decided against it as there could potentially be multiple things which causes a fighter's situation to change during battle; I wanted to avoid supporting non-deterministic state machines as I thought they could get unnecessarily complex.

Your graph is accurate! (also makes me realize the documentation is still lacking beyond typos and poor phrasings... more images would help). Small thing, there is no issue with your use of auto-advance in principle. It's just Fray does not provide a state out-of-the-box which would "end" when your attack ends. I could add one but i'd have to make a lot of assumptions when creating it... assumptions such as you using an AnimationPlayer and the attack being "done" when the animation ends. Of course you could create a custom state for your purposes to automate the transition back to idle. However the CombatStateMachine also has goto_start_state() that you can call for this purpose.

@20milliliter

This comment was marked as outdated.

@20milliliter
Copy link
Contributor Author

It's intended to be handled externally by the user.

Gotcha. That's a reasonable decision.

However, it doesn't read to me like that due to how (it appears at first glance, at least) half of the functionality is already there ([again,] SwitchMode.AT_END, RootStates extending FrayState, etc).

Were someone to implement their own approach, are FrayStateMachineTransitions intended to be used? If not, a FrayCombatStateMachine.current_situation_ended signal feels required.

@20milliliter
Copy link
Contributor Author

Alright, finally got my hands dirty with this.

How so? If the AnimationPlayer animates FrayCombatStateMachine.allow_transitions in the way that the docs suggest, the states would resolve as follows:

I tested it and found out the states, in fact, do not resolve that way. I could go into more detail but

However the CombatStateMachine also has goto_start_state() that you can call for this purpose.

after taking this approach I find it much cleaner and so I won't bother.
(Perhaps it goes without saying, but) a mention of goto_start_state() within the docs would be ideal.

@20milliliter
Copy link
Contributor Author

Also, another question about FrayConditions:

The class itself claims to be an abstract class but the documentation shows it being instantiated instead of extended. Which is the intended way to use it?

@Pyxus
Copy link
Owner

Pyxus commented Apr 3, 2023

The class itself claims to be an abstract class but the documentation shows it being instantiated instead of extended. Which is the intended way to use it?

This is actually left over from an earlier version where they're were multiple condition types. The documentation will be corrected, thanks for pointing it out.

@Pyxus
Copy link
Owner

Pyxus commented Apr 3, 2023

The wording also confuses me: states themselves don't end, RootStates can end by being in their designated end_state state.

All states can "end". A state is considered to have "ended" when is_done_processing() returns true. The base state class always returns true by default so in that sense base states are always in an "end" state. The root state overrides that method's implementation to return true only when it's current state = a specified end state.

It's set up this way in order to make the state machine system more extendible. You could in principle extend the base State class and code it so your own custom state which ends when... for example, it counts to 5 seconds.

How so? If the AnimationPlayer animates FrayCombatStateMachine.allow_transitions in the way that the docs suggest, the states would resolve as follows:

1. `standing_punch` state is transitioned into. This begins the `standing_punch` animation, disabling `allow_transitions`.

2. The animation finishes, re-enabling `allow_transitions`.

3. As the `GroundSituation` `FrayRootState` has reached its `end_node`, the `auto_advance` flagged transition between the RootState and itself activates, reentering the situation at its start node.

4. `idle` state is transitioned into.

Having now hopefully explained what I'm going for this this whole state ending thing, maybe you can better see what I meant by "Fray does not provide a state out-of-the-box which would 'end' when your attack ends". It's not that I cant, or that it would be difficult, but rather I would have to make a lot of assumptions of how the user is handling their animation states. Which would, in my opinion, lead to a class without much flexibility. Though if you disagree feel free to open a new issue relating to that, I'm open to discussion.

@Pyxus
Copy link
Owner

Pyxus commented Apr 3, 2023

Gotcha. That's a reasonable decision.

However, it doesn't read to me like that due to how (it appears at first glance, at least) half of the functionality is already there ([again,] SwitchMode.AT_END, RootStates extending FrayState, etc).

I've made some comments above this so maybe by the time you read this things will change... But could you help me understand what you find confusing? Maybe I just don't do a good enough job clarifying the difference between the root state, state machine node, and situations.

The transitions options, and more broadly transitions, only exist within root states. Some options take into account any state's "state", for a lack of a better term, but they all still live in a root state.

The state machine node is only responsible for processing the root state, if Godot's processing wasn't all done from the scene tree, and if it weren't based around nodes this class probably wouldn't exist.

Situations are a conceptual split up where you create a root state for each "situation" your fighter can be in. How you define a situation is arbitrary and entirely up to you. If you really wanted to theres nothing stopping you from only having 1 situation. It is a hierarchical state machine So you can on paper describe some complex systems with nested states.

Were someone to implement their own approach, are FrayStateMachineTransitions intended to be used? If not, a FrayCombatStateMachine.current_situation_ended signal feels required.

There is a situation_changed signal which was added in addition to the state_changed signal in the state management PR. However, I don't really understand the connection here. If by "implement their own approach" you mean handle changing situations then no the transition class would not be required as those are specifically for state transitions in the root state class.

If by "implement their own approach" you're instead referring to my comment about creating custom states then yes, state machine transitions would still be involved.. But none of that directly relates to situations.

@20milliliter
Copy link
Contributor Author

Maybe I just don't do a good enough job clarifying the difference between the root state, state machine node, and situations.

I believe this is likely the root cause of most of my confusion.

However, I don't really understand the connection here

By "implement their own approach" I did indeed mean handle changing situations.

The connection arose due to a pattern I thought was present between user-defined states and (hypothetical) user-defined transitions. As you said, its intended for a user to be permitted to extend the base FrayState class, if extra functionality is desired. Because FrayRootState extends FrayState, and the description for FrayStateMachineTransition is "Represents transition from one state to another.", I thought perhaps FrayStateMachineTransition was intended to be extended in the same way to allow for extra functionality to transition between FrayRootStates.

Again, though, I believe this was only due to the few remaining holes I have in understanding how all of Fray works together.

@Remi123
Copy link

Remi123 commented Aug 24, 2023

I now have a relatively simple character controller that jump around. I think I figure it out how combat_state_machine works, which is a fancy names for a State machine that manage button, sequences and situation, the latter being a FrayRootState that can have starting and ending states.

My approach is quite similar to 20milliliter drawing, with two situations. One for ground and the other for air.

  var  _cond_on_floor := FrayCondition.new("on_floor") # = body.is_on_floor()
  var  _cond_in_air := FrayCondition.new("in_air ") # = not body.is_on_floor()
  
  var locomotion :FrayRootState= (FrayRootState.builder()
    .add_state("idle",MyGroundMovementClassFrayState.new())
    .add_state('jump',MyJumpingClassFrayState.new())
    .add_state('attack_1',MyAttackClassFrayState.new())
    
    .tag_multi(['idle','falling'],['start']) # starting tag
    .tag_multi(["attack_1",'jump'],['end']) # ending tag
    .add_rule('end','start') # everything tagged end can transition to a starting tag at any moment once finished
    .transition_global("idle",{
	    auto_advance=true,prereqs=[_cond_on_floor],priority = 1
    })
    .transition_global("falling",{
	    auto_advance=true,prereqs=[_cond_in_air],priority = 1
    })
    .transition_button('idle','jump',{
	    input='push',prereqs=[_cond_on_floor]
    })  # can jump if on floor. Since it's tagged end the next state is either falling or idle, most likely falling
    .transition_button("idle", "attack_1", {input="attack",min_input_delay=0.5})
    
    .start_at("idle")
    .end_at('falling')
    .build()
  )
  
   var in_air :FrayRootState = (FrayRootState.builder()
    .add_state('falling',MyAirMovementClassFrayState.new())
    .transition('falling','to_land',{
	    prereqs=[_cond_on_floor]
    })
    .start_at("falling")
    .end_at('to_land')
    .build()
  )
  
  combat_state_machine.add_situation("locomotion",locomotion)
  combat_state_machine.add_situation("in_air",in_air)

To transition between situation I ended up connecting to the FrayRootState signals:

  locomotion.transitioned.connect(
    func(from,to):
      match [from,to]:
        [_,'jump']:
          pass # jump state class handle the jump and it automatically transition to falling state.
        [_,'falling']:
          csm.change_situation('in_air')
  ,CONNECT_DEFERRED)
  
  in_air.transitioned.connect(
    func(from,to):
      match[from,to]:
        [_,"to_land"]:
          csm.change_situation('locomotion')
  ,CONNECT_DEFERRED)

I must insist for you to connect the callable with CONNECT_DIFFERED if you are changing the situation, since this signal is emitted inside the logic when transitioning. If you don't do that it's possible to have a continuous changes of states.

It's a basic example but I think it exemplify a need to facilitate changing situation when entering some specific states. The problem with CONNECT_DEFERRED was easy to encounter and was a source of frustration until I figured that changing situation while the library handled changing wasn't a good idea.

I'm using situation as a parent states, and since I saw in another issues that sub states exists, it's probably better to handle it that way but I don't know how yet. Also, in my cases it's possible to have multiple End state in a situation. Falling is one that can be triggered by either quitting the floor or jumping, but every button or sequence referencing an attack would go to a AttackSituation.

I'm just writing my findings of the last two days, if it can help someone.

@Pyxus
Copy link
Owner

Pyxus commented Aug 28, 2023

I'm using situation as a parent states, and since I saw in another issues that sub states exists, it's probably better to handle it that way but I don't know how yet

I saw your question on that issue and i'll address it shortly over there. But I wanted to say that while I do intend to support substates, I decided to not use substates to represent situations. I believe my reasoning at the time was that state machines should be deterministic; one input for each transition. However, I've considered changing that mainly because there's nothing else enforcing determinism plus users can already manually manage transitions if desired, and it might be more intuitive to use what's already there rather than adding this new concept that exists only on the CombatStateMachine. It really wouldn't require too much work on my part either.

The problem with CONNECT_DEFERRED was easy to encounter and was a source of frustration until I figured that changing the situation while the library handled changing wasn't a good idea.

I'll make a not to mention this in the documentation when I get to resolving this issue. However I wonder if there a way I could design this to avoid the need for a deferred connection, I wouldn't want anyone else to encounter the same frustrations even if the documentation mentions a solution.

I'm just writing my findings of the last two days, if it can help someone.

Thank you for sharing!

@Pyxus Pyxus added the documentation Improvements or additions to documentation label Aug 28, 2023
@Pyxus
Copy link
Owner

Pyxus commented Jan 25, 2024

Closing as the state machine has changed fairly drastically since this issue was made. Also I believe the new documentation clarifies the issues discussed here. Though if you feel i'm wrong do open another issue!

@Pyxus Pyxus closed this as completed Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants