godotengine
Add animation_node_notification signal for animation trees
Closes https://github.com/godotengine/godot-proposals/issues/10253.
Related to https://github.com/godotengine/godot/pull/102398
This adds animation_node_notification(path: StringName, what: int) to AnimationTree as a generic signal used for node state changes during processing/playback. Notification values are defined for state machine, transition, and one shot nodes. Rather than include several screenshots and an output, I've attached a test project which automatically runs through a few operations and then quits, printing timing information for each step.
See also:
- https://github.com/godotengine/godot/pull/88179
- https://github.com/godotengine/godot/pull/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.
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_nameto 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.
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.
However, that PR should be a separate PR from the
StateMachinePlaybacksignal 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)
Forgot to push up some changes the other day. The most recent changes fix the multi-emit for nested state machines and the error which could happen when starting a state machine. I also noticed playing on state machine is only reset to false when the current state does not exist and when the state machine is being reset. Should it be set to false when the state machine finishes normal playback? I could make a helper which also emits that finished notification.
Fixed an issue where fade signals would incorrectly fire when playback terminates, and rebased + retested the other cases.
In my opinion, I cannot fully agree unless you add "animation_node_class_name" and "sub_indices", as the above enum indexing issue is still there.
Any opinions from the animation team on this? @SaracenOne @lyuma @fire
That's fair. Were that part internal I would just make the change, but since it's part of the public api I think getting other opinions on how it should be set up is good.
I'll try changing this to use InputEvent as inspiration rather than notification. I think passing polymorphic refcounted instances best matches the use case and extensions for this pr, but wrote it off before due to the potential cost combined with how frequently the AnimationTree node is already used in existing projects. I'll actually benchmark it though and if it is too expensive it can be mitigated by making the feature opt-in (which might be nice to do regardless of performance).
I've pushed up a sort of demo of a 3rd possible approach, names and docs TBD. As mentioned in the earlier comment, this approach more closely follows InputEvent rather than _notification so single polymorphic value is passed through the signal and so users will type check it rather than compare against a constant. Compared to the simple single-value signal, this has the advantage that extra data can be included in the event and deferred callbacks can be used directly in more cases. Compared to including extra info in the signal's signature, this has the advantage that fields can be added as needed over time without breaking compatibility and users wouldn't need to know how to deserialize the int array into its constituent values.
Doing a quick and dirty benchmark, it looks like creating and emitting the new resource instance is very cheap (0.01-0.05 ms) but adding even a single gdscript callback changes that to 0.25-0.5 ms. Based on that, I think it would be worth keeping events_enabled around regardless of how the signal ends up being implemented.
Edit: Forgot to add but the state machine event is intentionally a bit empty atm; I figured the other pr would be changed to use this event system if it's approved.
Resource seems quite overdesigned, since this kind of hook is not reused as often as shader. It should at least be a Callable.
I feel it would be cleaner if we could pass a Callable, but in any case we need to pass the ClassName as a Callable argument, so it would not be much different from having a signal with several arguments, as I suggested above.
Resource seems quite overdesigned, since this kind of hook is not reused as often as shader.
I agree there's a lot of boilerplate; is there a more concise way to declare simple properties with their getter/setters? I chose Resource because for this approach some class hierarchy is required and Resource is the most natural class for use in the public api (also InputEvent uses Resource). One possible use case is the ability to create/save instances of these events for dynamic matching against runtime events, similar to how InputEvent can be used, for more flexible scripts which could be reused across animation trees.
I feel it would be cleaner if we could pass a Callable, but in any case we need to pass the ClassName as a Callable argument, so it would not be much different from having a signal with several arguments, as I suggested above.
Could you give an example of what you mean by passing Callable? Would the signal values consist of the class name and some callable which then provides certain data when invoked by the user? In that case I think an intermediate refcounted would still be necessary to support deferred callbacks, but I don't know if that's what you meant.
I also think it's still fundamentally different from passing multiple arguments in the signal itself as it allows for each event type to define any number of members and for members to be added over time without it being a breaking change.
In any case I've added animation_node to the base event so the node type can be checked both by event.animation_node is AnimationNodeOneShot and event.animation_node.get_class() == &"AnimationNodeOneShot".
For example:
animation_tree.cpp
void AnimationTree::_bind_methods() {
ADD_SIGNAL(MethodInfo(SNAME("animation_node_notification"), PropertyInfo(Variant::STRING_NAME, "animation_node_path"), PropertyInfo(Variant::STRING_NAME, "animation_node_class_name"), PropertyInfo(Variant::CALLABLE, "notification")));
animation_blend_tree.h
class AnimationNodeOneShot : public AnimationNodeSync {
GDCLASS(AnimationNodeOneShot, AnimationNodeSync);
public:
static void animation_node_one_shot_started();
static void animation_node_one_shot_finished();
~~~
class AnimationNodeTransition : public AnimationNodeSync {
GDCLASS(AnimationNodeTransition, AnimationNodeSync);
public:
static void animation_node_transition_started(int p_index);
static void animation_node_transition_finished(int p_index);
animation_blend_tree.cpp
AnimationNode::NodeTimeInfo AnimationNodeOneShot::_process(const AnimationMixer::PlaybackInfo p_playback_info, bool p_test_only) {
process_state->tree->emit_signal(SNAME("animation_node_notification"), callable_mp_static(&AnimationNodeOneShot::animation_node_one_shot_started));
~~~
AnimationNode::NodeTimeInfo AnimationNodeTransition::_process(const AnimationMixer::PlaybackInfo p_playback_info, bool p_test_only) {
process_state->tree->emit_signal(SNAME("animation_node_notification"), callable_mp_static(&AnimationNodeTransition::animation_node_transition_started).bind(transition_index));
BTW, the AnimationNodeEvent resource approach has AnimationNode as a member, which as I have explained several times, is quite dangerous from a standpoint of the pointer safety (one AnimationNode entity can be referenced by multiple AnimationTree instances, so we should not allow easy access to the AnimationNode in the process), therefore it should never be approved.
In any case, rather than implementing a hook with a Callable or creating a Resource, it should be simpler and easier for the user to understand to just pass a class name string. The Godot's philosophy is that the solution to a purpose should be minimal.
For example: [..]
Would the intended usage of such an approach (in user code) be similar to:
if notification.get_method() == &"animation_node_transition_started":
var args = notification.get_bound_arguments()
print("From %d to %d" % args)
as a sort of makeshift untyped (from the user POV) tuple with a name? I would definitely prefer passing the name and an array/dict of values compared to this, which is what you were getting at. In either case I dislike forcing the user to match against a magic string and to use a magic int (in the case of multiple binds) to get the value they want. Users could also do something like args[-1] and as a result any values that get added over time would be breaking. I think it is also significant that names and (static) types are lost here compared to either 1) the trivial case of passing minimal data and forcing the user to immediately query what they need or 2) having fully structured events.
BTW, the AnimationNodeEvent resource approach has AnimationNode as a member, which as I have explained several times, is quite dangerous from a standpoint of the pointer safety (one AnimationNode entity can be referenced by multiple AnimationTree instances, so we should not allow easy access to the AnimationNode in the process), therefore it should never be approved.
The previous issue you had explained was related to what kind of state can exist on the animation node instances due to them being resources (which was definitely my mistake not realizing what was allowed within the animation system), rather than users being disallowed "easy access" to these nodes in the first place. This seems like a strange position to take when for example the most obvious (only?) public api way to convert from int p_index in your example to a usable name uses the AnimationNode instance.
In any case, rather than implementing a hook with a Callable or creating a Resource, it should be simpler and easier for the user to understand to just pass a class name string. The Godot's philosophy is that the solution to a purpose should be minimal.
Frankly, it is frustrating to read this as from my point of view the earlier approach for this pr was dead simple and very easy to extend as use cases arose but faced objections which required added complexity/redundancy for questionable benefit. When I disagreed with or tried to clarify the objections it felt you were only reading every other line of my comment or something. Then later I changed the pr to allow for the use cases you wanted, and then came the pushback with complexity and use of Resource, even though considerations were made for performance and it's a base class for a notable transient type - InputEvent.
That being said, I respect that I'm not entitled to have my pr merged into your code. I don't really want my name attached to the interfaces you've suggested changing this to and I'm not hugely interested in wasting more time on this so I'll be closing this pr. https://github.com/godotengine/godot/pull/102398 on its own addresses the proposal's use case, although I personally have use for these signals so I'll rebase this to my own dev branch. Others are free to use this code as a starting point for their own prs/use. Sorry for the essay this comment turned into.
You may have misunderstood some sentence; in my opinion, the signal approach was fine before you changed to the Event resource approach, except for the arguments. I suggested passing a StringName of class_name which is guaranteed to be compatible compared to an enum which could be broken over time/version (depends on the addition of AnimationNode and signals), but you disagreed.
Passing the Transition's port number through the sub_indice array does not imply access to the AnimationNode; if you think of the Transition as a StateMachine, it simply indicates which state it has transitioned to. Since Transition's current_index is an AnimationTree parameter which is guaranteed to be one per AnimationTree instance (AnimationTreeInstance n vs n AnimationTreeParameter), it is clearly different from accessing the internal data of an AnimationNode resource (AnimationTreeInstance n vs 1 AnimationNodeResource).
So I think it is salvageable by revert to approach that uses signal and pass class_name as StringName argument.
You may have misunderstood some sentence; in my opinion, the signal approach was fine before you changed to the Event resource approach, except for the arguments. I suggested passing a StringName of class_name which is guaranteed to be compatible compared to an enum which could be broken over time/version (depends on the addition of AnimationNode and signals), but you disagreed.
I haven't misunderstood your position, I have only disagreed. I've already said as much before but I consider use of a magic string to be more brittle than other options and I disagreed with your characterization of when enums work or do not work. The issues you described with enums are a direct result of what you were requiring the values to be and didn't reflect how enums are used by _notification or more broadly. The event classes are in my opinion the best design for the requirements you at various points described:
- having a single generic signal for all animation nodes
- allowing extra data to be included for easy use with deferred callbacks
- distinct values which will not break/shift as things are changed in the future
Passing the Transition's port number through the sub_indice array does not imply access to the AnimationNode; if you think of the Transition as a StateMachine, it simply indicates which state it has transitioned to. Since Transition's current_index is an AnimationTree parameter which is guaranteed to be one per AnimationTree instance (AnimationTreeInstance n vs n AnimationTreeParameter), it is clearly different from accessing the internal data of an AnimationNode resource (AnimationTreeInstance n vs 1 AnimationNodeResource).
My point was that users would need to either 1) maintain a duplicate map of index to state name or 2) call AnimationNode::get_input_name to get a value to either compare against or pass as a transition request. The code could be modified to also include prev_state in the parameters, or to accept ints for transition requests, but that's not currently the state of things. In any case, given the path it's trivial to get access to the resource anyways. If access and possible mutation during process is really a problem then it is currently already a problem.
My primary issue with sub_indices is that it is shoehorned into the signature and would be used by exactly one class and have zero other use within this pr. You mentioned other hypothetical use cases, but they all involve overloading the meaning of the int[] which I consider a significant issue with the proposed interface. A more general data structure could be used but, compared to structured events, more magic values are introduced into user code and static typing is impossible.
I disagree with your opinion that class_name is a magic string, since class_name is frequently used elsewhere. You seem to be hung up on the enum traversing the all AnimationNodes, but I have decided that an independent enum + class_name for each AnimationNode is preferable, since it is obviously more maintainable.
My point was that users would need to either 1) maintain a duplicate map of index to state name or 2) call AnimationNode::get_input_name to get a value to either compare against or pass as a transition request. The code could be modified to also include prev_state in the parameters, or to accept ints for transition requests, but that's not currently the state of things. In any case, given the path it's trivial to get access to the resource anyways. If access and possible mutation during process is really a problem then it is currently already a problem.
It makes sense, passing the StringName of the port name as sub_info instead of sub_indices is may be one of the solutions, but it would also be fine to make the index available to the Transition request.
In any case, passing an Event resource or dictionary is not allowed due to performance concerns. In the past, for the same reason, AnimationNodeExtention was implemented using the approach of packing struct NodeTimeInfo into a PackedIntArray by https://github.com/godotengine/godot/pull/99181.
If struct will be implemented in GDScript, I assume the best approach would be to use struct for sub informations. However, even in that case, the class_name should still be passed as StringName, not passing a reference to the AnimationNode resource directly.
given the path it's trivial to get access to the resource anyways. If access and possible mutation during process is really a problem then it is currently already a problem.
It has actually been a problem several times in the past such as https://github.com/godotengine/godot/issues/22887, https://github.com/godotengine/godot/issues/100018 and https://github.com/godotengine/godot/pull/75759. We have now resolved it and should not add any new concerns; Although the user can still intentionally access the AnimationNode to corrupt the process, but we should not add more places where the user could unintentionally corrupt it.
You're right, they're not true magic values. Specifically I meant the use of literals with little to no static checking possible, which is my main issue with the recommendations. I don't mind class name that much (besides the line length), although it's surprising the resource is so off limits. I don't like passing array/dict for additional data though and forcing users to know which indices/keys mean what for which animation nodes. I'm not sure what you mean by the enum traversing the animation nodes; I think we only disagreed on if each enum should be zero indexed or offset such that it is more easily associated with its class (which overlaps in function with passing class name). I hope it comes across that for each of the approaches considered in this thread I have implemented the changes and written short scripts from the user perspective to test the ergonomics.
The reason I'm surprised about the node instances is that they offer much more than just the class name and they're already controlled/easily accessible by the user. If corrupting/changing state during process is an issue, that should be itself addressed. For example, warning/erroring when calling a public non-const method while the state pointer is set. Similarly, that pr looks like it's fixing issues inherent to process state being a part of the resource state rather than for example having the process calls be entirely functional or having a stack to prevent clobbering (although it's a huge pr so I skimmed; sorry if I missed something).
Benchmarking a few hundred instances, they're both very fast but using a resource was on average 1.6 microseconds slower than using packed array. Calling emit_signal with nothing connected is more expensive than creating + setting up either, and adding a simple callback makes it an order of magnitude slower than either option. And of course it can be opt-in on a node by node basis regardless of implementation anyways. The packed array is still a dynamically sized cow vector and isn't free; to me the performance difference is more than appropriate for the advantages of resource. Structs could be a good alternative, especially if they offered a more concise way of declaring the type and its fields than this.