1650714 - Correct Matrix Decompose and SetRotationFromQuaternion

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Daosheng Mu[:daoshengmu]]

Per Bug 1635363, our current result from Decompose() and SetRotationFromQuaternion() doesn't match.

 gfx::Matrix4x4Double transformMatrix;
 transformMatrix.RotateX(0.785f);   // 45 degree.
 transformMatrix.Decompose(trans, orient, scale);   // orient: (x: -0.38, y:0, z:0, w: 0.92)

 gfx::Matrix4x4Double transformMatrix1;
 transformMatrix1.SetRotationFromQuaternion(orient);
 transformMatrix1.Decompose(trans, orient, scale);   // orient: (x: 0.38, y:0, z:0, w: 0.92)

The correct result orient should be (x: 0.38, y:0, z:0, w: 0.92).

I have rewritten it according to Blender's source code [1][2] (Chromium's as ours, so they did a workaround as I mentioned at Bug 1635363).

After that

gfx::Matrix4x4Double transformMatrix;
transformMatrix.RotateX(0.785f);   // 45 degree.
transformMatrix.Decompose(trans, orient, scale);   // orient: (x: 0.38, y:0, z:0, w: 0.92)

gfx::Matrix4x4Double transformMatrix1;
transformMatrix1.SetRotationFromQuaternion(orient);
transformMatrix1.Decompose(trans, orient, scale);   // orient: (x: 0.38, y:0, z:0, w: 0.92)

The orient here be matched now, and we can remove all our invert() workaround code not only in viewerpose but also in XRInputSource. I also checked Babylon.js [3] work properly after this change.

However, we still need to refine our viewpose matrix. It seems like we did wrong when applying translate transform. In addition, I haven't verify it with WebVR sites, we need to remove other invert() in Gamepad code [4] at least.

[1] https://github.com/blender/blender/blob/b0da78084bd73fd1bec25570da12515f127f1fd1/source/blender/blenlib/intern/math_rotation.c#L232
[2] https://github.com/blender/blender/blob/b0da78084bd73fd1bec25570da12515f127f1fd1/source/blender/blenlib/intern/math_rotation.c#L322
[3] https://github.com/MozillaReality/FirefoxReality/issues/3578
[4] https://searchfox.org/mozilla-central/rev/c86c19bd64f8f19590a4190c282781d3a9631422/gfx/vr/VRDisplayClient.cpp#171

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1650714 - Part 1: Correct Matrix Decompose and SetRotationFromQuaternion.

Comment 2

[Imanol Fernandez]

Attachment: Bug 1650714 - Part 2: Correct XRRigidTransform inverse math

Comment 3

[Daosheng Mu[:daoshengmu]]

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdab330a52b610f8ff5bf735e40cf3d158d880da. Let me see if we are able to add tests to avoid regressions.

Comment 4

[Daosheng Mu[:daoshengmu]]

Checked PlayCanvas also works properly.

Comment 5

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1650714 - Part 3: Add Matrix rotate transform unit tests.

Depends on D82391

Comment 6

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1650714 - Part 3: Remove quaternion invert from OpenVRSession after refining Matrix rotate func.

Comment 7

[Daosheng Mu[:daoshengmu]]

Try looks good after adding a matrix unit test. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c75214676ec245bb1f33543471ead2d608fe1af8

We also can run this test via ./mach gtest Matrix.RotateTransform

Comment 8

[Daosheng Mu[:daoshengmu]]

Per Comment 6, I am trying to verify if it has no regression on OpenVRSession, but the current FF Windows build from m-c is only able to show a blank page and chrome UI. I will check it again once m-c resolve this issue.

Comment 9

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1650714 - Part 4: Adjust OpenVR and OculusVR rotate transform.

Depends on D83008

Comment 10

[Daosheng Mu[:daoshengmu]]

OculusVR and OpenVR are checked.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c68259c8d81
Part 1: Correct Matrix Decompose and SetRotationFromQuaternion. r=kip,imanol,lsalzman
https://hg.mozilla.org/integration/autoland/rev/16d11232b22a
Part 2: Correct XRRigidTransform inverse math r=daoshengmu,kip,lsalzman
https://hg.mozilla.org/integration/autoland/rev/687fd0ab5bbc
Part 3: Add Matrix rotate transform unit tests. r=kip,lsalzman
https://hg.mozilla.org/integration/autoland/rev/f6618ced82a8
Part 4: Adjust OpenVR and OculusVR rotate transform. r=kip

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3c68259c8d81
https://hg.mozilla.org/mozilla-central/rev/16d11232b22a
https://hg.mozilla.org/mozilla-central/rev/687fd0ab5bbc
https://hg.mozilla.org/mozilla-central/rev/f6618ced82a8

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 9161628 [details]
Bug 1650714 - Part 1: Correct Matrix Decompose and SetRotationFromQuaternion.

Beta/Release Uplift Approval Request

• User impact if declined: We implemented wrong math in matrix rotate, we need to fix it as soon as we can in order to avoid affect our WebVR/XR content users. It will cause us lose our users if we deliver wrong matrix transform results. (Please uplift patches all of part 1 to part 4)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is not risky. We only use matrix decompose() and SetRotationFromQuaternion() in WebVR/XR modules, and we have verified all possible content work properly in our team internally. In addition, we also added an unit test for matrix rotate transform that is run by g-test (in part 3). Therefore, I think we should be able to uplift it to beta.
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161628 [details]
Bug 1650714 - Part 1: Correct Matrix Decompose and SetRotationFromQuaternion.

Approved for 79.0b8/GV79. Thanks for including tests.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4bcf353d839f
https://hg.mozilla.org/releases/mozilla-beta/rev/05cdc7620607
https://hg.mozilla.org/releases/mozilla-beta/rev/892cc0aa2171
https://hg.mozilla.org/releases/mozilla-beta/rev/109e35518744