Skip to content

Commit 284f48f

Browse files
committed
refactor(studio): share keyframe clip-% precision across lane writers
Review follow-ups on the expanded-lanes tip: - TimelinePropertyLanes.groupKeyframes re-derived the clip-relative percentage inline and skipped toClipKeyframes' rounding, the one precision every keyframe-cache writer has to agree on (selection keys embed the number). It now loops the shared helper per animation with the group filter and only overrides the lane's own group and ease. - TimelineClipDiamonds rebuilt and re-sorted each tween's sibling row inside the marker loop, so a row of N diamonds allocated N sorted copies of itself on every playhead tick. Built once per render instead, keyed by animation id. - Add the missing symmetry test at the real callback boundary: the diamond test mocks onMoveKeyframe, so nothing proved the rapid-second retime resolves a pending clip-% the keyframe cache has not caught up to. The new test asserts the identity-carrying target retimes and that the same drag without identity fields cannot.
1 parent c10df90 commit 284f48f

3 files changed

Lines changed: 97 additions & 23 deletions

File tree

‎packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,63 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
499499
view.unmount();
500500
});
501501

502+
// The diamond's rapid-second-retime path reports the PENDING clip-% (where the
503+
// first drag put the keyframe), which the keyframe cache has not caught up to.
504+
// TimelineClipDiamonds' own test mocks onMoveKeyframe, so only this one proves
505+
// the real callback resolves that stale-cache position off the identity fields
506+
// instead of failing the lookup.
507+
it("retimes from a pending position the keyframe cache has not caught up to", async () => {
508+
const authored = authoredInteriorAnimation();
509+
mocks.animations = [authored];
510+
usePlayerStore.setState({
511+
elements: [element],
512+
gsapAnimations: new Map([["index.html#box", [authored]]]),
513+
// Still the pre-drag positions: 75% is not in here.
514+
keyframeCache: new Map([
515+
[
516+
"index.html#box",
517+
{
518+
format: "percentage" as const,
519+
keyframes: [
520+
{ percentage: 0, properties: { x: 0 } },
521+
{ percentage: 50, properties: { x: 210 } },
522+
{ percentage: 100, properties: { x: 420 } },
523+
],
524+
},
525+
],
526+
]),
527+
});
528+
const view = renderCallbacks();
529+
530+
await expect(
531+
view.callbacks.onMoveKeyframe?.(
532+
"index.html#box",
533+
{
534+
percentage: 75,
535+
propertyGroup: "position",
536+
tweenPercentage: 50,
537+
animationId: authored.id,
538+
},
539+
85,
540+
),
541+
).resolves.toBe(true);
542+
expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith(
543+
authored.id,
544+
50,
545+
85,
546+
mocks.selection,
547+
);
548+
549+
// Control: the same drag WITHOUT the identity fields falls back to the cache
550+
// lookup, finds nothing at 75%, and cannot retime.
551+
mocks.actions.handleGsapMoveKeyframe.mockClear();
552+
await expect(
553+
view.callbacks.onMoveKeyframe?.("index.html#box", { percentage: 75 }, 85),
554+
).resolves.toBe(false);
555+
expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled();
556+
view.unmount();
557+
});
558+
502559
it("uses the clip timing basis when retiming a duration-less tween", async () => {
503560
const durationless = {
504561
...authoredInteriorAnimation(),

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,29 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
149149
keyframe.animationId === undefined
150150
? sorted
151151
: sorted.filter((k) => k.animationId === keyframe.animationId);
152+
// Compose each sibling's pending destination in first: clamping against
153+
// cached positions while the dragged keyframe reads its pending one let a
154+
// second drag cross a neighbour that had already moved past it. Built once
155+
// per render, keyed by tween: every diamond of a row needs the same row, and
156+
// rebuilding + re-sorting it inside the marker loop below made this
157+
// O(keyframes squared) allocations on every playhead tick.
158+
const pendingClipPctOf = (keyframe: TimelineDiamondKeyframe) =>
159+
pendingRetimes.get(timelineKeyframeSelectionKey(elementId, keyframeTarget(keyframe)))
160+
?.clipPct ?? keyframe.percentage;
161+
const siblingRows = new Map<
162+
string | undefined,
163+
{ keyframes: TimelineDiamondKeyframe[]; clipPcts: number[] }
164+
>();
165+
for (const keyframe of sorted) {
166+
if (siblingRows.has(keyframe.animationId)) continue;
167+
const row = siblingRowOf(keyframe)
168+
.map((k) => ({ keyframe: k, clipPct: pendingClipPctOf(k) }))
169+
.sort((a, b) => a.clipPct - b.clipPct);
170+
siblingRows.set(keyframe.animationId, {
171+
keyframes: row.map((s) => s.keyframe),
172+
clipPcts: row.map((s) => s.clipPct),
173+
});
174+
}
152175
const centerXOf = (percentage: number) =>
153176
Math.max(0, Math.min(clipWidthPx, (percentage / 100) * clipWidthPx));
154177
// One record per diamond, carrying its own geometry, so the connector and
@@ -202,19 +225,9 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
202225
const target = keyframeTarget(kf);
203226
const kfKey = timelineKeyframeSelectionKey(elementId, target);
204227
// Clamp against this keyframe's own tween, not the whole merged row.
205-
// Compose each sibling's pending destination in first: clamping against
206-
// cached positions while the dragged keyframe reads its pending one let
207-
// a second drag cross a neighbour that had already moved past it.
208-
const siblingRow = siblingRowOf(kf)
209-
.map((k) => ({
210-
keyframe: k,
211-
clipPct:
212-
pendingRetimes.get(timelineKeyframeSelectionKey(elementId, keyframeTarget(k)))
213-
?.clipPct ?? k.percentage,
214-
}))
215-
.sort((a, b) => a.clipPct - b.clipPct);
216-
const siblingClipPcts = siblingRow.map((s) => s.clipPct);
217-
const siblingIndex = siblingRow.findIndex((s) => s.keyframe === kf);
228+
const siblingRow = siblingRows.get(kf.animationId);
229+
const siblingClipPcts = siblingRow?.clipPcts ?? [];
230+
const siblingIndex = siblingRow?.keyframes.indexOf(kf) ?? -1;
218231
// While dragging this diamond, render it at the live preview clip-%.
219232
const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage;
220233
// Center the marker's non-overlapping hit region ON its keyframe %, so

‎packages/studio/src/player/components/TimelinePropertyLanes.tsx‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {
44
type GsapAnimation,
55
type PropertyGroupName,
66
} from "@hyperframes/core/gsap-parser";
7-
import { toAbsoluteTime } from "../../hooks/gsapShared";
7+
import { toClipKeyframes } from "../../hooks/gsapShared";
88
import { synthesizeFlatTweenKeyframes } from "../../hooks/gsapTweenSynth";
99
import { TimelineDiamondLane, type TimelineDiamondKeyframe } from "./TimelineClipDiamonds";
1010
import { LANE_H, getTimelineLaneTop } from "./timelineLayout";
@@ -67,6 +67,13 @@ function keyframeEase(keyframe: { ease?: string }, animation: GsapAnimation): st
6767
return keyframe.ease ?? animation.keyframes?.easeEach ?? animation.ease;
6868
}
6969

70+
/**
71+
* One lane row per keyframe of `group`. The clip-% re-basing goes through the
72+
* shared toClipKeyframes so lane rows land on the exact same percentage the
73+
* keyframe cache writes: this file used to derive it inline and skipped that
74+
* helper's rounding, which is the one precision every keyframe-cache writer has
75+
* to agree on (selection keys embed the number).
76+
*/
7077
function groupKeyframes(
7178
animations: readonly GsapAnimation[],
7279
group: PropertyGroupName,
@@ -75,18 +82,15 @@ function groupKeyframes(
7582
): TimelineDiamondKeyframe[] {
7683
const keyframes: TimelineDiamondKeyframe[] = [];
7784
for (const animation of animations) {
78-
const tweenStart =
79-
animation.resolvedStart ?? (typeof animation.position === "number" ? animation.position : 0);
80-
const tweenDuration = animation.duration ?? clipDuration;
81-
for (const keyframe of animationKeyframes(animation)) {
82-
if (!hasGroupProperty(keyframe.properties, group)) continue;
83-
const absoluteTime = toAbsoluteTime(tweenStart, tweenDuration, keyframe.percentage);
85+
const inGroup = animationKeyframes(animation).filter((keyframe) =>
86+
hasGroupProperty(keyframe.properties, group),
87+
);
88+
for (const keyframe of toClipKeyframes(inGroup, animation, clipStart, clipDuration)) {
8489
keyframes.push({
8590
...keyframe,
86-
percentage: ((absoluteTime - clipStart) / clipDuration) * 100,
87-
tweenPercentage: keyframe.percentage,
91+
// The LANE's group, not the tween's own classification: a mixed-property
92+
// tween classifies to undefined yet still feeds every group it touches.
8893
propertyGroup: group,
89-
animationId: animation.id,
9094
ease: keyframeEase(keyframe, animation),
9195
});
9296
}

0 commit comments

Comments
 (0)