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]

Using ENUM is the right direction to go. However, for consistency with something similar to this that already exists in Godot, we recommend the signal name NOTIFICATION_XXX instead of NOTIFY_XXX.

[a-johnston]

Hm, would NOTIFICATION_XYZ cause confusion with the existing notification stuff? I preemptively changed the enum name as well but that causes the c# build to complain about hiding Notification so that at least can't be shared. Maybe EVENT_STARTED etc with enum name Event?

[TokageItLab]

How about

enum AnimationNodeNotification {
  ANIMATION_NODE_NOTIFICATION_XXX,
  ANIMATION_NODE_NOTIFICATION_YYY,
}

?

[TokageItLab]

I believe this approach is redundant. At the very least, storing the same String in two places is not the right direction, and we must do indirect mapping, such as storing only the index, and the entity as a String must be only one.

[a-johnston]

isn't StringName only storing the unified data pointer within either map, so there is just one backing string? this is just to avoid the linear cost of only having nodes to determine a name in the current context. ideally there would just be a bimap template available, but alternatively the name could also be copied to NodeState at the same time that the base path and parent are set.

[TokageItLab]

I am saying that the concatnated path to the AnimationNode already exists in the property_reference_map, so you don't need to create a new map. I assume you can just retrieve it from property_reference_map.

[a-johnston]

Do you think it would be better to parse out the final part of the path or emit the full concatenated path?

[TokageItLab]

The signal must pass the full path to the AnimationNode. As I have said many times, since the AnimationNode is a Resource, the same reference AnimationNodes can be placed in the same hierarchy. To distinguish between them, the user must know the full path.

For example, if you have two BlendTrees with the same reference that have “OneShotA” as a child, you cannot determine which BlendTree's "OneShotA" if the only notification is "OneShotA".

BlendTree.tres

[Anim1]
　|
[OneShotA]-[Output]
　|
[Anim2]

---

RootAnimationNode

[BlendTree1(BlendTree.tres)]
　|
[Blend2]-[Output]
　|
[BlendTree2(BlendTree.tres)]

In the case above, we should distinguish between BlendTree1/OneShotA and BlendTree2/OneShotA.

[a-johnston]

Ah yes that makes sense. I was only considering blend trees as roots but of course the names alone would break if they were nested.

Edit: Although outside of this use case, it still might be worthwhile to split the current map into StringName -> Ref<AnimationNode> and AnimationNode * -> AnimationNodeBlendTree::Node so that blend_input can lookup connections in constant time. I won't do that in this pr though.

Edit 2: Also maybe worth mentioning that although it's not a necessary part of the example, get_node_name unfortunately will not be reliable in either case with the tree you've described as both names map to a valid value and it'll return whichever it finds first.