Closed Bug 639179 Opened 9 years ago Closed 9 years ago

Improve kinetic panning

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

defect
Not set

Tracking

(fennec-)

VERIFIED FIXED
Firefox 5
Tracking Status
fennec - ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Keywords: polish)

Attachments

(5 files, 1 obsolete file)

This problem has been around for a while, but it hadn't been obvious until checkerboarding was a little smoother. We were ignoring drags made by the user until they hit kinetic panning.
Attached patch Consecutive pans are jumpy (obsolete) — Splinter Review
Attachment #517141 - Flags: review?(mbrubeck)
tracking-fennec: --- → ?
Attachment #517141 - Flags: review?(mbrubeck) → review+
Assignee: nobody → ben
Keywords: polish
Whiteboard: [has patch][has review][needs approval]
Blocks: 639677
Comment on attachment 517141 [details] [diff] [review]
Consecutive pans are jumpy


>+        this._dragBy(this.dX, this.dY);
>+        // dragBy will reset dX and dY values to 0.


I'd like the comment before the code
Attachment #517141 - Flags: approval2.0+
Whiteboard: [has patch][has review][needs approval] → [has patch][has review][has approval][can land with nit fix]
Comment on attachment 517141 [details] [diff] [review]
Consecutive pans are jumpy

Please don't land this without sorting out the speed issues
Attachment #517141 - Flags: approval2.0+ → approval2.0-
Whiteboard: [has patch][has review][has approval][can land with nit fix] → [has patch][has review]
tracking-fennec: ? → 2.0-
Attachment #524702 - Flags: review+
Whiteboard: [has patch][has review]
Comment on attachment 524703 [details] [diff] [review]
Part 2: Carefully track values sent to dragger

Thanks for breaking up this patch into chunks I can understand even in my current sleep-deprived state. :)
Attachment #524703 - Flags: review?(mbrubeck) → review+
Attachment #517141 - Attachment is obsolete: true
Comment on attachment 524704 [details] [diff] [review]
Part 3: Tapping during kinetic panning stops panning

>+    if (!dragData.isPan()) {
>+      this._kinetic.end();
>       this._dragger.dragStop(0, 0, this._targetScrollInterface);
>       this._dragger = null;
>     } else if (dragData.isPan()) {

Change "else if" to just "else" (and maybe swap the order of the if/else blocks).

>+      if (Date.now() - this._dragStartTime > 300) {
>+        // XXX
>+        this._kinetic._velocity.set(0, 0);
>+      }

Remove the XXX comment and the curly braces.

>+      currentVelocityX = clampFromZero(this._velocity.x + this._acceleration.x * currentTime, 0, 6);
>+      currentVelocityY = clampFromZero(this._velocity.y + this._acceleration.y * currentTime, 0, 6);

This could be Util.clamp(value, -6, 6) instead of clampFromZero(value, 0, 6).

>+    this._velocity.x = clampFromZero((distanceX / swipeLength) + currentVelocityX, Math.abs(currentVelocityX), 6);
>+    this._velocity.y = clampFromZero((distanceY / swipeLength) + currentVelocityY, Math.abs(currentVelocityY), 6);

Please replace "6" with a named constant (in these lines and the previous ones).

>+  resume: function resume() {
>+    if (this._paused)
>+      this._startTimer();
>+  },

Is resume() actually needed?
Attachment #524704 - Flags: review?(mbrubeck) → review+
Comment on attachment 524705 [details] [diff] [review]
Part 4: Panning speed determines initial kinetic speed

>+    this._velocity.x = clampFromZero((distanceX / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityX, Math.abs(currentVelocityX), 6);
>+    this._velocity.y = clampFromZero((distanceY / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityY, Math.abs(currentVelocityY), 6);

You could pull "Math.min(swipeLength, lastTime - mb[0].t))" out into a local variable.
Attachment #524705 - Flags: review?(mbrubeck) → review+
Attachment #524706 - Flags: review?(mbrubeck) → review+
> Change "else if" to just "else" (and maybe swap the order of the if/else
> blocks).

OK.

> Remove the XXX comment and the curly braces.

Done.

> This could be Util.clamp(value, -6, 6) instead of clampFromZero(value, 0, 6).

Good call. This works for this code, but not for the later code. Changed here, but still need clampFromZero sadly.

> Please replace "6" with a named constant (in these lines and the previous
> ones).

Done.

> Is resume() actually needed?

Nope! Removed.

> >+    this._velocity.x = clampFromZero((distanceX / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityX, Math.abs(currentVelocityX), 6);
> >+    this._velocity.y = clampFromZero((distanceY / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityY, Math.abs(currentVelocityY), 6);
> 
> You could pull "Math.min(swipeLength, lastTime - mb[0].t))" out into a local
> variable.

Done.
Summary: Consecutive pans are jumpy → Improve kinetic panning
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out due to massive mobile unittest orange following cedar merge (as the most suspicious candidate):
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=fe3f7889918b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Not sure if bug 652219 is related to the changes?
Re-landed on 2011-04-11:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=65740bbb1aa8
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 5
Blocks: 653772
Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Droid 2 
OS: Android 2.2

Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.