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

Add state transition signals to AnimationNodeStateMachinePlayback #55423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheOrioli
Copy link
Contributor

Adds signals to the AnimationNodeStateMachinePlayback which allow user-level scripts to react to transitions between the various states.

What this means in practice is that it allows users to know when an animation/blend/sub-machine has stopped or started playing inside an AnimationNodeStateMachine. This can be used to solve problems presented in godotengine/godot-proposals#1462, #28311 and #41771

Example project using this PR

This fix can also be easily backported to 3.x , there seem to be no conflicts.

The current ways to do the same thing on a user level are:

  • create call method tracks in animations to notify scripts of completion. This is not a scalable solution for games with lots of animations as it's prone to user error.
  • apply a special script to every AnimationTree node which manually checks for state changes every idle or physics frame, depending on settings. This is also very prone to user error.

@TheOrioli TheOrioli requested review from a team as code owners November 28, 2021 21:55
@Calinou Calinou added this to the 4.0 milestone Nov 28, 2021
@TheOrioli TheOrioli force-pushed the state_machine_playback_signals branch 2 times, most recently from 08d79d9 to 72ec76c Compare November 29, 2021 09:59
@lufog
Copy link
Contributor

lufog commented Jan 29, 2022

@akien-mga, Is there any chance that this pr will be merged? As I can see, a pretty big amount of people want this functionality.
Some requests/mentions: #1462 #8521 #22137 #26408 #28311, previous attempts to add similar functionality: #24328 #30508 #41771.

@akien-mga
Copy link
Member

It needs to be reviewed and approved by contributors familiar with the animation workflows and their implementation.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looks good! Please address the comments and should be ready to merge.

scene/animation/animation_node_state_machine.cpp Outdated Show resolved Hide resolved
scene/animation/animation_node_state_machine.cpp Outdated Show resolved Hide resolved
scene/scene_string_names.h Show resolved Hide resolved
@Macksaur
Copy link
Contributor

Macksaur commented Feb 5, 2022

This doesn't seem to initially emit any signals when using .travel() or .start(). Only when the state machine transitions from the first state into the second (i.e. only on the second node).

Should it not emit signals in this case also? Something-something like:

+       // check if we have specifically jumped/teleported into a new state
+       if (current != previous) {
+               if (previous) {
+                       emit_signal(StaticCString::create("state_exit"), previous);
+                       emit_signal(StaticCString::create("state_changed"), previous, current);
+               }
+               emit_signal(StaticCString::create("state_enter"), current);
+       }

above https://github.com/godotengine/godot/blob/72ec76c4c4381d0b09155c8534cde2b91e96e504/scene/animation/animation_node_state_machine.cpp#L381

@TheOrioli TheOrioli force-pushed the state_machine_playback_signals branch from 72ec76c to 99bd057 Compare February 7, 2022 13:19
@TheOrioli TheOrioli force-pushed the state_machine_playback_signals branch from 99bd057 to da2b578 Compare February 7, 2022 15:29
@TheOrioli
Copy link
Contributor Author

@reduz Thank you for the review, I have applied the requested changes to SNAME.

I've also added additional emit_signal calls that should cover the use case mentioned by @Macksaur

The new updated example project that allows teleporting/traveling to a specific state animation_state_machine_test.zip

@TheOrioli TheOrioli requested review from reduz and removed request for a team February 16, 2022 11:23
@@ -331,6 +338,11 @@ double AnimationNodeStateMachinePlayback::process(AnimationNodeStateMachine *p_s
// teleport to start
if (p_state_machine->states.has(start_request)) {
path.clear();
if (current != StringName()){
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we should have
state_enter, state_changed, state_exit be emitted exclusively and without overlap from one to the other, so the order would be

  • state_enter
  • state_changed
  • state_exit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"exclusively and without overlap" meaning:

  • state_enter is only emitted when entering into the machine for the first time, on the first state entered
  • state_changed is emitted any time a state transitions into another valid state, automatically or using Start or Travel
  • state_exit is only emitted when a machine stops

or something else?

@ballerfuturistic
Copy link

ballerfuturistic commented Apr 11, 2022

Any updates on this? I think still waiting on clarification from @reduz

@AbrahamBrookes
Copy link

poke. I too would like this feature

@TheOrioli
Copy link
Contributor Author

tbh, this is such an ancient PR, it would probably be best to rebuild it from the ground up since a bunch of new animation changes have been made in the mean time.

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.

9 participants