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

[rmodels] Optimized UpdateModelAnimationBones() function #4602

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kirandeep-Singh-Khehra
Copy link

@Kirandeep-Singh-Khehra Kirandeep-Singh-Khehra commented Dec 14, 2024

  • Updating bones only once instead for each mesh.
  • Updating only one model.meshes[].boneMatrices and then using deep copy for other meshes instead of calculating for each bone in each mesh.
  • Tried to keep asserts and condition at their places.

Other points:

  • Makes it a clean base/template/reference for other/new bone updation functions. Because if using this as template or modifying it, then some calculations done in one mesh can affect bones in other mesh in next iteration(doubles the effect in for next mesh) If some new variable used in loops is declared outside loop.

  • This new template is helpful for me to create new functions to update bones of model for animation system I'm working on.

  • Tested and is working fine for me.

 - Updating bones only once instead for each mesh.
 - Updating only one `model.meshes[].boneMatrices` and then using deep copy for other meshes instead of calculating for each bone in each mesh.

**Other points:**
 - Makes it a clean base/template/reference for bone updation functions. Because if using this as template then some calculations done in one mesh can affect bones in other mesh in next iteration(doubles the effect in for next mesh).

Signed-off-by: Kirandeep-Singh-Khehra <[email protected]>
@raysan5
Copy link
Owner

raysan5 commented Dec 15, 2024

@Kirandeep-Singh-Khehra Thanks for the review. Did you profile this change to verify the optimization gain? Also, could it happen that only one model mesh has bones info and not others? Won't this solution implement one specific mesh transformations to all other meshes? 🤔

@Kirandeep-Singh-Khehra
Copy link
Author

Did you profile this change to verify the optimization gain?

Hi Ray, I have not profiled for optimization gain but i can explain in terms of time complexity. Initially matrix transformations were calculated in O(mn) (m is number of meshes and n is number of bones.) but after this change it will be *O(n) (n is number of bones). As all the calculations were resulting in same bone matrix array for each mesh. So, instead of calculating it for each bone of each mesh i tried to calculate the bone matrices of only first mesh which have bone Matrices and then copying the bone matrix to other meshes if they already have it.

Also, could it happen that only one model mesh has bones info and not others?

Asserts are at same place as they were before. And there is a if (model.mesh[i].boneMatrices) check while selecting the index of first mesh with bones and while copying it to other meshes.
Yes it is checking the presence of bone matrices before updating them.

Won't this solution implement one specific mesh transformations to all other meshes? 🤔

Yes it will. As it was the same thing happening before. So, instead of calculating same boneMatrix array for each mesh i am calculating it only once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants