Trackpad scrolling in VR feels bad |
||||||||||||
Issue descriptionWe occasionally get incorrect flings/hiccups and the constants (eg scroll scale) feel off.
,
Jul 2
I've just confirmed that the patch is working as expected in Canary.
,
Jul 2
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 2
That does not seem to be a trivial change to merge. Please can you add the rational behind this merge request? Any risk associated ?
,
Jul 2
Without this change, flings were much more likely to go in unpredictable directions and speeds. It came up as a blocking issue in review. This patch adds some filtering to mitigate the problem. I don't think there's too much risk since the code is VR-specific.
,
Jul 2
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b648651b1ac5f0a40a118ffb39249b09ea92724 commit 7b648651b1ac5f0a40a118ffb39249b09ea92724 Author: Ian Vollick <vollick@chromium.org> Date: Tue Jul 03 04:22:21 2018 [vr] Tune scroll event stream This change applies three heuristics to make trackpad scrolling feel smoother. We increase the frequency at which we submit scroll events to the platform, we also decrease the scroll scale factor (to make the scrolls less jumpy), and we do some simple filtering of our scroll values. I pair-programmed much of this change with asimjour@ and he wrote the initial code to interpolate events in the android ui gesture target. (cherry picked from commit 0e9a15da019624f9e5d6a3aba35b0102175da5c0) Bug: 859075 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I2fe42940214d01d99618ddacd182f202ff271dad Reviewed-on: https://chromium-review.googlesource.com/1120425 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Commit-Queue: Ian Vollick <vollick@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#571535} Reviewed-on: https://chromium-review.googlesource.com/1123463 Reviewed-by: Ian Vollick <vollick@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#584} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/7b648651b1ac5f0a40a118ffb39249b09ea92724/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/AndroidUiGestureTarget.java [modify] https://crrev.com/7b648651b1ac5f0a40a118ffb39249b09ea92724/chrome/browser/android/vr/android_ui_gesture_target.cc [modify] https://crrev.com/7b648651b1ac5f0a40a118ffb39249b09ea92724/chrome/browser/android/vr/android_ui_gesture_target.h [modify] https://crrev.com/7b648651b1ac5f0a40a118ffb39249b09ea92724/chrome/browser/android/vr/vr_controller.cc
,
Jul 3
cmasso@, as you'd mentioned in #4, this didn't merge automatically and so I applied the change to a local M68 checkout. When I applied and checked the change vs ToT, I found that the "smoothing" part of the ToT change was just scaling the scroll deltas by 60%. This simplified the merge and should have the same behavior as the code at ToT. I've merged that and plan to land the same simplification at ToT, but please let me know if that's the wrong approach; this should be simple to revert if so.
,
Jul 3
The corresponding simplification is up here crrev.com/c/1124405.
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/097090b12e1d75e871a46c3d296d3a54fddbdecd commit 097090b12e1d75e871a46c3d296d3a54fddbdecd Author: Ian Vollick <vollick@chromium.org> Date: Tue Jul 03 18:11:54 2018 Revert "[vr] Tune scroll event stream" This reverts commit 7b648651b1ac5f0a40a118ffb39249b09ea92724. Reason for revert: did not address scrolling issues. Original change's description: > [vr] Tune scroll event stream > > This change applies three heuristics to make trackpad scrolling feel > smoother. We increase the frequency at which we submit scroll events to > the platform, we also decrease the scroll scale factor (to make the > scrolls less jumpy), and we do some simple filtering of our scroll > values. > > I pair-programmed much of this change with asimjour@ and he wrote the > initial code to interpolate events in the android ui gesture target. > > (cherry picked from commit 0e9a15da019624f9e5d6a3aba35b0102175da5c0) > > Bug: 859075 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I2fe42940214d01d99618ddacd182f202ff271dad > Reviewed-on: https://chromium-review.googlesource.com/1120425 > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Commit-Queue: Ian Vollick <vollick@chromium.org> > Cr-Original-Commit-Position: refs/heads/master@{#571535} > Reviewed-on: https://chromium-review.googlesource.com/1123463 > Reviewed-by: Ian Vollick <vollick@chromium.org> > Cr-Commit-Position: refs/branch-heads/3440@{#584} > Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} TBR=vollick@chromium.org Change-Id: I5ba9707aea9dbde21910358df0e599a21085f715 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 859075 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1124821 Reviewed-by: Ian Vollick <vollick@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#590} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/097090b12e1d75e871a46c3d296d3a54fddbdecd/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/AndroidUiGestureTarget.java [modify] https://crrev.com/097090b12e1d75e871a46c3d296d3a54fddbdecd/chrome/browser/android/vr/android_ui_gesture_target.cc [modify] https://crrev.com/097090b12e1d75e871a46c3d296d3a54fddbdecd/chrome/browser/android/vr/android_ui_gesture_target.h [modify] https://crrev.com/097090b12e1d75e871a46c3d296d3a54fddbdecd/chrome/browser/android/vr/vr_controller.cc
,
Jul 3
update: I am reverting both changes, will reland with simplification.
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a426047793ed441bbe408b41b16f7c065b7f8108 commit a426047793ed441bbe408b41b16f7c065b7f8108 Author: Ian Vollick <vollick@chromium.org> Date: Tue Jul 03 19:22:42 2018 Revert "[vr] Tune scroll event stream" This reverts commit 0e9a15da019624f9e5d6a3aba35b0102175da5c0. Reason for revert: smoothing was simply scaling. Original change's description: > [vr] Tune scroll event stream > > This change applies three heuristics to make trackpad scrolling feel > smoother. We increase the frequency at which we submit scroll events to > the platform, we also decrease the scroll scale factor (to make the > scrolls less jumpy), and we do some simple filtering of our scroll > values. > > I pair-programmed much of this change with asimjour@ and he wrote the > initial code to interpolate events in the android ui gesture target. > > Bug: 859075 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I2fe42940214d01d99618ddacd182f202ff271dad > Reviewed-on: https://chromium-review.googlesource.com/1120425 > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Commit-Queue: Ian Vollick <vollick@chromium.org> > Cr-Commit-Position: refs/heads/master@{#571535} TBR=vollick@chromium.org,mthiesse@chromium.org,asimjour@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 859075 Change-Id: I6c82a12e0ba687e43c169206ad9b85d82f08570c Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1124999 Reviewed-by: Ian Vollick <vollick@chromium.org> Commit-Queue: Ian Vollick <vollick@chromium.org> Cr-Commit-Position: refs/heads/master@{#572317} [modify] https://crrev.com/a426047793ed441bbe408b41b16f7c065b7f8108/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/AndroidUiGestureTarget.java [modify] https://crrev.com/a426047793ed441bbe408b41b16f7c065b7f8108/chrome/browser/android/vr/android_ui_gesture_target.cc [modify] https://crrev.com/a426047793ed441bbe408b41b16f7c065b7f8108/chrome/browser/android/vr/android_ui_gesture_target.h [modify] https://crrev.com/a426047793ed441bbe408b41b16f7c065b7f8108/chrome/browser/vr/gesture_detector.cc [modify] https://crrev.com/a426047793ed441bbe408b41b16f7c065b7f8108/chrome/browser/vr/gesture_detector.h
,
Jul 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98c28a6f258f58f00ef7caec2c8048b886d40df4 commit 98c28a6f258f58f00ef7caec2c8048b886d40df4 Author: Ian Vollick <vollick@chromium.org> Date: Tue Jul 03 21:31:27 2018 Reland "[vr] Tune scroll event stream" This is a reland of 0e9a15da019624f9e5d6a3aba35b0102175da5c0 In this reland, I've combined the change with https://crrev.com/c/1124405 since the smoothing in the original CL was actually just a scaling. Original change's description: > [vr] Tune scroll event stream > > This change applies three heuristics to make trackpad scrolling feel > smoother. We increase the frequency at which we submit scroll events to > the platform, we also decrease the scroll scale factor (to make the > scrolls less jumpy), and we do some simple filtering of our scroll > values. > > I pair-programmed much of this change with asimjour@ and he wrote the > initial code to interpolate events in the android ui gesture target. > > Bug: 859075 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I2fe42940214d01d99618ddacd182f202ff271dad > Reviewed-on: https://chromium-review.googlesource.com/1120425 > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Commit-Queue: Ian Vollick <vollick@chromium.org> > Cr-Commit-Position: refs/heads/master@{#571535} Bug: 859075 Change-Id: Ib1a9f801c6930b709624233d8e00aeabc59f576d Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1124908 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: Amirhossein Simjour <asimjour@chromium.org> Commit-Queue: Ian Vollick <vollick@chromium.org> Cr-Commit-Position: refs/heads/master@{#572364} [modify] https://crrev.com/98c28a6f258f58f00ef7caec2c8048b886d40df4/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/AndroidUiGestureTarget.java [modify] https://crrev.com/98c28a6f258f58f00ef7caec2c8048b886d40df4/chrome/browser/android/vr/android_ui_gesture_target.cc [modify] https://crrev.com/98c28a6f258f58f00ef7caec2c8048b886d40df4/chrome/browser/android/vr/android_ui_gesture_target.h [modify] https://crrev.com/98c28a6f258f58f00ef7caec2c8048b886d40df4/chrome/browser/vr/gesture_detector.cc
,
Jul 4
I've reverted the old approach (see comments 10 and 12) and I've landed the simplified approach on ToT. This has made it into Canary and I've checked it on device and I didn't see anything strange. This version should be easier to merge to M68. Although there is a change to a constant in a file that has moved, the same constant does exist in M68 (in vr_controller.cc) and would not be difficult to update.
,
Jul 4
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 9
Thanks for doing it nicely!
,
Jul 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6493beffec9f03ad1e1d91421a05ffae44ce133b commit 6493beffec9f03ad1e1d91421a05ffae44ce133b Author: Ian Vollick <vollick@chromium.org> Date: Mon Jul 09 19:11:41 2018 Reland "[vr] Tune scroll event stream" This is a reland of 0e9a15da019624f9e5d6a3aba35b0102175da5c0 In this reland, I've combined the change with https://crrev.com/c/1124405 since the smoothing in the original CL was actually just a scaling. Original change's description: > [vr] Tune scroll event stream > > This change applies three heuristics to make trackpad scrolling feel > smoother. We increase the frequency at which we submit scroll events to > the platform, we also decrease the scroll scale factor (to make the > scrolls less jumpy), and we do some simple filtering of our scroll > values. > > I pair-programmed much of this change with asimjour@ and he wrote the > initial code to interpolate events in the android ui gesture target. > > Bug: 859075 > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel > Change-Id: I2fe42940214d01d99618ddacd182f202ff271dad > Reviewed-on: https://chromium-review.googlesource.com/1120425 > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Commit-Queue: Ian Vollick <vollick@chromium.org> > Cr-Commit-Position: refs/heads/master@{#571535} (cherry picked from commit 98c28a6f258f58f00ef7caec2c8048b886d40df4) Bug: 859075 Change-Id: Ib1a9f801c6930b709624233d8e00aeabc59f576d Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/1124908 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: Amirhossein Simjour <asimjour@chromium.org> Commit-Queue: Ian Vollick <vollick@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#572364} Reviewed-on: https://chromium-review.googlesource.com/1129626 Reviewed-by: Ian Vollick <vollick@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#619} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/6493beffec9f03ad1e1d91421a05ffae44ce133b/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/AndroidUiGestureTarget.java [modify] https://crrev.com/6493beffec9f03ad1e1d91421a05ffae44ce133b/chrome/browser/android/vr/android_ui_gesture_target.cc [modify] https://crrev.com/6493beffec9f03ad1e1d91421a05ffae44ce133b/chrome/browser/android/vr/android_ui_gesture_target.h [modify] https://crrev.com/6493beffec9f03ad1e1d91421a05ffae44ce133b/chrome/browser/android/vr/vr_controller.cc
,
Aug 2
Would this bug also cover the fact that you need to scroll at absurd speeds in order to properly trigger the "scroll to refresh" feature?
,
Jan 14
This looks fixed according to the comments. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by bugdroid1@chromium.org
, Jun 29 2018