[webkit-reviews] review granted: [Bug 236116] Web animations- Composite operation accumulation support for transform properties : [Attachment 451592] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 10 13:25:57 PST 2022
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 236116: Web animations- Composite operation accumulation support for
transform properties
https://bugs.webkit.org/show_bug.cgi?id=236116
Attachment 451592: Patch
https://bugs.webkit.org/attachment.cgi?id=451592&action=review
--- Comment #21 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 451592
--> https://bugs.webkit.org/attachment.cgi?id=451592
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=451592&action=review
> Source/WebCore/ChangeLog:19
> + This patch brings further support for accumulation on transform
properties. The main aspect being addressed
> + is handling accumulation of TransformationMatrices. This involves
accumulating on the decomposed functions,
> + which occurs in blendFloat() and accumulateQuaternion(). Since we
need to perform accumulation for one-based
> + values on certain decomposed functions, (scale, diagonals of
matrices, etc), we set the operation to accumulate
> + only for these cases.
> +
> + A couple of bugs introduced by this additional support are also
addressed. For a property with a
> + different transform operation than the keyframes given,
transformFunctionListsMatch() would return
> + true when it shouldn't. Addressed by checking in
blendByMatchingOperations and falling back on matrix
> + interpolation. Another bug is that we need to pass the composite
operation to TransformationMatrix blend().
> + Another bug is that we should to use Replace behavior if either of
the transform matrices are
> + non-invertible.
Much better!
> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:88
> + unsigned fromOperationCount = from.operations().size();
> + unsigned toOperationCount = operations().size();
> + unsigned maxOperationCount = std::max(fromOperationCount,
toOperationCount);
> +
> + if (context.compositeOperation == CompositeOperation::Accumulate &&
(!from.isInvertible(boxSize) || !isInvertible(boxSize)))
> + return blendByUsingMatrixInterpolation(from, context, boxSize);
> +
> + for (unsigned i = 0; i < maxOperationCount; i++) {
More readable now.
> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> + // When compositeOperation is accumulate, if the decomposed function is
a one based value (for affine matrix these properties are
> + // m11, m22, scaleX and scaleY), use one based accumulation. Otherwise,
use behavior for add.
> + auto operationForNonOneBasedValues = compositeOperation ==
CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation;
Nice. Maybe "one based" -> "1-based".
> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1737
> + accumulateQuaternion(&fromDecomp.quaternionX,
&toDecomp.quaternionX);
> + else
> + slerp(&fromDecomp.quaternionX, &toDecomp.quaternionX, progress);
Not new, but it's weird that slerp() and now accumulateQuaternion() assume
that's it's OK to cast from `double quaternionX, quaternionY, quaternionZ,
quaternionW;` in Decomposed4Type to double[4]. There's nothing to prevent the
compiler from adding padding between those doubles.
More information about the webkit-reviews
mailing list