Avoid duplicate math when computing VR scene hierarchy.

Avoid duplicate math when computing VR scene hierarchy.

When computing the transformations of UI elements in the scene, re-use
the calculations done by parents when computing child element
transforms.

BUG=680253
Review-Url: https://codereview.chromium.org/2777633003
Cr-Commit-Position: refs/heads/master@{#459807}
Committed: https://chromium.googlesource.com/chromium/src/+/5513e50629e5cb3d2bea187c6c8eaab7922c5fdf

[cjgrant, 2017-03-24 20:44:56]

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

[cjgrant, 2017-03-24 20:44:57]

The Friday afternoon technical debt burn-down break.

[tiborg, 2017-03-24 21:58:37]

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:89: Transform*
GetInheritableTransform() { return &inheritable_transform_; }
Do we need this? inheritable_transform_ is public anyways.

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:160: Transform
inheritable_transform_;
Remove trailing _?

[cjgrant, 2017-03-27 14:07:52]

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:89: Transform*
GetInheritableTransform() { return &inheritable_transform_; }
On 2017/03/24 21:58:37, tiborg wrote:
> Do we need this? inheritable_transform_ is public anyways.

My motivation:  This object started as a struct because it basically just stored
some coordinates and a transform.  We've since added a bunch of methods on it,
and have started using the methods for rendering.  I'm trying to set this thing
up to be switched to a class, but it should probably be an all-or-nothing move. 
I'll switch back.

[tiborg, 2017-03-27 14:30:18]

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:89: Transform*
GetInheritableTransform() { return &inheritable_transform_; }
On 2017/03/27 14:07:52, cjgrant wrote:
> On 2017/03/24 21:58:37, tiborg wrote:
> > Do we need this? inheritable_transform_ is public anyways.
> 
> My motivation:  This object started as a struct because it basically just
stored
> some coordinates and a transform.  We've since added a bunch of methods on it,
> and have started using the methods for rendering.  I'm trying to set this
thing
> up to be switched to a class, but it should probably be an all-or-nothing
move. 
> I'll switch back.

I think we can also make a gradual switch (even though I also prefer the
all-or-nothing solution). I would then make the variable private though so that
it is clear to only use the getter and setter.

[cjgrant, 2017-03-27 15:20:16]

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:89: Transform*
GetInheritableTransform() { return &inheritable_transform_; }
On 2017/03/27 14:30:18, tiborg wrote:
> On 2017/03/27 14:07:52, cjgrant wrote:
> > On 2017/03/24 21:58:37, tiborg wrote:
> > > Do we need this? inheritable_transform_ is public anyways.
> > 
> > My motivation:  This object started as a struct because it basically just
> stored
> > some coordinates and a transform.  We've since added a bunch of methods on
it,
> > and have started using the methods for rendering.  I'm trying to set this
> thing
> > up to be switched to a class, but it should probably be an all-or-nothing
> move. 
> > I'll switch back.
> 
> I think we can also make a gradual switch (even though I also prefer the
> all-or-nothing solution). I would then make the variable private though so
that
> it is clear to only use the getter and setter.

Done. Lets go all-or-nothing here.

https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:160: Transform
inheritable_transform_;
On 2017/03/24 21:58:37, tiborg wrote:
> Remove trailing _?

Done.

[tiborg, 2017-03-27 15:29:26]

On 2017/03/27 15:20:16, cjgrant wrote:
>
https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
> File chrome/browser/android/vr_shell/ui_elements.h (right):
> 
>
https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/ui_elements.h:89: Transform*
> GetInheritableTransform() { return &inheritable_transform_; }
> On 2017/03/27 14:30:18, tiborg wrote:
> > On 2017/03/27 14:07:52, cjgrant wrote:
> > > On 2017/03/24 21:58:37, tiborg wrote:
> > > > Do we need this? inheritable_transform_ is public anyways.
> > > 
> > > My motivation:  This object started as a struct because it basically just
> > stored
> > > some coordinates and a transform.  We've since added a bunch of methods on
> it,
> > > and have started using the methods for rendering.  I'm trying to set this
> > thing
> > > up to be switched to a class, but it should probably be an all-or-nothing
> > move. 
> > > I'll switch back.
> > 
> > I think we can also make a gradual switch (even though I also prefer the
> > all-or-nothing solution). I would then make the variable private though so
> that
> > it is clear to only use the getter and setter.
> 
> Done. Lets go all-or-nothing here.
> 
>
https://codereview.chromium.org/2777633003/diff/1/chrome/browser/android/vr_s...
> chrome/browser/android/vr_shell/ui_elements.h:160: Transform
> inheritable_transform_;
> On 2017/03/24 21:58:37, tiborg wrote:
> > Remove trailing _?
> 
> Done.

lgtm

[mthiesse, 2017-03-27 15:31:16]

lgtm

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

https://codereview.chromium.org/2777633003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:62: Transform* GetTransform() {
return &transform_; }
nit: maybe use MutableTransform()? Correct style is probably actually
mutable_transform(), using lowercase is encouraged for simple getters/setters.

[cjgrant, 2017-03-27 15:58:25]

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

https://codereview.chromium.org/2777633003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:62: Transform* GetTransform() {
return &transform_; }
On 2017/03/27 15:31:16, mthiesse wrote:
> nit: maybe use MutableTransform()? Correct style is probably actually
> mutable_transform(), using lowercase is encouraged for simple getters/setters.

Done.  Good call on that!

[cjgrant, 2017-03-27 15:58:28]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-03-27 15:58:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, tiborg@chromium.org
Link to the patchset: https://codereview.chromium.org/2777633003/#ps40001
(title: "s/GetTransform()/mutable_transform()/")

[commit-bot: I haz the power, 2017-03-27 15:58:57]

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-03-27 17:09:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490630308660900,
"parent_rev": "6ff8b957444977d427d4c1cde49a409e561ed01f", "commit_rev":
"5513e50629e5cb3d2bea187c6c8eaab7922c5fdf"}

[commit-bot: I haz the power, 2017-03-27 17:09:54]

Description was changed from

==========
Avoid duplicate math when computing VR scene hierarchy.

When computing the transformations of UI elements in the scene, re-use
the calculations done by parents when computing child element
transforms.

BUG=680253
==========

to

==========
Avoid duplicate math when computing VR scene hierarchy.

When computing the transformations of UI elements in the scene, re-use
the calculations done by parents when computing child element
transforms.

BUG=680253

Review-Url: https://codereview.chromium.org/2777633003
Cr-Commit-Position: refs/heads/master@{#459807}
Committed:
https://chromium.googlesource.com/chromium/src/+/5513e50629e5cb3d2bea187c6c8e...
==========

[commit-bot: I haz the power, 2017-03-27 17:09:55]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5513e50629e5cb3d2bea187c6c8e...