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

Animate morph targets by name #4327

Merged
merged 15 commits into from
Jun 21, 2022
Merged

Animate morph targets by name #4327

merged 15 commits into from
Jun 21, 2022

Conversation

ellthompson
Copy link
Contributor

@ellthompson ellthompson commented Jun 14, 2022

Fixes #4314

Currently, all the morph targets that animate on a given node are included in a single AnimCurve, then referenced by their indices in the curves output data. This poses problems when morph targets are deleted from either a model asset or it's related animation asset, as the indices of each morph target in the curve will no longer match up with those found in the mesh's MorphInstance.

In this PR the glb parser instead creates a single curve for each morph target, which contains the morph target's name in the curve path. When the anim binder applies the value of each curve to their bound properties, it can then reference this morph target name to set the appropriate morph target on the MorphInstance. To support this, the MorphInstance class now contains a lookup table of morph names and allows the setting of morphTarget values using this lookup. Including individual curves for each morph target has the added benefit of making debugging much simpler as their keyframe output data is now stored in sequential order with a reference to the given morph targets name.

If morph target names are not present in the glb data, the index of each morph target is used as a fallback.

I have tested this on a set of model & animation assets which have had their morph target orders randomised.
Below you can see this animation playing on the current stable branch of the engine:
indexed-morph-targets
And here it is on this branch, making use of the morph target names to bind correctly:
named-morph-targets

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@ellthompson ellthompson marked this pull request as ready for review June 14, 2022 15:06
@ellthompson ellthompson requested a review from a team June 14, 2022 15:36
ellthompson and others added 4 commits June 15, 2022 10:09
Co-authored-by: Will Eastcott <will@playcanvas.com>
Co-authored-by: Will Eastcott <will@playcanvas.com>
} else {
index = key;
}
if (Number.isFinite(this._weights[key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what are you testing here? Existing weight? What if key which you use as index to an array is string .. what do you expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry this was a bug from the refactor. Weights should be retrieved using the index instead of the key. Then if we cant retrieve a number from the weights array, we shouldn't be setting a new value or flagging the MorphInstance as dirty.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.
@slimbuck - would you mind to give the binder / parser part a quick look as well, you understand that code more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: animation Animation related issue area: gltf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Match animations to blend shapes using name instead of index when available
4 participants