feat(core): add deterministic keyframe ease runtime#2561
Conversation
7e38b13 to
5304a03
Compare
1e325a7 to
3a59b01
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE — new mechanism, backward-compat verified, deterministic by construction, well-tested.
Mechanism
In-runtime evaluators for four Studio ease tokens — hold, spring(<bounce>), wiggle(<n>,<type>[,amp]), custom(M0,0 C x1,y1 x2,y2 1,1) — plus installStudioCustomEase patches BOTH parseEase (public) and registerEase (internal _easeMap used at keyframe-segment resolution). Determinism comes from closed-form evaluation (spring: endpoint-normalized damped cosine; wiggle: sinusoidal envelope by type) and fixed-step bisection (24 iters → ~6e-8 accuracy on t) for the cubic-bezier path. Bisection over Newton is a deliberate determinism tradeoff — a doc-comment naming that would help future maintainers resist "optimizing" it, non-blocking.
Ground-truth-first — no pre-existing runtime evaluator
Grepped base branch: no installStudioCustomEase, no parseSpringBounce/evaluateSpringEase, no wiggleEase. The custom(...) token was parser-recognized (gsapParser.test.ts:644 at base) but had no runtime evaluator. Prior parsers/springEase exported SVG-path-based SpringPreset/SPRING_PRESETS used at figma emit time via CustomEase.create(...) — a distinct mechanism (path-based via GSAP plugin) that stays in place. Figma-emitted CustomEase.create(...) still routes through GSAP's plugin; spring(0.5) routes through the new closed-form evaluator. Two mechanisms coexist because they cover disjoint notations. No redundant-alias risk.
Spec correctness per ease type
hold—(p) => p >= 1 ? 1 : 0. figmaNAMED_EASE.holdshifts from"steps(1)"→"hold"; the two are functionally equivalent (step at end).custom(...)— bisection onx(t) = progress, theny(t). Monotonicity guard rejectsx1,x2 ∉ [0,1].y1,y2deliberately unbounded (overshoot beziers legitimate). Value-side lock:first(0.5).toBeCloseTo(0.5, 6)— I re-derived analytically for the symmetric(0.42,0,0.58,1):x(0.5) = 3·0.25·0.5·(0.42+0.58) + 0.125 = 0.5andy(0.5) = 0.5. Matches.first(0.25).toBeCloseTo(0.1292, 4)matches CSSease-in-outreference.spring(<bounce>)—y = (1 − e^{−decay·p}·cos(ω·p)) / endpoint,decay = 12−6·bounce,ω = 2π(1+1.5·bounce). Endpoint never crosses zero forbounce ∈ [0,1](checked bounds: ≈0.999994 at bounce=0, ≈1.00248 at bounce=1). Bounce clamped[0,1]at parse.wiggle(N, type[, amp])— sinusoidal envelope by type; endpoint-pinned;Number.isSafeIntegerguard on N; amplitude in[0,1]at parse.
All four short-circuit progress <= 0 → 0 and progress >= 1 → 1. Invalid inputs return null from resolve* and fall through to originalParseEase. GSAP config-args path handled: custom(M0,0 C0.25,0.1 0.25,1 1,1) comma-splits into 5 args, params.join(",") reconstitutes losslessly — customEase.test.ts:66 exercises this.
Backward-compat on existing goldens
The 22 regression shards + preview-parity + player-perf lanes are all green at head — the load-bearing signal for byte-level parity on existing compositions. Two potentially-breaking touchpoints checked:
hold → holdvs priorhold → steps(1)for figma-imported motion. Functionally equivalent (both step-at-end).- Parser fix:
durationfield in%-keyframe object entries now skipped (was previously round-tripping as a bogus animatable lane — a bug that no base-branch test exercised, hence no golden regression from correcting it).
Both safe.
Test coverage — value-side and reachability-side
- Value-side: exact
toBeCloseToon cubic-bezier decimals; endpoint pins on wiggle/spring; monotonicity on spring bounce; extrema count≥ 8on 6-wiggle 401-sample series. - Reachability-side:
init.test.tsfixtures route throughinitSandboxRuntimeModular— the tests verifywindow.gsap.parseEase("hold")andparseEase("custom(...)")resolve via the installed path, not just via direct-import of the exported helpers. The inner-timeline_ease-repair path has its own fixture that pre-builds a keyframes tween with_ease: undefinedand asserts it becomes a function post-init, which pins the load-order race repair.
GSAP-race handling
ensureStudioCustomEase is idempotent via __hfCustomEaseRegistered and gets re-called from bindRootTimelineIfAvailable — closes the "GSAP not ready at DOMContentLoaded" race. repairKeyframeInnerEase iterates getChildren(true, true, true) at bind-time and re-resolves any keyframe tween whose inner timeline._ease bakes to undefined from _parseEase running before registration completed. Well-motivated: the composition inline script builds tweens before this runtime's install call, and GSAP resolves the inner ease exactly once at build (gsap-core: tl._ease = _parseEase(keyframes.ease || vars.ease || "none")), so registering-after can't retro-fix an already-baked undefined without this explicit repair.
Perf
Per-sample: 24 fixed-iteration bisection (bezier) or 1-2 transcendentals (spring/wiggle). All eases cached per-parameter-signature; wiggle cache is module-level but bounded in practice by few integers × 4 types × few amplitudes. No per-sample allocation. Determinism → frame-time-independence: same (progress, params) → byte-identical output across platforms, which is the render-reproducibility invariant the PR title claims.
CI / peer
All 22 regression shards + preview-parity + player-perf + preflight green at 3a59b01. mergeable_state=unstable because downstack #2559 open — expected for the Graphite stack; Graphite mergeability_check is IN_PROGRESS and non-blocking. No prior reviews.
Nits (non-blocking)
WIGGLE_CACHEmodule-level (bounded, small state — no real leak).- Bisection-over-Newton is intentional but undocumented in-file; a one-line comment would future-proof against optimizer-drift.
- No test for
custom(...)withy1,y2 > 1(overshoot bezier). Behavior trivially correct by construction; a doctest would catch a future guard that accidentally clamps.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Adversarial pass at HEAD 3a59b01f6ff5d719e8712822c29500f2fe7b1e94. Context: HF#2561 is slice 3/20 — the foundational deterministic keyframe ease runtime that the next 17 PRs in the keyframe-UI series will build on. Decomposition from unreviewed golden reference #2387 (OPEN, REVIEW_REQUIRED). Deepest scrutiny goes here because contract flaws are exponentially harder to walk back at slice 15/20 than at 3/20.
Byte-identity vs #2387 (informational)
19/20 IDENTICAL — only init.test.ts differs (ref 99e235b vs new 072b3416), presumably because the ease tests exist at HEAD but not in the golden reference (or the reference has other unrelated test churn). The mechanism code is a clean carve-out from the unreviewed parent.
Determinism invariants (verified)
- No
Math.random/Date/performance.now/crypto/new Datein the ease evaluation path — grep-verified acrosswiggleEase.ts,customEase.ts,springEase.ts. - No PRNG for wiggle — closed-form
sin(TAU * wiggles * progress)envelope. Deterministic by construction given(progress, wiggles, type, amplitude). - Cubic bezier x-monotonicity preserved — guard
Math.min(x1,x2) < 0 || Math.max(x1,x2) > 1atcustomEase.ts:53ensures x(t) is monotonic non-decreasing whenx1, x2 ∈ [0, 1], so bisection converges to a unique t. - Boundary exact — all three curves return exact 0 at
progress <= 0and exact 1 atprogress >= 1(customEase.ts:33-34,springEase.ts:22-23,wiggleEase.ts:36-37). No float drift at endpoints. - Spring endpoint never zero — with
bounce ∈ [0,1],endpoint = 1 - exp(-decay) * cos(ω) ∈ ~[0.9996, 1.0025]. No div-by-zero risk. - Regex not ReDoS-vulnerable — bounded, non-nested quantifiers with disjoint alternations.
Cross-engine caveat (unverified, not violation): ECMAScript spec does NOT mandate correctly-rounded Math.exp/sin/cos. Preview + render both run in V8 (Chrome + headless Chrome), so should agree in practice — but this PR has zero cross-engine determinism test. The claim "preview and render agree" is asserted, not proven. Worth pinning a golden byte-level hash of sampled outputs on CI (proves stability across V8 versions too).
Findings
Medium concerns (4):
-
C1
WIGGLE_CACHEis a module-global unbounded Map — worker-fleet memory leak.packages/core/src/runtime/wiggleEase.ts:13declaresconst WIGGLE_CACHE = new Map<string, WiggleEase>()at module scope with no eviction. A render worker services many compositions and accumulates entries forever. MeanwhilespringEaseCacheandcustomEaseCacheare install-scoped (perinstallStudioCustomEasecall) — inconsistent scoping for no stated reason. User-controlled ease strings feed keys, so adversarial input can drive unbounded growth. Fix: either moveWIGGLE_CACHEintoinstallStudioCustomEase(matches spring/custom) or bound all three with an LRU. -
C2 Silent-catch on registration hides a total-failure mode with no telemetry.
packages/core/src/runtime/init.ts:180-184—ensureStudioCustomEaseswallows a throw frominstallStudioCustomEasewith a comment "falling back to GSAP's default ease is preferable to a broken runtime". If the install throws,hold/spring(...)/wiggle(...)/custom(...)all silently fall through to GSAP'soriginalParseEase, which returnsnull/undefinedfor these strings → downstream "_ease is not a function" masked as cross-origin Script error (which the PR itself calls out at :1216). This is the "invalidation story" — a subtly wrong render will produce no observable failure in production monitoring. Given this is the foundational contract for the next 17 PRs, silent registration failure is a landmine. Fix:swallow(err)through./diagnostics(imported at:52) or emit apostMessageanalytics event on catch. Zero-cost signal. -
C3
__hfCustomEaseRegisteredguards onwindow, notwindow.gsapidentity.packages/core/src/runtime/init.ts:174-184— the flag is set onwindowafter the first successful install. Ifwindow.gsapis later replaced with a fresh GSAP instance (composition script reloads, race with the CDN import re-executing), the guard says "already done" and skips re-installation on the NEW gsap.parseEaseon the new gsap is unpatched →hold/spring/etc. fall through to GSAP native → break. The test atinit.test.ts:482reproduces this exact scenario by explicit deletion, but a natural GSAP replacement in production wouldn't clear the flag. Fix:WeakSet<gsap>or stampgsap.__hfCustomEase = true(key on gsap identity, not onwindow). -
C4
evaluateSpringEaserecomputesdecay,angularFrequency, andendpointper call.packages/core/src/parsers/springEase.ts:25-29. The spring closure(progress) => evaluateSpringEase(progress, bounce)incustomEase.ts:65captures onlybounce; every eval call recomputes1 - Math.exp(-decay) * Math.cos(angularFrequency)— a fixed value oncebounceis fixed. At 60 fps × N springs × M keyframes-per-clip this is O(N·M·60) redundantexp+coson the hot path. Fix: inresolveSpringEase(customEase.ts:63), precompute the three values once and close over them. -
C13
repairKeyframeInnerEaseruns only insidebindRootTimelineIfAvailable, not on subsequent rebinds.packages/core/src/runtime/init.ts:1204-1274(repair helper at :1216ff, invocation at ~:1276). If a keyframes tween is added to the timeline AFTER the initial bind (e.g. via a delayed inline script or arequestAnimationFrame-deferred build), its inner_easebakes to undefined and never gets repaired — subsequent renders throw the "_ease is not a function" the fix was supposed to prevent. The comment at :1204 acknowledges the load-order race but the repair only runs atbindRootTimelineIfAvailable; periodic rebinds pertimelineRebindPolicydo re-run it, but only if the polling still catches new tweens. Worth confirming with thetimelineRebindPolicyowner whether "children added after first bind" is a supported flow. If yes, repair should run on every rebind pass; the current code only runs it after a successful primary bind.
Low / informational (8):
- C5
evaluateCubicBezierbisection has no early-termination — always 24 iterations. Consistent-time is arguably a feature for determinism; but Newton's method converges in ~4 steps. 24 × 2 × 4 = ~200 float ops per eval, ×60 fps ×N tweens. Perf note, not a defect. - C6
custom(...)regex"i"flag is dead code —customEase.ts:16-19has case-insensitive regex, butcustomEase.ts:71prefix-checks"custom("case-sensitively. Pick one. - C7
wiggleanticipateamplitude default (0.12 atwiggleEase.ts:41) is untested. Add mirror assertionevaluateWiggleEase(p,6,"anticipate") === evaluateWiggleEase(p,6,"anticipate",0.12). - C8
parseSpringBouncegrammar (springEase.ts:6) doesn't allow scientific notation;custom'sNUMBER_SOURCEdoes. Asymmetric grammars. - C9
holdsemantic —motionEase.ts:31changedhold: "steps(1)"tohold: "hold". Behaviorally identical, but re-imported Figma comps now serializeholdinstead ofsteps(1)— round-trip identity broken for that specific case (visual output identical). Existing on-disk"steps(1)"still works via GSAP native. Consider making the Figma mapper leave"steps(1)"for backwards-compat, or document the string migration. - C10
custom(...)grammar is strictly 4-point single-segment SVG (M 0,0 C x1,y1 x2,y2 1,1). GSAP's CustomEase plugin supports multi-segment. If any future slice or user-supplied path uses multi-segmentC c1..c6orS/Q/L, this runtime returns null and falls through tooriginalParseEasewhich requires the CustomEase plugin loaded → silent preview↔render parity break if plugin isn't injected in headless render.studio-server/src/routes/preview.ts:20injects the CDN CustomEase only whenhasCustomEase === true, based onmotion.customEasedetection — but the newcustom(...)string form is NOT scanned by that detector. Fix: (a) document that only the exact 4-point form is supported and pin an assertion; or (b) extendhtmlHasCustomEase/hasCustomEaseatstudio-server/src/routes/preview.ts:54-58to also check forcustom(strings. - C11
SUPPORTED_EASESatgsapConstants.ts:92-124listsholdbut doesn't listspring(0)/spring(1),wiggle(N,...),custom(...). Whatever validates ease strings againstSUPPORTED_EASES(e.g.validateCompositionGsap, referenced ingsapWriter.acorn.test.ts:399) will acceptholdbut rejectspring(0.5),wiggle(6,easeInOut),custom(...). Discoverability contract needs to know both. - C12 [informational] No property-based/fuzz test on determinism. All determinism assertions compare the same-run output twice — that's sequence-idempotency, not real determinism. A hidden
Math.random()would still pass every current test. Consider pinning a golden byte-level hash of sampled outputs, committed to the repo, asserted on CI (also proves V8-version stability).
Green surface (verified)
- Non-hyperframes eases (
power2.out, native GSAP names) fall through to the original parser unchanged (customEase.ts:107-108). installStudioCustomEaseshort-circuits on non-string input (customEase.ts:96).- Studio already emits
custom(M0,0 C... 1,1)today (velocityEaseFitter.ts:12-14,EaseCurveSection.tsx:99) — this PR is backfilling a resolver for strings already in the wild, not introducing a new grammar. installStudioCustomEaseidempotency confirmed — second-invocation test atinit.test.ts:478-480verifies the installedparseEaseis the same function object across repeated init calls.- Bezier bisection precision — 24 iterations ⇒ ~6e-8 t-precision, ample for pixel-accurate rendering.
Contract observations for the next 17 PRs
custom(...)is 4-point-only. Multi-segment path support = resolver upgrade + extend the CDN-injection detector instudio-server/src/routes/preview.ts:82— otherwise silent preview↔render parity break.- Spring is single-parameter (
bounce ∈ [0, 1]). GSAP supports 2-parameter (mass/damping); legacy Studio hasspring-gentle/spring-bouncynamed presets. If a slice introduces 2-param spring,SPRING_TOKENgrammar atspringEase.ts:6is exclusive of scientific notation and multi-arg. - Wiggle types are hard-enumerated to 4 at
wiggleEase.ts:12regex and again at:22-24. Adding a type = touching both. - Ease-registration side effects —
installStudioCustomEasemutatesgsap.parseEaseandgsap.registerEase's internal map (viaregisterEase("hold"/"spring"/"wiggle"/"custom", ...)). If any future slice tries to register a plugin named "custom"/"spring"/"wiggle" viaregisterPlugin, it collides. Unlikely but noted. SUPPORTED_EASESis the discoverability contract. New tokenized forms are silently supported by the runtime but not listed. Downstream UI slices need to know both surfaces.- Cache scoping is inconsistent —
WIGGLE_CACHEmodule-scope vs spring/custom install-scoped (C1). Whichever pattern is right, later slices should match. - Determinism claim relies on V8
Math.exp/sin/coscross-context stability — undocumented anywhere in the PR. Later slices that add golden-frame comparisons should be aware and pin a byte-level golden hash on CI.
What I didn't verify
- Did NOT run vitest suites; the 2,800/853 test counts in the PR body are as-claimed.
- Did NOT verify "_ease is not a function" cross-origin script error root cause in a real GSAP build (trusting the comment at
init.ts:1204-1215). - Did NOT check whether
getChildren(true, true, true)in the vendored GSAP returns fully-flattened descendants — third arg isignoreBeforeTimein newer GSAP releases, a semantic pun that could bite. - Did NOT verify
installStudioCustomEaseruns in the Studio browser preview path — tests are all vitest jsdom. - Did NOT verify
gsap.registerEaseAPI uniformity across the versions of GSAP hyperframes ships and pulls from CDN. - Did NOT benchmark the 24-iteration bisection cost numerically.
Merge gate
Solid mechanism, cleanly scoped, deterministic by construction. The four medium concerns (C1 unbounded cache, C2 silent registration failure, C3 gsap-identity guard, C4 hot-path recomputation) are the ones I'd address before merge because they compound over the next 17 slices — subtly wrong renders + memory leaks + registration-race breakage will be harder to root-cause once more of the stack is in play. C13 (repair scope) merits a quick verification with the timelineRebindPolicy owner. The low-severity items are follow-up candidates.
LGTM from my side on the core determinism contract — leaving as COMMENTED.
3 participants
Summary
Keyframe easing now evaluates deterministically in preview and render, including custom curves, spring, wiggle, and hold semantics.
Stack
Part 3 of 20. Parent: #2559. Next: #2572. The golden reference, #2387, remains open and unchanged.
Test plan
Post-Deploy Monitoring & Validation
Validation window: first 24 hours after the stack merges. Owner: Studio maintainers. Watch browser console and support reports for
[Timeline],gsap-parser, failed keyframe mutations, or preview/render easing mismatches. Healthy means edits persist and preview/render agree; revert the first failing layer if authored animation data changes unexpectedly.