Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1089663002:
(NOT FOR REVIEW) Smooth gamepad scroll via scrollBy (Closed)
|
Created:
5 years, 8 months ago by jdduke (slow) Modified:
5 years, 6 months ago Reviewers:
sshelke CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description(NOT FOR REVIEW) Smooth gamepad scroll via scrollBy
BUG=454355
Patch Set 1 #Patch Set 2 : scrollBy #
Total comments: 8
Messages
Total messages: 17 (1 generated)
sshelke@nvidia.com changed reviewers: + sshelke@nvidia.com
https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:43: if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) { Do we miss first event each time since we are checking mLastAnimateTimeMillis != 0? We can initialize mLastAnimateTimeMillis in onMotion, so that first event wont be missed. https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:44: final long dt = timeMillis - mLastAnimateTimeMillis; run() function always be called after animation interval which is 16 ~ 17 ms, so dt will be mostly around 16 ~17ms. I guess, dt is not playing any role in linear equation since it is constant rather than variable. We can save computation by considering dt as constant.
https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1863: private void scrollByImpl(float dxPix, float dyPix) { Implementation for ScrollBy and ScrollTo looks same. How do we eliminate race condition and get consistent behavior?
This is looking good. Would you mind adding a basic integration test? There's a ContentViewScrollingTest.java, I'd be OK if you simply added a test there that ensures that by a few joystick MotionEvents + a few animation ticks produces a reasonable scroll offset change. https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1863: private void scrollByImpl(float dxPix, float dyPix) { On 2015/05/12 12:09:05, sshelke wrote: > Implementation for ScrollBy and ScrollTo looks same. > How do we eliminate race condition and get consistent behavior? The difference is that |ScrollTo| computes the delta based on the "current" scroll offset as tracked by the *browser* process. However, scrolling is asynchronous, so there may be scroll events already in-flight or already queued that will change the current scroll offset at some point in the future. So we're effectively operating on stale data. If we simply use ScrollBy, then we can be sure that the "impulse" applied to the system is preserved and depends in no way on the current state of the scroll offset. https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:43: if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) { On 2015/05/12 12:03:47, sshelke wrote: > Do we miss first event each time since we are checking mLastAnimateTimeMillis != > 0? > We can initialize mLastAnimateTimeMillis in onMotion, so that first event wont > be missed. > Yup, that sounds reasonable. Missing the first event isn't quite as critical as, say, touch scrolling, as we're less latency sensitive, but if we can scroll the first frame might as well try. https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:44: final long dt = timeMillis - mLastAnimateTimeMillis; On 2015/05/12 12:03:47, sshelke wrote: > run() function always be called after animation interval which is > 16 ~ 17 ms, so dt will be mostly around 16 ~17ms. I guess, dt is not playing > any role in linear equation since it is constant rather than variable. We can > save computation by considering dt as constant. Subtractions are cheap, I don't think we should make any assumptions about the consistency of animation callbacks.
On 2015/05/12 15:13:11, jdduke wrote: > This is looking good. > > Would you mind adding a basic integration test? There's a > ContentViewScrollingTest.java, I'd be OK if you simply added a test there that > ensures that by a few joystick MotionEvents + a few animation ticks produces a > reasonable scroll offset change. > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1863: > private void scrollByImpl(float dxPix, float dyPix) { > On 2015/05/12 12:09:05, sshelke wrote: > > Implementation for ScrollBy and ScrollTo looks same. > > How do we eliminate race condition and get consistent behavior? > > The difference is that |ScrollTo| computes the delta based on the "current" > scroll offset as tracked by the *browser* process. However, scrolling is > asynchronous, so there may be scroll events already in-flight or already queued > that will change the current scroll offset at some point in the future. So we're > effectively operating on stale data. > > If we simply use ScrollBy, then we can be sure that the "impulse" applied to the > system is preserved and depends in no way on the current state of the scroll > offset. > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java > (right): > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:43: > if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) { > On 2015/05/12 12:03:47, sshelke wrote: > > Do we miss first event each time since we are checking mLastAnimateTimeMillis > != > > 0? > > We can initialize mLastAnimateTimeMillis in onMotion, so that first event wont > > be missed. > > > > Yup, that sounds reasonable. Missing the first event isn't quite as critical as, > say, touch scrolling, as we're less latency sensitive, but if we can scroll the > first frame might as well try. > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:44: > final long dt = timeMillis - mLastAnimateTimeMillis; > On 2015/05/12 12:03:47, sshelke wrote: > > run() function always be called after animation interval which is > > 16 ~ 17 ms, so dt will be mostly around 16 ~17ms. I guess, dt is not playing > > any role in linear equation since it is constant rather than variable. We can > > save computation by considering dt as constant. > > Subtractions are cheap, I don't think we should make any assumptions about the > consistency of animation callbacks. Thanks for reply, I will add test case in ContentViewScrollingTest.java. Should I add all changes including scrollBy and test cases into single patch, or you want to add scrollBy patch at your end and then I will upload remaining change? Either way is preferable to me.
On 2015/05/13 08:33:25, sshelke wrote: > Thanks for reply, I will add test case in ContentViewScrollingTest.java. > Should I add all changes including scrollBy and test cases into single patch, or > you want to add scrollBy patch at your end and then I will upload remaining > change? > Either way is preferable to me. I was planning to land https://codereview.chromium.org/1131723006/ in the meantime, if that's what you meant? Then you can just use scrollBy in your code? Does that work?
On 2015/05/13 15:01:55, jdduke wrote: > On 2015/05/13 08:33:25, sshelke wrote: > > Thanks for reply, I will add test case in ContentViewScrollingTest.java. > > Should I add all changes including scrollBy and test cases into single patch, > or > > you want to add scrollBy patch at your end and then I will upload remaining > > change? > > Either way is preferable to me. > > I was planning to land https://codereview.chromium.org/1131723006/ in the > meantime, if that's what you meant? Then you can just use scrollBy in your code? > Does that work? Yep,sure.
On 2015/05/13 15:01:55, jdduke wrote: > On 2015/05/13 08:33:25, sshelke wrote: > > Thanks for reply, I will add test case in ContentViewScrollingTest.java. > > Should I add all changes including scrollBy and test cases into single patch, > or > > you want to add scrollBy patch at your end and then I will upload remaining > > change? > > Either way is preferable to me. > > I was planning to land https://codereview.chromium.org/1131723006/ in the > meantime, if that's what you meant? Then you can just use scrollBy in your code? > Does that work? Hi Jared, I have some out of context issue regarding building chrome shell. Recently, I synced to master branch and ran install-build-deps-android.sh. After that, I ran GYP_DEFINES=OS=android gclient runhooks and tried to build code using ninja -C out/Debug chrome_shell_apk, I received following error. UNEXPECTED TOP-LEVEL EXCEPTION: com.android.dex.DexIndexOverflowException: method ID not in [0, 0xffff]: 65536 at com.android.dx.merge.DexMerger$6.updateIndex(DexMerger.java:502) at com.android.dx.merge.DexMerger$IdMerger.mergeSorted(DexMerger.java:277) at com.android.dx.merge.DexMerger.mergeMethodIds(DexMerger.java:491) at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:168) I know how to fix this issue with individual app using build.gradle. But, no clue of fixing this issue in chrome shell. Any pointer would be helpful. I am using Ubuntu 14.04 for development.
On 2015/05/15 12:56:30, sshelke wrote: > On 2015/05/13 15:01:55, jdduke wrote: > > On 2015/05/13 08:33:25, sshelke wrote: > > > Thanks for reply, I will add test case in ContentViewScrollingTest.java. > > > Should I add all changes including scrollBy and test cases into single > patch, > > or > > > you want to add scrollBy patch at your end and then I will upload remaining > > > change? > > > Either way is preferable to me. > > > > I was planning to land https://codereview.chromium.org/1131723006/ in the > > meantime, if that's what you meant? Then you can just use scrollBy in your > code? > > Does that work? > > Hi Jared, > I have some out of context issue regarding building chrome shell. Recently, I > synced > to master branch and ran install-build-deps-android.sh. After that, I ran > GYP_DEFINES=OS=android gclient runhooks > and tried to build code using ninja -C out/Debug chrome_shell_apk, I received > following error. > > UNEXPECTED TOP-LEVEL EXCEPTION: > com.android.dex.DexIndexOverflowException: method ID not in [0, 0xffff]: 65536 > at com.android.dx.merge.DexMerger$6.updateIndex(DexMerger.java:502) > at com.android.dx.merge.DexMerger$IdMerger.mergeSorted(DexMerger.java:277) > at com.android.dx.merge.DexMerger.mergeMethodIds(DexMerger.java:491) > at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:168) > > I know how to fix this issue with individual app using build.gradle. But, no > clue > of fixing this issue in chrome shell. > > Any pointer would be helpful. I am using Ubuntu 14.04 for development. Hmm, are you using GN or gyp for building? Can you try building content_shell_apk?
On 2015/05/15 14:55:21, jdduke wrote: > On 2015/05/15 12:56:30, sshelke wrote: > > On 2015/05/13 15:01:55, jdduke wrote: > > > On 2015/05/13 08:33:25, sshelke wrote: > > > > Thanks for reply, I will add test case in ContentViewScrollingTest.java. > > > > Should I add all changes including scrollBy and test cases into single > > patch, > > > or > > > > you want to add scrollBy patch at your end and then I will upload > remaining > > > > change? > > > > Either way is preferable to me. > > > > > > I was planning to land https://codereview.chromium.org/1131723006/ in the > > > meantime, if that's what you meant? Then you can just use scrollBy in your > > code? > > > Does that work? > > > > Hi Jared, > > I have some out of context issue regarding building chrome shell. Recently, I > > synced > > to master branch and ran install-build-deps-android.sh. After that, I ran > > GYP_DEFINES=OS=android gclient runhooks > > and tried to build code using ninja -C out/Debug chrome_shell_apk, I received > > following error. > > > > UNEXPECTED TOP-LEVEL EXCEPTION: > > com.android.dex.DexIndexOverflowException: method ID not in [0, 0xffff]: 65536 > > at com.android.dx.merge.DexMerger$6.updateIndex(DexMerger.java:502) > > at com.android.dx.merge.DexMerger$IdMerger.mergeSorted(DexMerger.java:277) > > at com.android.dx.merge.DexMerger.mergeMethodIds(DexMerger.java:491) > > at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:168) > > > > I know how to fix this issue with individual app using build.gradle. But, no > > clue > > of fixing this issue in chrome shell. > > > > Any pointer would be helpful. I am using Ubuntu 14.04 for development. > > Hmm, are you using GN or gyp for building? Can you try building > content_shell_apk? Thanks, Jared. I was able to build content shell.
https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:47: mView.scrollBy(dx, dy); Hmm, we should probably include the "pointer" coordinate when sending the scroll gesture, that way the user can scroll the right element depending on where the pointer is located. We can extend the ContentViewCore API to do this.
https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:47: mView.scrollBy(dx, dy); On 2015/05/18 21:40:59, jdduke wrote: > Hmm, we should probably include the "pointer" coordinate when sending the scroll > gesture, that way the user can scroll the right element depending on where the > pointer is located. We can extend the ContentViewCore API to do this. > Hmm, we should probably include the "pointer" coordinate when sending the scroll > gesture, that way the user can scroll the right element depending on where the > pointer is located. How can we get pointer location? since view can determine fix pointer location if there is cursor or touch event. In joystick event, pointer location is equal to movement of stick around center axis. So, if we move joystick , pointer location will change. I mean, we wont get fix pointer location (which determines right element)for joystick, it varies according to joystick movement. > We can extend the ContentViewCore API to do this. So, ContentViewCore will contain scrollBy(PointerCoords Cords,dx,dy) API and we should use same API to call scroll functionality. So addition will be, PointerCoords coords = new PointerCoords(); Cords.x = event.getRawX(); // or mScrollVelocity.x/mScrollFactor since joystick output is in terms // of pointer coordinates Cords.y = event.getRawY(); //or mScrollVelocity.y/mScrollFactor scrollBy(Cords,dx,dy); Is that correct or have I misunderstood completely? But I think, scrollBy API should implicitly determine pointer coordinates and determine right element and make it scroll.
On 2015/05/19 10:54:31, sshelke wrote: > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java > (right): > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:47: > mView.scrollBy(dx, dy); > On 2015/05/18 21:40:59, jdduke wrote: > > Hmm, we should probably include the "pointer" coordinate when sending the > scroll > > gesture, that way the user can scroll the right element depending on where the > > pointer is located. We can extend the ContentViewCore API to do this. > > > Hmm, we should probably include the "pointer" coordinate when sending the > scroll > > gesture, that way the user can scroll the right element depending on where the > > pointer is located. > > How can we get pointer location? since view can determine fix pointer location > if there is cursor or touch event. > In joystick event, pointer location is equal to movement of stick around center > axis. So, if we move > joystick , pointer location will change. I mean, we wont get fix pointer > location (which determines right element)for joystick, it varies according to > joystick movement. > I see, so there's no concept of a cursor location even with a gamepad present? If not, I guess the ideal behavior would be to scroll whatever element is currently focused (with bubbling), but our current scroll gesture events don't do this, and I don't know that the browser has knowledge of coordinates for whatever element currently has focus. > > We can extend the ContentViewCore API to do this. > So, ContentViewCore will contain scrollBy(PointerCoords Cords,dx,dy) API and we > should use same > API to call scroll functionality. So addition will be, > > PointerCoords coords = new PointerCoords(); > Cords.x = event.getRawX(); // or mScrollVelocity.x/mScrollFactor since > joystick output is in terms > // of > pointer coordinates > Cords.y = event.getRawY(); //or mScrollVelocity.y/mScrollFactor > > scrollBy(Cords,dx,dy); > > Is that correct or have I misunderstood completely? More or less, it would be something like scrollBy(x, y, dx, dy); > > But I think, scrollBy API should implicitly determine pointer > coordinates and determine right element and make it scroll. That would be nice, not sure it's possible currently. I believe only keyboard scroll events take into accout which element is focused. Otherwise, all other gesture events already have the concept of a "cursor", e.g., wheel events, touch scroll events, etc... that they can use for targeting the scroll.
On 2015/05/19 15:24:06, jdduke wrote: > On 2015/05/19 10:54:31, sshelke wrote: > > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java > > (right): > > > > > https://codereview.chromium.org/1089663002/diff/20001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:47: > > mView.scrollBy(dx, dy); > > On 2015/05/18 21:40:59, jdduke wrote: > > > Hmm, we should probably include the "pointer" coordinate when sending the > > scroll > > > gesture, that way the user can scroll the right element depending on where > the > > > pointer is located. We can extend the ContentViewCore API to do this. > > > > > Hmm, we should probably include the "pointer" coordinate when sending the > > scroll > > > gesture, that way the user can scroll the right element depending on where > the > > > pointer is located. > > > > How can we get pointer location? since view can determine fix pointer location > > if there is cursor or touch event. > > In joystick event, pointer location is equal to movement of stick around > center > > axis. So, if we move > > joystick , pointer location will change. I mean, we wont get fix pointer > > location (which determines right element)for joystick, it varies according to > > joystick movement. > > > > I see, so there's no concept of a cursor location even with a gamepad present? > If not, I guess the ideal behavior would be to scroll whatever element is > currently focused (with bubbling), but our current scroll gesture events don't > do this, and I don't know that the browser has knowledge of coordinates for > whatever element currently has focus. > > > > We can extend the ContentViewCore API to do this. > > So, ContentViewCore will contain scrollBy(PointerCoords Cords,dx,dy) API and > we > > should use same > > API to call scroll functionality. So addition will be, > > > > PointerCoords coords = new PointerCoords(); > > Cords.x = event.getRawX(); // or mScrollVelocity.x/mScrollFactor > since > > joystick output is in terms > > // of > > pointer coordinates > > Cords.y = event.getRawY(); //or mScrollVelocity.y/mScrollFactor > > > > scrollBy(Cords,dx,dy); > > > > Is that correct or have I misunderstood completely? > > More or less, it would be something like scrollBy(x, y, dx, dy); > > > > > But I think, scrollBy API should implicitly determine pointer > > coordinates and determine right element and make it scroll. > > That would be nice, not sure it's possible currently. I believe only keyboard > scroll events take into accout which element is focused. Otherwise, all other > gesture events already have the concept of a "cursor", e.g., wheel events, touch > scroll events, etc... that they can use for targeting the scroll. There is no concept of cursor for gamepad left stick motion event. Need to find a way to get coordinates of focus element. Looking into it.
Hi Jared,
There are two ways to get focus element to scroll instead of entire view.
1) Blink contains information of all elements coordinates, so
we can get that info using WebContents. In order to achieve that we
have to follow long path.
ContentViewCore -> WebContents->render_view_host->render_view->blink.
2) Cache cursor position of last event happened, it can be touch or mouse
event.Assume that position as a current cursor location at the time of
left joystick motion. We can scroll from current cursor location.
So we can modify scroll API as follows,
nativeScrollBy(mNativeContentViewCore, time, cursorPosX,
cursorPosY, dxPix, dyPix);
Touch events are already getting cached in current ContentViewCore code,
we can add logic to cache mouse cursor position.
Uploading the patch containing logic mentioned in second point.
On 2015/05/28 12:23:07, sshelke wrote: > Hi Jared, > There are two ways to get focus element to scroll instead of entire view. > > 1) Blink contains information of all elements coordinates, so > we can get that info using WebContents. In order to achieve that we > have to follow long path. > ContentViewCore -> WebContents->render_view_host->render_view->blink. > > 2) Cache cursor position of last event happened, it can be touch or mouse > event.Assume that position as a current cursor location at the time of > left joystick motion. We can scroll from current cursor location. > So we can modify scroll API as follows, > nativeScrollBy(mNativeContentViewCore, time, cursorPosX, > cursorPosY, dxPix, dyPix); > > Touch events are already getting cached in current ContentViewCore code, > we can add logic to cache mouse cursor position. > > Uploading the patch containing logic mentioned in second point. Can gamepads manipulate the mouse cursor? If so, that makes sense, otherwise I wonder how much benefit there really is. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
