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

[Breaking][Anim Component] Layer blending update #4064

Merged
merged 12 commits into from
Mar 28, 2022
Merged

Conversation

ellthompson
Copy link
Contributor

This PR adds the option to toggle the normalisation of layer weights on or off.

If weight normalisation is turned on the algorithm functionality is the same as before:

  • Weights of all layers sum up to 1 (normalised by total weight)
  • Overwrite layers set all previous layer weights to 0
  • The resulting animation is a linear combination of all layer values and their weights

If weight normalisation is turned off the following effects take place:

  • Weight values are not normalised by their total weight
  • Layer weights are clamped between the 0 and 1 ranges
  • Additive layers add their values on top of previous layer values based on their weight value
  • Overwrite layers replace a portion of previous layer values based on their weight value
  • Overwrite layers no longer set the weights of previous layers to 0

The default setting going forward will be not to normalize weights. As the anim component will no longer use the previous blending algorithm as the default, this PR becomes a breaking change. If users wish to use the old blending algorithm, they will be required to set the normalizeWeights property to true.

This PR creates new public API:

/**
 * Blend from the current weight value to the provided weight value over a given amount of time.
 *
 * @param {number} weight - The new weight value to blend to.
 * @param {number} time - The duration of the blend in seconds.
 */
pc.AnimComponentLayer#blendToWeight

/**
 * If true the animation component will normalize the weights of its layers by their sum total.
 *
 * @type {boolean}
 */
pc.AnimComponent#normalizeWeights

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

@ellthompson ellthompson added enhancement area: animation Animation related issue labels Feb 28, 2022
@ellthompson ellthompson self-assigned this Feb 28, 2022
@mvaligursky
Copy link
Contributor

The default setting going forward will be not to normalize weights. As the anim component will no longer use the previous blending algorithm as the default, this PR becomes a breaking change. If users wish to use the old blending algorithm, they will be required to set the normalizeWeights property to true.

I wonder if we could handle this as non-breaking change? If normalizeWeights is undefined (existing assets), we should assume it's true. For new assets this will be initialized to false.

@ellthompson ellthompson marked this pull request as ready for review March 3, 2022 10:59
@ellthompson ellthompson requested a review from a team March 3, 2022 10:59
@mvaligursky mvaligursky changed the title [Anim Component] Layer blending update [Breaking][Anim Component] Layer blending update Mar 4, 2022
@mvaligursky mvaligursky added the release: next minor Ticket marked for the next minor release label Mar 24, 2022
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 now, I'm approving it.

But before the merge, I would just want to double check this is a breaking change and cannot be done as non-breaking. I think you explained it cannot, but please consider it one more time.

@ellthompson ellthompson merged commit 43df09a into main Mar 28, 2022
@ellthompson ellthompson deleted the layer-blending branch March 28, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: animation Animation related issue release: next minor Ticket marked for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants