Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Animation Engine - Set currentFrame when animation is finished #1892

Closed
wants to merge 1 commit into from

Conversation

nevil
Copy link
Contributor

@nevil nevil commented Jan 3, 2023

For the Core Animation Engine the currentFrame is calculated based on the animationProgress placeholder animation.
The animationProgress value is never copied from the presentation layer to the model layer.

Because of that value of currentFrame is lost once the place holder animation has reached completion and was automatically removed.

This PR changes the placeholder animation to remain and instead be manually removed in the animationDidStop(..) delegate.
That way it is possible to persist the currentFrame value.

Attempts to solve issues:
#1877
#1865

Before After
Simulator.Screen.Recording.-.iPhone.8.-.2023-01-03.at.15.56.02.mp4
Simulator.Screen.Recording.-.iPhone.8.-.2023-01-03.at.15.54.34.mp4

Notes
#1877 is not fully fixed as there still is an issue when setting the switch to off from the start. The wrong frame is shown.

Warning
I'm pretty certain that there are cases that I have missed and that I have now broken. 🙇‍♂️

For the Core Animation Engine the `currentFrame` is calculated based
on the `animationProgress` placeholder animation.
The `animationProgress` value is never copied from the presentation layer
to the model layer.

Because of that value of `currentFrame` is lost once the place holder animation
has reached completion and was automatically removed.

This PR changes the placeholder animation to remain and instead be manually
removed in the `animationDidStop(..)` delegate.
That way it is possible to persist the `currentFrame` value.

Attempts to solve issues:
@nevil
Copy link
Contributor Author

nevil commented Jan 3, 2023

I think the Snapshot tests are failing due to me creating on an m1 MBP and the CI builds running Intel.
Might need to start using the perceptualPrecision setting available in swift-snapshot-testing.

@nevil
Copy link
Contributor Author

nevil commented Jan 4, 2023

A possible alternative more localized to CoreAnimationLayer.

Use the transaction callback to update the model's animationProgress.
The unique animation key is to prevent removing the new animation in the callback when a new animation (with the same key) is added to the layer.

diff --git Sources/Private/CoreAnimation/CoreAnimationLayer.swift Sources/Private/CoreAnimation/CoreAnimationLayer.swift
index 787d4a0..bf36091 100644
--- Sources/Private/CoreAnimation/CoreAnimationLayer.swift
+++ Sources/Private/CoreAnimation/CoreAnimationLayer.swift
@@ -187,6 +187,9 @@ final class CoreAnimationLayer: BaseAnimationLayer {
   /// which is also the realtime animation progress of this layer's animation
   @objc private var animationProgress: CGFloat = 0

+  private var activeProgressAnimationName = #keyPath(animationProgress)
+  private var progressAnimationID = 0
+
   private let animation: LottieAnimation
   private let valueProviderStore: ValueProviderStore
   private let compatibilityTracker: CompatibilityTracker
@@ -280,11 +283,21 @@ final class CoreAnimationLayer: BaseAnimationLayer {
     let timedProgressAnimation = animationProgressTracker.timed(with: context, for: self)
     timedProgressAnimation.delegate = currentAnimationConfiguration?.animationContext.closure

-    // Remove the progress animation once complete so we know when the animation
-    // has finished playing (if it doesn't loop infinitely)
-    timedProgressAnimation.isRemovedOnCompletion = true
+    progressAnimationID += 1
+    activeProgressAnimationName = #keyPath(animationProgress) + String(progressAnimationID)

-    add(timedProgressAnimation, forKey: #keyPath(animationProgress))
+    let animationNameCopy = activeProgressAnimationName
+    CATransaction.begin()
+    CATransaction.setCompletionBlock {
+      self.animationProgress = self.presentation()?.animationProgress ?? self.animationProgress
+      // Remove the progress animation once complete so we know when the animation
+      // has finished playing (if it doesn't loop infinitely)
+      self.removeAnimation(forKey: animationNameCopy)
+    }
+
+    add(timedProgressAnimation, forKey: activeProgressAnimationName)
+
+    CATransaction.commit()
   }

   // Removes the current `CAAnimation`s, and rebuilds new animations
@@ -325,7 +338,7 @@ final class CoreAnimationLayer: BaseAnimationLayer {
 extension CoreAnimationLayer: RootAnimationLayer {

   var primaryAnimationKey: AnimationKey {
-    .specific(#keyPath(animationProgress))
+    .specific(activeProgressAnimationName)
   }

   var isAnimationPlaying: Bool? {
@@ -337,7 +350,7 @@ extension CoreAnimationLayer: RootAnimationLayer {
     case nil:
       switch playbackState {
       case .playing:
-        return animation(forKey: #keyPath(animationProgress)) != nil
+        return animation(forKey: activeProgressAnimationName) != nil
       case nil, .paused:
         return false
       }
@@ -348,7 +361,8 @@ extension CoreAnimationLayer: RootAnimationLayer {
     get {
       switch playbackState {
       case .playing, nil:
-        return animation.frameTime(forProgress: (presentation() ?? self).animationProgress)
+        let progress = (isAnimationPlaying ?? false) ? (presentation() ?? self).animationProgress : animationProgress
+        return animation.frameTime(forProgress: progress)
       case .paused(let frame):
         return frame
       }

@calda
Copy link
Member

calda commented Jan 5, 2023

Thanks for the investigation! I ended up landing a slightly different fix for this issue in #1897.

Really appreciate the added test case though! I'll merge the new test case as a part of #1897 and mark you as a co-author of that PR. Thanks again :)

@calda calda closed this Jan 5, 2023
@nevil
Copy link
Contributor Author

nevil commented Jan 6, 2023

@calda
Thanks for fixing the issue!
This PR was mainly intended as a discussion point and a way for me to learn more about Lottie and CoreAnimation :-)

@nevil nevil deleted the issue-1877-pr1 branch January 6, 2023 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants