Repository navigation
fix(core): a set that interrupts a tween holds in a real render at any worker count - #7318
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 9c71ac09
The fix itself looks right. The only blocker is the failing required Comments check. ↓ inline
Scope
Read gsap.ts (rerenderGsapTimelineAt, renderOntoInAuthoredOrder, settleOverlapsEndedBetween, landOn) and the new test. The settle replay runs inside the _ts = 0 window for children that start after t, and before the landing marks are restored, so it can't pull later tweens forward. Verified the 'fails without the fix:' claim locally with gsap 3.15.1 and vitest 4: with gsap.ts at merge-base 410a516c, the new test fails and 10 pass. At this head all 11 pass, and the gsap adapter suites pass (24). The rest of CI was still running.
| if (!below) { | ||
| reapplyStepsInAuthoredOrder(timeline, own, t); | ||
| } else if (own) { | ||
| // The step back reopened any tween ending within it, and its end overwrote the settled set. |
There was a problem hiding this comment.
blocker (CI): this comment line fails the required Comments check (RAISED comment share: gsap.ts 0.5% (3/636), base 0.3% (2/630)). Drop it, or put the reason into a name, e.g. extract settleWhatTheStepBackReopened(own, t).
There was a problem hiding this comment.
Fixed at dffbae7. I dropped the comment line and made no other change. The new test's name carries the reason.
9c71ac0 to
dffbae7
Compare
There was a problem hiding this comment.
Verdict: APPROVE — 1 resolved, 0 open, 0 new · 9c71ac09 → dffbae74
The only change is the removed comment line, and Comments passes. The code under review is the same as at 9c71ac09. The rest of CI was still running, so required checks still gate the merge.
…view to heygen-com#7258 Adopts heygen-com#7306 (motion-blur composition counts in render_complete telemetry, adapted to discoverMovers; readEvents stays false without heygen-com#7191) and heygen-com#7318 (a set that interrupts a tween holds at any worker count) together with its merged prerequisite heygen-com#6851. PR axis advances to heygen-com#7258; heygen-com#7253 and heygen-com#7257 are open and pinned as monitored (97->99). Co-authored-by: Vance Ingalls <vance@heygen.com> Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
A set that interrupts a running tween now holds in a real render at any worker count. On 0.9.19 the same film strobes at 2 or more workers and stays blank at 1 worker. This finishes what #7283 set out to do.
Why
#7283 settles the overlap inside
seekGsapTimeline. A real seek does one more thing: when the seek isn't silent,init.tsrunsrerenderGsapTimelineAtstraight after it. That re-render steps back 1 ms and forward again. On the frame that lands exactly on the tween's end, the step back reopens the tween and the step forward writes its end value over the settled set. Later frames have nothing left to settle, so that worker keeps the wrong value.Workers that jump across the tween's end don't land on it, so they come out right. That is the strobe: one worker in N is blank and the rest are correct.
#7283's tests call
seekGsapTimelinedirectly with events suppressed, so they never reach the re-render.Frame by frame, 90 frames, a panel fading for 1 s with a set at 0.3 s. P is the panel, B is black, f is mid-fade:
BBBBBB...BBBBBB...BPPBPPBPP...PPPBPPPB..., black on one worker's framesPPPPPP...Related work
Refs #7283, which this completes, and #7061, which settled preview.
How
renderOntoInAuthoredOrdersettles again after its forward step, using the samesettleOverlapsEndedBetweenthatlandOnuses, over the 1 ms it stepped through. Every re-render path goes through that function, so this covers them all. It only replays when a tween with a set inside it ended in that window. Any other re-render pays one scan of the timeline's overlap list.During a render
init.tspasses no structure, so this reads the overlaps fromownStructureOf(children). That is the real timeline, not the producer's stand-in.Test plan
gsap.order.test.ts, "keeps the set's value through the runtime's re-render, however frames are shared out". It runs the seek and then the re-render, asinit.tsdoes, with 1, 2, 3, 4 and 6 workers, with and without the stand-in.gsap.ts, that test fails and the other 10 in the file pass.packages/core/src/runtime: 119 files, 2,827 tests pass.tofixture on a Linux box, rebuilt from this head, at 1, 2, 3 and 6 workers in BeginFrame and at 3 workers in screenshot capture. Results are in "Per-frame results" below. An earlier build, the same code before I removed the comment, also gave the same result for fix(core): a render lands where preview does when a set interrupts a tween #7283's exactfromToform at 1, 2, 3, 4 and 6 workers. The CLI build's last step failed copying the studio files, which I hadn't built. The runtime manifest was already copied, and the frames changed from 0.9.19 to the fix.planthenrenderChunkfrom this head on one Linux box, whole film as one chunk. Results are in "Distributed path" below.The unit tests above ran before I put the new
ifin braces and removed its comment; CI runs them on this head. The renders in "Per-frame results" are from this head.Behaviour change
Same direction as #7283: a film with this overlap shows the set's value once the tween has ended. Frames inside the overlap are unchanged.
Per-frame results
90 frames, a panel fading for 1 s with a set at 0.3 s, read from the output video by average brightness. P is the panel, B is black, f is mid-fade.
PPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBPPPffffffPfffffffffffBBBBBBBBBBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPBPPPPffffffPfffffffffffBBBBBBBBBBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPPPPffffffPfffffffffffBBBBBBBBBBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPBPPPPPPPPffffffPfffffffffffBBBBBBBBBBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPBPPPPPffffffPfffffffffffBBBBBBBBBPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPffffffPfffffffffffBBBBBBBBBPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPffffffPfffffffffffBBBBBBBBBPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPffffffPfffffffffffBBBBBBBBBPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPffffffPfffffffffffBBBBBBBBBPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPDistributed path
The same bug shows differently here, because each worker takes a run of frames. The worker that plays through the fade lands exactly on its end, then keeps the wrong value until the next worker's run starts. This is the same fixture with a linear fade and the panel's centre read per frame.
The 6-worker rows really split: 7 browsers and 54 capture sessions, on 0.9.19 and on this PR. The 0.9.19 rows ran from the 0.9.19 producer in another environment, not on the box that ran this PR's rows.
Fade ending between two frames
This checks the cause. A fade lasting 1.01 s ends between frames, so no seek lands exactly on its end, and the re-render's 1 ms step back doesn't reach it. Such a fade should already be right on 0.9.19, and it is, at 1 and 3 workers, with the same frames as this PR:
PPPffffffPfffffffffffBBBBBBBBBBPPPPPP..., the panel back from frame 31 and holding to frame 89.Size
43 added lines and 2 removed, mostly the regression test; the fix itself is a few lines in
gsap.ts. It stands alone because it is one render bug with nothing else to ship with, and it completes #7283.