Rendering Daydream controller in a fixed position.

Rendering Daydream controller in a fixed position.

- Model file is not included, as it will be delivered through component updater.
- UI continues to work as before when the model file is missing.
- Textures included for each button state.
- Conform to DD UX specs pending.
- Refactored rendering-classes inheritance to be able to generalize them to non quads.

BUG=644562
Review-Url: https://codereview.chromium.org/2775283004
Cr-Commit-Position: refs/heads/master@{#460625}
Committed: https://chromium.googlesource.com/chromium/src/+/e4a64eab960f6b5a25df0270b03e86311206d802

[acondor_, 2017-03-27 20:43:19]

acondor@google.com changed reviewers:
+ bajones@chromium.org, cjgrant@chromium.org, mthiesse@chromium.org

[acondor_, 2017-03-27 20:43:19]

The initial implementation for visualizing the controller is done. It is still
left to conform to the DD UX specs. Have a look, please.

[acondor_, 2017-03-27 21:25:58]

Description was changed from

==========
Rendering Daydream controller in a fixed position.

- Model file is not included, as it will be delivered through component updater.
- UI continues to work as before when the model file is missing.
- Textures included, but only IDLE texture is used.
- Conform to DD UX specs pending.
- Refactored rendering-classes inheritance to be able to generalize them to non
quads.

BUG=644562
==========

to

==========
Rendering Daydream controller in a fixed position.

- Model file is not included, as it will be delivered through component updater.
- UI continues to work as before when the model file is missing.
- Textures included for each button state.
- Conform to DD UX specs pending.
- Refactored rendering-classes inheritance to be able to generalize them to non
quads.

BUG=644562
==========

[acondor_, 2017-03-28 19:04:27]

Patchset #3 (id:40001) has been deleted

[mthiesse, 2017-03-28 20:26:23]

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_asset.h:113: void FreeBuffers();
I don't really like that we're clearing the buffer at some point in the future
and leaving this class in a weird state where it has metadata but no buffer. Can
we split metadata and buffer into separate classes easily? Then we could just
delete the buffer class and leave the metadata around without confusion.

[cjgrant, 2017-03-29 13:20:55]

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:87:
.Append(VrControllerModel::kDefaultVersion)
Formatting looks off here.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:92: if
(!base::PathExists(model_path))
Is this worth a warning-level print?

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:117: LOG(ERROR) <<
"LoadControllerModelTask: Failed to read texture";
Rather than including the function name, how about "Failed to read controller
texture"?

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1105: // TODO(acondor): Placing
controller in temporary position before
Maybe I'm missing something, but in addition to putting the controller where the
hand is, why not rotate the matrix according to the controller orientation as
well?

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1112: VrControllerModel::State
state = VrControllerModel::IDLE;
- Volume buttons?
- Can multiple buttons be pressed simultaneously?

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1224: controller_model_->Free();
Michael already commented on this Free() approach.  It doesn't look healthy to
me either.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:720: :
textured_quad_renderer_(base::MakeUnique<TexturedQuadRenderer>()),
Thank you. :)

[bajones, 2017-03-29 17:38:59]

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:35: const
gltf::BufferView* buffer_view =
The elements buffer and indices buffer are something that get looked up from the
table multiple times every frame. Since this class is for a very narrow use case
let's cache the BufferView once the model is loaded instead of looking it up
every time.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:108: auto it =
mesh->primitives[0]->attributes.find(key);
Same goes for the accessor, but even more so. Doing multiple string compares
each frame to find a static resource is something that should be avoided. Let's
cache the accessors in question so that PositionAccessor() and
TextureCoordinateAccessor() don't have to do any searches.

[acondor_, 2017-03-29 20:13:26]

Now, during setup, the renderer receives the ownership of the ControllerModel
and grabs all the metadata it needs.
I also added the simple rotation of the controller model before implementing the
elbow model.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_asset.h:113: void FreeBuffers();
On 2017/03/28 20:26:23, mthiesse wrote:
> I don't really like that we're clearing the buffer at some point in the future
> and leaving this class in a weird state where it has metadata but no buffer.
Can
> we split metadata and buffer into separate classes easily? Then we could just
> delete the buffer class and leave the metadata around without confusion.

Done.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:35: const
gltf::BufferView* buffer_view =
On 2017/03/29 17:38:58, bajones wrote:
> The elements buffer and indices buffer are something that get looked up from
the
> table multiple times every frame. Since this class is for a very narrow use
case
> let's cache the BufferView once the model is loaded instead of looking it up
> every time.

Actually, they are used just once (during SetUp call in the renderer).

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.cc:108: auto it =
mesh->primitives[0]->attributes.find(key);
On 2017/03/29 17:38:58, bajones wrote:
> Same goes for the accessor, but even more so. Doing multiple string compares
> each frame to find a static resource is something that should be avoided.
Let's
> cache the accessors in question so that PositionAccessor() and
> TextureCoordinateAccessor() don't have to do any searches.

No need with the new approach on the Renderer.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:87:
.Append(VrControllerModel::kDefaultVersion)
On 2017/03/29 13:20:54, cjgrant wrote:
> Formatting looks off here.

This is what cl format does, even if I try to change it.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:92: if
(!base::PathExists(model_path))
On 2017/03/29 13:20:54, cjgrant wrote:
> Is this worth a warning-level print?

Done.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:117: LOG(ERROR) <<
"LoadControllerModelTask: Failed to read texture";
On 2017/03/29 13:20:54, cjgrant wrote:
> Rather than including the function name, how about "Failed to read controller
> texture"?

Done.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1105: // TODO(acondor): Placing
controller in temporary position before
On 2017/03/29 13:20:54, cjgrant wrote:
> Maybe I'm missing something, but in addition to putting the controller where
the
> hand is, why not rotate the matrix according to the controller orientation as
> well?

Done. It will be changed to the elbow model soon, though.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1112: VrControllerModel::State
state = VrControllerModel::IDLE;
On 2017/03/29 13:20:54, cjgrant wrote:
> - Volume buttons?
> - Can multiple buttons be pressed simultaneously?

I'm not sure if they can, but the textures provided don't account for that. Here
I'm going in order of preference. The volume buttons are not even present in the
mesh and/or texture.

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1224: controller_model_->Free();
On 2017/03/29 13:20:54, cjgrant wrote:
> Michael already commented on this Free() approach.  It doesn't look healthy to
> me either.

The new approach is that the renderer gathers all the information it needs
during setup and then the object is automatically deleted.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:720: :
textured_quad_renderer_(base::MakeUnique<TexturedQuadRenderer>()),
On 2017/03/29 13:20:54, cjgrant wrote:
> Thank you. :)
:)

[cjgrant, 2017-03-29 20:22:38]

lgtm

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:87:
.Append(VrControllerModel::kDefaultVersion)
On 2017/03/29 20:13:26, acondor wrote:
> On 2017/03/29 13:20:54, cjgrant wrote:
> > Formatting looks off here.
> 
> This is what cl format does, even if I try to change it.

Acknowledged.

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

https://codereview.chromium.org/2775283004/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1105: // TODO(acondor): Placing
controller in temporary position before
On 2017/03/29 20:13:26, acondor wrote:
> On 2017/03/29 13:20:54, cjgrant wrote:
> > Maybe I'm missing something, but in addition to putting the controller where
> the
> > hand is, why not rotate the matrix according to the controller orientation
as
> > well?
> 
> Done. It will be changed to the elbow model soon, though.

Thanks.  From a live demo, this looks really good now.

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

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1108: TranslateM(mat, mat, 0, 0,
0.05);
Normally you'd want a constant for 0.05, but assuming the elbow model CL
addresses this, it's cool.

[bajones, 2017-03-29 20:38:29]

On 2017/03/29 20:13:26, acondor wrote:
> On 2017/03/29 17:38:58, bajones wrote:
> > The elements buffer and indices buffer are something that get looked up from
> the
> > table multiple times every frame. Since this class is for a very narrow use
> case
> > let's cache the BufferView once the model is loaded instead of looking it up
> > every time.
> 
> Actually, they are used just once (during SetUp call in the renderer).

Ah, I misread the renderer behavior. Thanks for pointing this out.
 
> On 2017/03/29 17:38:58, bajones wrote:
> > Same goes for the accessor, but even more so. Doing multiple string compares
> > each frame to find a static resource is something that should be avoided.
> Let's
> > cache the accessors in question so that PositionAccessor() and
> > TextureCoordinateAccessor() don't have to do any searches.
> 
> No need with the new approach on the Renderer.

LGTM! Thanks!

[mthiesse, 2017-03-29 20:40:14]

lgtm

[asimjour1, 2017-03-29 20:40:43]

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

[asimjour1, 2017-03-29 20:40:44]

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:81: void LoadControllerModelTask(
It should be moved to VrController

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

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1100: void
VrShellGl::DrawController(const gvr::Mat4f& view_proj_matrix) {
I think this method also doesn't belong here. it should be inside VrController

[amp, 2017-03-29 20:54:56]

Small nit on TODO comments.  If you are already working on the TODOs (and they
will be removed soon) feel free to ignore my comments.

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.h (right):

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.h:27: // TODO(acondor):
Hardcoded paths before VrShell resources are
Describe TODO as what needs to be done.

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

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1103: // TODO(acondor): Placing
the controller in a fixed position before
Format the TODO's as what needs to be done next, not what the current state is
(but having another comment after the TODO explaining the current state can be
useful to).

[acondor_, 2017-03-29 22:46:45]

Pieces of code where moved to more appropriate locations according to the
suggestions and discussions with the team.

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller_model.h (right):

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller_model.h:27: // TODO(acondor):
Hardcoded paths before VrShell resources are
On 2017/03/29 20:54:56, amp wrote:
> Describe TODO as what needs to be done.

Done.

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:81: void LoadControllerModelTask(
On 2017/03/29 20:40:44, asimjour1 wrote:
> It should be moved to VrController

Done.

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

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1103: // TODO(acondor): Placing
the controller in a fixed position before
On 2017/03/29 20:54:56, amp wrote:
> Format the TODO's as what needs to be done next, not what the current state is
> (but having another comment after the TODO explaining the current state can be
> useful to).

Done.

https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1108: TranslateM(mat, mat, 0, 0,
0.05);
On 2017/03/29 20:22:38, cjgrant wrote:
> Normally you'd want a constant for 0.05, but assuming the elbow model CL
> addresses this, it's cool.

Acknowledged.

[asimjour1, 2017-03-29 22:59:30]

On 2017/03/29 22:46:45, acondor wrote:
> Pieces of code where moved to more appropriate locations according to the
> suggestions and discussions with the team.
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_controller_model.h (right):
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_controller_model.h:27: // TODO(acondor):
> Hardcoded paths before VrShell resources are
> On 2017/03/29 20:54:56, amp wrote:
> > Describe TODO as what needs to be done.
> 
> Done.
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.cc:81: void LoadControllerModelTask(
> On 2017/03/29 20:40:44, asimjour1 wrote:
> > It should be moved to VrController
> 
> Done.
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:1103: // TODO(acondor): Placing
> the controller in a fixed position before
> On 2017/03/29 20:54:56, amp wrote:
> > Format the TODO's as what needs to be done next, not what the current state
is
> > (but having another comment after the TODO explaining the current state can
be
> > useful to).
> 
> Done.
> 
>
https://codereview.chromium.org/2775283004/diff/60001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell_gl.cc:1108: TranslateM(mat, mat, 0,
0,
> 0.05);
> On 2017/03/29 20:22:38, cjgrant wrote:
> > Normally you'd want a constant for 0.05, but assuming the elbow model CL
> > addresses this, it's cool.
> 
> Acknowledged.

lgtm

[acondor_, 2017-03-29 23:23:02]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-29 23:23:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2775283004/#ps80001
(title: "Moving code to more appropriate locations.")

[commit-bot: I haz the power, 2017-03-29 23:23:48]

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-30 01:26:21]

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

[commit-bot: I haz the power, 2017-03-30 01:26:22]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[acondor_, 2017-03-30 02:00:43]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-03-30 02:01:52]

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-30 02:09:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490839243793540,
"parent_rev": "72687bdd262209de03d258b03d14f9eff4554a8d", "commit_rev":
"e4a64eab960f6b5a25df0270b03e86311206d802"}

[commit-bot: I haz the power, 2017-03-30 02:10:10]

Description was changed from

==========
Rendering Daydream controller in a fixed position.

- Model file is not included, as it will be delivered through component updater.
- UI continues to work as before when the model file is missing.
- Textures included for each button state.
- Conform to DD UX specs pending.
- Refactored rendering-classes inheritance to be able to generalize them to non
quads.

BUG=644562
==========

to

==========
Rendering Daydream controller in a fixed position.

- Model file is not included, as it will be delivered through component updater.
- UI continues to work as before when the model file is missing.
- Textures included for each button state.
- Conform to DD UX specs pending.
- Refactored rendering-classes inheritance to be able to generalize them to non
quads.

BUG=644562

Review-Url: https://codereview.chromium.org/2775283004
Cr-Commit-Position: refs/heads/master@{#460625}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4a64eab960f6b5a25df0270b03e...
==========

[commit-bot: I haz the power, 2017-03-30 02:10:11]

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