Optimize animation performance and improve internals

[Ryan-000]

Supersedes #110474

See #112308 (comment) for benchmarks

Bugsquad edited:

• Closes Errors when switching animations while in Capture blend mode in BlendSpace1D and BlendSpace2D #112053
• Closes Fix un-init base path when switch AnimationNode in BlendSpace2D #112050

[TokageItLab]

I think the difference from #110474 is that this was added. Are there any other changes? Is a new benchmark comparison possible?

If #110474 (review) issue was due to multiple same AnimationNode resources being instantiated and placed, then the fix to manage each resource's instance makes sense. I confirmed that this no longer causes #110474 (review) issue.

However, since I recall past issues similar to #110506, I'm a bit concerned about whether using thread_local is appropriate. Testing in a project editing scenes containing a large number of AnimationTree instances might be necessary.

Also, I have some concerns regarding code readability. Passing instance pointers as arguments is indeed safe, but since there should be never occur to access different ProcessState and AnimationNodeInstance within a single AnimationNode like:

double length_a = instance_a.get_parameter(current_length);
double length_b = instance_b.get_parameter(current_length);

, so it always p_instance.get_parameter(), it would be ideal if there is a way to omit them, same with old code. This would significantly reduce the number of lines requiring modification. Do you have any ideas @lyuma @SaracenOne?

If there are no other way, I assume it can be accepted depending on the benchmark improvement. In other words, we want a benchmark comparison before and after, focusing solely on changes to the AnimationTree instance, excluding any other changes. This was one reason why I suggested separating the PR.

[TokageItLab]

We briefly discussed this at the animation meeting and agreed that passing references as arguments is acceptable.

We still have some concerns about thread_local, so we will continue reviewing it. cc @lyuma

[Ryan-000]

I'll upload performance benchmarks soon, but for review, is it better if I squash the commits? Or leave it as it is?

Also regarding this

Also, I have some concerns regarding code readability. Passing instance pointers as arguments is indeed safe, but since there should be never occur to access different ProcessState and AnimationNodeInstance within a single AnimationNode like:

It actually is present in BlendSpace1D and 2D https://github.com/godotengine/godot/blob/dee1843b12d9950f21bed4c3986e14090388e490/scene/animation/animation_blend_space_1d.cpp#L386 https://github.com/godotengine/godot/blob/dee1843b12d9950f21bed4c3986e14090388e490/scene/animation/animation_blend_space_2d.cpp#L552

[Ryan-000]

And I recommend splitting this into several PRs. The reason is to make it easier to focus on reviewing each feature in more detail and, if any issues arise, only part of the changes need to be reverted.

I agree, but splitting it now would be very painful. It's already squashed into a single commit, I don’t have a good workflow to split it apart, and many of the changes are interdependent (so I’m not sure how to structure the PRs correctly).

Although it’s not ideal, I’d prefer to just get this PR through the door as-is. If a major issue comes up afterward, I’ll take a look and consider splitting it then.

[TokageItLab]

If dependencies exist, you can create a PR using that as the branch base (like #112302 tree). For example, you could create one commit for the HashMap change and then create three PRs branching from it. It also helps visualize where dependencies occur so that we can prioritize and carefully review the ancestor of the dependencies.

If they truly must be merged simultaneously, it would mean class dependencies are broken (e.g. the case that AnimationTree causes strong dependencies on AnimationPlayer). However, this PR doesn't appear to be that case, so it should be splittable.

[TokageItLab]

@Ryan-000 For now, I've split it into the following 5 parts as examples, please verify if they are correct.

• Optimize NodePath #113447
• Optimize Skeleton3D #113441
• Add get_value_ref_or_add_default() to HashMap #113448
  • Optimize Animation Resource, Library, Mixer and Player #113442
    • Optimize AnimationTree #113444

If you need to manage them on your master, I will close those PRs and supersede them with yours, so please mention me after opening the PR.

[Ryan-000]

Thanks a lot for taking the time to split these PRs out for me, the structure looks good.

One question though, there seems to be a merge conflict in misc/extension_api_validation/4.5-stable.expected. Should I resolve it in my branch here, or will you be handling that conflict in your PRs?

[TokageItLab]

If you prefer, I can resolve the conflicts on my PR. However, if you wish to manage them yourself (such as when you want to make changes based on the points raised), please create a new pull request (you can check out my branch, and reset -mixed once and force-push then the commit author become your username again) and I will supersede my PR with yours. However, since the compatibility file conflicts aren't particularly significant, I think nothing needs to be done yet.

My guess is that whether we accept #113448 or not will change the branch base, so I believe we should focus first on measuring #113448 benchmarks.

[Ivorforce]

I recommend working ahead without #113448 first. The difference in code should be minimal with or without it.

If #113448 does turn out to make a big difference, it should be easy to show this once the rest is merged.

[TokageItLab]

I rebased #113442 and #113444 to exclude #113448 and resolve conflict.

[Mike-Litton]

@Ryan-000 @TokageItLab What is the state of these improvements ? Is there anything I can try in my Godot 4.6 based game? Animation performance is currently a bottleneck and maybe I can use some of this work instead of modifying the engine myself?

[TokageItLab]

Since the addition of high-priority features for 4.6 has been completed, I think that 4.7 will primarily focus on optimization work aimed at improving performance. We are currently busy with fixing bugs that occurred in 4.6, but I will start working on these optimizations as soon as that is finished.

[Mike-Litton]

Since the addition of high-priority features for 4.6 has been completed, I think that 4.7 will primarily focus on optimization work aimed at improving performance. We are currently busy with fixing bugs that occurred in 4.6, but I will start working on these optimizations as soon as that is finished.

That sounds great. I am open to taking some risks with an early version of the optimizations that I build myself if they were mergable 😅

[Ryan-000]

@TokageItLab If needed I can rebase and reopen #113442 and #113444, I just need to know once the maintainers have time to review them, so I’m not rebasing unnecessarily.

[TokageItLab]

@Ryan-000 Sounds good. I'd really like to see that merged into 4.7, so it would be a huge help if you'd do that :) Then, I will close them after you send that PRs.

[akien-mga]

Is this superseded by #117277 ?