Skip to content

Commit e0ad04d

Browse files
miguel-heygenmiguel-heygen
committed
fix(studio): target colliding keyframes exactly
1 parent 3a7950f commit e0ad04d

15 files changed

Lines changed: 337 additions & 131 deletions

‎packages/parsers/src/gsapParserExports.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ export type {
1414
GsapMethod,
1515
GsapKeyframesData,
1616
GsapPercentageKeyframe,
17+
SourcedGsapPercentageKeyframe,
1718
ParsedGsap,
1819
ArcPathConfig,
1920
ArcPathSegment,

‎packages/parsers/src/gsapSerialize.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,20 @@ export interface WritableGsapPercentageKeyframe extends GsapPercentageKeyframe {
9494
auto?: boolean;
9595
}
9696

97+
/**
98+
* A keyframe that still knows which tween emitted it, and where inside that
99+
* tween it sat. Merging several tweens onto one timeline row drops that
100+
* provenance unless it rides along on the keyframe, and an editor needs it to
101+
* route an edit back to the animation the user actually clicked. Required, not
102+
* optional: a keyframe that reaches a merge without it cannot be attributed at
103+
* all, and silently treating that as "no collision" is how an edit lands on the
104+
* wrong tween.
105+
*/
106+
export interface SourcedGsapPercentageKeyframe extends GsapPercentageKeyframe {
107+
animationId: string;
108+
tweenPercentage: number;
109+
}
110+
97111
/**
98112
* Collapse duplicate percentage entries before serializing an object literal.
99113
* Matches addKeyframeToScript's merge contract: later properties/ease win while
@@ -122,9 +136,9 @@ export function mergePercentageKeyframes(
122136

123137
export type GsapKeyframeFormat = "percentage" | "object-array" | "simple-array";
124138

125-
export interface GsapKeyframesData {
139+
export interface GsapKeyframesData<K extends GsapPercentageKeyframe = GsapPercentageKeyframe> {
126140
format: GsapKeyframeFormat;
127-
keyframes: GsapPercentageKeyframe[];
141+
keyframes: K[];
128142
ease?: string;
129143
easeEach?: string;
130144
}

‎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: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@
55
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
66
import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore";
77
import { resolveClipTimingBasis, resolveSelectorElementIds, toClipKeyframes } from "./gsapShared";
8-
import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth";
8+
import {
9+
deduplicateKeyframes,
10+
synthesizeFlatTweenKeyframes,
11+
type MergeableKeyframe,
12+
} from "./gsapTweenSynth";
913

1014
export function updateKeyframeCacheFromParsed(
1115
animations: GsapAnimation[],
@@ -16,7 +20,11 @@ export function updateKeyframeCacheFromParsed(
1620
): void {
1721
const { setKeyframeCache, elements, domClipChildren } = usePlayerStore.getState();
1822
const idsWithKeyframes = new Set<string>();
19-
const merged = new Map<string, KeyframeCacheEntry>();
23+
// Attributed keyframes only: everything in here came from a parsed tween via
24+
// toClipKeyframes, so the merge can rely on the source identity. It widens
25+
// back into KeyframeCacheEntry on the way to the store, which also holds the
26+
// runtime scan's unattributed keyframes.
27+
const merged = new Map<string, KeyframeCacheEntry & { keyframes: MergeableKeyframe[] }>();
2028
const sourceAnimations = new Map<string, GsapAnimation[]>();
2129
for (const anim of animations) {
2230
const kfSource =
@@ -49,9 +57,9 @@ export function updateKeyframeCacheFromParsed(
4957

5058
const existing = merged.get(id);
5159
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.
60+
// deduplicateKeyframes owns the same-% merge (including the colliding
61+
// animation targets downstream lanes read); a second copy of that rule
62+
// here is how the two writers drift.
5563
existing.keyframes = deduplicateKeyframes([...existing.keyframes, ...clipKeyframes]);
5664
} else {
5765
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: 50 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type {
22
GsapAnimation,
33
GsapKeyframesData,
4-
GsapPercentageKeyframe,
4+
SourcedGsapPercentageKeyframe,
55
} from "@hyperframes/core/gsap-parser";
66
import { PROPERTY_DEFAULTS } from "./gsapShared";
77

@@ -22,33 +22,60 @@ export function isStaticPositionHold(anim: GsapAnimation): boolean {
2222
return propKeys.length > 0 && propKeys.every((k) => k === "x" || k === "y");
2323
}
2424

25-
export function deduplicateKeyframes<
26-
T extends GsapPercentageKeyframe & { animationId?: string; easeAmbiguous?: boolean },
27-
>(keyframes: T[]): T[] {
25+
export interface AnimationKeyframeTarget {
26+
animationId: string;
27+
tweenPercentage: number;
28+
}
29+
30+
function accumulateCollidingAnimationTargets(
31+
keyframe: AnimationKeyframeTarget & {
32+
collidingAnimationTargets?: AnimationKeyframeTarget[];
33+
},
34+
incoming: AnimationKeyframeTarget,
35+
): void {
36+
const primaryId = keyframe.animationId;
37+
// One tween meeting itself is not a collision. Both identity fields are
38+
// required by the parameter types rather than guarded at runtime: a keyframe
39+
// that arrives without them cannot be attributed to a tween at all, and an
40+
// early return here would silently record no collision and let the inline
41+
// ease button edit an arbitrary one of the tweens that met at this
42+
// percentage. The compiler now refuses the incomplete keyframe instead.
43+
if (primaryId === incoming.animationId) return;
44+
const collisionTargets = keyframe.collidingAnimationTargets;
45+
if (collisionTargets?.some((target) => target.animationId === incoming.animationId)) return;
46+
keyframe.collidingAnimationTargets = [
47+
...(collisionTargets === undefined || collisionTargets.length === 0
48+
? [{ animationId: primaryId, tweenPercentage: keyframe.tweenPercentage }]
49+
: collisionTargets),
50+
{ animationId: incoming.animationId, tweenPercentage: incoming.tweenPercentage },
51+
];
52+
}
53+
54+
/**
55+
* What a keyframe looks like once it has been attributed to its source tween
56+
* and is ready to be merged with the other tweens landing on the same row. The
57+
* runtime scan produces unattributed keyframes and they never reach a merge, so
58+
* they are deliberately not this type.
59+
*/
60+
export type MergeableKeyframe = SourcedGsapPercentageKeyframe & {
61+
propertyGroup?: string;
62+
collidingAnimationTargets?: AnimationKeyframeTarget[];
63+
};
64+
65+
export function deduplicateKeyframes<T extends MergeableKeyframe>(keyframes: T[]): T[] {
2866
const byPct = new Map<number, T>();
2967
for (const kf of keyframes) {
3068
const existing = byPct.get(kf.percentage);
3169
if (existing) {
3270
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;
71+
accumulateCollidingAnimationTargets(existing, kf);
72+
// Whichever tween iterated last used to win `ease`, so the merged keyframe
73+
// carried an arbitrary one of the colliding curves. Readers that show a
74+
// single curve (drag readouts, lane hints, the inline ease button) then
75+
// displayed one belonging to a different animation than the one an edit
76+
// targets. A collision means "no single ease", and dropping it is the only
77+
// honest answer; collidingAnimationTargets still names every tween there.
78+
if ((existing.collidingAnimationTargets?.length ?? 0) > 1) delete existing.ease;
5279
else if (kf.ease) existing.ease = kf.ease;
5380
} else {
5481
byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } });

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
deduplicateKeyframes,
1717
isStaticPositionHold,
1818
synthesizeFlatTweenKeyframes,
19+
type MergeableKeyframe,
1920
} from "./gsapTweenSynth";
2021

2122
export { resolveSelectorElementIds };
@@ -83,7 +84,7 @@ export async function populateKeyframeCacheFromAst(
8384
const { setKeyframeCache } = usePlayerStore.getState();
8485
clearKeyframeCacheForFile(sf);
8586
const { elements, domClipChildren } = usePlayerStore.getState();
86-
const mergedByElement = new Map<string, GsapKeyframesData>();
87+
const mergedByElement = new Map<string, GsapKeyframesData<MergeableKeyframe>>();
8788
const sourceByElement = new Map<string, GsapAnimation[]>();
8889
for (const anim of parsed.animations) {
8990
if (anim.hasUnresolvedKeyframes) continue;

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
deduplicateKeyframes,
1313
isStaticPositionHold,
1414
synthesizeFlatTweenKeyframes,
15+
type MergeableKeyframe,
1516
} from "./gsapTweenSynth";
1617
import { fetchParsedAnimations, populateKeyframeCacheFromAst } from "./keyframeCacheAstLoad";
1718

@@ -266,13 +267,7 @@ export function useGsapAnimationsForElement(
266267
domClipChildren,
267268
);
268269

269-
const allKeyframes: Array<
270-
GsapKeyframesData["keyframes"][0] & {
271-
tweenPercentage?: number;
272-
propertyGroup?: string;
273-
animationId?: string;
274-
}
275-
> = [];
270+
const allKeyframes: MergeableKeyframe[] = [];
276271
let format: GsapKeyframesData["format"] = "percentage";
277272
let ease: string | undefined;
278273
let easeEach: string | undefined;

‎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)