mdgriffith / elm-animator

A timeline-based animation engine for Elm
https://package.elm-lang.org/packages/mdgriffith/elm-animator/latest/
BSD 3-Clause "New" or "Revised" License
133 stars 14 forks source link

Order of applying interruptions #12

Closed illyas3 closed 4 years ago

illyas3 commented 4 years ago

Problem

When scheduling animations very quickly (i.e. with touch events), sometimes they get into the same cycle (i.e. multiple scheduled before Tick occurs). Due to the current implementation of applying interruptions, there is a possible scenario when touchMove animation happens after touchEnd.

How to reproduce?

  1. Go to Ellie
  2. Open Chrome Dev Tools && Device Toolbar (to get mobile touch events)
  3. Start moving the Box down. (Do it number of times to get the described scenario) Expected behaviour: box follows the move and on release returns to original position. Wrong behaviour: box gets stuck at the last touchMove position, instead of returning back.

    Why it happens?

    When applying Interruptions, they are applied as LIFO (Last-in-First-Out). This sometimes results in touchEnd being applied first, and then touchMove. Because of this final position is touchMove, instead of touchEnd.

    applyInterruptionHelper : List (Schedule event) -> TimelineDetails event -> TimelineDetails event
    applyInterruptionHelper interrupts timeline =
    case interrupts of
        [] ->
            timeline
    
        inter :: remaining ->
            let
                delay =
                    case inter of
                        Schedule d _ _ ->
                            d
    
                newEvents =
                    interrupt timeline (Time.advanceBy delay timeline.now) inter
            in
            applyInterruptionHelper remaining { timeline | events = newEvents }

    Possible solution

    I have tried reversing the list, before applying it, so that we follow a FIFO (First-in-First-Out) strategy. So that, we apply events in the same order as they arrived into schedule. Like so in src/Internal/Timeline.elm:

    applyInterruptions : TimelineDetails event -> TimelineDetails event
    applyInterruptions timeline =
    case timeline.interruption of
        [] ->
            timeline
    
        _ ->
            applyInterruptionHelper (List.reverse timeline.interruption) { timeline | interruption = [] }

    This seems to solve the issue.

    Questions

  4. Was it intentional to apply scheduled items as LIFO?
  5. Is there a better way to solve this issue?
  6. Can we introduce this change to master?

Others

Original description: slack

mdgriffith commented 4 years ago

Nicely written issue!

There was no strong reasoning for the LIFO behavior. I'll go ahead and make the change.

mdgriffith commented 4 years ago

Published as 1.0.2!

illyas3 commented 4 years ago

@mdgriffith Thanks a lot man! Appreciate your works!