VR: System indicator rework.

VR: System indicator rework.

Refactoring to make the indicators easier and cleaner to manage:

- Use a single parameterized class for all indicators.
- Change how indicator textures are drawn, such that we only need to
  specify height, and the widths are computed.
- In general, allow textured elements to override how their world size is
  computed (rather than forcing all to compute height based on width).
- Dynamically position the indicators according to which are showing.

BUG=

Review-Url: https://codereview.chromium.org/2950173002
Cr-Commit-Position: refs/heads/master@{#481696}
Committed: https://chromium.googlesource.com/chromium/src/+/9f6167b0bd426ff61d54f54be51066873818c8b5

[cjgrant, 2017-06-21 21:12:10]

cjgrant@chromium.org changed reviewers:
+ asimjour@chromium.org, bshe@chromium.org

[cjgrant, 2017-06-21 21:12:10]

Amir, here's the indicator position and file cleanup.  Biao, now that you are
immersed in textures, feel free to comment.

Note that the system by which texture dimensions and element sizes relate is
pretty fragile.  This takes a small step in the right direction I think, but not
a big one.

[asimjour1, 2017-06-22 14:46:00]

Please modify the commit msg and add BUG=732812

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:227: 512, kIndicatorHeight,
indicator.icon, indicator.resource_string);
I'm not sure if I get this part. Do we use 512 for all textures? I think we
should add this number to struct Indicator.

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.h:63: void OnGLInitialized()
override;
Is this the only instance that VrShellGl calling out to UI?

[cjgrant, 2017-06-22 15:51:40]

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:227: 512, kIndicatorHeight,
indicator.icon, indicator.resource_string);
On 2017/06/22 14:46:00, asimjour1 wrote:
> I'm not sure if I get this part. Do we use 512 for all textures? I think we
> should add this number to struct Indicator.

We use 512 for many textures, but the number should be based on how large the
element will actually appear in the scene.  For these indicators however, the
texture actually specifies narrower textures for icon-only elements.  This lets
each indicator choose its ideal size, based on a consistent maximum size.  If
anything, I would change the meaning to be the height in pixels, rather than
width, and replace the meters-based height I'm passing in.  What do you think of
that?

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_thread.h:63: void OnGLInitialized()
override;
On 2017/06/22 14:46:00, asimjour1 wrote:
> Is this the only instance that VrShellGl calling out to UI?

No, the other GL to UI calls already exist:

AppButtonGesturePerformed()
AppButtonClicked()

This interface is basically GL calling out to "anything else", and hides (from
GL) who or what handles the call.

[amp, 2017-06-22 18:34:42]

amp@chromium.org changed reviewers:
+ amp@chromium.org

[amp, 2017-06-22 18:34:43]

naming nit

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:72: static constexpr float
kIndicatorHeight = 0.0625;
Does this include an implicit distance in it already?  Kind of related to my
question about ZOffset below.

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:75: static constexpr float
kIndicatorZOffset = 0.1;
We call it Distance everywhere else.  Switching to ZOffset threw me off.

If it varies depending on number of elements I guess it's fine, but it looks
like all of them use the same value.

I like the changes to the other values though as it makes it clearer that they
are relative rather than absolute values.

[cjgrant, 2017-06-22 19:16:33]

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:75: static constexpr float
kIndicatorZOffset = 0.1;
On 2017/06/22 18:34:43, amp wrote:
> We call it Distance everywhere else.  Switching to ZOffset threw me off.
> 
> If it varies depending on number of elements I guess it's fine, but it looks
> like all of them use the same value.
> 
> I like the changes to the other values though as it makes it clearer that they
> are relative rather than absolute values.

I'm not sure what actions are being suggested here. :)  Our distance constants
specify absolute Z distance, but these offsets are relative to the parent object
(the content quad).  It seems like calling them distance would be misleading.  

What do you mean about varying with the number of elements?  The Y and Z apply
to all the indicators, and X position gets computed based on the number of
indicators, and the gap between them.

Let me know if I'm missing something!

[asimjour1, 2017-06-22 19:18:36]

lgtm with nit

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:227: 512, kIndicatorHeight,
indicator.icon, indicator.resource_string);
On 2017/06/22 15:51:39, cjgrant wrote:
> On 2017/06/22 14:46:00, asimjour1 wrote:
> > I'm not sure if I get this part. Do we use 512 for all textures? I think we
> > should add this number to struct Indicator.
> 
> We use 512 for many textures, but the number should be based on how large the
> element will actually appear in the scene.  For these indicators however, the
> texture actually specifies narrower textures for icon-only elements.  This
lets
> each indicator choose its ideal size, based on a consistent maximum size.  If
> anything, I would change the meaning to be the height in pixels, rather than
> width, and replace the meters-based height I'm passing in.  What do you think
of
> that?

Please use a constant for 512.

[amp, 2017-06-22 20:36:23]

lgtm

I'm just bikeshedding at this point, feel free to ignore the nits in naming. :)

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:75: static constexpr float
kIndicatorZOffset = 0.1;
On 2017/06/22 19:16:33, cjgrant wrote:
> On 2017/06/22 18:34:43, amp wrote:
> > We call it Distance everywhere else.  Switching to ZOffset threw me off.
> > 
> > If it varies depending on number of elements I guess it's fine, but it looks
> > like all of them use the same value.
> > 
> > I like the changes to the other values though as it makes it clearer that
they
> > are relative rather than absolute values.
> 
> I'm not sure what actions are being suggested here. :)  Our distance constants
> specify absolute Z distance, but these offsets are relative to the parent
object
> (the content quad).  It seems like calling them distance would be misleading. 

> 
> What do you mean about varying with the number of elements?  The Y and Z apply
> to all the indicators, and X position gets computed based on the number of
> indicators, and the gap between them.
> 
> Let me know if I'm missing something!

Thanks for the explanation.  I was used to seeing 'distance' and other word
based dimensions, and Z offset didn't immediately make sense.  Using it as an
offset from the anchor element sounds like a good use, although maybe having
'anchor[dimension]Offset' as the name would have prevented my confusion.  Still,
we don't use Z anywhere else for the constants.  It's not a big deal, but maybe
we should use distanceOffset (or convert the 'distance' and 'width' etc names to
use something like Z, X, etc).  

My varying by number comment was just referring to that computation of the X
position that you mention.

For the height comment, how do dmm values connect in with dynamically (anchored
to a different element) calculated distances?  When the content gets moved back
for fullscreen mode should the height/size of the indicators change as well? 
(I'm not sure if we have decided if the indicators show up in fullscreen, but
asking the question assuming they do).

[cjgrant, 2017-06-22 20:58:11]

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

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:75: static constexpr float
kIndicatorZOffset = 0.1;
On 2017/06/22 20:36:22, amp wrote:
> On 2017/06/22 19:16:33, cjgrant wrote:
> > On 2017/06/22 18:34:43, amp wrote:
> > > We call it Distance everywhere else.  Switching to ZOffset threw me off.
> > > 
> > > If it varies depending on number of elements I guess it's fine, but it
looks
> > > like all of them use the same value.
> > > 
> > > I like the changes to the other values though as it makes it clearer that
> they
> > > are relative rather than absolute values.
> > 
> > I'm not sure what actions are being suggested here. :)  Our distance
constants
> > specify absolute Z distance, but these offsets are relative to the parent
> object
> > (the content quad).  It seems like calling them distance would be
misleading. 
> 
> > 
> > What do you mean about varying with the number of elements?  The Y and Z
apply
> > to all the indicators, and X position gets computed based on the number of
> > indicators, and the gap between them.
> > 
> > Let me know if I'm missing something!
> 
> Thanks for the explanation.  I was used to seeing 'distance' and other word
> based dimensions, and Z offset didn't immediately make sense.  Using it as an
> offset from the anchor element sounds like a good use, although maybe having
> 'anchor[dimension]Offset' as the name would have prevented my confusion. 
Still,
> we don't use Z anywhere else for the constants.  It's not a big deal, but
maybe
> we should use distanceOffset (or convert the 'distance' and 'width' etc names
to
> use something like Z, X, etc).  
> 
> My varying by number comment was just referring to that computation of the X
> position that you mention.
> 
> For the height comment, how do dmm values connect in with dynamically
(anchored
> to a different element) calculated distances?  When the content gets moved
back
> for fullscreen mode should the height/size of the indicators change as well? 
> (I'm not sure if we have decided if the indicators show up in fullscreen, but
> asking the question assuming they do).

- I'll update the naming to Distance and Vertical to match others. SGTM.
- On DMMs, bottom line, we are broken.  Same as we assign static texture sizes
to all these elements instead of computing what the actual texture size should
be.  This is one big ball of post-BBB fun to solve. :)

https://codereview.chromium.org/2950173002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:227: 512, kIndicatorHeight,
indicator.icon, indicator.resource_string);
On 2017/06/22 19:18:35, asimjour1 wrote:
> On 2017/06/22 15:51:39, cjgrant wrote:
> > On 2017/06/22 14:46:00, asimjour1 wrote:
> > > I'm not sure if I get this part. Do we use 512 for all textures? I think
we
> > > should add this number to struct Indicator.
> > 
> > We use 512 for many textures, but the number should be based on how large
the
> > element will actually appear in the scene.  For these indicators however,
the
> > texture actually specifies narrower textures for icon-only elements.  This
> lets
> > each indicator choose its ideal size, based on a consistent maximum size. 
If
> > anything, I would change the meaning to be the height in pixels, rather than
> > width, and replace the meters-based height I'm passing in.  What do you
think
> of
> > that?
> 
> Please use a constant for 512.

As mentioned offline, all our sizes are hard-coded, because they're wrong and
need fixing.  Making this a constant furthers the illusion that it's okay. :)

[cjgrant, 2017-06-22 20:58:20]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-06-22 20:58:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
asimjour@chromium.org, amp@chromium.org
Link to the patchset: https://codereview.chromium.org/2950173002/#ps40001
(title: "Address comments; increase size slightly for readability, now that
position is dynamic.")

[commit-bot: I haz the power, 2017-06-22 20:58:49]

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-06-22 22:46:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498165100705000,
"parent_rev": "32a53aee550cd0a5472464b12a4fc1abbf0cdbdb", "commit_rev":
"9f6167b0bd426ff61d54f54be51066873818c8b5"}

[commit-bot: I haz the power, 2017-06-22 22:46:48]

Description was changed from

==========
VR: System indicator rework.

Refactoring to make the indicators easier and cleaner to manage:

- Use a single parameterized class for all indicators.
- Change how indicator textures are drawn, such that we only need to
  specify height, and the widths are computed.
- In general, allow textured elements to override how their world size is
  computed (rather than forcing all to compute height based on width).
- Dynamically position the indicators according to which are showing.

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

to

==========
VR: System indicator rework.

Refactoring to make the indicators easier and cleaner to manage:

- Use a single parameterized class for all indicators.
- Change how indicator textures are drawn, such that we only need to
  specify height, and the widths are computed.
- In general, allow textured elements to override how their world size is
  computed (rather than forcing all to compute height based on width).
- Dynamically position the indicators according to which are showing.

BUG=

Review-Url: https://codereview.chromium.org/2950173002
Cr-Commit-Position: refs/heads/master@{#481696}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f6167b0bd426ff61d54f54be510...
==========

[commit-bot: I haz the power, 2017-06-22 22:46:49]

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