705114 - Kinetic panning doesn't work on certain devices (such as the HTC Flyer)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

On some devices, kinetic panning doesn't work, as it uses only the last two motion events to calculate the velocity of the pan.

On the HTC Flyer, this breaks as the last two motion events before a touch-up event always have identical coordinates. Someone else mentioned having the same problem on IRC, but I forget what device they had - it's fair to say that this is likely in more than one device in the wild, so we ought to do something about it.

My previous attempt was to just use the Android gestures system (which works correctly), but this introduces some latency in our response to touch events (see bug #703311).

This time, I've introduced some filtering on the calculated velocity so that it isn't allowed to suddenly drop off in the space of a single event.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Add velocity change filtering

Introduce a velocity change factor, so that the velocity can only change by a
certain amount per event. This has the effect of smoothing velocity changes
and fixes the bug on the HTC Flyer, at least.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Add velocity change filtering (revised)

Realised, this really ought to be time-based, as event delivery frequency can (and does) vary between devices.

As a bonus, implemented handling of historical motion events.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Add velocity change filtering (revised)

Sorry for the spam, accidentally left some logging in.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Add velocity change filtering (revised, unbreak axis-lock)

And then I realised it broke axis-lock.

Comment 5

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

Comment on attachment 577248 [details] [diff] [review]
Add velocity change filtering (revised, unbreak axis-lock)

>+    // The maximum velocity change factor between events, in pixels/ms.
>+    // Direction changes are excluded.
>+    private static final float MAX_EVENT_ACCELERATION = 0.012f;

I don't think this is pixels/ms. Based on the way it's used, it's more like maximum allowed % change in velocity, per ms. (Code is fine, but I think the comment should be updated).

>                 // lock to x-axis
>-                y = mY.firstTouchPos;
>+                mY.locked = true;
>             } else if (Math.abs(angle - (Math.PI / 2)) < AXIS_LOCK_ANGLE) {
>                 // lock to y-axis
>-                x = mX.firstTouchPos;
>+                mX.locked = true;

I'm a little concerned that these hunks lock an axis without unlocking the other axis. I think there might be cases where you can drag along one axis, then move back to the firstTouchPos, and drag along the other axis, ending up with both axes locked, and then you have the break the axis lock entirely to get it to drag at all. Can you try this scenario and make sure it works?

>+    private void track(MotionEvent event) {
>+        mX.lastTouchPos = mX.touchPos;
>+        mY.lastTouchPos = mY.touchPos;
>+
>+        for (int i = 0; i < event.getHistorySize(); i++) {
>+            float x = event.getHistoricalX(0, i);
>+            float y = event.getHistoricalY(0, i);

Have you been able to exercise this code? It looks fine as far as I can tell but just wondering if you were able to test it on a device or in the emulator.

>+            track (x, y, mX.touchPos, mY.touchPos, timeDelta);

nit - space between "track" and "("

>+        track (event.getX(0), event.getY(0), mX.touchPos, mY.touchPos, timeDelta);

ditto here.

>         public float firstTouchPos;             /* Position of the first touch. */
>         public float touchPos;                  /* Position of the last touch. */
>+        public float lastTouchPos;              /* Position of the touch before. */

The comments/variable names are confusing here - touchPos is the last touch, but lastTouchPos is the touch before? Seems almost-backwards. I'd update the comments so that they're clearer - "Position of the first point on the current touch/drag", "Position of the current point on the current touch/drag", and "Final position of the last touch/drag", if I understand correctly.

r+ with comment/nit fixes and assuming the axis lock can't get applied to both axes at the same time. (If it does, then r+ for unlocking the other axis when you lock one axis)

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #5)
> Comment on attachment 577248 [details] [diff] [review] [diff] [details] [review]
> Add velocity change filtering (revised, unbreak axis-lock)
> 
> >+    // The maximum velocity change factor between events, in pixels/ms.
> >+    // Direction changes are excluded.
> >+    private static final float MAX_EVENT_ACCELERATION = 0.012f;
> 
> I don't think this is pixels/ms. Based on the way it's used, it's more like
> maximum allowed % change in velocity, per ms. (Code is fine, but I think the
> comment should be updated).

Thanks, left over from when I made it work a little differently. Fixed (although debatable whether it should be pixel/ms instead of change/ms...)

> >                 // lock to x-axis
> >-                y = mY.firstTouchPos;
> >+                mY.locked = true;
> >             } else if (Math.abs(angle - (Math.PI / 2)) < AXIS_LOCK_ANGLE) {
> >                 // lock to y-axis
> >-                x = mX.firstTouchPos;
> >+                mX.locked = true;
> 
> I'm a little concerned that these hunks lock an axis without unlocking the
> other axis. I think there might be cases where you can drag along one axis,
> then move back to the firstTouchPos, and drag along the other axis, ending
> up with both axes locked, and then you have the break the axis lock entirely
> to get it to drag at all. Can you try this scenario and make sure it works?

You were right, fixed.

> >+    private void track(MotionEvent event) {
> >+        mX.lastTouchPos = mX.touchPos;
> >+        mY.lastTouchPos = mY.touchPos;
> >+
> >+        for (int i = 0; i < event.getHistorySize(); i++) {
> >+            float x = event.getHistoricalX(0, i);
> >+            float y = event.getHistoricalY(0, i);
> 
> Have you been able to exercise this code? It looks fine as far as I can tell
> but just wondering if you were able to test it on a device or in the
> emulator.

Works on my HTC Flyer - you very rarely get historical events in practice though.

> >+            track (x, y, mX.touchPos, mY.touchPos, timeDelta);
> 
> nit - space between "track" and "("
> 
> >+        track (event.getX(0), event.getY(0), mX.touchPos, mY.touchPos, timeDelta);
> 
> ditto here.

Fixed... Old habits die hard :)

> >         public float firstTouchPos;             /* Position of the first touch. */
> >         public float touchPos;                  /* Position of the last touch. */
> >+        public float lastTouchPos;              /* Position of the touch before. */
> 
> The comments/variable names are confusing here - touchPos is the last touch,
> but lastTouchPos is the touch before? Seems almost-backwards. I'd update the
> comments so that they're clearer - "Position of the first point on the
> current touch/drag", "Position of the current point on the current
> touch/drag", and "Final position of the last touch/drag", if I understand
> correctly.

Added clarification in comments.

> r+ with comment/nit fixes and assuming the axis lock can't get applied to
> both axes at the same time. (If it does, then r+ for unlocking the other
> axis when you lock one axis)

Will attach fixed patch for reference and push, thanks!

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Add velocity change filtering (review comments addressed)

Comment 8

[Chris Lord [:cwiiis]]

http://hg.mozilla.org/projects/birch/rev/4e745f151abd