Closed Bug 1866904 Opened 1 year ago Closed 1 year ago

AxisPhysicsMSDModel can overshoot destination even when the damping ratio is >= 1

Categories

(Core :: Panning and Zooming, defect, P3)

defect

Tracking

()

RESOLVED INVALID

People

(Reporter: botond, Unassigned)

References

Details

Attachments

(1 file)

We have observed AxisPhysicsMSDModel exhibiting "overshoot" behaviour, where the position takes on values which are past the destination (e.g. if the starting position is < the destination, then the position can take on values > the destination) over the course of the animation, even when the damping ratio is >= 1.

We've observed this in two places where we use AxisPhysicsMSDModel in APZ:

  1. Overscroll animations (bug 1720240 comment 19). We use a damping ratio of 1.1 for these.
  2. MSD smooth scroll animations (bug 1846935 comment 14). We use a damping ratio of 1 for these.

I attached a JS file which demonstrates a set of initial conditions that triggers the overshoot behaviour with a damping ratio of 1 (in a JS implementation of AxisPhsyicsMSDModel that Markus sent me, which follows the C++ one closely).

My understanding is that overshoot should not happen when the damping ratio is >= 1, and this therefore indicates a bug in AxisPhyscisMSDModel.

Setting P3:S3 since this issue has been there and has been exposed in overscrolling cases.

Severity: -- → S3
Priority: -- → P3

I'm still trying to understand the physics here. Everything I'm reading tells me that critically damped springs should never overshoot, even with a high initial velocity.

Yet, the graph that's plotted on this page clearly shows that we're overshooting:

https://www.wolframalpha.com/input?i=w%22%28x%29+%2B+2*1*sqrt%2820%29*w%27%28x%29+%2B+20*w%28x%29+%3D+0%2C+w%280%29+%3D+-500*60%2C+w%27%280%29+%3D+500000

Botond, can you see if this makes a difference?

diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -4037,10 +4037,20 @@ void AsyncPanZoomController::SmoothMsdSc
                       Metrics().GetZoom();
   }
 
+  auto springConstant = StaticPrefs::layout_css_scroll_behavior_spring_constant();
+
+  // Limit the current velocity by a somewhat arbitrary amount in order to
+  // hopefully ensure that the animation doesn't overshoot the destination.
+  auto initialPosition = Metrics().GetVisualScrollOffset();
+  auto velocityLimitX = sqrt(springConstant) * abs(aDestination.mPosition.x - initialPosition.x);
+  auto velocityLimitY = sqrt(springConstant) * abs(aDestination.mPosition.y - initialPosition.y);
+  initialVelocity.x = clamped(initialVelocity.x.value, -velocityLimitX, velocityLimitX);
+  initialVelocity.y = clamped(initialVelocity.y.value, -velocityLimitY, velocityLimitY);
+
   StartAnimation(do_AddRef(new SmoothMsdScrollAnimation(
-      *this, Metrics().GetVisualScrollOffset(), initialVelocity,
+      *this, initialPosition, initialVelocity,
       aDestination.mPosition,
-      StaticPrefs::layout_css_scroll_behavior_spring_constant(),
+      springConstant,
       StaticPrefs::layout_css_scroll_behavior_damping_ratio(),
       std::move(aDestination.mTargetIds), aTriggeredByScript)));
 }

It's based on an idea from Bas: We calculate the maximum velocity that an undamped spring would achieve if it were initialized with zero velocity and the same offset from the destination. With that velocity, we hope that the critically-damped spring would not overshoot.

(In reply to Markus Stange [:mstange] from comment #3)

Botond, can you see if this makes a difference?

The fix needs to go in a different place (*), but yes, adding this type of velocity clamping there seems to fix the issue in local runs.

(* The codepath in comment 3 is used for script-driven smooth scroll animations. The affected scenario I ran into in bug 1846935 involves smooth scroll animations for a user input event, updated with a new destination when another input event is received.)

So, does this suggest that we're modelling something unrealistically when updating the destination of a smooth scroll animation?

Oh hey, look what I found:

@param aInitialVelocity sets the initial velocity of the simulated spring,
in AppUnits / second. Critically-damped and over-damped systems are
guaranteed not to overshoot aInitialDestination if this is set to 0;
however, it is possible to overshoot and oscillate if not set to 0 or
the system is under-damped.

Ah, great find! That at least means things are working as expected.

I'm going to close this as INVALID since the overshoot is not actually unexpected if the initial velocity is high enough.

I put in place a mitigation for the overshoot based on the approach in comment 3 in bug 1846935 (D195353).

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: