-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fix(studio): retime flat tween keyframe diamonds #2670
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
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 |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { resolveTimelineKeyframeTarget } from "./useTimelineEditCallbacks"; | ||
|
|
||
| const FLAT_ANIMATION = { | ||
| id: "#title-fromTo-2900", | ||
| propertyGroup: null, | ||
| }; | ||
|
|
||
| describe("resolveTimelineKeyframeTarget", () => { | ||
| it("resolves a synthesized diamond when one mixed flat tween is selected", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 8.75, | ||
| [{ percentage: 8.75, tweenPercentage: 100 }], | ||
| [FLAT_ANIMATION], | ||
| ), | ||
| ).toEqual({ animId: "#title-fromTo-2900", tweenPct: 100 }); | ||
| }); | ||
|
|
||
| it("keeps multiple ungrouped flat tweens unresolved instead of guessing", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 8.75, | ||
| [{ percentage: 8.75, tweenPercentage: 100 }], | ||
| [FLAT_ANIMATION, { id: "#title-to-2900", propertyGroup: null }], | ||
| ), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("keeps multiple flat tweens in the same property group unresolved", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 50, | ||
| [{ percentage: 50, tweenPercentage: 100, propertyGroup: "position" }], | ||
| [ | ||
| { id: "position-a", propertyGroup: "position" }, | ||
| { id: "position-b", propertyGroup: "position" }, | ||
| ], | ||
| ), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("keeps a keyframed and flat tween in the same property group unresolved", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 50, | ||
| [{ percentage: 50, tweenPercentage: 25, propertyGroup: "position" }], | ||
| [ | ||
| { id: "position-keyframed", propertyGroup: "position", keyframes: {} }, | ||
| { id: "position-flat", propertyGroup: "position" }, | ||
| ], | ||
| ), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("keeps multiple ungrouped keyframed tweens unresolved", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 50, | ||
| [{ percentage: 50, tweenPercentage: 25 }], | ||
| [ | ||
| { id: "ungrouped-a", keyframes: {} }, | ||
| { id: "ungrouped-b", keyframes: {} }, | ||
| ], | ||
| ), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("does not infer an animation when the rendered diamond misses the cache", () => { | ||
| expect(resolveTimelineKeyframeTarget(60, [], [FLAT_ANIMATION])).toBeNull(); | ||
| }); | ||
|
|
||
| it("resolves the sole candidate in an explicit property group", () => { | ||
| expect( | ||
| resolveTimelineKeyframeTarget( | ||
| 50, | ||
| [{ percentage: 50, tweenPercentage: 25, propertyGroup: "position" }], | ||
| [ | ||
| { id: "opacity", propertyGroup: "opacity" }, | ||
| { id: "position", propertyGroup: "position" }, | ||
| ], | ||
| ), | ||
| ).toEqual({ animId: "position", tweenPct: 25 }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,41 @@ export interface TimelineEditCallbackDeps { | |
| handleRazorSplitAll: (splitTime: number) => Promise<void> | void; | ||
| } | ||
|
|
||
| interface TimelineKeyframeTargetAnimation { | ||
| id: string; | ||
| propertyGroup?: string | null; | ||
| keyframes?: unknown; | ||
| } | ||
|
|
||
| interface TimelineCachedKeyframe { | ||
| percentage: number; | ||
| tweenPercentage?: number; | ||
| propertyGroup?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve a rendered timeline diamond back to the animation that authored it. | ||
| * Flat tweens use synthesized diamonds, so a mixed flat tween may have neither | ||
| * a property group nor real keyframes. The cache currently carries a property | ||
| * group, not an animation id, so resolution is safe only when that group has a | ||
| * single candidate. Ambiguous candidates remain unresolved rather than | ||
| * retiming an arbitrary tween. | ||
| */ | ||
| export function resolveTimelineKeyframeTarget( | ||
| pct: number, | ||
| keyframes: ReadonlyArray<TimelineCachedKeyframe>, | ||
| animations: ReadonlyArray<TimelineKeyframeTargetAnimation>, | ||
| ): { animId: string; tweenPct: number } | null { | ||
| const kf = keyframes.find((item) => Math.abs(item.percentage - pct) < 0.2); | ||
| if (!kf) return null; | ||
| const group = kf?.propertyGroup; | ||
| const candidates = group | ||
| ? animations.filter((animation) => animation.propertyGroup === group) | ||
| : animations.filter((animation) => !animation.propertyGroup); | ||
| const animation = candidates.length === 1 ? candidates[0] : undefined; | ||
| return animation ? { animId: animation.id, tweenPct: kf.tweenPercentage ?? pct } : null; | ||
| } | ||
|
|
||
| /** | ||
| * Builds the timeline edit callback bag (move/resize/split/razor plus the | ||
| * keyframe-diamond callbacks) provided to `<Timeline>` via TimelineEditProvider. | ||
|
|
@@ -76,12 +111,7 @@ export function useTimelineEditCallbacks({ | |
| // fallow-ignore-next-line complexity | ||
| (pct: number): { animId: string; tweenPct: number } | null => { | ||
| const cached = usePlayerStore.getState().keyframeCache.get(domEditSelection?.id ?? ""); | ||
| const kf = cached?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.2); | ||
| const group = kf?.propertyGroup; | ||
| const anim = | ||
| (group ? selectedGsapAnimations.find((a) => a.propertyGroup === group) : undefined) ?? | ||
| selectedGsapAnimations.find((a) => a.keyframes); | ||
| return anim ? { animId: anim.id, tweenPct: kf?.tweenPercentage ?? pct } : null; | ||
| return resolveTimelineKeyframeTarget(pct, cached?.keyframes ?? [], selectedGsapAnimations); | ||
| }, | ||
| [domEditSelection?.id, selectedGsapAnimations], | ||
| ); | ||
|
|
@@ -154,12 +184,22 @@ export function useTimelineEditCallbacks({ | |
| decision.position != null && | ||
| decision.duration != null | ||
| ) { | ||
| handleGsapResizeKeyframedTween( | ||
| target.animId, | ||
| decision.position, | ||
| decision.duration, | ||
| decision.pctRemap, | ||
| ); | ||
| if (anim.keyframes) { | ||
| handleGsapResizeKeyframedTween( | ||
| target.animId, | ||
| decision.position, | ||
| decision.duration, | ||
| decision.pctRemap, | ||
| ); | ||
| } else { | ||
| // resize-keyframed-tween requires an authored `keyframes` AST node | ||
| // and intentionally no-ops for a flat tween. Update its real tween | ||
| // window through the metadata writer (and SDK cutover path) instead. | ||
| handleGsapUpdateMeta(target.animId, { | ||
|
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. ❓ Question — why the two-branch dispatch instead of |
||
| position: decision.position, | ||
| duration: decision.duration, | ||
| }); | ||
| } | ||
| } | ||
| }, | ||
| onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => { | ||
|
|
||
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.
🟡 Nit — priority chain has 4 tiers, tests cover 3. The resolver's fallback chain is
groupedKeyframed ?? soleGroupedFlat ?? keyframed ?? soleFlat. Tests cover:soleFlat(test 1), null on multi-ungrouped-flat (test 2), null on multi-grouped-flat (test 3), andsoleGroupedFlat(test 4 — despite the name «prefers an explicit property-group match», both animations are flat here, so it's actually testing thesoleGroupedFlatbranch). What's NOT tested:groupedKeyframedwinning over a sibling flat in the same group (the intentional behavior change from the old.find(a => a.propertyGroup === group)which took ANY match). A one-liner test with a keyframed + flat in the same group would lock in that priority. — Rames D Jusso