Add animation node extension

[GuilhermeGSousa]

TODO:

• Write docs

Bugsquad edit:

Fixes: godotengine/godot-proposals#11123

[fire]

The same criticism of RC suffix as the last comment.

[TokageItLab]

I have concerns about future compatibility breakdown if struct is implemented in GDScript. We will need to consider whether we need to rush.

[GuilhermeGSousa]

@TokageItLab what were your concerns? I don't have any strong opinions, I though this was the cleanest of the ways to do this but I'm open to suggestions!

[TokageItLab]

More specifically, I am concerned that I guess that GDScript struct will not be implemented as refcounted (maybe it should be one of the Variant types) in the future.

In the discussion regarding the implementation of struct, there was mention of it being implemented as a similar to a typed Dictionary.

NodeTimeInfo is composed of several properties and a get_remain_time() method that returns a value based on them. It is like a combination of a typed Dictionary and a Callable, but both are built-in types in the variant, not extends RefCounted class.

So what you are trying to do should actually be one of the struct as one of the Variant types, not Refcounted.

Even if we rush to merge this with inheriting Refcounted because we don't have struct, we will still need to replace it with struct in the future. Then it is doubtful that we can migrate Refcounted into Variant since base classes are fundamentally different.

[GuilhermeGSousa]

@TokageItLab are you suggesting waiting until structs are implemented? That might not be coming to the engine any time soon, and I wouldn't mind refactoring this code once it is available.

Another idea would be to do what the AudioStreamPlayback _mix method does and use pointers instead. That way it wouldn't be exposed to GDScript, and would be easy to convert to using structs once avaiable.

[TokageItLab]

are you suggesting waiting until structs are implemented?

Right, that's the direction that should be most recommended. Insteads, you can use the custom module.

That might not be coming to the engine any time soon

Since the PR is already there #82198, you could help with that work and feedback to support it.

Then, it is fine to include that PR before this PR and mentions "- Prerequisite #82198" to the description of this PR

---

If we are in a hurry to implement something, we will need to expose NodeTimeInfo as some Variant type instead of RefCounted. RefCounted is referential, so compatibility will be hard broken in the future if there are scripts that reference the same NodeTimeInfo.

From a performance point of view, we should avoid this but you can use Dictionary in some cases, but there is a problem that the get_remain() method is not available. Or you can use a encoded PackedByteArray, or a Callable object to exchange information.

Strictly, these are also referential, but since they are Variant types, if a reference is broken, only the Initial value is passed, allowing for relatively safe compatibility breaking and migration.

[GuilhermeGSousa]

Hi @TokageItLab! I updated this PR before having read your comment! You make very valid points, and I think you might prefer these last changes I pushed, I'm no longer using ref counted objects and instead encoding the return value on a PackedFloat32Array, let me know what you think!

As for the struct PR, I do like the idea you suggested. I'm not familiar with the struct changes, but I can least help with feedback. Nonetheless, if possible I'd like to submit these changes first, and later refactor the return value to use a struct instead.

[TokageItLab]

PackedFloat32Array looks fine than RefCounted.

This is not a blocking opinion of this PR in particular, but I have two more concerns (as above your comment, probably do you work in progress?)

1. Shouldn't PlaybackInfo be an array so that it can be delivered as well?
2. NodeTimeInfo has two methods: bool is_looping() and double get_remain(bool p_break_loop = false). Maybe you should implement it as a static method somewhere to be able to retrieve these information externally by passing an array as an argument.

[GuilhermeGSousa]

@TokageItLab agree with all your comments, just updated the PR to include them.

[fire]

The documentation changed and requires to be generated with --doctool.

[TokageItLab]

LGTM, those API should be almost identical to the virtual function of the AnimationNode class provided in cpp, except for the struct format.

We don't know what struct will be in the future, but it is probably a Variant type, so it should be able to provide a migration path.

[Repiteo]

For posterity: there was discussion on RocketChat & in the most recent GDExtension meeting on the potential to only use *Extension classes when necessary; that is, if they're exposing functions/methods that could not be utilized with standard scripting (eg: anything with pointers). In this case, the virtual functions could be added to AnimationRootNode instead, removing the need for AnimationNodeExtension entirely

However, as no "official" stance was taken, this is perfectly fine to be merged as-is

[GuilhermeGSousa]

For posterity: there was discussion on RocketChat & in the most recent GDExtension meeting on the potential to only use *Extension classes when necessary; that is, if they're exposing functions/methods that could not be utilized with standard scripting (eg: anything with pointers). In this case, the virtual functions could be added to AnimationRootNode instead, removing the need for AnimationNodeExtension entirely

However, as no "official" stance was taken, this is perfectly fine to be merged as-is

I'll defer to the animation team for this one, I have no strong opinion on this either (@TokageItLab @fire @lyuma )

[dsnopek]

For posterity: there was discussion on RocketChat & in the most recent GDExtension meeting on the potential to only use *Extension classes when necessary; that is, if they're exposing functions/methods that could not be utilized with standard scripting (eg: anything with pointers). In this case, the virtual functions could be added to AnimationRootNode instead, removing the need for AnimationNodeExtension entirely

However, as no "official" stance was taken, this is perfectly fine to be merged as-is

This probably needs it's own proposal/discussion at this point, but I personally think the "official stance" (if/when we have one) should only be a recommendation. Like, we recommend using *Extension classes only when necessary (and give some examples of what "when necessary" could be) but then it's up to the team responsible for the API to make the call about if it's necessary or not.

[TokageItLab]

I am not against replacing AnimationNode with this.

Since the AnimationNodeExtention does not add any additional properties to the AnimationNode, so replacing the AnimationNode with it is not expected to cause any significant loss, at least from a memory usage perspective.

However, if we replace AnimationNode with this, it breaks compatibility with the old _process() method, so code for compatibility must be written in the .compat file. Also, the experimental flag should be set for some methods instead of whole Class.

As @reduz explained earlier, GDVIRTUAL declarations can affect the performance of the process, but I think it should not be a problem as long as the registration with GDREGISTER_VIRTUAL_CLASS is used. Well, I believe it is reliable and safe to have @reduz take a look at this PR once.

[TokageItLab]

Related to the comment above, AnimationNodeExtention should replace AnimationNode, not AnimationRootNode.

The AnimationRootNode does not need an input port and can be placed in a StateMachine. If not, it is because that would imply that an input port is required.

I guess you want it to be the AnimationRootNode for placing it in the BlendSpace or StateMachine purposes, but if you replace it and replace the AnimationNode with the AnimationNodeExtention, the AnimationRootNode will extend it, thus solving this classification problem.

[Bromeon]

Btw, was this comment overlooked?

[TokageItLab]

Yes I think if it is an Extension, we will need two (or three) Extensions, AnimationRootNodeExtension and AnimationNodeSyncExtension (and AnimationNodeExtention). If it is not an Extension by replacement, then only the AnimationNode needs to be replaced. cc @GuilhermeGSousa

[TokageItLab]

#100416

[GuilhermeGSousa]

Completely missed this comment, my bad @TokageItLab. I agree with you, do you want me to make PRs for those other extension nodes as well?

[GuilhermeGSousa]

Just updated with the latest comments. I do like having extension specific logic be separated in an extension class, and avoid having experimental stuff mixed in with core classes like AnimationNode.

[Repiteo]

Thanks!

[GuilhermeGSousa]

Thank you @Repiteo ! How does the workflow go for godot-cpp, should I open a PR there?

[Bromeon]

The added virtual method in AnimationNodeExtension:

func _process(playback_info: PackedFloat64Array, test_only: bool) -> PackedFloat32Array

is in conflict with the method of same name in AnimationNode (which it indirectly inherits, through AnimationRootNode):

func _process(time: float, seek: bool, is_external_seeking: bool, test_only: bool) -> float

You added a deprecation warning in the docs. However, in GDExtension, such deprecation can currently not be communicated in extension_api.json -- AnimationNode::_process() is still advertised as a regular virtual function.

This breaks godot-rust codegen, but likely affects other bindings as well. Multiple methods of the same name in the same class aren't occuring anywhere else, to my knowledge.

---

Is it not possible to register _process with a different name? Given that the thing is called "node" but isn't a Node and Node::_process has yet another signature, there's already some baseline confusion anyway, and this adds on top 😅

[Repiteo]

Dang, I'm surprised a conflict like this wasn't caught in the CI

[Bromeon]

In C++, having multiple methods of the same name is generally not a problem, and I don't know if there are CI checks beyond that (especially for virtual functions). But I was also wondering what might be a good way to detect this 🤔

[GuilhermeGSousa]

How could I help fix this, would @TokageItLab 's PR do it? Should I rename this method?

[Bromeon]

How could I help fix this, would @TokageItLab 's PR do it?

No, that one changes a different thing (the base class).

Should I rename this method?

Yes, I think that would fix the problem 🙂
Something like _process_animation should already solve it (or maybe there's a better name, but I think this is already much clearer than plain _process).

[GuilhermeGSousa]

Here goes!
#100442

[Flynsarmy]

The documentation for this new node makes no mention of what the test_only method argument is or does.