State Management Refactor #70
Conversation
Rather than having a specialized state machine with support for buffered inputs, this new node can be used to control the state machine and advance using buffered inputs. This change also removes the concept of "situations". If desired users can create sub-state-machines and use that to mimic the previous situation model.
Root states can have other root states as children. I think the name Composite state better reflects the idea that it is just a state which contains other states.
It only existed to facilitate root swapping in the now removed CombatStateMachine
The current state should only be changed through the travel or goto method so having a public property might be misleading.
Also made it so sub-states attempt to advance after the parent state. This is because advancement of the parent state means the sub-state would become active anyway.
For now, I'm just re-using the combat state machine icon.
|
Ok this is a big refactor. I'll state that I didn't read the new changes in the code yet, so I'll just comment on the description.
In theory this is a good idea, since there will always be some sort of combination of input or behavior that will fall outside of what the interface of CSM can help. I was already making some custom logic for my is_held vs is_tapped logic so if you provide a BufferInputAdvancer to define those "workaround" it can help and I'm in favor. However I didn't read the implementation so I don't know how easy it is. Is_tapped vs Is_held is for me a good test case.
The
This is always nice to let users define their own object, but I didn't play enough with Transitions to know when it's a good idea apart from the obvious button and sequence. Maybe a FrayTimedTransition would be nice to add. I do look for a solution for a future problem where I need to communicate to my next state a value that I would calculate inside the transition. Speaking of user-defined objects, I find conditions clunky to work with. I have to
Goto behavior needs to be very well documented and tested. I think it is still useful to offer the possibility to teleport to the target_state, calling exit on the current one. I may be old school, but the name CompositeState default to go to start is great but I'm sure someone would want to keep the current substate on one node. Maybe upon exiting, set the start value of the CompositeState to the current substate if not resetable ? In any case, I need to test this onto my controller.
Not sure if I helped 😄 or if I'm clear enough. But you are doing a great job and I do like the direction this is going. I'll keep you updated, I'm just a little bit busy. |
Could you help me better understand what you're trying to accomplish? Assuming you want to set up transitions that occur when a button "is_held" or "is_tapped". One of these is already possible. "is_held" would be a button transition with a min_time_held set. "is_tapped" should just require me to add a max_time_held constraint. You also mention your setup being similar to a filter so I may just be misunderstanding your goals.
Edit: Implement
I think the greater ease of use trumps the added expense so I'm all for this idea.
After thinking about it I believe it would be better to rename this as it is more similar to travel than the previous goto. It just travels vertically instead of horizontally. And Edit: Did some more thinking. I'm open for discussion but I think
Edit: Implemented
Thank you! I appreciate you taking the time to leave some feedback |
|
My first impression is good. One strange bug I have is that I have a custom fraystate for one of the state, but only the enter_impl is called. Exit_impl isn't called. I found that adding substate quite easy if i use two builder and add the substate. The default state being gdscript class isn't as versatile as I hoped. Conditions being callable is a win. So easy. I'll share the test case soon. But it's a very barebone parent-child hierarchical state machine |
|
Found the issue about the exit function not being called on my custom fraystate. I changed it to extends FrayCompoundState instead of FrayState and the exit function is now called. I don't know how fraystate are supposed to interact within FrayCompoundState, but it's something to add to the docs. The issues remains that classes that extends FrayState doesn't get called their exit function. For adding substate, here how I did it : extends CharacterBody3D
@onready var sm: FrayStateMachine = $FrayStateMachine
@onready var buffer: FrayBufferedInputAdvancer = $FrayBufferedInputAdvancer
func _unhandled_input(event: InputEvent) -> void:
if event.is_action_pressed('ui_right'):
buffer.buffer_button('btn_punch')
if event.is_action_pressed('ui_left'):
buffer.buffer_button('btn_kick')
var combat : FrayCompoundState
var loco : FrayCompoundState
func _ready() -> void:
combat = (FrayCompoundState.builder()
.start_at('start')
.add_state("punch",DebugTrace.new("Punch")) # DebugTrace is printing when entering and exiting states
.add_state("kick",DebugTrace.new("Kick")) # kick attack, another entry point
.transition('start','punch',{auto_advance=true}) # Default attack is the punch
.transition_button("punch", "combo", {input="btn_punch", prereqs=["on_hit"]}) #can combo on hit
.tag_multi(['punch',"combo","kick"],['finish']) # Tagged finish combo
.tag('end',['to_end'])
.add_rule('finish','to_end') # transition to end rule
.transition_global('end',{auto_advance=true,prereqs="!on_hit"}) # auto end combo
.end_at('end')
).build()
loco = (FrayCompoundState.builder()
.register_conditions({
on_hit = func(): return ..., # hit logic, not ready yet
on_ground = func(): return is_on_floor(),
in_air = func(): return not is_on_floor() and not is_on_wall(),
on_wall =func():return is_on_wall()
})
.start_at('ground')
.add_state('combat',combat) # add the compoundstate of combat to the combat state inside locomotion.
.add_state('ground',DebugTrace.new('on ground'))
.transition('air','ground',{prereqs=['on_ground'],auto_advance=true})
.transition('ground','air',{prereqs=['in_air'],auto_advance=true})
.transition_button('ground','combat',{input="btn_punch"}) # can enter combat state, auto_advance to punch
.transition_button('ground','combat/kick',{input="btn_kick"}) # Kick
.transition('combat','ground',{auto_advance=true,switch_mode=FrayStateMachineTransition.SwitchMode.AT_END}) # Return to ground when attack finish.
).build()
sm.root = loco # assign locomotion as the rootThis is REALLY easy. Not sure if this is how you would do it but I'm already imagining a lot of functionality from this. Ideally I would like to register the conditions in their substate that require them ( loco doesn't need on_hit, but I must register it there otherwise it isn't registered). Not a deal breaker. Also, about the default state, the fact that it's a GDScript class doesn't allow me to configure it as I go, but I don't know how to improve it. For now I'm mainly using |
|
Unless you have other changes that you want to make, I think this is a great refactoring. It work like a charm on my test cases and the conditions are easy to setup. Hierarchical states solved my previous problem with multiple situations in CombatStateMachine, About what's left:
About I think the only thing missing is some helpers for transition using a signal. Animations have a lot of points of interest like I'll help with the documentation soon |
|
Hey all, I've been messing around with this PR in a pretty generic 3d environment and for the most part, I think that this refactor is very well thought out! The builder syntax is very descriptive, and the
I'm in agreement with Remi123 on helpers for transitions. Outside of rolling my own What I'm aiming to do kinda already goes against keeping these 3 modules separate, as I'd like to annotate my animations with all these properties (what FrayState is linked to a HitState? At what point can this animation be considered finished processing to return back to the idle state? at what point can a player input a button to 'cancel' into another action?) rather than having it as part of the state setup. In my case, I'm trying to replicate a JRPG's action combat system, in which a single character has 3 attacks that can be comboed from 1, to 2, to 3. These animations have a very long follow-through (pretty much putting them in an idle state) in which the player can delay the next part of the string, in order to have other characters come in and do parts of their combo. In attempting to emulate this, I had the animation player controlling both the For example, this is the test player's script: extends CharacterBody3D
@onready var sprite = $Sprite3D
@onready var animation_player = $AnimationPlayer
@onready var state_machine: FrayStateMachine = $FrayStateMachine
@onready var input: FrayBufferedInputAdvancer = $FrayBufferedInputAdvancer
var state_container: FrayCompoundState
var target_velocity = Vector3.ZERO
var direction = Vector3.ZERO
var button_input_name = null
func _ready() -> void:
_setup_state_machine()
func _unhandled_input(event: InputEvent) -> void:
if event.is_action_pressed(button_input_name):
input.buffer_button('attack')
if event.is_action_pressed("battle_shoulder_right"):
input.buffer_button('special')
func _setup_state_machine() -> void:
state_container = ( FrayCompoundState.builder()
.register_conditions({
on_hit = func(): return true,
})
.transition('start', 'idle')
.transition_button('idle', 'neutral_attack_1', { input='attack' })
.transition_button('neutral_attack_1', 'neutral_attack_2', { input='attack' })
.transition_button_global('special_attack', { input='special' })
.tag_multi(['idle'], ['neutral'])
.tag_multi(['neutral_attack_1', 'neutral_attack_2',], ['normal'])
.tag_multi(['special_attack'], ['special'])
.add_rule('neutral', 'special')
.add_rule('normal', 'special')
.add_rule('normal', 'neutral')
.add_rule('special', 'neutral')
.start_at('start')
).build()
state_machine.root = state_container
animation_player.play('start')
func _on_fray_state_machine_state_changed(from, to):
print("state change from: " + from + " to: " + to)
if(anim_player.get_animation_list().has(to)):
anim_player.stop()
anim_player.play(to)and an image link that hopefully helps explain my current setup. I also tried to make it so that every animation would return to idle by setting them up as a default transition ( something like One final note, an example project to showcase a typical setup similar to the example gif in the project's README would be greatly appreciated, as trying to use the documentation was a bit painful. |
|
I wouldn't really control the state machine active property, as it add complexity when it's active or not. I would add a lot more @export var can_transition :bool = false
...
root_machine.register_conditions(
{can_transition = func() return self.can_transition,
}
)
.transition_button('idle', 'neutral_attack_1', { input='attack' })
.transition_button('neutral_attack_1', 'neutral_attack_2', { input='attack', prereqs=[can_transition,on_hit] })Then modify the |
|
I've found a small bug in the collision library. # hit_state_3d.gd
## Sets whether the hitbox at the given [kbd]index[/kbd] is active or not.
func set_hitbox_active(index: int, is_active: bool) -> void:
var flag = 1 << index
#if flag > active_hitboxes:
#push_warning("Index out of bounds.")
#return
# <-- Removed this warning since once all hit_box are disable with active_hitboxes = 0,
# it prevent me from re-activate them.
if is_active:
active_hitboxes |= flag
else:
active_hitboxes &= ~flag # <-- missing the ~ |
This should be fine, _init is a special function but Godot doesn't treat _init_XYZ as anything special. That being said I settled on the name _ready_impl since I think that better captures that the virtual method is used for.
A few other devs I know agreed that unless specifically told they assume time is in seconds. Additionally I think your argument of ms and frame count being confused holds some weight. I've gone back to having the time in seconds, and added static methods under the Also going forward, to keep things consistent, all times in Fray should use seconds, and non-second times will be given an appropriate suffix.
This should be fixed.
I changed it so default states now take a state instance instead of a script. The state is duplicated when a new one needs to be instantiated. 2 caveats, to support this I made
Noted. What I will likely do is support global conditions (located on the root) and local conditions. Conditions will be checked locally first, and then globally so local conditions will shadow global conditions. I'll also add a character to explicitly check a global condition. Maybe: "$on_hit"?
Thanks for checking out the project! And yes the documentation needs a big overhaul. This PR will be merged within the next few days and after that my next priority is rewriting the documentation and including examples.
I'll create a discussion soon enough for this so I can better understand what sort of solution might address this pain point. It won't be addressed in this PR.
Would be great if Godot had support for namespaces... I'll give it some thought and tackle this in another PR if I decided to go forward. |
oct31st.webmDuring the last weeks I've been experimenting with this branch and implemented everything in this video. Don't mind the lack of walking animation I'm working on a custom animationplayer and it's a separate project. Jumping, rolling and the ladder are all First, I'd like to share that since it's more of an action-platformer, I didn't do much in terms of combos so I've learned to use less the builder and add my transitions manually. I think I was a little bit too much focused on the builder previously, which is great for combos and combat-related overview, but I was looking to encapsulate some behavior. This is where the newly named # In JumpState.gd extends FrayCompoundState
func _ready_impl():
set_condition(...) # Local condition are supported in this setup
get_root().transition_button("ground","jump",setup) # Dependency on root having ground but I know it has it.
add_state("anticipation") # Deal with internal states
... The only way I would improve this is to find a way to link object. My state modify the position of the Second, seconds being the standard time reference is great. I have some plans that will need
I've implemented a relatively simple state designed to play an animation and await either the end of the anim, or a signal. In fact all "actions" in the video is using this state. I know you will create a discussion about this but here is my pseudo-code on this state so you can think about it: # The signal can_advance is set in the animation
var ended :bool = false
func _is_done_processing_impl() -> bool:
return ended
func anim_finish(_anim_name = ""):
ended = true
get_root().advance()
func _enter_impl(args:Dictionary):
ended = false
if not statemachine.can_advance.is_connected(on_anim_finish):
statemachine.can_advance.connect(anim_finish,CONNECT_ONE_SHOT)
if not animationplayer.animation_finished.is_connected(on_anim_finish):
animationplayer.animation_finished.connect(anim_finish,CONNECT_ONE_SHOT)
animationplayer.play(args.anim_name)
func _exit_impl() -> void:
ended = false
# Make sure we disconnect signals.
if statemachine.can_advance.is_connected(anim_finish):
statemachine.can_advance.disconnect(anim_finish)
if animationplayer.animation_finished.is_connected(anim_finish):
animationplayer.animation_finished.disconnect(anim_finish)Conclusion : I continue to think this is a great library, and this is a great redesign. Can't wait to pull those last changes. |
I've actually considered doing something like this for a while now. It shouldn't be a difficult inclusion, but I won't be addressing it in this PR. I'll start a discussion to share my thoughts on potential approaches before I proceed.
Oh I see. It was very easy to add support for local conditions so when I saw you mention your experience I went ahead and implemented it. Global conditions are really just the root's local conditions. I personally can't think of a use-case in this moment but I think i'll keep it as I do like the idea. I see the behavior as similar to a function scope var shadowing a class scope var, but the class var can still be accessed with 'self'. Also the distinction between global and local isn't that significant. I'll be merging this PR tonight or tomorrow as i'm satisfied with the current state of the redesign. The remaining pain points / Quality of Life improvements will be dealt with in subsequent PRs. Thanks for your ongoing interest and support in this project! Your feedback and code snippets have been very helpful while working on this. It's just also interesting to see how other people make use of the library. |
I decided to share my thoughts in an issue rather than using discussions. Let me know what you think when you have time. |
This is being discussed here: #73 |
The goal with these changes was to provide better support for nested state machines in addition to eliminating the need for the
CombatStateMachine. I felt it needlessly introduced new concepts in the form of 'situations' which to some extent restricted how users could structure their state machines. Now there is only 1 state machine class which provides processing to the root and all its sub-states, and an optional advancer that can attempt to advance any state machine by feeding it buffered inputs.Added:
BufferedInputAdvancer: Contains input buffering functionality previously belonging to theCombatStateMachine. This is a node which attempts to manually advance a target state machine using inputs buffered in it. I'd like any feedback on this class concept or its name.CompositeState's, formerlyRootState, buildertransition()method a 'transition' parameter. This allows users to pass custom transition objects into the system.Changed:
goto(),has_state(), andget_state()now accept state paths allowing you to more easily interact with sub-states.goto(to_state_path)the new behavior is to exit up each state leading to the most common ancestor of the current state and target state, and then follow along the path to the target starting from the most common ancestor state entering each state along the way.CompositeStateis to set their current state to the start state. Additionally, sub-states are now entered if the current state is aCompositeState.Renamed:
RootState->CompositeState: This is to reflect the fact that this class contains a collection of sub-states including other composite states. I feltRootStatedidn't make that immediately obvious and may have implied it could only serve as the root of a state machine node.Removed:
CombatStateMachine: The combat state machine provided 2 features. A way of buffering inputs fed to it self, and a way of organizing root states into 'situations'. TheBufferedInputAdvancernow provides the first functionality, and the improvements to using nested states now allows for sub states to take the place of situations. As pointed out by user it really was just a fancy name for a state machine that supported buffered inputs, I think this new approach is more flexible.What's Left:
transitionedtravelmethod needs testing and potentially a redesign.Fixes: #64
Fixes: #66