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

Improve performance when apply bone transform in get_bone_global_pose() #42681

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Oct 9, 2020

Improve performance when apply bone transform in get_bone_global_pose() by implementing bone_children and associating them with dirty flag and option: bypass updating skins.

The current implementation of Skeleton is that if you call get_bone_global_pose() after transforming a bone, recalculate transformation all the bones in the skeleton and access the visual (rendering) server for updating the skin. However, the current implementation causes a lot of waste waste when computing the bones in the same thread.

For example, a skeleton has 100 bones. If the transformation is applied to the bone at the base of the skeleton's right index finger, then the necessary recalculation would normally only require 3 times recalculation, so including the child. However, in the current implementation, it would be recalculated 100 times.

Also, under normal circumstances, applying the skin only needs to be done once at the end of the thread. If you want to transform a bone multiple times with get_bone_global_pose() in the same thread, slows down performance because accessing the visual server multiple times.

These issues have resulted in very poor performance when using get_bone_global_pose() in add-ons that simulate bone transformation, such as IK and DynamicBone. In order to lay the groundwork for future add-on development, I make two main changes in this PR

  • Implements a list of bone children as a property, reducing unnecessary recalculation of bones
  • Implements an option: don't update skins in get_bone_global_pose()

I actually saw a large performance improvement in the DynamicBone I created for 3D model's hair.

bone_global_pose

@TokageItLab TokageItLab marked this pull request as draft October 9, 2020 20:58
@Calinou Calinou added this to the 4.0 milestone Oct 9, 2020
@TokageItLab TokageItLab force-pushed the implement_bone_children_and_bypass_vs_opt branch 5 times, most recently from fff91bc to db3ca58 Compare October 9, 2020 22:26
@TokageItLab TokageItLab marked this pull request as ready for review October 9, 2020 22:27
@TokageItLab TokageItLab requested a review from a team as a code owner October 9, 2020 22:27
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 9, 2020

I later found that the destination for improving performance is the same as #41322, although the way we define the dirty flags to reduce the number of calculations and how to bypass the visual server are different. However, I would like to implement bone_children and bones_root are useful in GDScript.

@TokageItLab TokageItLab marked this pull request as draft January 5, 2021 06:30
@TokageItLab TokageItLab force-pushed the implement_bone_children_and_bypass_vs_opt branch from db3ca58 to e172800 Compare January 7, 2021 14:36
@TokageItLab TokageItLab marked this pull request as ready for review January 7, 2021 14:36
@TokageItLab TokageItLab force-pushed the implement_bone_children_and_bypass_vs_opt branch 2 times, most recently from ee005ad to 2ae4fee Compare January 7, 2021 17:17
@TokageItLab TokageItLab force-pushed the implement_bone_children_and_bypass_vs_opt branch from 2ae4fee to 81f4bdd Compare January 7, 2021 17:18
@TokageItLab
Copy link
Member Author

TokageItLab commented Nov 20, 2021

Skeleton implementations have changed a lot during the IK implementation #51368. As discussed with @lyuma, it need to be refactored for improved performance in the future.

@TokageItLab TokageItLab deleted the implement_bone_children_and_bypass_vs_opt branch September 16, 2022 21:04
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.

2 participants