Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770353002:
WebVR: add angular velocity estimate to pose (Closed)
DescriptionWebVR: add angular velocity estimate to pose
The WebVR spec supports angular velocity as an optional field. We're not
currently supplying that, but we need the data for motion-to-photon latency
testing.
See also https://github.com/w3c/webvr/issues/212 for a spec clarification
request. Currently, this is returning sensor-relative velocity, may need
to be transformed to a static reference frame.
BUG=705084
Review-Url: https://codereview.chromium.org/2770353002
Cr-Commit-Position: refs/heads/master@{#462331}
Committed: https://chromium.googlesource.com/chromium/src/+/3150789a49a47adba3ca3e625b67b5a2cbd3d282
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comment changes and reformatting as suggested #
Total comments: 2
Patch Set 3 : Enable linear accel, fix angular velocity to be in seated space #
Total comments: 6
Patch Set 4 : Review feedback, move constants to vr_shell.h #
Total comments: 3
Patch Set 5 : Rebase (no changes) #Patch Set 6 : Refactor to use new gvr_delegate base methods #Patch Set 7 : Remove unneeded vr_math include #
Total comments: 2
Patch Set 8 : Move angular velocity calculation to helper function #
Messages
Total messages: 29 (12 generated)
klausw@chromium.org changed reviewers: + bajones@chromium.org
See also the linked clarification request.
cjgrant@chromium.org changed reviewers: + cjgrant@chromium.org
https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:709: double epsilon_seconds = epsilon_nanos * 1e-9; Should protect against epsilon_nanos being 0 to avoid a possible div-by-zero if input is bad (on startup, shutdown, corner case, etc). https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.h:186: static device::mojom::VRPosePtr VRPosePtrFromGvrPose(gvr::Mat4f head_mat, Could you: - Document the function and what each matrix is - s/mat/matrix/ as per the style guide preference to not abbreviate https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:42: // Time offset used for calculating angular velocity from a pair nit: Most editors can be used to auto-format comment blocks. A few of the comments don't wrap properly. "git cl format" might also do it.
https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:709: double epsilon_seconds = epsilon_nanos * 1e-9; On 2017/03/24 21:15:40, cjgrant wrote: > Should protect against epsilon_nanos being 0 to avoid a possible div-by-zero if > input is bad (on startup, shutdown, corner case, etc). That can't happen accidentally since it's a compile-time constant we pick. Adding a comment to clarify. https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.h (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.h:186: static device::mojom::VRPosePtr VRPosePtrFromGvrPose(gvr::Mat4f head_mat, On 2017/03/24 21:15:40, cjgrant wrote: > Could you: > - Document the function and what each matrix is > - s/mat/matrix/ as per the style guide preference to not abbreviate Done, but only using "matrix" in the header for documentation purposes. For the .cc file I'd prefer to stick with the pre-existing "mat", especially since writing out matrix would make some lines excessively long. https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2770353002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:42: // Time offset used for calculating angular velocity from a pair On 2017/03/24 21:15:40, cjgrant wrote: > nit: Most editors can be used to auto-format comment blocks. A few of the > comments don't wrap properly. "git cl format" might also do it. I was using auto format, but Emacs for some reason defaults to 70 columns. Fixed.
billorr@chromium.org changed reviewers: + billorr@chromium.org
https://codereview.chromium.org/2770353002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:711: pose->angularVelocity.value()[0] = can you explain the math here in a comment? Looking at https://en.wikipedia.org/wiki/Rotation_matrix#Determining_the_axis, the formula you have isn't quite the same. We are basically trying to find the axis of the rotation part of (head_mat_2-head_mat), which is a rotation matrix (ignoring the translation component).
PTAL, some larger changes. bajones@, please clarify if the intent is indeed to report vectors in seated space, not sensor relative. Based on my updated interpretation in https://github.com/w3c/webvr/issues/212 and assuming that the Chromium/Vive build works as intended, I think the vectors are supposed to be in seated space. That wasn't the case for the pre-existing Daydream controller poses reporting angular velocity. I've updated the Daydream angular velocity to use seated space, added linear acceleration which was missing (it was returning constant [0, 0, 0], not null, which doesn't seem specs compliant), and also used seated space to report the headset's angular velocity. See https://klausw.github.io/webvr.info/samples/XX-vr-controllers.html along with my test build -seatedvec at the usual location to try it. I've compared to the Vive implementation which now seems equivalent. This is unfortunately an incompatible change for the Daydream controller, but that seems unavoidable if the old behavior wasn't specs compliant. Applications could distinguish the old code if needed by checking if the headset has an angular velocity, if yes it's the new interpretation. https://codereview.chromium.org/2770353002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:711: pose->angularVelocity.value()[0] = On 2017/03/24 22:52:51, billorr wrote: > can you explain the math here in a comment? Looking at > https://en.wikipedia.org/wiki/Rotation_matrix#Determining_the_axis, the formula > you have isn't quite the same. > > We are basically trying to find the axis of the rotation part of > (head_mat_2-head_mat), which is a rotation matrix (ignoring the translation > component). I'm using https://en.wikipedia.org/wiki/Angular_velocity#Calculation_from_the_orientati... , I added a link to that in a comment. My original calculation wasn't quite correct, it was missing a multiplication with the inverse of the first matrix. I added that, in addition to an additional rotation to transform it into seated space.
A few minor comments, but looks good overall. (non-committer LGTM) If we had a matrix library to use to do this with higher-level primitives that would be great. (decrease cognitive load to read the code later). The math part of this could be a good candidate for unit testing. https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:16: // TODO(klausw): These are duplicates from vr_shell_gl.cc, put in header file? any reason not to do this now? https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:112: // Use orientation to rotate acceleration/gyro into seated space. nit: Accelerometer measurement or acceleration? Is gravity included? https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:730: pose->angularVelocity.value()[0] = accel_vec.x; nit: velocity not accel?
One more question - do you think we may want to apply the change to the reported angular position to M58 too, assuming the current behavior isn't specs compliant? If yes, I can look into pulling this part out into a separate cherry-pickable CL.
> If we had a matrix library to use to do this with higher-level primitives that > would be great. (decrease cognitive load to read the code later). Sigh. We used to have matrix transpose in vr_math.h, but it got deleted in a cleanup due to being unused. I could restore it and add a rotation_delta function. (My code was just using the 3x3 rotation component, would need to make sure we don't run into issues if the poses include positions.) > The math part of this could be a good candidate for unit testing. I was careful to match the existing test coverage practices for this code ;-) I agree we should have this, but IMHO that would be better suited to a separate CL. https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:16: // TODO(klausw): These are duplicates from vr_shell_gl.cc, put in header file? On 2017/03/27 17:33:53, billorr wrote: > any reason not to do this now? Done - though it's annoying that it's now no longer possible to fit a simple "a += b" on a single line due to the added "VrShell::" prefix. https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:112: // Use orientation to rotate acceleration/gyro into seated space. On 2017/03/27 17:33:53, billorr wrote: > nit: Accelerometer measurement or acceleration? Is gravity included? Acceleration and gravity are the same thing (blame Einstein), so it does include (0, 9.81 m/s^2, 0) when used on Earth. https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2770353002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:730: pose->angularVelocity.value()[0] = accel_vec.x; On 2017/03/27 17:33:53, billorr wrote: > nit: velocity not accel? Indeed, done.
LGTM!
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1265: VrShell::kPredictionTimeWithoutVsyncNanos; Instead of moving this constant to the header, and using it in 4 different spots, can we add a static function that gets the predicted head pose with neck model? (Will be easier after my refactor in the cq lands (https://codereview.chromium.org/2787873002/))
https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1265: VrShell::kPredictionTimeWithoutVsyncNanos; On 2017/04/05 18:43:18, mthiesse wrote: > Instead of moving this constant to the header, and using it in 4 different > spots, can we add a static function that gets the predicted head pose with neck > model? > > (Will be easier after my refactor in the cq lands > (https://codereview.chromium.org/2787873002/)) Actually, don't worry about this for your CL. I'll do some refactoring in a followup.
On 2017/04/05 18:43:18, mthiesse wrote: > https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... > File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): > > https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... > chrome/browser/android/vr_shell/vr_shell_gl.cc:1265: > VrShell::kPredictionTimeWithoutVsyncNanos; > Instead of moving this constant to the header, and using it in 4 different > spots, can we add a static function that gets the predicted head pose with neck > model? > > (Will be easier after my refactor in the cq lands > (https://codereview.chromium.org/2787873002/)) OK, I'll wait for that to land and will redo it then.
mthiesse@: PTAL, I've rebased on top of your gvr_delegate refactor. https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2770353002/diff/60001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1265: VrShell::kPredictionTimeWithoutVsyncNanos; On 2017/04/05 18:57:00, mthiesse wrote: > On 2017/04/05 18:43:18, mthiesse wrote: > > Instead of moving this constant to the header, and using it in 4 different > > spots, can we add a static function that gets the predicted head pose with > neck > > model? > > > > (Will be easier after my refactor in the cq lands > > (https://codereview.chromium.org/2787873002/)) > > Actually, don't worry about this for your CL. I'll do some refactoring in a > followup. Done, now there's just one place that applies the neck model.
The CQ bit was checked by klausw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2770353002/diff/120001/device/vr/android/gvr/... File device/vr/android/gvr/gvr_delegate.cc (right): https://codereview.chromium.org/2770353002/diff/120001/device/vr/android/gvr/... device/vr/android/gvr/gvr_delegate.cc:120: // Add headset angular velocity to the pose. nit: Split this out into a separate function for readability?
https://codereview.chromium.org/2770353002/diff/120001/device/vr/android/gvr/... File device/vr/android/gvr/gvr_delegate.cc (right): https://codereview.chromium.org/2770353002/diff/120001/device/vr/android/gvr/... device/vr/android/gvr/gvr_delegate.cc:120: // Add headset angular velocity to the pose. On 2017/04/06 00:15:37, mthiesse wrote: > nit: Split this out into a separate function for readability? Done.
The CQ bit was checked by klausw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from billorr@chromium.org, bajones@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2770353002/#ps140001 (title: "Move angular velocity calculation to helper function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491442144940480,
"parent_rev": "d8c43f8e7d8c730a082122680008cdbf8b6ed43d", "commit_rev":
"3150789a49a47adba3ca3e625b67b5a2cbd3d282"}
Message was sent while issue was closed.
Description was changed from ========== WebVR: add angular velocity estimate to pose The WebVR spec supports angular velocity as an optional field. We're not currently supplying that, but we need the data for motion-to-photon latency testing. See also https://github.com/w3c/webvr/issues/212 for a spec clarification request. Currently, this is returning sensor-relative velocity, may need to be transformed to a static reference frame. BUG=705084 ========== to ========== WebVR: add angular velocity estimate to pose The WebVR spec supports angular velocity as an optional field. We're not currently supplying that, but we need the data for motion-to-photon latency testing. See also https://github.com/w3c/webvr/issues/212 for a spec clarification request. Currently, this is returning sensor-relative velocity, may need to be transformed to a static reference frame. BUG=705084 Review-Url: https://codereview.chromium.org/2770353002 Cr-Commit-Position: refs/heads/master@{#462331} Committed: https://chromium.googlesource.com/chromium/src/+/3150789a49a47adba3ca3e625b67... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/3150789a49a47adba3ca3e625b67... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
