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

Make auto-advance flag a requirement for conditional/expression evaluation #65312

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

SaracenOne
Copy link
Member

This is a PR to address a potential major design discontinuity with Godot's StateMachine system. This has been bothering me for a while, and I believe I understand why certain aspects of AnimationStateMachine's feel so counter-inuitive. I feel it is a consequence of two distinct ways of traversing them: via travel or via auto-advance/conditions. With travel, conditions are actually ignored, and it is a regular A* traversal to simply get to a desired node, whereas auto-advance can use conditional branch to have a state machine essentially drive itself.

The problem I feel with the current implementation is that you have mainly three types of transitions:

  • No auto-advance and no condition: does nothing on its own
  • No auto-advance but has conditions: advances but if conditions are met
  • Auto-advance: ignores all conditions and advances regardless

My problem is that auto-advance more or less overwrites the behaviour conditions, and this feels very strange from a usability standpoint. It essentially makes the it an either/or scenario, and the relationship between these two elements is not obvious. My proposal therefore is to make it so that for any automatic traversal, auto-advance must always be on, and conditional evaluation is only done if auto-advance is set. While this PR only changes the actual behaviour and documentation to provide better explanation, I think more could be done on the editor side to make the relationship between the two more obvious. I could also supplement this with features such as allowing you to set the default state of auto-advance on new transitions in the same way you can for switch mode.

I feel strongly on the whole that short of removing auto-advance entirely (which doesn't feel fair given not having auto-advance active can make it useful for the 'travel' method), this is the best way to address a pretty glaring design issue, and it feels relevant to address this given the potential that a lot of game logic can be implemented purely via state machines.

@SaracenOne SaracenOne added this to the 4.0 milestone Sep 4, 2022
@SaracenOne SaracenOne requested review from a team as code owners September 4, 2022 05:58
@SaracenOne SaracenOne force-pushed the auto_advance_behaviour branch from 7901c1c to 249df04 Compare September 4, 2022 06:46
@SaracenOne
Copy link
Member Author

SaracenOne commented Sep 4, 2022

godot windows tools x86_64_szWO18vJIt
Added a new auto-advance toggle button next to the transition switch mode for creating new transitions. Feels important to do for the sake of workflow speed, though I do wonder if there's ways we could make the functionality more obvious (also hoping it would not be confused with old autoplay toggle which was no longer needed with the changes supporting sub-state machines).

One question I would ask from a usability standpoint though is if it's nessecary to show these options all time. They're highly context-sensitive, so maybe it would make their use clearer if this option was only displayed during 'new transitions mode'.

@SaracenOne SaracenOne force-pushed the auto_advance_behaviour branch from 249df04 to 2ccb412 Compare September 4, 2022 07:16
@SaracenOne
Copy link
Member Author

SaracenOne commented Sep 4, 2022

I went and added the context-sensitive behaviour anyway. I couldn't really see a reason not to do it given the way the state machine editor currently work, and feel like it will make it easier for people to understand it.
godot windows tools x86_64_0zuIXhKQ07

@fire
Copy link
Member

fire commented Sep 4, 2022

@TokageItLab what do you think?

@TokageItLab
Copy link
Member

Does this PR solve this issue?
godotengine/godot-proposals#2200

@SaracenOne
Copy link
Member Author

@TokageItLab It doesn't, but I've started experimenting with transition interrupts as part of major evaluation of state machine usability. I'm hoping I can add that as a new feature soon, along with more granular support of the exact timing of when transitions should occur.

@reduz
Copy link
Member

reduz commented Oct 11, 2022

I think I recall we discussed using an enum for this, what happened in the end?

@mhilbrunner
Copy link
Member

mhilbrunner commented Dec 13, 2022

Animation meeting: change to use an enum, maybe add disable (the latter could/should probably be a followup PR). @SaracenOne wants to get this done in the next few days.

@SaracenOne SaracenOne force-pushed the auto_advance_behaviour branch from 2ccb412 to c3d5e18 Compare December 16, 2022 07:03
@SaracenOne
Copy link
Member Author

This has been updated with the change to a single enum. I sincerely apologize for not following up on this sooner.

… flag.

Expressions and conditions now require auto mode to be set to auto.
Adds a toggle button to the state machine editor for whether new transitions
advance settings should default to auto mode or not.
@akien-mga akien-mga merged commit 9b4888b into godotengine:master Dec 23, 2022
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the auto_advance_behaviour branch January 14, 2023 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants