TransformationMatrix::Recompose() and Decompose() incorrectly transpose rotation. #17162
Conversation
mattwoodrow
added
the
Animations
|
EWS run on current version of this PR (hash 9a4b128)
|
It shouldn't cause any functional changes, since we computed the backwards quaternion in decompose, but then accumulate/slerp and RotateTransformOperation::blend all took that into account when using it. This is just cleanup and removal of TODO's, but will make it easier to make future changes (which I have a few of locally). |
|
The larger blink change did this cleanup, but also did extra cleanup to their 'Quaternion' class (which we don't have), and then used all of that to fix some actual bugs with interpolation (covered by WPT). I intend to add a Quaternion class as a followup, and then finally fix the same bugs. |
| @@ -419,103 +419,131 @@ static bool decompose4(const TransformationMatrix::Matrix4& mat, TransformationM | |||
| result.translateZ = localMatrix[3][2]; | |||
| localMatrix[3][2] = 0; | |||
|
|
|||
| // Vector4 type and functions need to be added to the common set. | |||
| Vector3 row[3], pdum3; | |||
| // Note: Deviating from the spec in terms of variable naming. The matrix is | |||
There was a problem hiding this comment.
Should we file a PR on the spec?
There was a problem hiding this comment.
…se rotation. https://bugs.webkit.org/show_bug.cgi?id=220856 <rdar://problem/73747851> Reviewed by Dean Jackson. The computation of quaternions during matrix decomposition appeared to be based off of the matrix transpose. Indexing in the quaternion calculation is consistent with row-major ordering; however, the "rows" variable is actually column major order (goes back to the original implementation of unmatrix in Graphics Gems II). The rotation matrix constructed from the quaternions also appeared to be assuming row-major ordering. These discrepancies were mostly corrected for by flipping the direction of the quaternion when extracting from the matrix. This is a (partial) cherry-pick from Blink: - https://chromium-review.googlesource.com/c/chromium/src/+/2489727 * Source/WebCore/platform/graphics/transforms/RotateTransformOperation.cpp: (WebCore::RotateTransformOperation::blend): * Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp: (WebCore::decompose4): (WebCore::slerp): (WebCore::accumulateQuaternion): (WebCore::TransformationMatrix::recompose4): Canonical link: https://commits.webkit.org/267424@main
webkit-commit-queue
force-pushed
the
eng/transpose-decompose
branch
from
9a4b128
to
a959acf
Compare
|
Committed 267424@main (a959acf): https://commits.webkit.org/267424@main Reviewed commits have been landed. Closing PR #17162 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
a959acf
9a4b128