Skip to content

Commit ab97f8f

Browse files
miguel-heygenmiguel-heygen
committed
fix(studio): target colliding keyframes exactly
1 parent 557569d commit ab97f8f

11 files changed

Lines changed: 302 additions & 116 deletions

‎packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,44 @@ describe("pruneKeyframeCacheToFiles", () => {
155155
});
156156

157157
describe("updateKeyframeCacheFromParsed", () => {
158+
it("records colliding animation targets with their own tween percentages", () => {
159+
const animation = (
160+
id: string,
161+
propertyGroup: string,
162+
properties: Record<string, number>,
163+
percentage: number,
164+
resolvedStart: number,
165+
): GsapAnimation => ({
166+
...animWithKeyframes(id),
167+
targetSelector: "#hero",
168+
propertyGroup,
169+
resolvedStart,
170+
keyframes: { format: "percentage", keyframes: [{ percentage, properties }] },
171+
});
172+
173+
usePlayerStore.setState({
174+
elements: [{ id: "hero", domId: "hero", tag: "div", start: 0, duration: 4, track: 0 }],
175+
});
176+
177+
updateKeyframeCacheFromParsed(
178+
[
179+
animation("hero-position", "position", { x: 100 }, 50, 0.5),
180+
animation("hero-visual", "visual", { opacity: 1 }, 80, 0.2),
181+
animation("hero-position", "position", { y: 50 }, 25, 0.75),
182+
animation("hero-scale", "scale", { scale: 2 }, 60, 0.4),
183+
],
184+
"scene.html",
185+
"hero",
186+
{},
187+
);
188+
189+
expect(cache().get("scene.html#hero")?.keyframes[0]?.collidingAnimationTargets).toEqual([
190+
{ animationId: "hero-position", tweenPercentage: 50 },
191+
{ animationId: "hero-visual", tweenPercentage: 80 },
192+
{ animationId: "hero-scale", tweenPercentage: 60 },
193+
]);
194+
});
195+
158196
it("serializes a multi-keyframe tween with a stable shape and animation identity", () => {
159197
const animation: GsapAnimation = {
160198
...animWithKeyframes("hero"),

‎packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,9 @@ export function updateKeyframeCacheFromParsed(
4949

5050
const existing = merged.get(id);
5151
if (existing) {
52-
// deduplicateKeyframes owns the same-% merge (including the easeAmbiguous
53-
// flag downstream lanes read); a second copy of that rule here is how the
54-
// two writers drift.
52+
// deduplicateKeyframes owns the same-% merge (including the colliding
53+
// animation targets downstream lanes read); a second copy of that rule
54+
// here is how the two writers drift.
5555
existing.keyframes = deduplicateKeyframes([...existing.keyframes, ...clipKeyframes]);
5656
} else {
5757
merged.set(id, {

‎packages/studio/src/hooks/gsapTweenSynth.test.ts‎

Lines changed: 70 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -54,31 +54,86 @@ describe("synthesizeFlatTweenKeyframes", () => {
5454
});
5555
});
5656

57-
describe("deduplicateKeyframes ease ambiguity", () => {
58-
it("flags a same-% collision from different animations (different eases)", () => {
57+
describe("deduplicateKeyframes colliding animation targets", () => {
58+
it("records each animation's tween percentage in first-seen order", () => {
5959
const merged = deduplicateKeyframes([
60-
{ percentage: 45, properties: { x: 10 }, ease: "power2.in", animationId: "#a-position" },
61-
{ percentage: 45, properties: { opacity: 1 }, ease: "power2.out", animationId: "#a-visual" },
60+
{
61+
percentage: 45,
62+
tweenPercentage: 20,
63+
properties: { x: 10 },
64+
ease: "power2.in",
65+
animationId: "#a-position",
66+
},
67+
{
68+
percentage: 45,
69+
tweenPercentage: 80,
70+
properties: { opacity: 1 },
71+
ease: "power2.out",
72+
animationId: "#a-visual",
73+
},
6274
]);
6375
const kf = merged.find((k) => k.percentage === 45);
64-
expect(kf?.easeAmbiguous).toBe(true);
76+
expect(kf?.collidingAnimationTargets).toEqual([
77+
{ animationId: "#a-position", tweenPercentage: 20 },
78+
{ animationId: "#a-visual", tweenPercentage: 80 },
79+
]);
6580
});
6681

67-
it("flags a cross-animation collision even when the raw eases match", () => {
68-
// The button can still only target one arbitrary animation, and each may
69-
// inherit a different easeEach/animation ease that raw comparison misses.
82+
it("deduplicates three colliding animations while preserving first-seen order", () => {
7083
const merged = deduplicateKeyframes([
71-
{ percentage: 45, properties: { x: 10 }, ease: "power2.in", animationId: "#a-position" },
72-
{ percentage: 45, properties: { opacity: 1 }, ease: "power2.in", animationId: "#a-visual" },
84+
{
85+
percentage: 45,
86+
tweenPercentage: 20,
87+
properties: { x: 10 },
88+
ease: "power2.in",
89+
animationId: "#a-position",
90+
},
91+
{
92+
percentage: 45,
93+
tweenPercentage: 80,
94+
properties: { opacity: 1 },
95+
ease: "power2.in",
96+
animationId: "#a-visual",
97+
},
98+
{
99+
percentage: 45,
100+
tweenPercentage: 40,
101+
properties: { y: 20 },
102+
ease: "power2.out",
103+
animationId: "#a-position",
104+
},
105+
{
106+
percentage: 45,
107+
tweenPercentage: 60,
108+
properties: { scale: 2 },
109+
ease: "power2.in",
110+
animationId: "#a-scale",
111+
},
112+
]);
113+
expect(merged.find((k) => k.percentage === 45)?.collidingAnimationTargets).toEqual([
114+
{ animationId: "#a-position", tweenPercentage: 20 },
115+
{ animationId: "#a-visual", tweenPercentage: 80 },
116+
{ animationId: "#a-scale", tweenPercentage: 60 },
73117
]);
74-
expect(merged.find((k) => k.percentage === 45)?.easeAmbiguous).toBe(true);
75118
});
76119

77-
it("does not flag a same-% collision within a single animation", () => {
120+
it("leaves the collision set undefined within a single animation", () => {
78121
const merged = deduplicateKeyframes([
79-
{ percentage: 45, properties: { x: 10 }, ease: "power2.in", animationId: "#a-position" },
80-
{ percentage: 45, properties: { y: 20 }, ease: "power2.out", animationId: "#a-position" },
122+
{
123+
percentage: 45,
124+
tweenPercentage: 20,
125+
properties: { x: 10 },
126+
ease: "power2.in",
127+
animationId: "#a-position",
128+
},
129+
{
130+
percentage: 45,
131+
tweenPercentage: 80,
132+
properties: { y: 20 },
133+
ease: "power2.out",
134+
animationId: "#a-position",
135+
},
81136
]);
82-
expect(merged.find((k) => k.percentage === 45)?.easeAmbiguous).toBeFalsy();
137+
expect(merged.find((k) => k.percentage === 45)?.collidingAnimationTargets).toBeUndefined();
83138
});
84139
});

‎packages/studio/src/hooks/gsapTweenSynth.ts‎

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,33 +22,59 @@ export function isStaticPositionHold(anim: GsapAnimation): boolean {
2222
return propKeys.length > 0 && propKeys.every((k) => k === "x" || k === "y");
2323
}
2424

25+
export interface AnimationKeyframeTarget {
26+
animationId: string;
27+
tweenPercentage: number;
28+
}
29+
30+
function accumulateCollidingAnimationTargets(
31+
keyframe: {
32+
animationId?: string;
33+
tweenPercentage?: number;
34+
collidingAnimationTargets?: AnimationKeyframeTarget[];
35+
},
36+
incoming: { animationId?: string; tweenPercentage?: number },
37+
): void {
38+
const primaryId = keyframe.animationId;
39+
if (
40+
primaryId === undefined ||
41+
keyframe.tweenPercentage === undefined ||
42+
incoming.animationId === undefined ||
43+
incoming.tweenPercentage === undefined ||
44+
primaryId === incoming.animationId
45+
) {
46+
return;
47+
}
48+
const collisionTargets = keyframe.collidingAnimationTargets;
49+
if (collisionTargets?.some((target) => target.animationId === incoming.animationId)) return;
50+
keyframe.collidingAnimationTargets = [
51+
...(collisionTargets === undefined || collisionTargets.length === 0
52+
? [{ animationId: primaryId, tweenPercentage: keyframe.tweenPercentage }]
53+
: collisionTargets),
54+
{ animationId: incoming.animationId, tweenPercentage: incoming.tweenPercentage },
55+
];
56+
}
57+
2558
export function deduplicateKeyframes<
26-
T extends GsapPercentageKeyframe & { animationId?: string; easeAmbiguous?: boolean },
59+
T extends GsapPercentageKeyframe & {
60+
animationId?: string;
61+
tweenPercentage?: number;
62+
collidingAnimationTargets?: AnimationKeyframeTarget[];
63+
},
2764
>(keyframes: T[]): T[] {
2865
const byPct = new Map<number, T>();
2966
for (const kf of keyframes) {
3067
const existing = byPct.get(kf.percentage);
3168
if (existing) {
3269
existing.properties = { ...existing.properties, ...kf.properties };
33-
// Two DIFFERENT source animations with a keyframe at the same clip %: a
34-
// single inline ease button can only target one of them, and which one is
35-
// arbitrary (each may also inherit a different easeEach/animation ease, so
36-
// comparing raw keyframe eases isn't enough). Flag it so the collapsed row
37-
// hides the button there and the user edits per-lane instead.
38-
if (
39-
existing.animationId !== undefined &&
40-
kf.animationId !== undefined &&
41-
existing.animationId !== kf.animationId
42-
) {
43-
existing.easeAmbiguous = true;
44-
}
45-
// Whichever tween iterated last used to win `ease`, so the merged
46-
// keyframe carried an arbitrary one of the colliding curves. Readers that
47-
// do not check easeAmbiguous (drag readouts, lane hints) then showed a
48-
// curve belonging to a different animation than the one an edit targets.
49-
// Drop it instead: ambiguous means "no single ease", and the flag is the
50-
// only honest answer.
51-
if (existing.easeAmbiguous) delete existing.ease;
70+
accumulateCollidingAnimationTargets(existing, kf);
71+
// Whichever tween iterated last used to win `ease`, so the merged keyframe
72+
// carried an arbitrary one of the colliding curves. Readers that show a
73+
// single curve (drag readouts, lane hints, the inline ease button) then
74+
// displayed one belonging to a different animation than the one an edit
75+
// targets. A collision means "no single ease", and dropping it is the only
76+
// honest answer; collidingAnimationTargets still names every tween there.
77+
if ((existing.collidingAnimationTargets?.length ?? 0) > 1) delete existing.ease;
5278
else if (kf.ease) existing.ease = kf.ease;
5379
} else {
5480
byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } });

‎packages/studio/src/player/components/TimelineClipDiamonds.test.tsx‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -658,7 +658,18 @@ describe("TimelineClipDiamonds", () => {
658658
<TimelineDiamondLane
659659
keyframesData={{
660660
format: "percentage",
661-
keyframes: [kf(0), kf(50), kf(100, { easeAmbiguous: lastAmbiguous })],
661+
keyframes: [
662+
kf(0),
663+
kf(50),
664+
kf(100, {
665+
collidingAnimationTargets: lastAmbiguous
666+
? [
667+
{ animationId: "anim-1", tweenPercentage: 100 },
668+
{ animationId: "anim-2", tweenPercentage: 75 },
669+
]
670+
: undefined,
671+
}),
672+
],
662673
}}
663674
clipWidthPx={clipWidthPx}
664675
clipHeightPx={48}
@@ -675,15 +686,16 @@ describe("TimelineClipDiamonds", () => {
675686
return { host, root };
676687
};
677688

678-
it("hides the inline ease button on an ambiguous merged segment", () => {
679-
// Segments 0->50 and 50->100; the 50->100 segment ends on the ambiguous
680-
// keyframe, so its hover/ease-button area is not rendered.
689+
it("hides the inline ease button on a colliding merged segment", () => {
690+
// The 50->100 segment ends on a keyframe shared by two animations, so one
691+
// button cannot honestly stand for the several curves that meet there. Only
692+
// the unambiguous 0->50 segment keeps its button.
681693
const { host, root } = renderSegmentLane(true);
682694
expect(host.querySelectorAll("[data-keyframe-ease-segment]").length).toBe(1);
683695
act(() => root.unmount());
684696
});
685697

686-
it("keeps the inline ease button on unambiguous merged segments", () => {
698+
it("shows the inline ease button on single-animation merged segments", () => {
687699
const { host, root } = renderSegmentLane(false);
688700
expect(host.querySelectorAll("[data-keyframe-ease-segment]").length).toBe(2);
689701
act(() => root.unmount());

0 commit comments

Comments
 (0)