VR: Fix cursor draw order.

VR: Fix cursor draw order.

Prior to this CL, the cursor was always drawn last, meaning it could not be seen through transparent objects.
This CL draws the cursor after the object it's supposed to be sitting on, so that transparencies work as expected.

BUG=721390
Review-Url: https://codereview.chromium.org/2869273008
Cr-Commit-Position: refs/heads/master@{#471321}
Committed: https://chromium.googlesource.com/chromium/src/+/39500a44e5e31f81001ddc4f718344fb593401a5

[mthiesse, 2017-05-11 15:56:19]

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

[mthiesse, 2017-05-11 15:56:20]

PTAL

[cjgrant, 2017-05-11 17:21:52]

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

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1117: bool draw_cursor) {
draw_reticle (maybe elsewhere, based on your other rename).

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1123: // If we have no element to
draw the cursor on, draw it after the
This seems fragile (ie. assuming which elements are in which draw phase).  We've
been working to separate knowledge of the scene from this code.  How about, if
the reticle isn't on an element (including floor or ceiling), draw it first, and
let the depth buffer handle floor and ceiling cases?  That's not perfect, but
seems less bad to me.

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1130: vr::Mat4f transform;
Suggestion - now that this method is mixing in reticle logic, how about breaking
out DrawElement() to handle the fill switch?

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1165:
vr_shell_renderer_->Flush();
I assume no side effects of now skipping this Flush in the no element case
(previously, it ran).

[mthiesse, 2017-05-11 17:55:03]

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

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1117: bool draw_cursor) {
On 2017/05/11 17:21:52, cjgrant wrote:
> draw_reticle (maybe elsewhere, based on your other rename).

Done.

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1123: // If we have no element to
draw the cursor on, draw it after the
On 2017/05/11 17:21:52, cjgrant wrote:
> This seems fragile (ie. assuming which elements are in which draw phase). 
We've
> been working to separate knowledge of the scene from this code.  How about, if
> the reticle isn't on an element (including floor or ceiling), draw it first,
and
> let the depth buffer handle floor and ceiling cases?  That's not perfect, but
> seems less bad to me.

Won't work because the cursor is partially transparent and will be transparent
through to the wrong thing.

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1130: vr::Mat4f transform;
On 2017/05/11 17:21:52, cjgrant wrote:
> Suggestion - now that this method is mixing in reticle logic, how about
breaking
> out DrawElement() to handle the fill switch?

Done.

[mthiesse, 2017-05-11 17:55:29]

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

https://codereview.chromium.org/2869273008/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1165:
vr_shell_renderer_->Flush();
On 2017/05/11 17:21:51, cjgrant wrote:
> I assume no side effects of now skipping this Flush in the no element case
> (previously, it ran).

Correct.

[cjgrant, 2017-05-12 13:49:04]

lgtm

Still wish we didn't have to care about draw phase for the reticle.  Maybe in
the future the reticle could become its own UI element, or, we could sort its
distance into the element distances calculate during hit testing, so we know
when to draw it...  anything to cut that link.

[mthiesse1, 2017-05-12 14:21:41]

Patchset #3 (id:40001) has been deleted

[mthiesse1, 2017-05-12 14:23:09]

The CQ bit was checked by mthiesse@google.com

[mthiesse1, 2017-05-12 14:23:09]

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

[commit-bot: I haz the power, 2017-05-12 14:23:31]

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-05-12 16:01:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494598989093170,
"parent_rev": "dfb837a97c2ee14bbcfb04a8bdd1a23813cab054", "commit_rev":
"39500a44e5e31f81001ddc4f718344fb593401a5"}

[commit-bot: I haz the power, 2017-05-12 16:01:11]

Description was changed from

==========
VR: Fix cursor draw order.

Prior to this CL, the cursor was always drawn last, meaning it could not be seen
through transparent objects.
This CL draws the cursor after the object it's supposed to be sitting on, so
that transparencies work as expected.

BUG=721390
==========

to

==========
VR: Fix cursor draw order.

Prior to this CL, the cursor was always drawn last, meaning it could not be seen
through transparent objects.
This CL draws the cursor after the object it's supposed to be sitting on, so
that transparencies work as expected.

BUG=721390

Review-Url: https://codereview.chromium.org/2869273008
Cr-Commit-Position: refs/heads/master@{#471321}
Committed:
https://chromium.googlesource.com/chromium/src/+/39500a44e5e31f81001ddc4f7183...
==========

[commit-bot: I haz the power, 2017-05-12 16:01:12]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/39500a44e5e31f81001ddc4f7183...

[acondor_, 2017-05-24 16:34:53]

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

[acondor_, 2017-05-24 16:34:54]

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

https://codereview.chromium.org/2869273008/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1109:
DrawLaser(view_proj_matrix);
mthiesse, what was the reason for altering this order (just laser and
controller). It draws odd this way.

[mthiesse, 2017-05-24 17:37:08]

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

https://codereview.chromium.org/2869273008/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:1109:
DrawLaser(view_proj_matrix);
On 2017/05/24 16:34:54, acondor wrote:
> mthiesse, what was the reason for altering this order (just laser and
> controller). It draws odd this way.

Oh, I didn't mean to invert the order of these two, should be fine to reverse
the order again.