1282245 - Re-work the way APZ animations are handled

Status: NEW

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

Description

[Botond Ballo [:botond]]

APZ animations are currently handled by allowing each APZC to have a single animation (at a time), whose creation is triggered by some action, and is then sampled on each frame until it finishes.

There are a couple of limitations to this model:

  - There is no way to model two distinct forces (represented by
    two different animations) acting on the APZC at the same time.

    An example of when we might want to model this is when you
    fling the page while in an overscrolled state. For realistic
    physics, the effects of the overscroll and fling animations
    should be combined. In particular, this would solve the
    problem described in bug 1143904 comment 3 (even though that
    bug is closed, the problem described there is still relevant).

  - There is no way to model different animations on the different
    axes.

    An example of when we might want to model this is when a
    diagonal fling hits one edge of a page. We'd like to be able
    to continue the fling in one direction while running the
    overscroll animation in the other.

To overcome these limitations, I'd like to re-workthe way APZ animations are modelled, so that we can have more than one per APZC (and perhaps separate ones on each Axis, although I'm not sure about that yet; we might be able to represent them by having multiple animations on the APZC, each of which only causes motion along one axis).

Comment 1

[Kevin Wern]

I'd be interested in taking on this bug.

I'll be able to start looking into it tonight--any info you think might be helpful is appreciated.

Comment 2

[Botond Ballo [:botond]]

Hey Kevin, good to hear from you! :)

A good way to start here is probably to familiarize yourself with the relevant code:

  - AsyncPanZoomAnimation [1] and its subclasses [2], each of which represent
    a specific animation.
  - The code in AsyncPanZoomController to sample the active animation [3]
    and its call site in AsyncCompositionManager [4].

The general idea is to refactor this code to satisfy the two use cases mentioned in comment 0. I don't know what exactly that will look like; I'll give it some thought, and I'm also definitely open to any ideas / suggestions you might have!

One thing to note is that OverscrollAnimation is no longer used (it was only used on the Firefox OS platform, which was discontinued), and we'll probably want to remove it. On the other hand, we eventually want to support overscroll on macOS, which may involve writing a new animation subclass (possibly also named "OverscrollAnimation", although its implementation would be quite different from the current Firefox OS one). I'll talk to Markus (our resident macOS expert) about this tomorrow and get back to you about what implications, if any, these changes have on this bug.

[1] https://dxr.mozilla.org/mozilla-central/rev/9c06e744b1befb3a2e2fdac7414ce18220774a1d/gfx/layers/apz/src/AsyncPanZoomAnimation.h#23
[2] https://dxr.mozilla.org/mozilla-central/search?q=%2Bderived%3Amozilla%3A%3Alayers%3A%3AAsyncPanZoomAnimation
[3] https://dxr.mozilla.org/mozilla-central/rev/9c06e744b1befb3a2e2fdac7414ce18220774a1d/gfx/layers/apz/src/AsyncPanZoomController.cpp#3088
[4] https://dxr.mozilla.org/mozilla-central/rev/9c06e744b1befb3a2e2fdac7414ce18220774a1d/gfx/layers/composite/AsyncCompositionManager.cpp#845

Comment 3

[Markus Stange [:mstange]]

Attachment: squashed wip patch for mac overscroll and a bunch of other stuff

Botond asked me to attach my work in progress for Mac overscrolling so that he can see what I'm doing to OverscrollAnimation.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8832572 [details]
squashed wip patch for mac overscroll and a bunch of other stuff

I moved this patch over to bug 1124108, which is where the finished version (or at least parts of it) would likely land.

Comment 5

[Botond Ballo [:botond]]

So the parts of Markus' patch that are relevant to this bug basically change the physics used by OverscrollAnimation, from the spring physics that B2G used, to the simpler physics (no oscillation, just snap back using an "MSD" model) we want for macOS (and also change the visual effect from a compression to a translation).

I think it might make sense to land these changes now, as a precursor to this refactoring. That way, we're not spending time catering to the needs of a complicated oscillating spring effect that won't actually be used.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> I think it might make sense to land these changes now, as a precursor to
> this refactoring. That way, we're not spending time catering to the needs of
> a complicated oscillating spring effect that won't actually be used.

I filed bug 1335958 for this, and posted a patch there.

Kevin, if you're looking at the OverscrollAnimation implementation, you'll probably want to apply that patch, and look at the modified implementation, not the original one with the oscillation logic.

Comment 7

[Botond Ballo [:botond]]

One thing that occurs to me as I'm thinking about this, is that currently the animation that's running in an AsyncPanZoomController is pretty tightly tied to the APZC's mState [1]. That is, we basically have a separate state for each animation.

If we're going to allow an APZC to have multiple animations at once, we need to think about how that's going to be reflected in the state.

[1] http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/gfx/layers/apz/src/AsyncPanZoomController.h#816

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> One thing that occurs to me as I'm thinking about this, is that currently
> the animation that's running in an AsyncPanZoomController is pretty tightly
> tied to the APZC's mState [1]. That is, we basically have a separate state
> for each animation.
> 
> If we're going to allow an APZC to have multiple animations at once, we need
> to think about how that's going to be reflected in the state.

So we currently have five kinds of AsyncPanZoomAnimations:

  ZoomAnimation
  SmoothScrollAnimation
  GenericFlingAnimation
  OverscrollAnimation
  WheelScrollAnimation

Of these, I don't think we want to allow Zoom or WheelScroll to happen in parallel with any others, so those can keep their own states, ANIMATING_ZOOM and WHEEL_SCROLL.

For the other three, I think we potentially want to allow multiple of those to run in parallel. 

Here's one possible architecture to explore:

  - Have a single state (say, PHYSICS_ANIMATION), and a single Animation subclass
    (say, PhysicsAnimation), for any combination of these three animation types.

  - The PhysicsAnimation could maintain a list of one or more currently active
    Force objects. Each of the three original animation types (smooth scroll,
    fling, oversroll) would have their own kind of Force. When sampling the
    PhyscisAnimation, you'd add the forces together and come up with a single
    resulting change to the displacement for that frame.

There's one additional complication: on Mac, instead of flings we have "momentum scrolling", where we're in the PAN_MOMENTUM state and we receive momentum scrolling events from the OS, which act very much like the samples of an animation (basically, the OS is maintaining the animation state, and just sending us samples). We want to be able to process these events and sample a physics-based animation of our own (such as overscroll) simultaneously. We need to figure out how our design will accomodate that. (Markus current patch in bug 1124108 adds states like OVERSCROLL_ANIMATION_X_PAN_MOMENTUM_Y. I'd like to avoid that if at all possible.)

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> There's one additional complication: on Mac, instead of flings we have
> "momentum scrolling", where we're in the PAN_MOMENTUM state and we receive
> momentum scrolling events from the OS, which act very much like the samples
> of an animation (basically, the OS is maintaining the animation state, and
> just sending us samples). We want to be able to process these events and
> sample a physics-based animation of our own (such as overscroll)
> simultaneously. We need to figure out how our design will accomodate that.
> (Markus current patch in bug 1124108 adds states like
> OVERSCROLL_ANIMATION_X_PAN_MOMENTUM_Y. I'd like to avoid that if at all
> possible.)

One possible approach here is to use a "dummy" Force to represent pan momentum. So, we would be in the PHYSICS_ANIMATION state whenever we're receiving pan momentum events (while possibly also being in overscroll), and whenever we receive an event, rather than updating the scroll offset directly we would update some state in the pan momentum's Force, which then causes the desired scroll offset update when the PhysicsAnimation is sampled. It's not immediately clear how clean this approach would be, but we could try it and see.

Comment 10

[Botond Ballo [:botond]]

Anyways, Kevin, let me know if the suggestions I posted so far are enough to go on to get started with this. Keep in mind, they are just suggestions; if, as you work on this, you come up with a better design/approach, I'm definitely open to it!

Comment 11

[Kevin Wern]

(In reply to Botond Ballo [:botond] from comment #10)
> Anyways, Kevin, let me know if the suggestions I posted so far are enough to
> go on to get started with this. Keep in mind, they are just suggestions; if,
> as you work on this, you come up with a better design/approach, I'm
> definitely open to it!

I was hoping to comment a few days ago but the physics stuff was honestly still jumbling about in my head. It's still settling a bit, but I figure it's best to confirm I'm working on this in some way :).

My thought was we could refactor AxisPhysicsModel to meet the end you're describing. If we take away some concerns so it is a (concrete) class only maintaining state (i.e. position and velocity) and a list of Components--subclasses that have their own acceleration model--we could use these subclasses to model each type of animation (one already exists for springs, but we could create one for frictional force and a starting velocity, e.g. Flings). We would then refactor the three animations into Components which would define:
- A start state (in the constructor).
- An Acceleration() function using aState.
- A function to determine if a Component should be removed from the list (like IsCompleted()).

There would be an instance of AxisPhysicsModel in each Axis. When Simulate() is called from somewhere in Axis, AxisPhysicsModel would sum the Components' Acceleration() results, update aState, and check if any components should be removed. The AxisPhysicsModel would be considered complete once the list is empty. This would be what triggers the end of our new PhysicsAnimation.

Then, where we handle relevant events in APZC, we would adjust the logic to create Components instead of these three Animations and add the Component to our ongoing PhysicsAnimation, creating and starting a new one if necessary.

I need to look more into PanMomentum, though, as my impression is equal parts speculation and code. I think we *will* need to model it as a Component somehow, but the fact that it Animations and events don't share the same time delta will make it more difficult. I'll post (or amend) these thoughts later.

Hopefully I'll have something more concrete after this weekend. Feedback is appreciated as always.

Comment 12

[Kevin Wern]

> the fact that it Animations and events

that Animations and events*

Comment 13

[Botond Ballo [:botond]]

(In reply to Kevin Wern from comment #11)
> My thought was we could refactor AxisPhysicsModel to meet the end you're
> describing.

One thing to keep in mind is that AxisPhysicsModel (or more specifically its derived class AxisPhysicsMSDModel) has uses outside of APZ: in main-thread smooth scrolling [1], and macOS back/forward history swiping [2].

One option would be to refactor it in such a way that it can continue to meet the needs of those two call sites as well. Another option would be to "fork" it, i.e. just write another class that's for APZ use only.

[1] http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/layout/generic/nsGfxScrollFrame.cpp#1800
[2] http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/widget/cocoa/SwipeTracker.h#83

Comment 14

[Botond Ballo [:botond]]

Hey Kevin, just wanted to check in to see how things are going here. Anything I can help with?

Comment 15

[Kevin Wern]

Hey Botond!

Sorry, I was hoping to work on this more, but I probably still won't be able to for a month or so. I can take myself off this bug until then.

Hope to commit to working on this (or something else, if I'm not in time) in the future.

Comment 16

[Botond Ballo [:botond]]

No problem, Kevin. Let me know if at some point you're ready to start working on this again.

In the meantime, if someone else is interested in picking this up, please feel free. The ideas in the comments so far (particularly comment 2, comment 8, comment 9, and comment 11) should be a good starting point.

Comment 17

[Dean Zhu]

Hello, I'd like to take on these bug. But I don't know whether it will be too much for me or not.  
I fixed a couple of bugs with you bug 1352564, but I was a little busy and eventually had to stop.  
Do you think I'll be fit for this? I'll probably need a day or two before I have time to sit down and read the documention.  
Thank you!

Comment 18

[Botond Ballo [:botond]]

Dean, you're welcome to give this bug a try! It's not a small/localized change like bug 1352564, so it will require some time and patience on your part, but beyond that I don't see why you couldn't do it.

The existing comments in the bug give a pretty good summary of what needs to be done, so you can get started by reading them over. Let me know if you have any questions!

Comment 19

[Mitch Ament]

Hi Botond,I'd like to work on this bug once my code for bug 1477335 is ready to be approved.

Comment 20

[Botond Ballo [:botond]]

Thanks for your interest!

However, the situation here has changed a bit since I last commented on this bug. The fact that we have been special-casing Android with custom physics code adapted from Chrome as part of bug 1475035, has made the sort of refactoring proposed here less realistic. We could still contemplate doing it for non-Android codepaths, but the value in that would be lower, and I'd want to wait until work on bug 1475035 settles down to avoid churn in the code that would have to be modified here.

As a result, I am going to take this bug off the mentored bug list for the time being.

I have another suggestion for what you could work on after bug 1477335.

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> I have another suggestion for what you could work on after bug 1477335.

That being bug 1477832.

You can also find bugs mentored by others over here:

https://www.joshmatthews.net/bugsahoy/