1100315 - Implement fling curving in java PZC

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michal Z.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141030112145

Steps to reproduce:

Scroll down endless website and then try to quick return to the top (flinging). Or just slowly fling website. I'm using Samsung Galaxy S4 running stock Samsung KitKat ROM.


Actual results:

Fling event feels weird. It does not feel like flinging Android's standard scrollable containers. I think that when I fling during a fling, the speed is not increased. It also feels "different" if I fling slowly. It's slowing down a little bit slower than it should like there was used a different interpolator or something. I tried to compare it to G+, Twitter and Chrome apps and all of them feels the same. I think Firefox should also be like this.


Expected results:

It should be like Android standard containers. If I fling during a fling then speed should be increased. If I fling slowly it should stop faster than it stops now.

Comment 1

[Lucas Rocha (:lucasr)]

Thanks for the report.  snorp, thoughts?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

It seems ok to me, but we've had several folks make this complaint recently.

It would be nice if Android exposed the fling interpolation stuff somewhere so we could just use exactly the same math they have, but I don't see that anywhere. We should still probably revisit what we're doing. Kats knows the most about this.

Comment 3

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

(In reply to Michal Z. from comment #0)
> Fling event feels weird. It does not feel like flinging Android's standard
> scrollable containers.

Correct, we're not using that.

> I think that when I fling during a fling, the speed
> is not increased.

Correct, this is bug 721421.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> It would be nice if Android exposed the fling interpolation stuff somewhere
> so we could just use exactly the same math they have, but I don't see that
> anywhere. We should still probably revisit what we're doing. Kats knows the
> most about this.

I know I keep saying this but all of this is going to change once we switch to the C++ APZ on Fennec. It would be great if we could get some people from mobile platform to help on this as I haven't had much time to work on it.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> 
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #2)
> > It would be nice if Android exposed the fling interpolation stuff somewhere
> > so we could just use exactly the same math they have, but I don't see that
> > anywhere. We should still probably revisit what we're doing. Kats knows the
> > most about this.
> 
> I know I keep saying this but all of this is going to change once we switch
> to the C++ APZ on Fennec. It would be great if we could get some people from
> mobile platform to help on this as I haven't had much time to work on it.

Do we have a good idea of what needs to be done in order to turn that on?

Comment 5

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

Bug 1055557 was next on my list. Feel free to steal it :) After that it's mostly a matter of hammering on it to see what's broken and fix things up bit by bit.

Comment 6

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

Bug 721421 is almost landed now and :psd said he wanted to look into fixing the rest of this bug which i believe can done by implementing fling curving as we did for B2G.

Comment 7

[Prabhjyot Sodhi [:psd]]

Now that bug 721421 is landed.
Let's get started!

Comment 8

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

Great. Take a look at the code at [1] (and maybe read through bug 1091049 which added the code) as an example. You basically want to apply the same transformation to the newVelocity computed at [2]. The gVelocityCurveFunction is an implementation of a bezier curve, but I don't know if there's an equivalent in the android library. If not you might have to write it. Take a look through these bits of code and let me know if you understand the goal here. Once you understand what needs to be done we can sort out more of the implementation details.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?rev=3910f8c77b51#78
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/Axis.java?rev=b75272e44087#215

Comment 9

[Prabhjyot Sodhi [:psd]]

I think I have got the essence of what is to be done, still I will go through the corresponding code and bug comments to get it right.

But right now I am facing a problem building Fennec.
I get the following output :
http://pastebin.mozilla.org/8139252

I have tried downgrading psutil to 1.0.0 as suggested in bug 930808, but I have the same output.
Thanks :)

Comment 10

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

Hm, I'm not sure what would cause that error. Did you make changes to your mozconfig since you successfully built last time? The error seems to be related to the mozconfig parsing module in the build system.

Comment 11

[Prabhjyot Sodhi [:psd]]

Hey Kats!
I setup a fresh built and upgraded psutil to 2.1.0 and have it up and running again. :)

Alrighty,
Regarding the bug: I think I am clear with what is to be achieved.
Also, I think, I will have to write the implementation myself since I didn't find any in the android library.

Comment 12

[Prabhjyot Sodhi [:psd]]

Hey kats!
In https://bugzilla.mozilla.org/show_bug.cgi?id=1091049#c14 you have mentioned the constants to be : 
0,0,0,58,1
But according to what I have read and understood, I would need Control Points for the brezier equation.
So What do your constants represent?

Also, for fixing bug 721421, I made 3 new prefs, I just added and used them inside in Axis.java, I did not make any additions corresponding to the bugs outside, is this why bug 1117010 has come up?
Thanks :)

Comment 13

[Prabhjyot Sodhi [:psd]]

Attachment: Mathematical repersentation of berzier curve

Kats: Check it out

Comment 14

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

(In reply to Prabhjyot Sodhi [:psd] from comment #12)
> Hey kats!
> In https://bugzilla.mozilla.org/show_bug.cgi?id=1091049#c14 you have
> mentioned the constants to be : 
> 0,0,0,58,1
> But according to what I have read and understood, I would need Control
> Points for the brezier equation.
> So What do your constants represent?

The two control points for the equation are (0, 0) and (0.58, 1). (The other two control points are (0, 0) and (1, 1)).

> Also, for fixing bug 721421, I made 3 new prefs, I just added and used them
> inside in Axis.java, I did not make any additions corresponding to the bugs
> outside, is this why bug 1117010 has come up?

Yeah, that was my mistake. I should have remembered that in Fennec the prefs always need to be added to the prefs file. I've written a patch to do this and will get it landed so you don't need to worry about it. Sorry about that!

Comment 15

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

(In reply to Prabhjyot Sodhi [:psd] from comment #13)
> Created attachment 8543264 [details]
> Mathematical repersentation of berzier curve
> 
> Kats: Check it out

Nice! So the equations for Bx and By look good here, but the last bit where you talk about solving for t and so on isn't right. What we need to do is to take the velocity's x and y components and plug them in for t in Bx and By, and then use the resulting values of Bx and By and convert them back to velocities. The picture I drew in attachment 8513736 [details] might help you visualize it.

The "threshold" and "max" values are also prefs in the C++ code, with values of 0.03 and 0.07 inches/ms. We can use the same values in the Java code, but since the velocities are stored in pixels/frame we'll have to multiply the pref values by GeckoAppShell.getDpi() and MS_PER_FRAME when comparing to the velocity.

Does that make sense?

Comment 16

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> The two control points for the equation are (0, 0) and (0.58, 1). (The other
> two control points are (0, 0) and (1, 1)).

Oh! I forgot that since that code landed we have changed the curve a bit. The control points we are currently using are (0.41, 0.0) and (0.80, 1.0) (see [1]).

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js?rev=08ad3521ebe6#995

Comment 17

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Hey!
I have implemented a the basic, with no prefs set.
I have tested it on my device, but I am not sure how it is supposed to feel.
Give it a try and I'll then add the prefs :)

P.S For the newton-raphson method I have used the value of By itself as the first guess. (I wrote a java program that showed that this approximation is decent)

Comment 18

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Hey!
I have implemented a the basic, with no prefs set.
I have tested it on my device, but I am not sure how it is supposed to feel.
Give it a try and I'll then add the prefs :)

P.S For the newton-raphson method I have used the value of By itself as the first guess. (I wrote a java program that showed that this approximation is decent)

Comment 19

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Figured an error in my logic!
Sorry for the spamming

Comment 20

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

Comment on attachment 8543860 [details] [diff] [review]
1100315.patch

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

This looks pretty good for a first cut. The code will likely change quite a bit as you hook up the prefs but assuming hard-coded values this all looks fine. There's one bug which I noted below. Also since it's pretty hard to visually observe the effect of this change, what I do is add a couple of log statements in the code to print out the newVelocity before and after the curve is applied to make sure the values seem reasonable.

::: mobile/android/base/gfx/Axis.java
@@ +245,3 @@
>          float newVelocity = (mTouchPos - pos) / timeDelta * MS_PER_FRAME;
>  
> +        if(newVelocity > curveVelocityThreshold && newVelocity < maxVelocity)

Note that newVelocity could be negative as well. If that's the case we should take the absolute value of it, run this code, and then make the post-curve newVelocity negative again.

Comment 21

[Prabhjyot Sodhi [:psd]]

Hi kats,
okay then, I will add the prefs and correct the bug that you pointed out.

off-topic though: where can I find, maybe a list of mozilla's upcoming projects or current projects that I could directly contribute code to along with the bug-fixing. I was interested in GSoC and doing it with mozilla will be an amazing opportunity for me

Comment 22

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

(In reply to Prabhjyot Sodhi [:psd] from comment #21)
> Hi kats,
> okay then, I will add the prefs and correct the bug that you pointed out.

Thanks!

> off-topic though: where can I find, maybe a list of mozilla's upcoming
> projects or current projects that I could directly contribute code to along
> with the bug-fixing. I was interested in GSoC and doing it with mozilla will
> be an amazing opportunity for me

Mozilla has a page for GSoC projects at https://wiki.mozilla.org/SummerOfCode but for now there's nothing listed yet for 2015. You could take a look at some of the projects for previous years and see if any of them inspire you to work on anything in particular. I also asked mhoye (our engineering community manager) and he said we don't have a good list of this kind of stuff but there's a list of other "important but unassigned" bugs that you could look at at http://tinyurl.com/lgh3hl7 to see if any of those strike your fancy. Hope that helps!

Comment 23

[Prabhjyot Sodhi [:psd]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> 
> Mozilla has a page for GSoC projects at
> https://wiki.mozilla.org/SummerOfCode but for now there's nothing listed yet
> for 2015. You could take a look at some of the projects for previous years
> and see if any of them inspire you to work on anything in particular. I also
> asked mhoye (our engineering community manager) and he said we don't have a
> good list of this kind of stuff but there's a list of other "important but
> unassigned" bugs that you could look at at http://tinyurl.com/lgh3hl7 to see
> if any of those strike your fancy. Hope that helps!

Will look into them! Thanks a lot :)

Comment 24

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Hey!

btw kats, How do you check for the small nits like whitespaces and stuff like that. Is there a tool or you just go through the code?

Comment 25

[Botond Ballo [:botond]]

(In reply to Prabhjyot Sodhi [:psd] from comment #24)
> btw kats, How do you check for the small nits like whitespaces and stuff
> like that. Is there a tool or you just go through the code?

For trailing whitespace, I find it best to configure my editor to avoid inserting it in the first place. For example, I use Eclipse as my editor, and there's an option in its preferences to remove trailing whitespace in edited lines when saving a file.

You can also look at your patch in bugzilla's splinter view (what you see when you click 'review' as if you were going to review the patch), there trailing whitespace is highlighted in red.

Comment 26

[Prabhjyot Sodhi [:psd]]

(In reply to Botond Ballo [:botond] from comment #25)
> For trailing whitespace, I find it best to configure my editor to avoid
> inserting it in the first place. For example, I use Eclipse as my editor,
> and there's an option in its preferences to remove trailing whitespace in
> edited lines when saving a file.
> 
> You can also look at your patch in bugzilla's splinter view (what you see
> when you click 'review' as if you were going to review the patch), there
> trailing whitespace is highlighted in red.

Oh Thanks a lot Botond :)
Yeah, I also use that review trick, but there is this desire to get it right the first time :P

Comment 27

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

Comment on attachment 8545160 [details] [diff] [review]
1100315.patch

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

This looks pretty good! The only comments I have are relating to the code style, once we have that cleaned up a little bit so it's more consistent with the rest of the file we should be good to go! Really nice work :)

::: mobile/android/base/gfx/Axis.java
@@ +40,5 @@
> +    private static final String PREF_FLING_CURVE_FUNCTION_Y2 = "ui.scrolling.fling_curve_function_y2";
> +    private static final String PREF_FLING_CURVE_THRESHOLD_VELOCITY = "ui.scrolling.fling_curve_threshold_velocity";
> +    private static final String PREF_FLING_CURVE_MAXIMUM_VELOCITY = "ui.scrolling.fling_curve_max_velocity";
> +    private static final String PREF_FLING_CURVE_NEWTON_ITERATIONS = "ui.scrolling.fling_curve_newton_iterations";
> +  

nit: trailing whitespace

@@ +69,5 @@
>      private static float FLING_ACCEL_BASE_MULTIPLIER;
>  
>      // The multiplication constant of the supplemental velocity in case of accelerated scrolling.
>      private static float FLING_ACCEL_SUPPLEMENTAL_MULTIPLIER;
> +    

More trailing whitespace here and below (see the red in the splinter view on bugzilla)

@@ +70,5 @@
>  
>      // The multiplication constant of the supplemental velocity in case of accelerated scrolling.
>      private static float FLING_ACCEL_SUPPLEMENTAL_MULTIPLIER;
> +    
> +    // x co-ordinate of the second brezier control point

This should be "bezier" rather than "brezier" (here and throughout the patch)

@@ +256,5 @@
>      }
>  
> +    // Calculates and return the slope of the curve at given parameter t
> +    float getSlope(float t)
> +    {

Move { to the end of the previous line (here and for all functions and loops)

@@ +260,5 @@
> +    {
> +        float y1 = FLING_CURVE_FUNCTION_Y1;
> +        float y2 = FLING_CURVE_FUNCTION_Y2;
> +
> +        return (3*y1 + (6*y2 - 12*y1)*t + (9*y1 - 9*y2 + 3)*t*t );

This is a little hard to read, I would prefer if it where formatted like so:

// derivative of cubic bezier with respect to t, with y0=0 and y3=1
return (3 * y1)
    + t * (6 * y2 - 12 * y1)
    + t * t * (9 * y1 - 9 * y2 + 3);

@@ +268,5 @@
> +    float calcBrez(float t, float By)
> +    {
> +        float y1 = FLING_CURVE_FUNCTION_Y1;
> +        float y2 = FLING_CURVE_FUNCTION_Y2;
> +        float brezVal = 3 *(1-t)*(1-t)*t * y1 + 3*(1-t)*t*t * y2 + t*t*t - By;

It would nice to have a simple "float cubicBezier(float p1, float p2, float t)" function that you can reuse here. Then this function would look like:

return cubicBezier(FLING_CURVE_FUNCTION_Y1, FLING_CURVE_FUNCTION_Y2, t) - By;

and in fact you can just inline it into the flingCurve function callsite. The "functOut" variable that you define later could also use this cubicBezier function passing in the _X1 and _X2 values instead.

@@ +281,5 @@
> +        guess[0] = By;
> +
> +        for (int i = 1; i < ni; i++)
> +        {
> +            guess[i] = guess[i-1] - calcBrez(guess[i-1], By)/getSlope(guess[i-1]);

nit: spaces around '/'

@@ +300,3 @@
>          float newVelocity = (mTouchPos - pos) / timeDelta * MS_PER_FRAME;
>  
> +        if(Math.abs(newVelocity) > curveVelocityThreshold && Math.abs(newVelocity) < maxVelocity)

nit: space after "if"

@@ +300,5 @@
>          float newVelocity = (mTouchPos - pos) / timeDelta * MS_PER_FRAME;
>  
> +        if(Math.abs(newVelocity) > curveVelocityThreshold && Math.abs(newVelocity) < maxVelocity)
> +        {
> +            Log.i("Before Fling", "newVelocity" + newVelocity);

The log lines are fine for debugging but we should take them out for the final patch.

@@ +305,5 @@
> +
> +            float sign = Math.signum(newVelocity);
> +            newVelocity = newVelocity * sign;
> +            float scale = maxVelocity - curveVelocityThreshold;
> +            float functInp = (newVelocity - curveVelocityThreshold)/scale;

spaces around '/'

@@ +308,5 @@
> +            float scale = maxVelocity - curveVelocityThreshold;
> +            float functInp = (newVelocity - curveVelocityThreshold)/scale;
> +            float functOut = flingCurve(functInp);
> +            newVelocity = functOut*scale + curveVelocityThreshold;
> +            newVelocity = newVelocity*sign;

spaces around '*'

Comment 28

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

(In reply to Prabhjyot Sodhi [:psd] from comment #24)
> btw kats, How do you check for the small nits like whitespaces and stuff
> like that. Is there a tool or you just go through the code?

Personally I just go through the code, but as Botond said, your editor/IDE of choice may have features that help you with this. For the most part we use standard Java code style (our specific code style is documented at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style) so if you make your code conform to that it should be fine. There are projects underway to have better code-formatting checks done automatically at review time but that's not done yet so unfortunately there's no "standard Mozilla way" to do it automatically yet :(

Comment 29

[Prabhjyot Sodhi [:psd]]

Hey Kats!
Will make the changes and upload the new attachment (may take a few days, travelling! not sure of a internet connection :) ) 
btw got a badge :) plus bug 721421 got tweeted :))
Thanks a lot for you help and support..
*feels good :P*

Comment 30

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Comment 31

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

Comment on attachment 8545875 [details] [diff] [review]
1100315.patch

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

Looks good! Two things: (1) The commit message still says "brezier" instead of "bezier", and (2) I meant to mention this on the last version but it slipped my mind - we need to add these prefs to the file at mobile/android/app/mobile.js with default values. See my patch on bug 1117010 for an example. Just add the new prefs below the ones I added there. For the values that are float prefs, add them as strings (for example the _X1 pref would be "0.41") and for the iterations pref, that would just be a plain integer without the quotes.

I'm giving this patch an r+ because there's only minor style issues left, but please update a final version of the patch with these things (and the whitespace nits below) corrected and I'll get it landed. Thanks!

::: mobile/android/base/gfx/Axis.java
@@ +259,5 @@
> +        float y1 = FLING_CURVE_FUNCTION_Y1;
> +        float y2 = FLING_CURVE_FUNCTION_Y2;
> +
> +        return (3 * y1)
> +             + t * (6 * y2 - 12 * y1) 

nit: some trailing whitespace here

@@ +265,5 @@
> +    }
> +
> +    // Calculates and returns the value of the bezier curve with the given parameter t and control points p1 and p2
> +    float cubicBezier(float p1, float p2, float t) {
> +        return (3 * t * (1-t) * (1-t) * p1) 

more trailing whitespace here

@@ +286,5 @@
> +        float t = guess[4];
> +
> +        float x1 = FLING_CURVE_FUNCTION_X1;
> +        float x2 = FLING_CURVE_FUNCTION_X2;
> +        

and more here (you can just get rid of this blank line entirely)

Comment 32

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

(In reply to Prabhjyot Sodhi [:psd] from comment #29)
> btw got a badge :) plus bug 721421 got tweeted :))
> Thanks a lot for you help and support..
> *feels good :P*

Awesome! Thank you again for taking the time to write these patches - it is greatly appreciated!

Comment 33

[Prabhjyot Sodhi [:psd]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> For the values that are float prefs, add them as strings (for example the
> _X1 pref would be "0.41") and for the iterations pref, that would just be a
> plain integer without the quotes.

Is adding the float prefs as a String, creating problems? (https://bugzilla.mozilla.org/show_bug.cgi?id=1117010#c5)

Comment 34

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

Comment 35

[Prabhjyot Sodhi [:psd]]

Comment on attachment 8547501 [details] [diff] [review]
1100315.patch

I have made the changes. I have added Strings values for float prefs :)

Comment 36

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

(In reply to Prabhjyot Sodhi [:psd] from comment #33)
> Is adding the float prefs as a String, creating problems?
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1117010#c5)

Yeah, my mistake - I got confused because in C++ code we read float prefs from strings but not in the Java code. I can update your patch accordingly and land it, since it was my fault for telling you the wrong thing :)

Comment 37

[Prabhjyot Sodhi [:psd]]

Hey kats, 
No wories :)
But then how are float values suposed to be added (as -1 in place of their default values?)
I have made another patch with the default float values as -1. if that's the way it is to be done, Then I can upload it!

Comment 38

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

That will work, yes. If we put -1 values in the prefs file then the getFloatPref function will just return the default value that we pass in to it, which is fine. Alternatively we could copy the default values into the prefs file but it probably makes more sense to just leave it as -1 because that's what we do in other places as well.

Comment 39

[Prabhjyot Sodhi [:psd]]

Attachment: 1100315.patch

I have corrected the prefs that you changed in bug 1117010 !!
:)

Comment 40

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

(In reply to Prabhjyot Sodhi [:psd] from comment #39)
> I have corrected the prefs that you changed in bug 1117010 !!
> :)

Ah thanks! I appreciate it, but unfortunately those changes need to be in a separate patch. The reason is that bug 1117010 landed in Firefox 37, and as of today mozilla-central is the Firefox 38 codeline. So the fix will land in Firefox 38 and will need to be backported ("uplifted") to Firefox 37, and for that it's better to keep it in a separate patch.

Comment 41

[Richard Newman [:rnewman]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #38)
> That will work, yes. If we put -1 values in the prefs file then the
> getFloatPref function will just return the default value that we pass in to
> it, which is fine.

Not 100% true -- it'll return the default value you passed in divided by 1000.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/Axis.java#69

Comment 42

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

Comment on attachment 8547585 [details] [diff] [review]
1100315.patch

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

This looks good, I will land it shortly.

Comment 43

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

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

I took out the pref changes for the prefs in bug 1117010, and also made the newton-raphson pref -1 for consistency with the rest.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Aaron Train [:aaronmt]]

This patch is scary. How can this be verified?

Comment 46

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

Kinda hard to verify. The main effect will be that "slow" flings will slow down a little more than they used and "fast" flings will go faster than they used to, but it should be a very subtle effect.