Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert Vector to LocalVector in animation system #97687

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

YYF233333
Copy link
Contributor

Implement godotengine/godot-proposals#10594

convert all Vector I found in animation_tree.h and animation_blend_tree.h except:

  • connections : they involve a copy which may benifit from COW
  • get_editable_animation_list : seems like an interface

Tested with godot-benchmark, no noticeable performance change. Is this change acceptable as a codestyle improvement?

Build with scons target=template_release production=yes, run with .\godot.windows.template_release.x86_64.console.exe .\godot-benchmarks\project.godot -- --run-benchmarks --include-benchmarks="animation/animated_models/*"

Test Result:

Master Branch:
{"engine":{"version":"v4.4.dev.custom_build","version_hash":"e3213aaef5e0e72b8272e65d989d3d8222be17ca"},"system":{"cpu_architecture":"x86_64","cpu_name":"12th Gen Intel(R) Core(TM) i5-12400","os":"Windows","cpu_count":12}}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.455,"render_gpu":64.95,"time":0.036}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.371,"render_gpu":64.87,"time":0.017}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.462,"render_gpu":64.97,"time":0.023}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.378,"render_gpu":64.82,"time":0.014}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.457,"render_gpu":65.08,"time":0.023}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.313,"render_gpu":64.85,"time":0.017}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.451,"render_gpu":65.03,"time":0.032}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.358,"render_gpu":64.81,"time":0.015}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.482,"render_gpu":64.93,"time":0.026}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.369,"render_gpu":64.82,"time":0.016}}]}

This PR:
{"engine":{"version_hash":"5d3d68dd405a13a4de63b8c39c000fc67bb5dea8","version":"v4.4.dev.custom_build"},"system":{"os":"Windows","cpu_count":12,"cpu_name":"12th Gen Intel(R) Core(TM) i5-12400","cpu_architecture":"x86_64"}}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.599,"render_gpu":65.46,"time":0.028}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.37,"render_gpu":65.55,"time":0.016}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.562,"render_gpu":65.04,"time":0.022}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.434,"render_gpu":64.74,"time":0.026}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.562,"render_gpu":66.36,"time":0.024}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.427,"render_gpu":64.78,"time":0.029}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.236,"render_gpu":63.03,"time":0.023}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.425,"render_gpu":62.2,"time":0.017}}]}
{"benchmarks":[{"name":"Animation Blend Tree 100","category":"Animation > Animated Models","results":{"render_cpu":6.927,"render_gpu":65.25,"time":0.025}},{"name":"Animation State Machine 1000","category":"Animation > Animated Models","results":{"render_cpu":6.874,"render_gpu":65.07,"time":0.017}}]}

# Animation Blend Tree 100 is changed to Animation Blend Tree 1000 because it takes too little time

@YYF233333 YYF233333 requested a review from a team as a code owner October 1, 2024 11:32
@Calinou Calinou added this to the 4.x milestone Oct 1, 2024
@YYF233333
Copy link
Contributor Author

oops, I forget to check dev_mode, now tested locally.

@fire fire requested review from fire and TokageItLab October 1, 2024 19:33
@fire fire changed the title convert Vector to LocalVector in animation system Convert Vector to LocalVector in animation system Oct 2, 2024
@fire
Copy link
Member

fire commented Oct 2, 2024

I didn't see any major problems, but I haven't tested.

@YYF233333
Copy link
Contributor Author

Is this PR still needed? It has been no feedback for several months. I'm going to close this if no body want.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, sorry for the lack of feedback. I have been using this in testing in a custom build for a weeks now and while there could be some nuances of this new approach I may not have fully evaluated the implications of, I haven't noticed any obvious issues with the animation system which seem related to it. I'll approve it for merging, but I'll also ping @TokageItLab in case you have any thoughts on it.

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 23, 2024
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just be sure to squash your commits first

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@Repiteo Repiteo merged commit 0c80b47 into godotengine:master Dec 23, 2024
11 of 19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 23, 2024

Thanks!

@YYF233333 YYF233333 deleted the animation branch December 23, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants