Refactor VR math off of GVR types, onto gfx types where possible.

Refactor VR math off of GVR types, onto gfx types where possible.

This CL removes as much reliance on GVR types as possible, using gfx types where feasible, allowing us to reuse gfx math code.

BUG=

Review-Url: https://codereview.chromium.org/2814443004
Cr-Commit-Position: refs/heads/master@{#463758}
Committed: https://chromium.googlesource.com/chromium/src/+/169bf5e0b403527a459c95bac5762a8190851589

[mthiesse, 2017-04-11 00:00:37]

Patchset #1 (id:1) has been deleted

[mthiesse, 2017-04-11 00:04:07]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, bshe@chromium.org

[mthiesse, 2017-04-11 00:04:07]

PTAL. No logic was changed. I've tried to verify everything still works and that
there were no typos, and things seem good.

[mthiesse, 2017-04-11 00:06:03]

Description was changed from

==========
Refactor VR math off of GVR types, onto gfx types where possible.

This CL removes as much reliance on GVR types as possible, using gfx types where
feasible, allowing us to reuse gfx math code.

BUG=
==========

to

==========
Refactor VR math off of GVR types, onto gfx types where possible.

This CL removes as much reliance on GVR types as possible, using gfx types where
feasible, allowing us to reuse gfx math code.

BUG=
==========

[mthiesse, 2017-04-11 00:06:03]

mthiesse@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-04-11 13:44:40]

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.h (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.h:58: void GetTransform(vr::Matf*
out) const;
Why not continue to return?

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1123: vr::QuatToMatrix(rot,
&face_transform);
Very curious about the switch away from returned values. :)

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h
File device/vr/vr_math.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:5: #ifndef DEVICE_VR_DEVICE_VR_MATH_H_
DEVICE_VR_VR_MATH_H_ (other files as well).

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:27: Matf* out);
Here we're returning a new matrix.  Possibly just return it instead for
consistence with MatrixVectorMul?

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:31: Matf* out);
Same here.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:38: void DEVICE_VR_EXPORT QuatToMatrix(const Quatf& quat,
Matf* out);
And here.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h
File device/vr/vr_types.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h#ne...
device/vr/vr_types.h:5: #ifndef DEVICE_VR_DEVICE_VR_TYPES_H_
DEVICE_VR_VR_TYPES_H_

[cjgrant, 2017-04-11 14:07:11]

On 2017/04/11 13:44:40, cjgrant wrote:
>
https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_controller.h (right):
> 
>
https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_controller.h:58: void
GetTransform(vr::Matf*
> out) const;
> Why not continue to return?
> 
>
https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:1123: vr::QuatToMatrix(rot,
> &face_transform);
> Very curious about the switch away from returned values. :)
> 
> https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h
> File device/vr/vr_math.h (right):
> 
>
https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
> device/vr/vr_math.h:5: #ifndef DEVICE_VR_DEVICE_VR_MATH_H_
> DEVICE_VR_VR_MATH_H_ (other files as well).
> 
>
https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
> device/vr/vr_math.h:27: Matf* out);
> Here we're returning a new matrix.  Possibly just return it instead for
> consistence with MatrixVectorMul?
> 
>
https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
> device/vr/vr_math.h:31: Matf* out);
> Same here.
> 
>
https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
> device/vr/vr_math.h:38: void DEVICE_VR_EXPORT QuatToMatrix(const Quatf& quat,
> Matf* out);
> And here.
> 
> https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h
> File device/vr/vr_types.h (right):
> 
>
https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h#ne...
> device/vr/vr_types.h:5: #ifndef DEVICE_VR_DEVICE_VR_TYPES_H_
> DEVICE_VR_VR_TYPES_H_

BTW, sorry that the same comment occurs 3 or 4 times - the same answer might
address most of them. :)

[mthiesse, 2017-04-11 14:46:44]

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.h (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.h:58: void GetTransform(vr::Matf*
out) const;
On 2017/04/11 13:44:40, cjgrant wrote:
> Why not continue to return?

As discussed offline, for consistency we're always taking matrices as
parameters. It allows optimizations for some functions. Also gvr used a struct
with an internal c-style array so that it could be returned from functions. If
we ever changed our matrix type to c-style arrays as well we wouldn't be able to
return them from functions.

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1123: vr::QuatToMatrix(rot,
&face_transform);
On 2017/04/11 13:44:40, cjgrant wrote:
> Very curious about the switch away from returned values. :)

Cleaned this up by doing the copy internally to MatrixMul

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h
File device/vr/vr_math.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:5: #ifndef DEVICE_VR_DEVICE_VR_MATH_H_
On 2017/04/11 13:44:40, cjgrant wrote:
> DEVICE_VR_VR_MATH_H_ (other files as well).

Done.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:27: Matf* out);
On 2017/04/11 13:44:40, cjgrant wrote:
> Here we're returning a new matrix.  Possibly just return it instead for
> consistence with MatrixVectorMul?

Acknowledged.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:31: Matf* out);
On 2017/04/11 13:44:40, cjgrant wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_math.h#new...
device/vr/vr_math.h:38: void DEVICE_VR_EXPORT QuatToMatrix(const Quatf& quat,
Matf* out);
On 2017/04/11 13:44:40, cjgrant wrote:
> And here.

Acknowledged.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h
File device/vr/vr_types.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h#ne...
device/vr/vr_types.h:5: #ifndef DEVICE_VR_DEVICE_VR_TYPES_H_
On 2017/04/11 13:44:40, cjgrant wrote:
> DEVICE_VR_VR_TYPES_H_

Done.

[cjgrant, 2017-04-11 14:55:36]

lgtm

[mthiesse, 2017-04-11 14:57:07]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-04-11 14:57:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[acondor_, 2017-04-11 14:59:59]

acondor@google.com changed reviewers:
+ acondor@google.com

[acondor_, 2017-04-11 15:00:00]

I'm already late, but I have some comments.

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:69: const gfx::Vector3dF&
translation = vr::GetTranslation(transform_.to_world);
Why not return translation directly?

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:77:
vr::MatrixVectorMul(transform, gfx::Vector3dF(0, 0, 0));
vr::GetTranslation should be faster

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:79:
vr::MatrixVectorMul(transform, gfx::Vector3dF(1, 0, 0));
vr::MatrixVectorRotate should be faster

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_gl_util.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_util.cc:28: ScaleRect(texture_rect,
static_cast<float>(texture_size.width()),
does this come from gfx?

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_renderer.h (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.h:49: struct RectF {
why not use the gfx types here?

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h
File device/vr/vr_types.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h#ne...
device/vr/vr_types.h:19: using Matf = std::array<std::array<float, 4>, 4>;
I'd keep the 4 in the name. It makes it clearer that this is not a general
matrix.

[commit-bot: I haz the power, 2017-04-11 15:01:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 15:01:15]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bajones, 2017-04-11 16:33:44]

LGTM, though apparently it needs a rebase.

[mthiesse, 2017-04-11 19:01:02]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-04-11 19:01:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2814443004/#ps60001
(title: "ADdress comments + rebase")

[commit-bot: I haz the power, 2017-04-11 19:01:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mthiesse, 2017-04-11 19:21:06]

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:69: const gfx::Vector3dF&
translation = vr::GetTranslation(transform_.to_world);
On 2017/04/11 15:00:00, acondor wrote:
> Why not return translation directly?

Translation's a vector, and we want to return a point.

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:77:
vr::MatrixVectorMul(transform, gfx::Vector3dF(0, 0, 0));
On 2017/04/11 15:00:00, acondor wrote:
> vr::GetTranslation should be faster

Added a TODO to do this in a followup.

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:79:
vr::MatrixVectorMul(transform, gfx::Vector3dF(1, 0, 0));
On 2017/04/11 15:00:00, acondor wrote:
> vr::MatrixVectorRotate should be faster

Acknowledged.

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_gl_util.cc (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_util.cc:28: ScaleRect(texture_rect,
static_cast<float>(texture_size.width()),
On 2017/04/11 15:00:00, acondor wrote:
> does this come from gfx?

yes

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_renderer.h (right):

https://codereview.chromium.org/2814443004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.h:49: struct RectF {
On 2017/04/11 15:00:00, acondor wrote:
> why not use the gfx types here?

Because we can guarantee the memory order for these types, which makes it easy
to copy them to gpu memory.

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h
File device/vr/vr_types.h (right):

https://codereview.chromium.org/2814443004/diff/20001/device/vr/vr_types.h#ne...
device/vr/vr_types.h:19: using Matf = std::array<std::array<float, 4>, 4>;
On 2017/04/11 15:00:00, acondor wrote:
> I'd keep the 4 in the name. It makes it clearer that this is not a general
> matrix. 

Done.

[mthiesse, 2017-04-11 19:31:34]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-04-11 19:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2814443004/#ps80001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-04-11 19:32:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 20:46:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491939094606320,
"parent_rev": "5795cf77e5bcc458a5768831377a3c672b725136", "commit_rev":
"169bf5e0b403527a459c95bac5762a8190851589"}

[commit-bot: I haz the power, 2017-04-11 20:47:10]

Description was changed from

==========
Refactor VR math off of GVR types, onto gfx types where possible.

This CL removes as much reliance on GVR types as possible, using gfx types where
feasible, allowing us to reuse gfx math code.

BUG=
==========

to

==========
Refactor VR math off of GVR types, onto gfx types where possible.

This CL removes as much reliance on GVR types as possible, using gfx types where
feasible, allowing us to reuse gfx math code.

BUG=

Review-Url: https://codereview.chromium.org/2814443004
Cr-Commit-Position: refs/heads/master@{#463758}
Committed:
https://chromium.googlesource.com/chromium/src/+/169bf5e0b403527a459c95bac576...
==========

[commit-bot: I haz the power, 2017-04-11 20:47:12]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/169bf5e0b403527a459c95bac576...