-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix repeat options in AnimationSequence #3650
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,7 +16,6 @@ import { | |||||||||||||||||||||
| import { | ||||||||||||||||||||||
| Easing, | ||||||||||||||||||||||
| getEasingForSegment, | ||||||||||||||||||||||
| invariant, | ||||||||||||||||||||||
| progress, | ||||||||||||||||||||||
| secondsToMilliseconds, | ||||||||||||||||||||||
| } from "motion-utils" | ||||||||||||||||||||||
|
|
@@ -51,6 +50,16 @@ export function createAnimationsFromSequence( | |||||||||||||||||||||
| const elementCache = {} | ||||||||||||||||||||||
| const timeLabels = new Map<string, number>() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Store per-value repeat options that can't be expanded into keyframes | ||||||||||||||||||||||
| * (e.g. repeat: Infinity) and need to be passed through to the | ||||||||||||||||||||||
| * final transition for the animation engine to handle. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const repeatPassthrough = new Map< | ||||||||||||||||||||||
| ValueSequence, | ||||||||||||||||||||||
| Pick<Transition, "repeat" | "repeatType" | "repeatDelay"> | ||||||||||||||||||||||
| >() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| let prevTime = 0 | ||||||||||||||||||||||
| let currentTime = 0 | ||||||||||||||||||||||
| let totalDuration = 0 | ||||||||||||||||||||||
|
|
@@ -198,46 +207,58 @@ export function createAnimationsFromSequence( | |||||||||||||||||||||
| * Handle repeat options | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| if (repeat) { | ||||||||||||||||||||||
| invariant( | ||||||||||||||||||||||
| repeat < MAX_REPEAT, | ||||||||||||||||||||||
| "Repeat count too high, must be less than 20", | ||||||||||||||||||||||
| "repeat-count-high" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| duration = calculateRepeatDuration( | ||||||||||||||||||||||
| duration, | ||||||||||||||||||||||
| repeat, | ||||||||||||||||||||||
| repeatDelay | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const originalKeyframes = [...valueKeyframesAsList] | ||||||||||||||||||||||
| const originalTimes = [...times] | ||||||||||||||||||||||
| ease = Array.isArray(ease) ? [...ease] : [ease] | ||||||||||||||||||||||
| const originalEase = [...ease] | ||||||||||||||||||||||
| if (repeat >= MAX_REPEAT) { | ||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * For large/infinite repeat counts, don't expand keyframes. | ||||||||||||||||||||||
| * Pass repeat options through to the final transition | ||||||||||||||||||||||
| * and let the animation engine handle repeating. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| repeatPassthrough.set(valueSequence, { | ||||||||||||||||||||||
| repeat, | ||||||||||||||||||||||
| repeatType: repeatType as Transition["repeatType"], | ||||||||||||||||||||||
| repeatDelay: repeatDelay || undefined, | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
Comment on lines
+216
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the user doesn't provide
Suggested change
|
||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
|
Comment on lines
209
to
+221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Consider this scenario: animate([
[el, { x: [0, 100] }, { duration: 1, repeat: 25 }],
[el, { y: [0, 100] }, { duration: 1 }], // Should start at t=26s, actually starts at t=1s
])For For if (repeat >= MAX_REPEAT) {
repeatPassthrough.set(valueSequence, {
repeat,
repeatType: repeatType as Transition["repeatType"],
repeatDelay: repeatDelay || undefined,
})
// Still adjust duration for finite repeats so subsequent segments are placed correctly
if (isFinite(repeat)) {
duration = calculateRepeatDuration(duration, repeat, repeatDelay)
}
} else { |
||||||||||||||||||||||
| duration = calculateRepeatDuration( | ||||||||||||||||||||||
| duration, | ||||||||||||||||||||||
| repeat, | ||||||||||||||||||||||
| repeatDelay | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for (let repeatIndex = 0; repeatIndex < repeat; repeatIndex++) { | ||||||||||||||||||||||
| valueKeyframesAsList.push(...originalKeyframes) | ||||||||||||||||||||||
| const originalKeyframes = [...valueKeyframesAsList] | ||||||||||||||||||||||
| const originalTimes = [...times] | ||||||||||||||||||||||
| ease = Array.isArray(ease) ? [...ease] : [ease] | ||||||||||||||||||||||
| const originalEase = [...ease] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for ( | ||||||||||||||||||||||
| let keyframeIndex = 0; | ||||||||||||||||||||||
| keyframeIndex < originalKeyframes.length; | ||||||||||||||||||||||
| keyframeIndex++ | ||||||||||||||||||||||
| let repeatIndex = 0; | ||||||||||||||||||||||
| repeatIndex < repeat; | ||||||||||||||||||||||
| repeatIndex++ | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| times.push( | ||||||||||||||||||||||
| originalTimes[keyframeIndex] + (repeatIndex + 1) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ease.push( | ||||||||||||||||||||||
| keyframeIndex === 0 | ||||||||||||||||||||||
| ? "linear" | ||||||||||||||||||||||
| : getEasingForSegment( | ||||||||||||||||||||||
| originalEase, | ||||||||||||||||||||||
| keyframeIndex - 1 | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| valueKeyframesAsList.push(...originalKeyframes) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for ( | ||||||||||||||||||||||
| let keyframeIndex = 0; | ||||||||||||||||||||||
| keyframeIndex < originalKeyframes.length; | ||||||||||||||||||||||
| keyframeIndex++ | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| times.push( | ||||||||||||||||||||||
| originalTimes[keyframeIndex] + | ||||||||||||||||||||||
| (repeatIndex + 1) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ease.push( | ||||||||||||||||||||||
| keyframeIndex === 0 | ||||||||||||||||||||||
| ? "linear" | ||||||||||||||||||||||
| : getEasingForSegment( | ||||||||||||||||||||||
| originalEase, | ||||||||||||||||||||||
| keyframeIndex - 1 | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| normalizeTimes(times, repeat) | ||||||||||||||||||||||
| normalizeTimes(times, repeat) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const targetTime = startTime + duration | ||||||||||||||||||||||
|
|
@@ -386,6 +407,7 @@ export function createAnimationsFromSequence( | |||||||||||||||||||||
| ease: valueEasing, | ||||||||||||||||||||||
| times: valueOffset, | ||||||||||||||||||||||
| ...sequenceTransition, | ||||||||||||||||||||||
| ...repeatPassthrough.get(valueSequence), | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new test covers
repeat: Infinitybut leaves several important cases untested:repeatTypepassthrough: No assertion thatrepeatType(e.g."mirror") is correctly propagated through to the final transition.repeatDelaypassthrough: No assertion that a non-zerorepeatDelaymakes it to the final transition.repeat >= MAX_REPEATin multi-segment sequences: A test like the existing "Repeating a segment correctly places the next segment at the end" test but withrepeat: 25would expose the timing discontinuity noted in the logic comment above.repeat: 20boundary: This is the smallest value that now follows the passthrough path, and its behavior (timing for subsequent segments) is not exercised at all.