Add node_notify signal for animation trees

[a-johnston]

Closes godotengine/godot-proposals#10253.

Related to #102398

This adds node_notify(StringName, NotifyReason) to AnimationTree as a generic signal used for node state changes during processing/playback. Currently NotifyReason can be either NOTIFY_STARTED or NOTIFY_FINISHED and is implemented for one shot and state machine nodes. For example in the following blend tree:

The new signal fires with the following values:

Node notify: MyStateMachine Started
Node notify: OneShot Started
Node notify: MyStateMachine Finished
Node notify: OneShot Finished

[TokageItLab]

See also:

• Add a signal to notify when the animation node changed inside state machine playback #88179
• Add animation state machine signals to animation tree #99293

As I have pointed out in the past with those, it should be implemented so that the ancestor StateMachine returns the nested paths when in Grouped mode (https://godotengine.org/article/migrating-animations-from-godot-4-0-to-4-3/#grouped). In other words, it must be implemented in such a way that the signal is propagated to the ancestor State as special case for the Grouped mode.

[a-johnston]

Thanks for the links, I hadn't seen those PRs. Do you think it would be worth adding fading_from_state_changed to this pr? From the comments, a few things jumped out at me

• Regarding performance concerns, is it more in the sense that adding new signals adds XYZ overhead even if there are no listeners, or have there been related performance issues in the past related to signal use which should be avoided? I can set up a synthetic case that triggers the new emit calls a ton if we want that measured.
• I hadn't considered the option of saving a state machine node for reuse; I'll change the code to not use Resource.name and instead opt for a reverse node -> name map on the tree. Also I don't know if or when this would be useful, but there is an existing odd edge case where the same node resource could be added twice to a tree which would cause get_node_name to return the wrong name / cause the wrong node to be updated on a rename callback within the editor. This could be detected; it might be worth adding a warning message or something I'm not sure.

As I have pointed out in the past with those, it should be implemented so that the ancestor StateMachine returns the nested paths when in Grouped mode [..]. In other words, it must be implemented in such a way that the signal is propagated to the ancestor State as special case for the Grouped mode.

I'll work on adding this. Just to be sure I understand correctly, the changes I'll make would change the example root sm node_changed values, assuming InternalStateMachine is set to grouped, to be the following:

Start
InternalStateMachine/Anim_Knight_Attack01
InternalStateMachine/Anim_Knight_Attack02
Anim_Knight_Hit01
End

Also regarding a comment you left on another PR the user cannot use the child StateMachine's StateMachinePlayback directly, it is possible right now to grab the playback for the grouped state machine and listen to the signal from within user code, even if control is not allowed. I'll filter events in the root sm to skip the grouped Start/End node but it'll still be possible for users to directly listen to the grouped sm to detect those states.

[TokageItLab]

Thanks for the links, I hadn't seen those PRs. Do you think it would be worth adding fading_from_state_changed to this pr?

I think so.

---

As for the StateMachinePlayback object, it should be a unique resource for each AnimationTree, so there should be no problem to fire a signal there.

However, if you want to fire a signal in the AnimationNode, signals should not be registered with AnimationNode as explained in the review below.

The simplest implementation would be to fire signal from the AnimationTree with its own BlendTree path. In this case, you can refer to AnimationTree::set_parameter() to get the path. At this point, it may be better to separate the method for obtaining its own path from it and make it reusable.

But then, my concern is that each time we have more signals that we want to fire in the AnimationNode, we need to register all of them in the AnimationTree.

So, instead of adding a signal called oneshot_finished() to the AnimationTree, we should add a generic abstract signal such as node_notified(animation_node_path, signal_name, signal_variants).

However, that PR should be a separate PR from the StateMachinePlayback signal addition.

[a-johnston]

I've updated the pr to have node_started/node_finished for state machine nodes rather than node_changed so that end of crossfades can be detected via signal. I've also added support for grouped state machines to pass up their signals to the containing playback, including when nested, although I still need to test a few more cases like when state traversal is interrupted by certain calls. In a minute I'll update the op to reflect the current state of the code.

Edit: One thing I'd like feedback on with the current code is if the signal emission should be put behind a call_deferred in all cases? I noticed it was like that for animation_started/animation_finished but wasn't sure if it should also be the case for the new signals. Easy change, just unsure what the preferred behavior would be.

[a-johnston]

However, that PR should be a separate PR from the StateMachinePlayback signal addition.

This pr is currently sitting at +124 -11 which imo is a good size but if you'd prefer I can break this up into two PRs (maybe a third to rename fadeing_from_nti to fading_from_nti since that's totally unrelated but was bugging me lol)

[TokageItLab]

I suggest that it should be a generic signal as explained above.

So, instead of adding a signal called oneshot_finished() to the AnimationTree, we should add a generic abstract signal such as node_notified(animation_node_path, signal_name, signal_variants).

For example, what happens with NodeTransition? Don't we want a signal when BlendSpace is in DC mode and the values switch discretely? What should we use when firing a signal from a custom AnimationNode? I suppose it is not good way to add a new signal to the AnimationTree for them every time.

Also I think it is better to distinguish between signals in StateMachinePlayback and AnimationNode (so signals add to both StateMachinePlayback and AnimationTree) than to mix them up. In case of mix-ups all into AnimationTree, the StateMachine should be named to avoid confusion.

For example, if you follow the description, it must be MyStateMachine/Outer/Inner/Attack1 not Outer/Inner/Attack1 since we can't distinguish between two StateMachines that have the same configuration if they are placed same level of the blend tree.

[TokageItLab]

However, the concern remains that generic signals use strings as signal identifiers. Another idea is to pass some kind of Callable object from the AnimationNode to the AnimationTree.

[a-johnston]

What sort of callable do you imagine, and would it make more sense to use an enum rather than bare strings? Also are there other examples of notification style multipurpose callbacks?

I'll think about it more but from my perspective a lot of these node notify events would be directly caused by user code, such as requesting a transition or changing the blendspace mode, and could be handled by an animation controller script. In comparison, I think node_started and node_finished would be harder to implement without extra coupling or polling of the tree's state. That said, it probably won't get to the point where the generic signal gets too expensive from unnecessary connections, so the question of which things should be node notified emits or not doesn't really matter. It's also the case that this doesn't need to be decided without some future use case in mind - the state machine signal addresses the proposal use case (and my use case) on its own.

Either way I'll give the generic signal a shot and also break off the state machine changes.

[a-johnston]

Also I think it is better to distinguish between signals in StateMachinePlayback and AnimationNode (so signals add to both StateMachinePlayback and AnimationTree) than to mix them up. In case of mix-ups all into AnimationTree, the StateMachine should be named to avoid confusion.

Sorry if the example is unclear but the indented lines are from signals on the state machine playback and the un-indented lines are from signals on the blend tree. They'll be more distinct in a bit since the tree will just have node_notify. Or am I misreading this and you would prefer the playback's internal node events be bubbled up to a root tree?

[a-johnston]

Ok, this has been updated to use a generic signal node_notify and the other class was split into its own pr #102398