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

Improvements to AnimationNodeStateMachine #24402

Merged

Conversation

guilhermefelipecgs
Copy link
Contributor

@guilhermefelipecgs guilhermefelipecgs commented Dec 16, 2018

Closes #19773.

Notes about the implementation: #24402 (comment)

Open the menu to add new animation nodes by dragging the transitions to empty areas and automatically connecting them:
peek 10-02-2019 16-34
Adds box selection to the state machine:
peek 10-02-2019 16-35
Add feature to group/ungroup selected nodes in a "sub" state machine:
peek 10-02-2019 16-37
Add popup to connect nodes in sub state machine:
peek 10-02-2019 16-40
Add new "type" of transition line when multiple transitions are grouped:
peek 10-02-2019 16-44
Add dialog to select which transitions can be deleted when they are grouped:
peek 10-02-2019 16-45

@akien-mga akien-mga added this to the 3.2 milestone Dec 16, 2018
@guilhermefelipecgs guilhermefelipecgs changed the title Improvement of state machine usability [WIP] Improvement of state machine usability Dec 16, 2018
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch 6 times, most recently from 7710ed0 to 6629a6e Compare December 18, 2018 00:36
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch from 6629a6e to 6712bcd Compare January 13, 2019 22:02
@guilhermefelipecgs guilhermefelipecgs changed the title [WIP] Improvement of state machine usability [WIP] Improvement of state machine Jan 13, 2019
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch 2 times, most recently from c77d3c5 to 7ca8ecd Compare January 15, 2019 20:48
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch 4 times, most recently from 1bd743d to 00b3990 Compare January 22, 2019 19:25
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch 6 times, most recently from befd11a to 0d8b779 Compare February 12, 2019 17:42
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch 4 times, most recently from dc0378a to 69d0060 Compare February 28, 2019 20:02
@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch from 69d0060 to 2f7215a Compare March 4, 2019 13:03
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me now

@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch from 5d79992 to efe5055 Compare March 14, 2022 16:30
@SaracenOne
Copy link
Member

@guilhermefelipecgs I know this keeps getting punted back, but is there any chance of rebasing this again and @reduz getting it merged now that the requested changes seems to be have been addressed? I was having a conversation earlier with @lyuma about some animation work he's looking at at the moment in terms of sub-state machine limitations he ran into, and remembered that this PR would likely address the issues he was having.

@lyuma
Copy link
Contributor

lyuma commented Apr 28, 2022

@SaracenOne I pushed a rebased version here. Feel free to use this commit as your own.
https://github.com/lyuma/godot/tree/guilhermefelipecgs_state_machine_improvement

so I've been playing with it. I was able to replicate one complex state machine example sent to me by a Unity animation guru so it seems pretty powerful. Here are the issues I ran into:

  1. (Not a new issue) transition order is not shown in the UI when going to two separate states so you either have to remember or hand edit the resource. In the .tres file, last transition takes precedence which I can work with but it's opposite of Unity.
  2. Still no way to have duplicate transitions from one state to another. This is useful if you have like override variable to go to state 1, then try to go to state 2,3,4 etc. then By default go to state 1 with autoadvance. The easiest way to do this is duplicate state 1.
  3. The example I did involved redirecting through multiple Start->End->Start transitions of different state machines same-frame. In Godot, I could see it flicker through these different transitions so it leads me to ask...what happens to cross fading when traveling from one state machine to another?
    ( In Unity there's a technical explanation why cross fading still works here: the transitions starting at a state machine, and transitions starting at Start are greyed out to indicate they can only add conditions. These "Transition", not "StateTransition" lines specifically do not affect cross fading, and they do not take an extra frame to evaluate. )
  4. Start and End settings got lost when migrating old animation trees. There needs to be a migration path, for example read the start flag and auto create a default transition from Start to x... and same for End. (There's no longer an End in the top level state machine)... also rename any existing states named "Start" or "End". Who knows what users did before.
  5. Not your fault but switching between the arrow tool and the transition tool is awkward as ever. I keep getting confused because I'm on the wrong tool and it's tedious to switch. I want like right cliick drag and "Add Transition" shows up in the context menu or have a dot you can drag like other node graphs in Godot
  6. Transition conditions are a bit tricky to use since there's no not, but (1) allowed me to workaround it by adding an autoadvance and then the true case as two separate transitions. Reduz has an open PR for this, so no need to fix it.

That's basically it. Overall, the PR works and seems to solve most issues I had last time I tried sub State Machines. I know I listed a lot of things, but the only one which is a blocker for me to use it is the frame delay / no cross fading... that's important to have for a character controller, but otherwise, this PR did most things right.

(Also note that if you take my cross fading suggestion (3), it may be possible to soft-lock with nested state machine transitions. The version of Unity I tried is dreadfully easy to soft lock if you add a loop between start and end of multiple state machines. If you're worried about this, set a limit of how many transitions you will follow at once.)

@guilhermefelipecgs guilhermefelipecgs force-pushed the state_machine_improvement branch from decea27 to c7f2c1e Compare April 28, 2022 19:49
@guilhermefelipecgs
Copy link
Contributor Author

Hi @lyuma, thanks for testing my changes, about the flickering, this is news to me, before the rebase this didn't happen and now I can see this happening in my projects too, I'll need to investigate this.

This is the flickering you mentioned right?:

Peek.2022-04-28.16-59.mp4

In slow motion:

Peek.2022-04-28.17-02.mp4

I don't know why this started now, maybe a new bug has been introduced somewhere.

About the crossfade, it should work normally, I did some tests, see #24402 (comment)

@guilhermefelipecgs
Copy link
Contributor Author

@SaracenOne Don't merge yet because I need to investigate this "flicker". This bug causes a lot of wrong behavior, it's not just a visual issue.

@lyuma
Copy link
Contributor

lyuma commented Apr 29, 2022

Ok so I did more testing, and I think my assessment was wrong, and it seems to work fine when using crossfade, at least after changing my example a bit.

Here is the state machine I ended up creating:
image
You can download the scene here with a cube and simple animations embedded: MultiStateMachineTest.tscn
EDIT: I left it set up to use the Loopback method which I explain below. To get the good working example, make sure to uncheck "Disabled" on the grey transition between Logic and Logic2.

It's a little absurd, but I structured it like this to match the unity example I was basing this on, and it seemed like a good challenge and a way to learn a bit more how things work.

So the part that actually threw me off is that transitions with xfade_time set seem to become "At End". it's not completely consistent, so it's hard for me to say what's correct.

The Loopback thing was my attempt to avoid needing two copies of Logic (allow transitioning back to my own state machine), so I connected Start->End. Imagine you make a state machine for Jump, and the player holds spacebar so you want to xfade from one jump directly into another jump. Anyway... when doing this, it does seem to have some flickering, but I'm not sure if this case is supposed to work or not.

Overall, I think I'm a little more comfortable with this system than when I started using it. I'm generally confused how xfade_time and "Immediate" are supposed to work together. I'm a little worried that I needed to use xfade both inside the state machine and outside of it, which makes me wonder if it's overlapping two crossfades, since that would cause interpolation between state machines to be non-linear.

What are your thoughts?

@guilhermefelipecgs
Copy link
Contributor Author

@lyuma I can't open your scene, could you send me a minimal reproduction project in a zip file for me to test?

Screenshot from 2022-04-29 10-29-17

- Open the menu to add new animation nodes by dragging the transitions to
empty areas and automatically connecting them.
- Adds box selection to the state machine.
- Add feature to group/ungroup selected nodes in a "sub" state machine.
- Add start/end node by default. In addition, add new color to these
nodes to differentiate then.
- Add tooltip for transitions to show the connection "from -> to".
- Add new "type" of transition line when multiple transitions are
grouped.
- Add popup to connect nodes in sub state machine.
- Add dialog to select which nodes can be deleted when they are grouped.
- Add classes:
	AnimationNodeStartState
	AnimationNodeEndState
	EditorAnimationMultiTransitionEdit
- Implements disabled transition

API Changes:
- Now it's posible to add transitions between state machines,
`AnimationNodeStateMachine::add_transition` will works with relative path,
this means you can use it like this `add_transition("Idle", "Walk", tr)`
or `add_transition("Idle", "StateMachine/Shoot)`.
@guilhermefelipecgs
Copy link
Contributor Author

@SaracenOne the bug I mentioned was a regression, I opened an issue here #60710 so if the PR is good I think we can continue with the merge,

@akien-mga akien-mga merged commit 77c9138 into godotengine:master May 3, 2022
@akien-mga
Copy link
Member

Thanks for keeping up with a lengthy review process :)

@ncollie42
Copy link

This commit makes StateMachine now only usable inside other StateMachine. This breaks compatibility between BlendTrees and StateMachine. A StateMachine created inside a BlendTrees will not be able to exit due to missing end nodes. Is this intentional?

@bruvzg bruvzg mentioned this pull request Aug 8, 2024
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.

[Proposal StateMachine] Add condition to transitions