Add better support for negative animation speeds #377

welshm · 2017-08-17T14:25:17Z

The progress can be calculated basd on negative speed
- The end frame would be a progress of 0 and the start frame a progress of 1
- Recalculate progress when the speed changes, otherwise a "jump" will be seen when restarting animation
The frame for the progress can be calculated using the newly inverted progress
- Determine the absolute progress based on current progress and determine which frame to use based on animation speed
This fixes issues of animations playing with negative speeds "bouncing" back to their final position
which was caused by the final frame being calculated based on a completed progress of 1 which then did not take into
account the negative animation speed.

- The progress can be calculated basd on negative speed + The end frame would be a progress of 0 and the start frame a progress of 1 + Recalculate progress when the speed changes, otherwise a "jump" will be seen when restarting animation - The frame for the progress can be calculated using the newly inverted progress + Determine the absolute progress based on current progress and determine which frame to use based on animation speed - This fixes issues of animations playing with negative speeds "bouncing" back to their final position which was caused by the final frame being calculated based on a completed progress of 1 which then did not take into account the negative animation speed.

…into reverseSupport

buba447 · 2017-08-17T17:16:54Z

lottie-ios/Classes/Private/LOTAnimationView.m

+ return absoluteProgress;
+ } else {
+ // If the animation is playing backwards, the progress is inverted.
+ return 1 - absoluteProgress;


_animationProgress should always be in absolute time, not relative time. So if an animation is playing in reverse the progress will decrease, if it plays in autoReverse it will go up then down.

My reasoning for inverting this was as follows:

If I have a 10 second animation that has run for 3 seconds and then I set the speed to -1, it would begin reversing the animation and the progress should jump from 30% to 70%.

Same for the frames. If there are 12 frames and I'm at frame 3 going forward, that is 25% completion but going in reverse it's 75% completion.

Otherwise, callers of the SDK who set the speed to negative one need to do all these inversions themselves. If that is the case, I'll keep the constant change above (kCompContainerAnimationKey) and add documentation to the header file about using negative speeds?

Yes, but speed implicitly changes the animation.
If you have auto-reverse animation going and you are tracking the animation in realtime via animationProgress it will be unclear where the animation is unless animationProgress is absolute.

Animation progress is intended to be a 1:1 mapping to the entire animation timeline, not to the currently playing animation.

This comes up in a lot of different states.
When playFromProgress:0.5 toProgress:1 is called then animationProgress is still absolute and reports a from 0.5 to 1 even though the internal animation 0-1 would map 0.5 to 1.

Another way to reverse playing an animation is to call playFromProgress:1 toProgress:0 with a positive speed. Then when you reverse the speed the animation would actually play forward.

buba447 · 2017-08-17T17:17:18Z

lottie-ios/Classes/Private/LOTAnimationView.m

@@ -14,6 +14,8 @@
 #import "LOTAnimationCache.h"
 #import "LOTCompositionContainer.h"

+static NSString * const kCompContainerAnimationKey = @"play";


Thanks for adding this! Things I always forget to do haha.

buba447 · 2017-08-17T17:17:47Z

lottie-ios/Classes/Private/LOTAnimationView.m

+ } else {
+ // It the animation is playing backwards, subtract the progress from the end.
+ return @(_sceneModel.endFrame.floatValue - absoluteProgress);
+ }


If _animationProgress stays absolute then this will also be fine as it was.

buba447 · 2017-08-17T17:18:50Z

lottie-ios/Classes/Private/LOTAnimationView.m

- frame = (NSNumber *)playAnimation.toValue;
+ // Set the final frame based on the animation to/from values. If playing forward, use the
+ // toValue otherwise we want to end on the fromValue.
+ frame = [self _isPlayingForwards] ? (NSNumber *)playAnimation.toValue : (NSNumber *)playAnimation.fromValue;


Why not just check the anim.speed here and to the binary switch?
This way the rest of the logic can be removed but it will still have the effect you are looking for?

Explained my reasoning for the change above. If you disagree, will keep this part of the change and revert the rest. Not sure which binary switch you're referring to, though.

Im referring to frame = [self _isPlayingForwards] ? (NSNumber *)playAnimation.toValue : (NSNumber *)playAnimation.fromValue;

Instead of [self _isPlayingForwards] ?
You can do anim.speed < 0 ?

I left it as [self _isPlayingForwards] because I think that still makes sense to use in

- (void)playFromFrame:(nonnull NSNumber *)fromStartFrame toFrame:(nonnull NSNumber *)toEndFrame withCompletion:(nullable LOTAnimationCompletionBlock)completion

That seems reasonable. In that case, on last nitpick can you change _isPlayingForwards to _isSpeedNegative or something similar? Since there are cases where speed can be negative and the animation is still technically playing forward.

Just to keep confusion down in the future.

Sounds good to me! Thanks for the speedy comments and review

…olutes - Update header documentation to make it clear how animatinoSpeed and animationProgress interact - Leave fixes in there that stop reversed animations "snapping" back

buba447 · 2017-08-17T21:22:15Z

lottie-ios/Classes/PublicHeaders/LOTAnimationView.h

@@ -46,9 +46,12 @@ typedef void (^LOTAnimationCompletionBlock)(BOOL animationFinished);

 // TODO
 /// Sets a progress from 0 - 1 of the animation. If the animation is playing it will stop and the compeltion block will be called.
+/// The animation progress is in terms of absolute progress of the defined animation and does not
+/// take into account negative speeds.


Mind changing the wordage here a bit? Currently it seems as if animationProgress does not work if the speed is negative, thats not true.

Something like 'the current progress of the animation in absolute time. EG a value of 0.75 always represents the same point in animaiton regardless of positive or negative speed.'

Sure.

Just a thought... if I call play, the progress goes from 0 to 1 (regardless of play direction) where as setting the value directly will cause a different behaviour so the property does different things depending on if the animation is playing or not.

Actually that's not necessarily true - never mind!

buba447 · 2017-08-17T21:23:18Z

lottie-ios/Classes/PublicHeaders/LOTAnimationView.h

 @property (nonatomic, assign) CGFloat animationProgress;

-/// Sets the speed of the animation. Accepts a negative value for reversing animation
+/// Sets the speed of the animation. Accepts a negative value for reversing animation.
+/// Negative speeds do not affect animationProgress


Also, not necessarily true. Negative speeds do affect animation progress in that as the animation plays the progress is updated to its absolute value.

Removed comment as I think the comment on animationProgress better captures the intent

…'s checking

buba447 · 2017-08-17T22:03:05Z

Great! Thank you for grabbing this and for working with me on it!

buba447 · 2017-08-17T22:03:40Z

@welshm Do you mind grabbing those merge conflicts?

buba447 · 2017-08-17T22:04:24Z

@welshm Nevermind! I got it!

welshm mentioned this pull request Aug 17, 2017

Reverse animations snap back to "completed" state after animation plays #378

Closed

9 tasks

welshm added 2 commits August 17, 2017 11:14

Merge branch 'reverseSupport' of https://github.com/welshm/lottie-ios …

0896678

…into reverseSupport

buba447 requested changes Aug 17, 2017

View reviewed changes

Revert some of the animationProgress changes to revert to reflect abs…

ab75236

…olutes - Update header documentation to make it clear how animatinoSpeed and animationProgress interact - Leave fixes in there that stop reversed animations "snapping" back

buba447 reviewed Aug 17, 2017

View reviewed changes

welshm added 2 commits August 17, 2017 17:37

Rename _isPlayingForward to _isSpeedNegative to be more clear what it…

b0b7b7c

…'s checking

Update LOTAnimationView header to have better comments

805a671

buba447 approved these changes Aug 17, 2017

View reviewed changes

Merge branch 'master' into reverseSupport

fbf6ca5

buba447 merged commit 8f86819 into airbnb:master Aug 17, 2017

welshm deleted the reverseSupport branch August 17, 2017 22:09

NSGolova pushed a commit to clario-tech/lottie-ios that referenced this pull request Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better support for negative animation speeds #377

Add better support for negative animation speeds #377

welshm commented Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

buba447 Aug 17, 2017

buba447 Aug 17, 2017

buba447 Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

welshm Aug 17, 2017

buba447 Aug 17, 2017

welshm Aug 17, 2017

buba447 commented Aug 17, 2017

buba447 commented Aug 17, 2017

buba447 commented Aug 17, 2017

Add better support for negative animation speeds #377

Add better support for negative animation speeds #377

Conversation

welshm commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buba447 commented Aug 17, 2017

buba447 commented Aug 17, 2017

buba447 commented Aug 17, 2017