Closed Bug 721421 Opened 9 years ago Closed 6 years ago

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

Categories

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

ARM
Android
defect

Tracking

(fennec+)

RESOLVED FIXED
Firefox 37
Tracking Status
fennec + ---

People

(Reporter: pcwalton, Assigned: psd, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(2 files, 2 obsolete files)

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.
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.
tracking-fennec: --- → ?
tracking-fennec: ? → +
Priority: -- → P2
See related bug https://bugzilla.mozilla.org/show_bug.cgi?id=708765 - that might be a better solution.
See Also: → 708765
Ian, what sort of physics do you want here?
Flags: needinfo?(ibarlow)
Whiteboard: [mentor=kats]
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.
See Also: 708765
Whiteboard: [mentor=kats] → [mentor=kats][lang=java]
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"
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.
(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.
Flags: needinfo?(ibarlow)
Both would be fine !
Mentor: bugmail.mozilla
Whiteboard: [mentor=kats][lang=java] → [lang=java]
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
Assignee: nobody → prabhjyotsingh95
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 ?
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 :)
(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.
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
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.
Attached patch 721421.patch (obsolete) — Splinter Review
Attachment #8540840 - Flags: review?(bugmail.mozilla)
Noticed some stupid comments just after putting the patch up.
Will correct them with the your suggestions :)
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.
Attachment #8540840 - Flags: review?(bugmail.mozilla) → feedback+
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)
Attached patch 721421.patch (obsolete) — Splinter Review
Also, this bug solves Bug 1100315, does'nt it? ( am I missing something ?)
Attached patch 721421.patchSplinter Review
Attachment #8541089 - Attachment is obsolete: true
Check the latest patch only. I figured that an error in the second patch caused the build to fail.
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").
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.
(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
Attached patch 721421.patchSplinter Review
Attachment #8541307 - Flags: review?(bugmail.mozilla)
Hi,
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8541307 - Flags: review?(bugmail.mozilla) → review+
(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.
Hey kats,
That is great :)

BTW Which IRC channel are you generally online on? and what will be the best time to find you?
(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
https://hg.mozilla.org/mozilla-central/rev/b75272e44087
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
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.
Depends on: 1117010
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.