Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367543002:
Implement UI hit testing + Reticle scaling (Closed)
DescriptionImplement UI hit testing + Reticle scaling
BUG=641413
Committed: https://crrev.com/7027cefe5f74c81a56e08c15de4d3cd021d5c2ea
Cr-Commit-Position: refs/heads/master@{#420623}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase + address comments #
Total comments: 2
Patch Set 3 : Address comment #
Messages
Total messages: 16 (7 generated)
mthiesse@chromium.org changed reviewers: + bshe@chromium.org
PTAL
Description was changed from ========== Implement UI hit testing + Reticle scaling BUG= ========== to ========== Implement UI hit testing + Reticle scaling BUG=641413 ==========
cjgrant@chromium.org changed reviewers: + cjgrant@chromium.org
https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (left): https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:312: // Scale the pointer. ... to have a fixed FOV size at any distance. (?) https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:177: float desktop_dist = ui_rects_[0]->GetRayDistance(translation, forward); Should use desktop_plane_ or scene_.GetUiElementById(kBrowserUiElementId) here. https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:178: gvr::Vec3f cursor_position = GetRayPoint(translation, forward, desktop_dist); If you want, there's a better way to do this. Since GetRayDistance() already finds the intersection, you could expose GetRayPlaneIntersection() in ui_elements.cc, then find the distance from that. https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:351: TranslateM(mat, mat, look_at_vector_.x * kReticleZOffset, Why are x and y also being offset?
https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (left): https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:312: // Scale the pointer. On 2016/09/22 18:59:04, cjgrant wrote: > ... to have a fixed FOV size at any distance. (?) Done. https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:177: float desktop_dist = ui_rects_[0]->GetRayDistance(translation, forward); On 2016/09/22 18:59:04, cjgrant wrote: > Should use desktop_plane_ or scene_.GetUiElementById(kBrowserUiElementId) here. Done. https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:178: gvr::Vec3f cursor_position = GetRayPoint(translation, forward, desktop_dist); On 2016/09/22 18:59:03, cjgrant wrote: > If you want, there's a better way to do this. Since GetRayDistance() already > finds the intersection, you could expose GetRayPlaneIntersection() in > ui_elements.cc, then find the distance from that. I'll get back to this when we have working controllers I can test with (I want to play with this some more). For now what we have here works so I'd prefer to leave as is. https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell.cc:351: TranslateM(mat, mat, look_at_vector_.x * kReticleZOffset, On 2016/09/22 18:59:03, cjgrant wrote: > Why are x and y also being offset? We're moving it slightly towards the viewer while maintaining its position in the user's FOV by translating it only 99% of the way along its vector.
On 2016/09/22 22:01:33, mthiesse wrote: > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/vr_shell.cc (left): > > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell.cc:312: // Scale the pointer. > On 2016/09/22 18:59:04, cjgrant wrote: > > ... to have a fixed FOV size at any distance. (?) > > Done. > > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > File chrome/browser/android/vr_shell/vr_shell.cc (right): > > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell.cc:177: float desktop_dist = > ui_rects_[0]->GetRayDistance(translation, forward); > On 2016/09/22 18:59:04, cjgrant wrote: > > Should use desktop_plane_ or scene_.GetUiElementById(kBrowserUiElementId) > here. > > Done. > > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell.cc:178: gvr::Vec3f cursor_position = > GetRayPoint(translation, forward, desktop_dist); > On 2016/09/22 18:59:03, cjgrant wrote: > > If you want, there's a better way to do this. Since GetRayDistance() already > > finds the intersection, you could expose GetRayPlaneIntersection() in > > ui_elements.cc, then find the distance from that. > > I'll get back to this when we have working controllers I can test with (I want > to play with this some more). For now what we have here works so I'd prefer to > leave as is. > > https://codereview.chromium.org/2367543002/diff/1/chrome/browser/android/vr_s... > chrome/browser/android/vr_shell/vr_shell.cc:351: TranslateM(mat, mat, > look_at_vector_.x * kReticleZOffset, > On 2016/09/22 18:59:03, cjgrant wrote: > > Why are x and y also being offset? > > We're moving it slightly towards the viewer while maintaining its position in > the user's FOV by translating it only 99% of the way along its vector. lgtm
lgtm https://codereview.chromium.org/2367543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2367543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:199: int pixel_x, pixel_y; you might get uninitalized variable error from ASAN builds. perhaps initialize it to a default value.
https://codereview.chromium.org/2367543002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2367543002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:199: int pixel_x, pixel_y; On 2016/09/22 23:28:42, bshe wrote: > you might get uninitalized variable error from ASAN builds. perhaps initialize > it to a default value. Done.
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org, cjgrant@chromium.org Link to the patchset: https://codereview.chromium.org/2367543002/#ps40001 (title: "Address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement UI hit testing + Reticle scaling BUG=641413 ========== to ========== Implement UI hit testing + Reticle scaling BUG=641413 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Implement UI hit testing + Reticle scaling BUG=641413 ========== to ========== Implement UI hit testing + Reticle scaling BUG=641413 Committed: https://crrev.com/7027cefe5f74c81a56e08c15de4d3cd021d5c2ea Cr-Commit-Position: refs/heads/master@{#420623} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7027cefe5f74c81a56e08c15de4d3cd021d5c2ea Cr-Commit-Position: refs/heads/master@{#420623} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
