Closed Bug 1402498 Opened 7 years ago Closed 7 years ago

Add an alternative smooth scroll physics implementation based on a Mass-Spring-Damper

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(7 files)

Bug 1402498 - Remove self-include.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1402498 - Mark two methods of AxisPhysicsModel as const.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1402498 - Compute precise velocities and apply the zoom factor correctly.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1402498 - Rename AsyncScrollBase to ScrollAnimationPhysics and use composition instead of inheritance.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1402498 - Clean up ScrollAnimationPhysics code after the separation.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1402498 - Separate out ScrollAnimationBezierPhysics and make ScrollAnimationPhysics an interface.
59 bytes, text/x-review-board-request
rhunt
: review+
Details
Bug 1402498 - Add ScrollAnimationMSDPhysics, can be enabled using general.smoothScroll.msdPhysics.enabled.
59 bytes, text/x-review-board-request
rhunt
: review+
Details 
The current smooth scroll timing function uses a cubic bezier to simulate acceleration and deceleration.

In this bug I'd like to add an alternative smooth scroll timing function which is based on Mass-Spring-Damper physics, because I think it feels a lot better. However, I'm not really qualified to justify that it actually *is* better, so I'm going to implement it behind a pref.
In another bug we can discuss whether that pref should be flipped, and what kind of data we need in order to make that decision.
There's one remaining bug that I want to look at before putting this up for review, and that's the fact that PositionAt and VelocityAt are not called with monotonously increasing time stamps - sometimes we get asked for values in the past. This causes problems for the MSD axis physics model because it can only simulate forwards, not backwards.
(In reply to Markus Stange [:mstange] from comment #9)
> PositionAt and VelocityAt are not called
> with monotonously increasing time stamps - sometimes we get asked for values
> in the past.

I think we've had this problem with APZ animations as well, and added a check to work around it:

http://searchfox.org/mozilla-central/rev/56ad02e34d0d36ca4d5ccaa885d26aff270b8ff7/gfx/layers/apz/src/AsyncPanZoomAnimation.h#37
Attachment #8911371 - Flags: review?(botond)
The part I'm least sure about is the patch that changes how velocities are computed in APZC. The MSD-based animation needs velocities to be accurate, otherwise you get weird oscillation effects. So the velocity needs to be computed by the animation and not guessed from the displacement, and the zoom factor needs to be applied. I don't really understand what the old code was doing with how it was ignoring the zoom in some cases, and I don't know if that part is still necessary. Botond, could you take a look?
Comment on attachment 8911372 [details]
Bug 1402498 - Rename AsyncScrollBase to ScrollAnimationPhysics and use composition instead of inheritance.

https://reviewboard.mozilla.org/r/182854/#review188318

::: layout/generic/ScrollAnimationPhysics.cpp:11
(Diff revision 1)
> -#include "AsyncScrollBase.h"
> +#include "ScrollAnimationPhysics.h"
>  #include "gfxPrefs.h"
>  
>  using namespace mozilla;
>  
> -AsyncScrollBase::AsyncScrollBase(nsPoint aStartPos)
> +ScrollAnimationPhysics::ScrollAnimationPhysics(nsPoint aStartPos)

Quick note: Our static analysis bot found a potential drive-by improvement for this line:

Warning: The parameter 'aStartPos' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
status-firefox57: --- → wontfix
status-firefox58: affected → fix-optional
Priority: -- → P3
Whiteboard: [gfx-noted]
(In reply to Markus Stange [:mstange] from comment #11)
> The part I'm least sure about is the patch that changes how velocities are
> computed in APZC. The MSD-based animation needs velocities to be accurate,
> otherwise you get weird oscillation effects. So the velocity needs to be
> computed by the animation and not guessed from the displacement, and the
> zoom factor needs to be applied. I don't really understand what the old code
> was doing with how it was ignoring the zoom in some cases, and I don't know
> if that part is still necessary. Botond, could you take a look?

So this "ignoring the zoom" business (basically, casting from ParentLayer units to CSS units without a conversion) was introduced in bug 1022825 (with ParentLayer units called Screen units at the time), and cargo-culted around ever since.

Quoting the relevant comment from the review of bug 1022825:

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35)
> > +void AsyncPanZoomController::StartSmoothScroll() {
> > +  SetState(SMOOTH_SCROLL);
> > +  nsPoint initialPosition = CSSPoint::ToAppUnits(mFrameMetrics.GetScrollOffset());
> > +  // Convert velocity from ScreenPoints/ms to appunits/second
> 
> Technically you're just casting from ScreenPoints/ms to CSSPoints/ms and
> *then* converting to appunits/second. But I do think it makes more sense to
> use Screen coordinates when computing the scroll because you don't want the
> scroll duration to be affected by zoom, for example. So this code is fine
> but the documentation might need a bit of correction.

Kats, do you recall what the original motivation was (why we "don't want the scroll duration to be affected by zoom"), and whether it still applies today?
Flags: needinfo?(bugmail)
I think what I meant by "don't want the scroll duration to be affected by zoom" was that the animation length shouldn't be dependent on the zoom level the user is at. So a smooth scroll of 200 CSS pixels should always take the same amount of time regardless of zoom level. However I don't see why that means casting from ScreenPoints/ms to CSSPoints/ms is ok. It's quite possible I was confused or misunderstood the code when I wrote that comment.

I think we still want to preserve the invariant that "scrolling a certain number of CSS pixels should take the same amount of time, regardless of zoom level" so if the new patches do that I think it's ok to remove the "ignoring the zoom" business :)
Flags: needinfo?(bugmail)
Comment on attachment 8911372 [details]
Bug 1402498 - Rename AsyncScrollBase to ScrollAnimationPhysics and use composition instead of inheritance.

https://reviewboard.mozilla.org/r/182854/#review188318

> Quick note: Our static analysis bot found a potential drive-by improvement for this line:
> 
> Warning: The parameter 'aStartPos' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

Thanks for the note. One of the later patches in the series ("Separate out ScrollAnimationBezierPhysics [...]") actually makes this parameter a const reference, and also fixes up a lot of other parameters in that file.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> I think we still want to preserve the invariant that "scrolling a certain
> number of CSS pixels should take the same amount of time, regardless of zoom
> level" so if the new patches do that I think it's ok to remove the "ignoring
> the zoom" business :)

After some testing, it seems this is still true with the patches. I've thought about it some more and realized that the duration of the smooth scroll animation has never been affected by the starting velocity, only by the scroll distance. I think the only animation whose duration depends on the velocity is the fling animation that's used for touch scrolling. But for that animation, the fling friction is applied by Axis::FlingApplyFrictionOrCancel, which works in ParentLayerCoords, so it should already be independent of zoom.
(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Markus Stange [:mstange] from comment #9)
> > PositionAt and VelocityAt are not called
> > with monotonically increasing time stamps - sometimes we get asked for values
> > in the past.
> 
> I think we've had this problem with APZ animations as well, and added a
> check to work around it:
> 
> http://searchfox.org/mozilla-central/rev/
> 56ad02e34d0d36ca4d5ccaa885d26aff270b8ff7/gfx/layers/apz/src/
> AsyncPanZoomAnimation.h#37

I reproduced this under rr, and it seems to happen whenever the parent process janks a bit while the compositor keeps running the animation. For every frame that the compositor composites, it re-evaluates the smooth scroll animation at the current vsync's time stamp, or even for the time of the *next* vsync [1]. But once the parent process main thread processes the wheel events that got queued at some point during the jank, AsyncPanZoomController::OnScrollWheel calls GenericScrollAnimation::UpdateDelta with aEvent.mTimeStamp, and the event's timestamp can be arbitrarily far in the past.

I'm not sure if respecting the event timestamp here is a good idea. It seems like we'd be restarting the scroll animation from a position that it was at some time ago, even after we've already animated past that position.

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/gfx/layers/composite/AsyncCompositionManager.cpp#1447
Comment on attachment 8911371 [details]
Bug 1402498 - Compute precise velocities and apply the zoom factor correctly.

https://reviewboard.mozilla.org/r/182852/#review188498

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1831
(Diff revision 1)
> -  // Cast velocity from ParentLayerPoints/ms to CSSPoints/ms then convert to
> -  // appunits/second. We perform a cast to ParentLayerPoints/ms without a
> +  // Convert velocity from ParentLayerPoints/ms to ParentLayerPoints/s and then
> +  // to appunits/second.
> -  // conversion so that the scroll duration isn't affected by zoom
>    nsPoint velocity =
> -    CSSPoint::ToAppUnits(CSSPoint(mX.GetVelocity(), mY.GetVelocity())) * 1000.0f;
> +    CSSPoint::ToAppUnits(
> +      ParentLayerPoint(mX.GetVelocity() * 1000.0f, mY.GetVelocity() * 1000.0f) /

You can just do |GetVelocityVector() * 1000.0f|, and similarly in the other places
Attachment #8911371 - Flags: review?(botond) → review+
(In reply to Markus Stange [:mstange] from comment #17)
> I reproduced this under rr, and it seems to happen whenever the parent
> process janks a bit while the compositor keeps running the animation. For
> every frame that the compositor composites, it re-evaluates the smooth
> scroll animation at the current vsync's time stamp, or even for the time of
> the *next* vsync [1]. But once the parent process main thread processes the
> wheel events that got queued at some point during the jank,
> AsyncPanZoomController::OnScrollWheel calls
> GenericScrollAnimation::UpdateDelta with aEvent.mTimeStamp, and the event's
> timestamp can be arbitrarily far in the past.
> 
> I'm not sure if respecting the event timestamp here is a good idea. It seems
> like we'd be restarting the scroll animation from a position that it was at
> some time ago, even after we've already animated past that position.

+1 for not letting the animation go backwards in time.
Comment on attachment 8911369 [details]
Bug 1402498 - Remove self-include.

https://reviewboard.mozilla.org/r/182848/#review189940
Attachment #8911369 - Flags: review?(rhunt) → review+
Comment on attachment 8911370 [details]
Bug 1402498 - Mark two methods of AxisPhysicsModel as const.

https://reviewboard.mozilla.org/r/182850/#review189942
Attachment #8911370 - Flags: review?(rhunt) → review+
Comment on attachment 8911372 [details]
Bug 1402498 - Rename AsyncScrollBase to ScrollAnimationPhysics and use composition instead of inheritance.

https://reviewboard.mozilla.org/r/182854/#review189944
Attachment #8911372 - Flags: review?(rhunt) → review+
Comment on attachment 8911373 [details]
Bug 1402498 - Clean up ScrollAnimationPhysics code after the separation.

https://reviewboard.mozilla.org/r/182856/#review189906
Attachment #8911373 - Flags: review?(rhunt) → review+
Comment on attachment 8911374 [details]
Bug 1402498 - Separate out ScrollAnimationBezierPhysics and make ScrollAnimationPhysics an interface.

https://reviewboard.mozilla.org/r/182858/#review189930

::: layout/generic/ScrollAnimationBezierPhysics.h:31
(Diff revision 2)
> +// adapts the animation duration based on the scrolling rate.
> +class ScrollAnimationBezierPhysics : public ScrollAnimationPhysics
> +{
> +public:
> +  explicit ScrollAnimationBezierPhysics(const nsPoint& aStartPos,
> +                                  const ScrollAnimationBezierPhysicsSettings& aSettings);

nit: indentation

::: layout/generic/ScrollAnimationPhysics.h:36
(Diff revision 2)
>  };
>  
>  // Helper for accelerated wheel deltas. This can be called from the main thread
>  // or the APZ Controller thread.
>  static inline double
>  ComputeAcceleratedWheelDelta(double aDelta, int32_t aCounter, int32_t aFactor)

You don't have to do this, but it might be nice to find a new home for these.
Attachment #8911374 - Flags: review?(rhunt) → review+
Comment on attachment 8911375 [details]
Bug 1402498 - Add ScrollAnimationMSDPhysics, can be enabled using general.smoothScroll.msdPhysics.enabled.

https://reviewboard.mozilla.org/r/182860/#review189946
Attachment #8911375 - Flags: review?(rhunt) → review+
This is awesome, thanks for doing this Markus!

Just some general comments/questions.

1. It'd be nice to be able to unify AsyncSmoothMSDScroll (used for scroll-behavior) with ScrollAnimationMSDPhysics, seeing as they both use AxisPhysicsModel. ScrollAnimationMSDPhysics is slightly more complex, because the spring constant depends on the time delta, so some work might need to be done. Or we might even be able to switch scroll-behavior to use this new model, and can just get rid of it.

2. Does it make sense to have origin dependent "general.smoothScroll.msdPhysics" prefs? From my brief testing I thought the default prefs worked well for key and wheel scrolling, so I don't think it's necessary, but it could be useful for UX.

I'd be happy to land this as is, and consider these later. The code looks good.
(In reply to Ryan Hunt [:rhunt] from comment #34)
> This is awesome, thanks for doing this Markus!

Thanks for the reviews!

> Just some general comments/questions.
> 
> 1. It'd be nice to be able to unify AsyncSmoothMSDScroll (used for
> scroll-behavior) with ScrollAnimationMSDPhysics, seeing as they both use
> AxisPhysicsModel. ScrollAnimationMSDPhysics is slightly more complex,
> because the spring constant depends on the time delta, so some work might
> need to be done. Or we might even be able to switch scroll-behavior to use
> this new model, and can just get rid of it.

I agree, unifying these will be nice. It's probably going to be easier to do once ScrollAnimationMSDPhysics is the only scroll physics implementation that we have, so I'm not going to do it here.

> 2. Does it make sense to have origin dependent
> "general.smoothScroll.msdPhysics" prefs? From my brief testing I thought the
> default prefs worked well for key and wheel scrolling, so I don't think it's
> necessary, but it could be useful for UX.

I've thought about this as well and have come to the same conclusion. The current settings feel fine to me for both input types. We can add origin-based prefs if UX asks for them.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/b84611f27715
Remove self-include. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/5e3ab60852bc
Mark two methods of AxisPhysicsModel as const. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/ddf078fad9f4
Compute precise velocities and apply the zoom factor correctly. r=botond
https://hg.mozilla.org/integration/autoland/rev/8f20a445bc31
Rename AsyncScrollBase to ScrollAnimationPhysics and use composition instead of inheritance. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/c63987a3b9de
Clean up ScrollAnimationPhysics code after the separation. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/884587d3b510
Separate out ScrollAnimationBezierPhysics and make ScrollAnimationPhysics an interface. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/9c91a596243b
Add ScrollAnimationMSDPhysics, can be enabled using general.smoothScroll.msdPhysics.enabled. r=rhunt
Blocks: 1760372
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: