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 a signal to notify when the animation node changed inside state machine playback #88179

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

CrayolaEater
Copy link
Contributor

Added ability to track current animation node using the new animation_playback_node_changed signal.
image
image

image

@CrayolaEater CrayolaEater requested review from a team as code owners February 10, 2024 18:48
@Calinou Calinou added this to the 4.x milestone Feb 10, 2024
@AThousandShips AThousandShips changed the title Added signal to notify when the animation node changed inside state m… Add a signal to notify when the animation node changed inside state machine playback Feb 10, 2024
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

It is not known how much this type of signal is needed for StateMachine. We may want signals for fades, etc., but for now we should summarise the demand and use cases in the proposal.

At least, adding signals to the AnimationTree probably needs to be thought about a bit more carefully, as it may not return the proper path in cases such as nested AnimationTree and GroupedStateMachine.

scene/animation/animation_node_state_machine.cpp Outdated Show resolved Hide resolved
@CrayolaEater
Copy link
Contributor Author

It is not known how much this type of signal is needed for StateMachine. We may want signals for fades, etc., but for now we should summarise the demand and use cases in the proposal.

At least, adding signals to the AnimationTree probably needs to be thought about a bit more carefully, as it may not return the proper path in cases such as nested AnimationTree and GroupedStateMachine.

There are a lot of use cases for this. For example I have multiple animations that I import from blender and I need to trigger different functions according to a different animation playing. I don't want to use call method tracks because when I do modifications and re-import those animations I have to add the tracks again each time.

@TokageItLab
Copy link
Member

At least this implementation should not be enough in a real use case. It should not be able to handle fade-ins and fade-outs, and it should not be able to detect changes in the GroupedStateMachine path.

@CrayolaEater
Copy link
Contributor Author

At least this implementation should not be enough in a real use case. It should not be able to handle fade-ins and fade-outs, and it should not be able to detect changes in the GroupedStateMachine path.

What do you mean "it should not be able to handle fade-in and fade-out" ? Its just signaling the node change.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 17, 2024

I mean that this kind of signal should be notified on fade-ins and fade-outs as well. This signal may be sufficient in the limited and specific case of a simple StateMachine, but it is not sufficient for a complex StateMachine (Although I understand the need for them).

@PPillau
Copy link

PPillau commented Apr 18, 2024

Pls can we merge this into master at some point? It is baffling to me how something as basic and fundamental as notifying/signaling upon a state change in a state machine is not already in the engine.
There are a lot of situations where I want to react to an animation change in code.

The AnimationNodeStateMachine currently exposes no signals at all, effectively leaving the user in the dark about anything that happens inside the state machine.

I think something like a animation_started(animation_name) signal would be even more useful because then you could not only detect a state transition (by comparing previous and current animation) but you can also get notified anytime an animation loops, but the signal in this PR is already a step in the right direction :)

@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch from 3b23a72 to 036496e Compare April 24, 2024 12:03
@CrayolaEater
Copy link
Contributor Author

CrayolaEater commented Apr 24, 2024

I mean that this kind of signal should be notified on fade-ins and fade-outs as well. This signal may be sufficient in the limited and specific case of a simple StateMachine, but it is not sufficient for a complex StateMachine (Although I understand the need for them).

If you mean transitions xfade that also works.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

What I mean is that changes in fading_from must be signaled as well. Without this, the user will not be able to detect where the fade ended.

Also, if the nested child StateMachine's StateMachineType is STATE_MACHINE_TYPE_GROUPED, the parent StateMachine must signal the internal changes in the child StateMachine on its behalf. Because the user cannot use the child StateMachine's StateMachinePlayback directly.

@CrayolaEater
Copy link
Contributor Author

Also, if the nested child StateMachine's StateMachineType is STATE_MACHINE_TYPE_GROUPED, the parent StateMachine must signal the internal changes in the child StateMachine on its behalf. Because the user cannot use the child StateMachine's StateMachinePlayback directly.

So the signal should be per StateMachine instead of per AnimationTree?

@TokageItLab
Copy link
Member

TokageItLab commented Apr 24, 2024

@CrayolaEater The change in current state of the StateMachine is tied to AnimationStateMachinePlayback, not to AnimationTree, so the signal should be fired by neither AnimationTree nor AnimationNodeStateMachine1, but by AnimationStateMachinePlayback.

And the signals should have appropriate names such as AnimationStateMachinePlayback.current_changed or AnimationStateMachinePlayback.fading_from_changed. e.g.

emit_signal(SNAME("current_changed"), current);

In this case, the child StateMachine which is is_grouped = true, does not fire the signal on its own, but instead must call a method so that the parent StateMachinePlayback fires the signal with path, e.g.

parent_playback->emit_signal(SNAME("current_changed"), SNAME("action/attack"));

Footnotes

  1. In particular, since the StateMachine is a resource, it can be reused, causing confusion when multiple AnimationTrees reference the same StateMachine

@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch 2 times, most recently from 3240a2d to 70cc2c5 Compare May 15, 2024 14:05
@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch from 70cc2c5 to aca279d Compare May 15, 2024 14:14
@CrayolaEater CrayolaEater requested a review from TokageItLab May 15, 2024 14:46
@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch from 9160a14 to f31c952 Compare May 16, 2024 11:27
@CrayolaEater CrayolaEater requested a review from TokageItLab May 16, 2024 11:40
scene/animation/animation_node_state_machine.cpp Outdated Show resolved Hide resolved
doc/classes/AnimationNodeStateMachinePlayback.xml Outdated Show resolved Hide resolved
doc/classes/AnimationNodeStateMachinePlayback.xml Outdated Show resolved Hide resolved
@CrayolaEater
Copy link
Contributor Author

CrayolaEater commented May 16, 2024

@TokageItLab I have difficulties understanding the concept of Grouped State Machine. What is it achieving? How can I properly set it up ? The docs don't mention anything.

@TokageItLab
Copy link
Member

TokageItLab commented May 16, 2024

A grouped type state machine differs from other types of state machines in that only indirect access is granted to the user through playback of the ancestor root or nested type state machine.

The concept is described in #75759, but better documentation is not yet available. I am writing an article about it now, but it is taking a while to put together due to the insane large number of changes in Animation from 4.0 to 4.3. I make effort to upload it by 4.3 stable.

@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch 2 times, most recently from 30bcfc0 to 0b94ebe Compare May 22, 2024 12:07
@CrayolaEater CrayolaEater force-pushed the animation-node-changed-signal branch from 0b94ebe to 8e41ba7 Compare May 22, 2024 12:08
@Musashi98
Copy link

Hello there and sorry about the next question: do you have an estimate of how much time untill this feature is added to the engine?

Thank you about your hard work Godot community!

@CrayolaEater
Copy link
Contributor Author

CrayolaEater commented Oct 14, 2024

Hello there and sorry about the next question: do you have an estimate of how much time untill this feature is added to the engine?

Thank you about your hard work Godot community!

Hello! Its not up to me. It must be reviewed by the Godot team to make sure its working properly.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

fading_from_state_changed must generate a path that takes into account the child_state_machine in the grouped case as well as the current_state_changed case. Some information on the behavior of grouped state machines is summarized here.

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.

5 participants