Skip to content

feat(studio): timeline magnetic snapping#2196

Closed
ukimsanov wants to merge 1 commit into
studio-dnd/pr04-timeline-collisionfrom
studio-dnd/pr05-timeline-snapping
Closed

feat(studio): timeline magnetic snapping#2196
ukimsanov wants to merge 1 commit into
studio-dnd/pr04-timeline-collisionfrom
studio-dnd/pr05-timeline-snapping

Conversation

@ukimsanov

@ukimsanov ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What

new pure module timelineSnapping — snap-target collection and pixel-threshold time snapping (collectTimelineSnapTargets, snapTimelineTime, snapMoveToTargets) with tests.

Why

the magnet math for clip drags/trims, reviewable standalone.

How

new files only; type-only playerStore imports; consumers land with the drag engine.

Test plan

bunx vitest run timelineSnapping.test.ts; tsc --noEmit; fallow audit clean.


Stack position 5/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary TEMP(studio-dnd) fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.

ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Jul 11, 2026
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch from e0cc5a5 to 7312dd0 Compare July 11, 2026 00:55
@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from 055fc12 to eb1e603 Compare July 11, 2026 01:15
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch from 7312dd0 to 7da8833 Compare July 11, 2026 01:15

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — Miga

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM (green) — units clean, tie-break deterministic

Snapping in miniature. The module keeps pixels and seconds strictly separated at the module boundary, resolves them once inside snapMoveToTargets, and never conflates the two again.

The pointed queries

  • Pixel-vs-time unit consistency. packages/studio/src/player/components/timelineSnapping.ts:11TIMELINE_SNAP_PX = 8 is documented as pixel radius; conversion to seconds happens exactly once at :80 (thresholdSecs = TIMELINE_SNAP_PX / Math.max(pixelsPerSecond, 1)). Every downstream comparison is in seconds. Math.max(pps, 1) guards divide-by-zero — commendable.
  • Deterministic tie-break. snapTimelineTime at :55–:60 chooses the winner among equidistant targets by TYPE_PRIORITY[type] (playhead=0 < clip-edge=1 < beat=2) — the same ordering used by the collector's dedup at :30. Order-independent and consistent between the collector's dedup and the picker's tie-break; that symmetry is what makes the fn deterministic.
  • Negative deltas / snap-to-zero. The collector at :27 rejects time < 0 outright, so no snap target can pull a clip below zero. snapMoveToTargets clamps the candidate to [0, timelineDuration − duration] at :99–:100 and drops the highlight if clamping moves the clip off-target (:101) — no phantom snap indicator when it can't actually resolve. Test at :86–:90 pins this explicitly.
  • Threshold scaling with zoom. The pps=10 vs pps=1000 case at :92–:96 exercises the two extremes of the pixel↔time projection. Both directions verified.

Nits (non-blocking)

  • Dedup tie-break at :30 is tested for playhead > beat (:41–:49) but not clip-edge > beat; the code path is identical (min priority wins) so the invariant is by construction, but a one-line case would give the test more surface area if you're feeling generous.

Nothing here that would keep a case awake at night.

R1 by Via

@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from eb1e603 to e21c151 Compare July 11, 2026 02:56
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch 2 times, most recently from 85079ea to f441a84 Compare July 11, 2026 08:18
@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from e21c151 to ed27a2e Compare July 11, 2026 08:18
@ukimsanov ukimsanov marked this pull request as ready for review July 11, 2026 09:06

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — snap indicator drops on frame-quantized durations

timelineSnapping.ts:100-101:

const clamped = Math.max(0, Math.min(maxStart, Math.round(candidate * 1000) / 1000));
if (target && Math.abs(clamped - candidate) > 1e-6) target = null;

The 1e-6 tolerance is meant to detect "did the timeline-bounds clamp actually pull us?" — but line 100 also rounds candidate to 1ms, and for the end-snap path candidate = endSnap.time - duration inherits duration's sub-ms bits. For any frame-quantized duration (1/24, 1/30, 1/60s), the rounding drift is ~5e-4, well above 1e-6, so target is nulled every single time.

Reproduced with dur = 1/24, snap target playhead at t=5:

{"start":4.958, "snapTime":null, "snapType":null}

The clip still snaps correctly (4.948→4.958); the snap-line indicator just vanishes at the moment it should show. UX regression not caught by tests because all shipped tests use integer durations.

Fix: gate the target-nulling on whether the clamp actually moved past the timeline bounds (candidate < 0 || candidate > maxStart), not on rounding delta. OR round candidate before the compare so both sides share the same precision.

— Rames Jusso

@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from ed27a2e to 7f9c812 Compare July 11, 2026 10:49
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch 3 times, most recently from a399c01 to ff3ddb7 Compare July 11, 2026 21:08
@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from 3916b0f to e0ad57b Compare July 11, 2026 21:08
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch from ff3ddb7 to b3ae6f0 Compare July 11, 2026 22:38
@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch 2 times, most recently from 28f4251 to ca5caa8 Compare July 12, 2026 01:02
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch from b3ae6f0 to bf710eb Compare July 12, 2026 01:02
What: new pure module timelineSnapping — snap-target collection and
pixel-threshold time snapping (collectTimelineSnapTargets, snapTimelineTime,
snapMoveToTargets) with tests.

Why: the magnet math for clip drags/trims, reviewable standalone.

How: new files only; type-only playerStore imports; consumers land with the
drag engine.

Test plan: bunx vitest run timelineSnapping.test.ts; tsc --noEmit; fallow
audit clean.
@ukimsanov ukimsanov force-pushed the studio-dnd/pr04-timeline-collision branch from ca5caa8 to b910ef1 Compare July 12, 2026 01:22
@ukimsanov ukimsanov force-pushed the studio-dnd/pr05-timeline-snapping branch from bf710eb to f1f90c1 Compare July 12, 2026 01:22

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head and verified the addressed feedback and required behavior. Approval is for this exact head; Graphite mergeability may still be pending.

@miguel-heygen miguel-heygen deleted the branch studio-dnd/pr04-timeline-collision July 12, 2026 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants