diff --git a/.agents/skills/test-musicxml/SKILL.md b/.agents/skills/test-musicxml/SKILL.md index adebd0c4d..c5203693d 100644 --- a/.agents/skills/test-musicxml/SKILL.md +++ b/.agents/skills/test-musicxml/SKILL.md @@ -53,6 +53,17 @@ Use this skill when adding or updating a `vexml` MusicXML rendering test case, e 5. Update the implementation in `src/` to fill the rendering gap. - Prefer a minimal, root-cause fix. - Keep changes consistent with the existing renderer and test patterns. + - **Collisions must go through the collision pipeline.** When a fix involves moving an + element so it clears another (an above-stave annotation over a notehead/tie, a chord + diagram next to another, two annotations stacking) — or detecting that content is clipped + ("no-man's land") — route it through `CollisionDetector` (`src/collision.ts`): + register the fixed thing as an obstacle (`add`/`addRect`), resolve the movable thing + (`liftClear` to lift above-stave text, `pushRightOf` to space diagrams), then draw at the + resolved position and register it. Do **not** add new bespoke magic-offset clearance + logic. Not everything is movable — noteheads, stems, and ties are obstacles, not nudge + targets. Fixed structural placement (page margins, part spacing, bracket offsets, + widget-internal geometry) is exempt — it is deterministic, not clash resolution. See + `docs/collision-audit.md` for what is and isn't in scope. 6. Run the test command again: diff --git a/cli/fix.ts b/cli/fix.ts index 9da3267c8..564791067 100644 --- a/cli/fix.ts +++ b/cli/fix.ts @@ -8,6 +8,7 @@ export async function fix({ check }: { check: boolean }) { '.', ]); await run('bunx', ['tsc', '--noEmit']); + console.log('Compilation successful.'); // Validate all .musicxml fixtures against the MusicXML XSD. await run('./xmllint/validate.sh', []); } diff --git a/docs/collision-audit.md b/docs/collision-audit.md new file mode 100644 index 000000000..25e791c02 --- /dev/null +++ b/docs/collision-audit.md @@ -0,0 +1,70 @@ +# Nudge / collision audit + +> **Caveat — not a maintained list.** This is a point-in-time snapshot taken while the +> collision pipeline (`src/geometry.ts`, `src/quadtree.ts`, `src/collision.ts`) was introduced. +> Line numbers and entries **will drift** as the code changes — treat it as a map, not a +> source of truth. Re-audit (`rg`-style sweep for offset/clearance code) if you need current +> data. It exists so the historical "where did we nudge things?" knowledge isn't lost. + +The renderer (`src/draw.ts`) draws to a `` in immediate mode. Historically, every +time two elements could clash, a one-off fix nudged one of them with its own magic constant — +a whack-a-mole. The pipeline replaces the recurring class of those with one mechanism. This +doc records what existed, split by whether the pipeline absorbs it. + +## How the pipeline works (the one true path) + +Compute a movable element's natural `Rect` → ask the `CollisionDetector` to resolve it against +everything already placed (`liftClear` for above-stave text, `pushRightOf` for diagrams) → +draw at the resolved position → register the placed `Rect` so later elements avoid it. Fixed +elements (noteheads, stems, ties) are registered as obstacles but never moved. The detector +also reports out-of-bounds content (`escaping`) — the "no-man's land" where renders get +clipped. A `QuadTree` (with an out-of-bounds overflow bucket) is the spatial index. + +**Rule for future work:** any new "move this so it clears that" (or "is this being clipped?") +goes through `CollisionDetector`. Do not add new bespoke magic-offset clearance code. + +## A. Recurring runtime clash resolution — the whack-a-mole + +| Site | What moves | Clears | Status | +|---|---|---|---| +| `draw.ts::noteTop` | (obstacle calc) notehead ∪ stem tip ∪ above-articulation | — | feeds `noteRect` | +| `draw.ts::drawHarmony` | chord symbol lifts | notehead, stem-down tie apex, other text | **migrated** → `liftClear` | +| `draw.ts::drawWords` | words text lifts | notehead, tie, other text | **migrated** → `liftClear` | +| `draw.ts` chord-diagram block | fret box pushes right | adjacent diagram across a barline | **migrated** → `pushRightOf` | +| `draw.ts::drawTempo` | metronome mark lifts (`shiftY`) | high first note | **deferred** (scaled mark + reserved headroom; Phase 2) | +| chord diagram vertical | fixed `CHORD_DIAGRAM_GAP`, does **not** clear notes | (latent clip bug) | **deferred** (Phase 3) | + +The migration was behavior-preserving: the full screenshot suite was byte-identical except a +1–2px improvement on `harmony_grace` (the symbol now also clears the grace note's stem tip). +The new capabilities the pipeline enables but no fixture yet exercises: a wide symbol clearing +a *neighbouring* tall note under its text, and stacking multiple annotations in one column. + +## B. Out-of-bounds / clip hazards ("no-man's land") — covered by `escaping` + +| Site | Hazard | +|---|---| +| `draw.ts` `pageTop`/`pageBottom` growth | crop grown ad hoc so content isn't clipped | +| `constants.ts::LEDGER_HEADROOM`, `draw.ts` `topSlack` | scratch slack; "bump if an extreme tessitura ever clips" | +| `draw.ts` system-overflow two-pass (`topOverflow`) | reserves vertical space so stacked systems don't collide | + +`CollisionDetector.escaping(scratchViewport)` now flags vertical clips per system and warns +(see `warnEscapes` in `draw.ts`). The existing `pageTop`/`pageBottom` crop logic is unchanged — +`escaping` is the generic home for clip handling going forward, the two-pass overflow could +fold into it later. + +## C. Fixed structural placement — left alone (NOT collisions) + +These are deterministic engraving placement, not runtime clash resolution. Routing them through +the detector would add indirection for no benefit. Documented so future work doesn't "migrate" +them by mistake: + +- Page margins (`PAGE_MARGIN_*`), `INTER/INTRA_PART_SPACING`, `BASE_VOICE_WIDTH`. +- Lead-glyph width estimates (`LEAD_BARLINE/CLEF/KEY/TIME`) — generous reservations so notes + never collide with clef/key/time glyphs. +- `LABEL_GAP` / part-label centering (`+1.5`), `BRACKET_X_SHIFT` (bracket sits just outside the + system line, `draw.ts` bracket blocks). +- Tab note-area centering, tab grace alignment, `GRACE_SPACING` pre-reservation (`layout.ts`). +- Chord-diagram-internal geometry (`barShiftX`, `bridgeWidth`, title/dot/barre positions in + `chord-diagram.ts`). +- Tab fret / harmonic-bracket centering (`notes.ts` `boldFret`/harmonic layout). +- Analytic slur control-point lift (`spanners.ts` `cpYFor`), wrapped tie/slur split. diff --git a/site/src/App.tsx b/site/src/App.tsx index 39cba2fee..1967f97fd 100644 --- a/site/src/App.tsx +++ b/site/src/App.tsx @@ -95,9 +95,23 @@ export default function App() { const width = config.layout?.type === 'standard' ? (config.layout.width ?? 900) : 900; const notationFont = config.fonts?.notation?.family ?? 'Bravura'; - const reset = ( - key: 'noteSpacing' | 'softmaxFactor' | 'systemSpacing' | 'maxSystemFill', - ) => setConfig(({ [key]: _, ...rest }) => rest); + const resetKeys = [ + 'noteSpacing', + 'softmaxFactor', + 'systemSpacing', + 'maxSystemFill', + ] as const; + const reset = (key: (typeof resetKeys)[number]) => + setConfig(({ [key]: _, ...rest }) => rest); + const canReset = resetKeys.some((k) => config[k] !== undefined); + const resetAll = () => + setConfig((c) => { + const next = { ...c }; + for (const k of resetKeys) { + delete next[k]; + } + return next; + }); // `config` stays live so the sliders/reset respond instantly; `renderConfig` lags // behind it by the debounce so dragging a slider re-renders once it settles, not on @@ -109,13 +123,20 @@ export default function App() { skipConfigDebounce.current = false; return; } + // If the last render was fast, apply config changes immediately; only debounce + // once renders get slow enough to lag the sliders. + if (renderMs != null && renderMs <= 50) { + setRenderConfig(config); + setDebouncing(false); + return; + } setDebouncing(true); const t = setTimeout(() => { setRenderConfig(config); setDebouncing(false); }, 500); return () => clearTimeout(t); - }, [config]); + }, [config, renderMs]); useEffect(() => { const canvas = canvasRef.current; @@ -216,6 +237,13 @@ export default function App() { setDebouncing(false); return; } + // If the last render was fast, just render on every keystroke; only debounce + // once renders get slow enough to lag typing. + if (renderMs != null && renderMs <= 50) { + setDebouncing(false); + setInput(value); + return; + } // Spinner shows while we wait out the typing, then render the settled text. setDebouncing(true); debounceRef.current = setTimeout(() => { @@ -369,9 +397,23 @@ export default function App() {
- - Config - +
+ + Config + + +
+

+ With only a single system, some controls (e.g. system spacing + and max system fill) won't have a visible effect. +