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

feat: updateBoneMatricesForInstance #7859

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hannojg
Copy link
Contributor

@hannojg hannojg commented May 15, 2024

Added a method to control to which instance to apply the bone matrices updates of an animator.

This is particularly useful if you want to apply the animation from one master asset to other instances that are different assets (have different meshes) but share the same skeleton as the master asset. A common example for this is e.g. clothing.

In our use case we have one main character which contains all the animations. We then have clothings that are rigged to the same skeleton of the main character.
We are then able to apply the transformations of the character to the clothing assets while playing an animation.

The initial discussion has been started here:

Added a method to control to which instance to apply the bone matrices updates of an animator.

This is particularly useful if you want to apply the animation from one master asset to other instances that are different assets but share the same skeleton as the master asset.
A common example for this is e.g. clothing.
*
* @param instance The instance to update.
*/
void updateBoneMatricesForInstance(FilamentInstance* instance);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like maybe we can formalize addInstance below instead of adding this method?

We can do some validation of the the animation associated with the "new", added instance vs the one that's used to build this animator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so as well first. However, the "problem" I found with that is that we are again creating all those channels and data for the new instance.
With the solution I have right now we don't need to recreate any data (however I admit that my solution is maybe a bit fragile as it relies on the name of the entities to synchronise entity transforms).

Copy link
Contributor Author

@hannojg hannojg May 16, 2024

Choose a reason for hiding this comment

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

(The solution I am talking about is not visible in this PR but it is what I outlined here)

@ninjz
Copy link

ninjz commented May 23, 2024

Wasn't able to get this solution working on Android. Are there any additional instructions you can provide on how this should be used?

@hannojg
Copy link
Contributor Author

hannojg commented Jun 3, 2024

Wasn't able to get this solution working on Android. Are there any additional instructions you can provide on how this should be used?

I provided instructions here:

@pixelflinger pixelflinger requested a review from poweifeng August 15, 2024 05:46
@hannojg
Copy link
Contributor Author

hannojg commented Oct 9, 2024

Hey, any chance to get this one reviewed? Is there any way how i can help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants