Add AnimationTrackFilter and implement filter as an AnimationNode's parameter.

[Daylily-Zeleen]

Implement #6808.

• Add resource type AnimationTrackFilter, remove AnimationNode menbers:
  • filter, filter_enabled
  • _get_filters(), _set_filters(), has_filter(), set_filter_path(), set_filter_path(),
• Implement filter like other AnimationNode's parameters.
• Modify AnimationNodeBlendTreeEditor to adapt to first change.
• Implement AnimationTrackFilterEditorPlugin to edit filter tracks in inspector.

2023-10-02.11-58-11.mp4

---

This pr implement filter for AnimationNodeBlend3, will close #76506.

The filter is implemented as gradual style, will close #7578.

Bugsquad Edited:
Closes godotengine/godot-proposals#7896

[Daylily-Zeleen]

How to fix this static checks?

[warriormaster12]

You basically add whatever is marked green to your code. If you have line of code that is marked red and right below it is the same line of code that is marked green then that means that you have to edit the line of code to look like the green one.

In your case you seem to be missing license related comments so adding those should fix static checking.

[Daylily-Zeleen]

You basically add whatever is marked green to your code. If you have line of code that is marked red and right below it is the same line of code that is marked green then that means that you have to edit the line of code to look like the green one.

In your case you seem to be missing license related comments so adding those should fix static checking.

You can my changed files, I already add the lisence info.

[Daylily-Zeleen]

Can someone help me?

[TokageItLab]

At any rate, the file must end with a new line.

[Daylily-Zeleen]

Refactor is done, please review again.

[lyuma]

There are some changes which break API compatibility. This is a hard blocker for merging this PR. It is okay to mark functions deprecated, but existing methods, including virtual methods, must not be removed between Godot 4 versions.

EDIT: See Godot's policy on compatibility across engine versions.

[lyuma]

Error: Validate extension JSON: API was removed: classes/AnimationNode/methods/_has_filter

It is not permitted to remove public APIs, such as AnimationNode.is_path_filtered(path:NodePath), AnimationNode.set_filter_path(path:NodePath,enable:bool), AnimationNode.set_filter_enabled(enable:bool), AnimationNode.is_filter_enabled() or the virtual function AnimationNode._has_filter(). The property filter_enabled may also be kept for script compatibility, but it is not required to do so for GDExtension support.

These AnimationNode methods must be kept but marked deprecated, such as using is_deprecated="true" in this documentation XML. I would suggest that it would be good practice to implement them if possible, for example accessing the attached AnimationTrackFilter if possible, or returning empty if this is not possible.

[lyuma]

Why do some animation nodes have a filter and some don't?

I find this use of macro really confusing. Why can't we have the "filter" parameter in the base class AnimationNode?

If we do have a special type of "filterable" animationnode, perhaps we should add an abstract class / level of inheritance to indicate the filterability (having the "filter" parameter).

[Daylily-Zeleen]

Because I "filter" just a normal parameter like "add_amount" in AnimationNodeAdd2, I think it should be added where they need in get_parameter_list(), this macros just for copying duplicate code and avoiding stupid mistake.

However, if user create their own AnimationNode in GDScript (godot/pull/69641) and need filter, it may not work expectantly because of misspell.
For user friendly, I readd has_filter() now.

But I am not sure which way is more reasonable.

[lyuma]

I don't like having a member variable named the same as its string constant value. If this is effectively a constant, it should be marked const and made capital, or renamed to _string or _parameter to indicate that it refers to the parameter / string constant, and it isn't itself a filter object or a time value.

The existing code is also bad practice and should be fixed in a separate PR. In particular, the use of time as a member variable name shadows the C library's time(time_t*) function. I wouldn't have as much of an issue with this additional parameter as long as it is next to the other PNAME values, namely time, remaining and so on.

This issue isn't a sticking point, just something I observed that made it especially difficult for me to read and follow the property list code.

[Daylily-Zeleen]

I agree your opinion, here I just abey the existing code.

[SaracenOne]

I like this PR, I think its the right direction to go for both ease of use and memory saving. Regarding the deprecation thing, the way I would suggest implementing this without breaking compat is keeping the old filter function, but marking them as deprecated as per the deprecation system we introduced for 4.x. You can then make the existing functions, for example, for when setting the filters property, generate a unique filter resource for them. This will ensure backwards compatibility with the old system and allow this to be introduced without breaking existing trees.

[Daylily-Zeleen]

I like this PR, I think its the right direction to go for both ease of use and memory saving. Regarding the deprecation thing, the way I would suggest implementing this without breaking compat is keeping the old filter function, but marking them as deprecated as per the deprecation system we introduced for 4.x. You can then make the existing functions, for example, for when setting the filters property, generate a unique filter resource for them. This will ensure backwards compatibility with the old system and allow this to be introduced without breaking existing trees.

"filter" in this pr is not belong to AnimationNode and should be referenced by specific AnimationTree nodes, unless we can find out correct AnimationTree nodes when loading an AnimationNode, it is impossible to keep old filter info from old resources datas.
Although I add filter, _set_filter(), _get_filter() to pass compat check, old filter info will be lost when opening old scenes.

[DucktatorQuack]

The filter_enabled doesn't work from code.
filter_enabled

I've tried to trigger it in multiple ways, but in 4.3 it doesn't work.