721421 - Increase acceleration when a new fling action is performed during a fling

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Patrick Walton (:pcwalton)]

When a new fling is performed when a fling is already going on, we should increase the acceleration. This makes it easier to scroll up and down long pages.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

It also makes it much easier to overshoot what you're trying to get it, if you want to go a "medium" distance on a long page. This needs to be tweaked carefully.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

See related bug https://bugzilla.mozilla.org/show_bug.cgi?id=708765 - that might be a better solution.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Ian, what sort of physics do you want here?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

For the record there was a bunch of discussion on bug 708765 about how best to scroll through large pages quickly and the consensus seemed to be to do accelerated scrolling when multiple flings are done in quick succession. The exact timing and friction parameters need to be worked out but I duped that bug over here and we can use this one to track this work.

Comment 6

[Terry]

Hey Kats, I was on bug 708765 but I have since graduated and got a job. I won't have enough time to work on this anytime soon, but I have been following the comments all along and would love to continue helping the effort if I can.

For those just coming in, the debate for two finger gestures is mostly covered in 708765.

The last comment in its defense suggested that it is plenty intuitive. I will have to respectfully disagree as the word intuitive implies that someone would be able to figure it out naturally, if not immediately. I think it's safe to say most people do not use two fingers on their mobile devices much at all. Also, again, it conflicts heavily with zoom and several other gestures that actually require two fingers.

For a clean, effective solution, a good implementation of accelerated scrolling is still the best way to go. It's just fine-tuning the physics behind it that is a problem.

Relevant code:
"/src/mobile/android/base/gfx" in "JavaPanZoomController.java" and "Axis.java"

Comment 7

[Philip Armstrong]

The consensus in that thread may be for more accelerated flinging, but to me that sounds very unintuitive. I would *much* rather have a scrollbar popup that I could choose to grab with a finger or thumb and simply drag to the point in the webpage I wanted to get to.

A classic example page to play around with would be a long dicussion on metafilter.com, eg: http://www.metafilter.com/132415/This-aint-chemistry-This-is-Art

A page like that is all text & renders quickly, but takes an *age* to scroll around in. Something like the scrollbar as implemented in the Android contacts App would be perfect & would present a UI element that was already very familiar to the Firefox on Android user base.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Philip Armstrong from comment #7)
> The consensus in that thread may be for more accelerated flinging, but to me
> that sounds very unintuitive. I would *much* rather have a scrollbar popup
> that I could choose to grab with a finger or thumb and simply drag to the
> point in the webpage I wanted to get to.

Why not both? I don't think one is necessarily a replacement for the other, but could see the either interaction being useful.

Comment 9

[Philip Armstrong]

Both would be fine !

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Assigning this bug to Prabhjyot who was interested in working on it. The goal here is to implement fling acceleration when a second fling is done within 500ms of the first fling. We've implemented this on B2G and it works quite well, so I think it would be a good starting point here.

The relevant code is at [1]. This is where, once you scroll and lift your finger, we try to compute the velocity the page should continue moving at (this is referred to as the "fling" portion of the scroll). Right now, while the finger is moving, we sample the velocity periodically and store it in the mRecentVelocities array. Then, when the finger is lifted, we compute the average of the 8 most recent velocities and use that to continue the movement.

What we want to do is to keep a copy of this computed average value, and then if we get another fling starting within 500ms, we want to accelerate the new computed average (for the second fling) using the value that we saved from the first fling.

On Firefox OS we have code that does this and is very similar, so I suggest that you read through the code we have for that as it will provide you with a template. The code for that can be found at [2] and is in C++. Note that the C++ version uses various prefs (for example gfxPrefs::APZFlingAccelInterval()) for some of the key parameters so that they can be adjusted easily without changing the code. In the case of APZFlingAccelInterval, you can see that it is mapped to the pref apz.fling_accel_interval_ms at [3] and has a default value of 500. (The runtime value of the pref is picked up from [4] and has the same value.)

In the Firefox for Android Java code the mechanism for accessing prefs is a little different, you can take a look at [5] and the setPrefs function to get an idea of how it works. For the first version of the patch though I'm happy if you just ignore prefs and get something working with hard-coded values. Once we have that we can figure out which values need to be turned into prefs and how to do it.

Let me know if you have any questions or if any of this doesn't make sense. Thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/Axis.java?rev=2878bf6cbc1f#313
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=4b6379ea8da0#503
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h?rev=e7b95eb855a2#151
[4] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js?rev=f7bd120de6e0#501
[5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/Axis.java?rev=2878bf6cbc1f#64

Comment 11

[Prabhjyot Sodhi [:psd]]

Hi,
I went through on B2G's c++ implementation of this idea.
The areas of concern are :
1: I will have to declare variables that hold the last fling time and velocity.
   or are they already there ? I didn't find any.
2: I am not exactly sure that I have understood timeDelta clearly.
   I have understood it as the difference in time in which we move from pos to some mTouchPos and hence we calculate the velocity as distance / timeDelta.
   timeDelta will not be used in this bug, right ?

Comment 12

[Prabhjyot Sodhi [:psd]]

One more doubt:
When you said we had to accelerate, Do you want me make a new function corresponding to the Accelerate function in the c++ code ?
Thanks :)

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Prabhjyot Sodhi from comment #11)
> 1: I will have to declare variables that hold the last fling time and
> velocity.
>    or are they already there ? I didn't find any.

Yeah, you will need to add new variables to hold this information. Right now they are not needed so we don't store them anywhere.

> 2: I am not exactly sure that I have understood timeDelta clearly.
>    I have understood it as the difference in time in which we move from pos
> to some mTouchPos and hence we calculate the velocity as distance /
> timeDelta.
>    timeDelta will not be used in this bug, right ?

Assuming you're referring to the timeDelta parameter to the updateWithTouchAt function, that's correct. And yes, you should not need to use timeDelta for this bug.

(In reply to Prabhjyot Sodhi from comment #12)
> One more doubt:
> When you said we had to accelerate, Do you want me make a new function
> corresponding to the Accelerate function in the c++ code ?
> Thanks :)

That would probably be best, yes. That way the code is more self-contained and easier to read/review.

Comment 14

[Prabhjyot Sodhi [:psd]]

Great!

I did try and make some naive changes to Axis.java
But I am probably doing something stupid since when I do a second fling, it just stops abruptly ( not the application, just the scrolling)

Kindly have a look at this :
Please ignore the naive variable naming etc, will make amends after the problem is solved :)

https://pastebin.mozilla.org/8112611

Comment 15

[Kartikaya Gupta (email:[e-mail])]

That looks like a pretty good start! I think the piece you're missing is that you still need to run mFlingState = FlingStates.FLINGING; in the accelerate case. At the if condition in line 14 mFlingState will never be FLINGING already (it will most likely be PANNING) and so that half of the if condition can be removed, and you need to make sure to update mFlingState regardless of whether or not you accelerate.

Comment 16

[Prabhjyot Sodhi [:psd]]

Attachment: 721421.patch

Comment 17

[Prabhjyot Sodhi [:psd]]

Noticed some stupid comments just after putting the patch up.
Will correct them with the your suggestions :)

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540840 [details] [diff] [review]
721421.patch

Review of attachment 8540840 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty good! I have some pretty minor stylistic comments below and a couple of comments about the functionality. And nice work on hooking up the prefs!

::: mobile/android/base/gfx/Axis.java
@@ +13,5 @@
>  import org.mozilla.gecko.util.FloatUtils;
>  
>  import android.util.Log;
>  import android.view.View;
> +import android.os.SystemClock;

nit: move this up a couple of lines so that this block of import statements is in alphabetical order

@@ +30,5 @@
>      private static final String PREF_SCROLLING_MAX_EVENT_ACCELERATION = "ui.scrolling.max_event_acceleration";
>      private static final String PREF_SCROLLING_OVERSCROLL_DECEL_RATE = "ui.scrolling.overscroll_decel_rate";
>      private static final String PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT = "ui.scrolling.overscroll_snap_limit";
>      private static final String PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE = "ui.scrolling.min_scrollable_distance";
> +    private static final String PREF_FLING_ACCEL_INTERVAL = "ui.scrolling.fling_accel_interval";                                                // new

remove the "new" comment

@@ +32,5 @@
>      private static final String PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT = "ui.scrolling.overscroll_snap_limit";
>      private static final String PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE = "ui.scrolling.min_scrollable_distance";
> +    private static final String PREF_FLING_ACCEL_INTERVAL = "ui.scrolling.fling_accel_interval";                                                // new
> +    private static final String PREF_FLING_ACCEL_BASE_MULTIPLIER = "ui.scrolling.fling_accel_base_multiplier";
> +    private static final String PREF_FLING_ACCEL_SUPPLEMENTAL_MULTIPLIER = "ui.scrolling.fling_accel_SUPPLEMENTAL_multiplier";

lowercase SUPPLEMENTAL

@@ +116,5 @@
>          MAX_EVENT_ACCELERATION = getFloatPref(prefs, PREF_SCROLLING_MAX_EVENT_ACCELERATION, GeckoAppShell.getDpi() > 300 ? 100 : 40);
>          OVERSCROLL_DECEL_RATE = getFloatPref(prefs, PREF_SCROLLING_OVERSCROLL_DECEL_RATE, 40);
>          SNAP_LIMIT = getFloatPref(prefs, PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT, 300);
>          MIN_SCROLLABLE_DISTANCE = getFloatPref(prefs, PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE, 500);
> +        FLING_ACCEL_INTERVAL = ( long ) getFloatPref(prefs, PREF_FLING_ACCEL_INTERVAL, 500);

Change this to getIntPref, and remove the (long) cast

@@ +118,5 @@
>          SNAP_LIMIT = getFloatPref(prefs, PREF_SCROLLING_OVERSCROLL_SNAP_LIMIT, 300);
>          MIN_SCROLLABLE_DISTANCE = getFloatPref(prefs, PREF_SCROLLING_MIN_SCROLLABLE_DISTANCE, 500);
> +        FLING_ACCEL_INTERVAL = ( long ) getFloatPref(prefs, PREF_FLING_ACCEL_INTERVAL, 500);
> +        FLING_ACCEL_BASE_MULTIPLIER = getFloatPref(prefs , PREF_FLING_ACCEL_BASE_MULTIPLIER, 1.0F);
> +        FLING_ACCEL_SUPPLEMENTAL_MULTIPLIER = getFloatPref(prefs , PREF_FLING_ACCEL_SUPPLEMENTAL_MULTIPLIER, 1.0F);

nit: (above two lines) remove space before the comma, and use a lowercase f after 1.0

@@ +153,5 @@
>      private int mRecentVelocityCount;       /* Number of values put into mRecentVelocities (unbounded). */
>      private boolean mScrollingDisabled;     /* Whether movement on this axis is locked. */
>      private boolean mDisableSnap;           /* Whether overscroll snapping is disabled. */
>      private float mDisplacement;
> +    private long now;

"now" doesn't need to be a class variable. You can just make a local variable inside startFling.

@@ +199,5 @@
>          mScrollingDisabled = false;
>          mFirstTouchPos = mTouchPos = mLastTouchPos = pos;
>          mRecentVelocityCount = 0;
> +        mLastFlingVelocity = 0.0f;
> +        mLastFlingTime = 0;

I think initializing these here is wrong, because they will get reset when you put your finger down to start the second fling. I don't think you need these lines at all; they will be initialized to default values when the Axis class is created, and you should never need to reset them after that.

@@ +328,5 @@
>          }
>          return average / usablePoints;
>      }
>  
> +    float accelerate(float aVelocity , float aLastFlingVelocity){

In the java code the style is to not have the "a" prefix on function parameters. So please rename these to just "float velocity" and "float lastFlingVelocity". Also please remove the space before the comma.

@@ +343,3 @@
>              mVelocity = calculateFlingVelocity();
> +
> +            if (now - mLastFlingTime < FLING_ACCEL_INTERVAL) {

There's just one other thing which I think we should add to this condition, which is the equivalent of the SameDirection check from the C++ version. That is, if you fling upwards and then really quickly fling downwards, you don't want the second fling to be accelerated. So the SameDirection check makes sure that both flings are in the same direction and only then applies the acceleration. In this case you can add "&& Math.signum(mVelocity) == Math.signum(mLastFlingVelocity) and that should do the job.

@@ +343,4 @@
>              mVelocity = calculateFlingVelocity();
> +
> +            if (now - mLastFlingTime < FLING_ACCEL_INTERVAL) {
> +                mVelocity = accelerate(mVelocity,mLastFlingVelocity);

nit: add a space after the comma here

@@ +343,5 @@
>              mVelocity = calculateFlingVelocity();
> +
> +            if (now - mLastFlingTime < FLING_ACCEL_INTERVAL) {
> +                mVelocity = accelerate(mVelocity,mLastFlingVelocity);
> +            } 

nit: remove the trailing whitespace after this brace.

Comment 19

[Prabhjyot Sodhi [:psd]]

I have made the changes.
I have also added the new pref variables to the log (was'nt sure about whether to do it or not)

Comment 20

[Prabhjyot Sodhi [:psd]]

Attachment: 721421.patch

Comment 21

[Prabhjyot Sodhi [:psd]]

Also, this bug solves Bug 1100315, does'nt it? ( am I missing something ?)

Comment 22

[Prabhjyot Sodhi [:psd]]

Attachment: 721421.patch

Comment 23

[Prabhjyot Sodhi [:psd]]

Check the latest patch only. I figured that an error in the second patch caused the build to fail.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Hi Prabhjyot, I built Fennec with this patch locally and ran it on my device but when I do a double-fling it actually stops suddenly instead of getting accelerated. Were you able to run Fennec on your device with this patch? It occurs to me I never asked you to do that and didn't really make it clear how to - I assumed you'd get that from the Fennec wiki page but that was my mistake.

In case you haven't, the section at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_and_deploying_the_Fennec_Android_package in particular describes how to install the package after you've built it, and how to start it on the device. Once you start the modified fennec, load any long page (e.g. planet.mozilla.org) and do two flings to scroll down in quick succession. What I would expect to happen with this bug fixed is that the second fling covers a lot more distance on the page than without this bug fixed, but that doesn't seem to be the case.

Let me know if you are able to reproduce this, or if it works fine for you. We'll need to debug why this is happening - I'm pretty sure I know but I want you to go through the process of debugging it anyway to get some experience with this. If you want to use an IDE to debug there are instructions on how to set that up at https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs. If you prefer debugging via print statements you can do that too (use the android.util.Log class to print things, and they will show up in the logcat which you can access via "adb logcat").

Comment 25

[Prabhjyot Sodhi [:psd]]

I am able to reproduce it. I did test it on my device before I made changes that you suggested. It was stupid of me to assume and post the patch.

I figured out the error by looking at getFloatPref one more time.
Should I make the default constants to be 2000 and 1000 for base and supplemental instead of 1000 and 1000?
The former seems better to use atleast on my device.

Also, I am not able to run ./mach install on my vm, since it is unable to detect my device connected through usb, is there anything that I might be missing?  
Since I believe I will need my device connected or will need a virtual device for debugging.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

(In reply to Prabhjyot Sodhi [:psd] from comment #25)
> I figured out the error by looking at getFloatPref one more time.
> Should I make the default constants to be 2000 and 1000 for base and
> supplemental instead of 1000 and 1000?
> The former seems better to use atleast on my device.

I think the problem is just that you're using 1 instead of 1000 there. Changing both to 1000 should be fine, that will result in a 1.0 value going into the float which should work. When I made that change locally it seemed to fix it.

> Also, I am not able to run ./mach install on my vm, since it is unable to
> detect my device connected through usb, is there anything that I might be
> missing?  
> Since I believe I will need my device connected or will need a virtual
> device for debugging.

Unfortunately I don't have much experience with VMware but in general VMs usually have settings to allow access to the host machine's USB devices. Perhaps the documentation at [1] can help? If not it might be good to ask in #mobile on IRC (although there's not a lot of people around at the moment because of holidays).

[1] https://www.vmware.com/support/ws45/doc/devices_usb_ws.html

Comment 27

[Prabhjyot Sodhi [:psd]]

Attachment: 721421.patch

Comment 28

[Prabhjyot Sodhi [:psd]]

Hi,
(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
 
> I think the problem is just that you're using 1 instead of 1000 there.
> Changing both to 1000 should be fine, that will result in a 1.0 value going
> into the float which should work. When I made that change locally it seemed
> to fix it.
I was wondering whether to try and fine tune the constants for a better a user experience. I have retained the default values in the above patch.
Don't you think there is the need of changing the constants or we leave that to someone else?

> Unfortunately I don't have much experience with VMware but in general VMs
> usually have settings to allow access to the host machine's USB devices.
> Perhaps the documentation at [1] can help? If not it might be good to ask in
> #mobile on IRC (although there's not a lot of people around at the moment
> because of holidays).
> 
> [1] https://www.vmware.com/support/ws45/doc/devices_usb_ws.html
I will go through this document.

Regarding bug 1100315, is there something additional that has to fixed apart from this bug?

Also,I would love to work upon  Bug 1106905(I have never coded in js) or anything that you have in mind for me.
Thanks for your time and patience :)

P.S I am in the process of installing my IDE to start learning how to debug, never really used it seriously

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8541307 [details] [diff] [review]
721421.patch

Review of attachment 8541307 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks! There's just a close-brace which you moved over (in the startTouch) function which shouldn't be there. I can move that back when I land the patch if you don't put up a new patch before then. I'm not at my computer right now but I can land the patch later today when I'm back home.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

(In reply to Prabhjyot Sodhi [:psd] from comment #28)
> I was wondering whether to try and fine tune the constants for a better a
> user experience. I have retained the default values in the above patch.
> Don't you think there is the need of changing the constants or we leave that
> to someone else?

I think the default values are fine for now; we can see what people say and adjust the feedback based on that. Usually the UX team has final say on this, but since they're generally too busy to look at stuff right away using the values from Firefox OS makes sense for now, since those are UX-approved.

> 
> Regarding bug 1100315, is there something additional that has to fixed apart
> from this bug?

Yes, i think there is some additional stuff that needs to be fixed in that bug. This is half of it but in that bug in comment 0 it says "If I fling slowly it should stop faster than it stops now." This is something we can change either by just changing the friction values or by implementing fling curving which is another thing we did in Firefox OS (bug 1091049). If that's something you want to work on I can definitely mentor you through that as well.

> Also,I would love to work upon  Bug 1106905(I have never coded in js) or
> anything that you have in mind for me.
> Thanks for your time and patience :)
> 
> P.S I am in the process of installing my IDE to start learning how to debug,
> never really used it seriously

Sure, let's do bug 1100315 first and then we can look at bug 1106905 after that.

Comment 31

[Prabhjyot Sodhi [:psd]]

Hey kats,
That is great :)

BTW Which IRC channel are you generally online on? and what will be the best time to find you?

Comment 32

[Kartikaya Gupta (email:[e-mail])]

(In reply to Prabhjyot Sodhi [:psd] from comment #31)
> Hey kats,
> That is great :)
> 
> BTW Which IRC channel are you generally online on? and what will be the best
> time to find you?

I'm usually in #mobile and #gfx among others. Generally I'm online between 9am and 5pm eastern time on weekdays but today and tomorrow are holidays so they're a bit of an exception.

 https://hg.mozilla.org/integration/fx-team/rev/b75272e44087

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b75272e44087

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Thanks for getting this done, Prabhjyot! I assigned you the fling curving bug as well since you said you were interested in doing that, but let me know if that's not the case.