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

Add reverseOnRepeat option to LottieAnimatable #2128

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

brentwatson
Copy link
Contributor

Resolves issue #2125 - LottieAnimation Composable: support LottieAnimationView (XML) repeatMode equivalent.

This PR adds a reverseOnRepeat boolean option to LottieAnimatable (defaults to false). When set to true the effect is the same as using app:lottie_repeatMode="reverse" in the Android XML lottie implementation.

Example:

Regular Repeat:

Screen.Recording.2022-09-14.at.14.01.14.mov

With reverseOnRepeat=true:

Screen.Recording.2022-09-14.at.14.02.59.mov

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

override var clipSpec: LottieClipSpec? by mutableStateOf(null)
private set

override var speed: Float by mutableStateOf(1f)
private set

private val speedValue: Float by derivedStateOf {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a property or can it be derived as a local variable on line 291?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - 71154c6

The value must remain a derivedStateOf instance otherwise the animation misbehaves. If you think that adds too much additional complexity to onFrame I can revert the change.

Thanks for the review!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it "misbehave" when you make this a local variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If speedValue is changed to a local val which isn't updated on L306 then the animation misbehaves and looks like this...

Screen.Recording.2022-09-14.at.12.49.45.mov

If you would prefer a var which is set on L287 and reassigned within onFrame on L306 I can make that change, it just seemed to complicate the function IMO instead of reading from a derivedStateOf property which does the calculation for you.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could you add some documentation for this property since speed and speedValue is ambiguous on its own. It's not much better but maybe rawSpeed or actualSpeed or speedWithRepeatMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I added some docs to the property to explain why & when a negative value can be returned. I opted for frameSpeed for the property since that's more descriptive of how it is used. I didn't want to change speed -> rawSpeed as that would effect that public API of the class (as where the new frameSpeed property is private).

08f6929

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@@ -278,9 +284,12 @@ private class LottieAnimatableImpl : LottieAnimatable {
val minProgress = clipSpec?.getMinProgress(composition) ?: 0f
val maxProgress = clipSpec?.getMaxProgress(composition) ?: 1f

val dProgress = dNanos / 1_000_000 / composition.duration * speed
val speedValue: Float by derivedStateOf {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the derivedStateOf doing here? This isn't a composable function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above -- #2128 (comment)

I took a closer look at why making speedValue a val on L287 doesn't work as intended. The reason is that the rest of onFrame needs an updated speedValue each time iteration changes. So we would need a var on L287 and a reassigment on L306.

Based on this I decided that might further complicate onFrame which is already fairly complex so I reverted back to using a property for speedValue. If you would prefer the var + reassignment instead let me know.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@gpeal
Copy link
Collaborator

gpeal commented Oct 5, 2022

Thanks for your contribution!

@gpeal gpeal merged commit c553ad3 into airbnb:master Oct 5, 2022
@brentwatson brentwatson deleted the lottie-reverse-on-repeat branch October 6, 2022 13:54
@agustinsivoplas
Copy link

I am trying to use reverseOnRepeat but is not working for me. Do you have an example? My idea is pretty simple. Is a checkbox that user can check or uncheck on click and the animation should be reversed on click. Any help?

Thanks in advance

@brentwatson
Copy link
Contributor Author

brentwatson commented Aug 24, 2023 via email

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

4 participants