mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 03:09:57 +00:00
refactor(highway): carve the 2D drawing layer into highway-draw.js (R3c) (#917)
18 functions, 1,245 lines. highway.js 3,972 -> 2,727 (-31%). The biggest R3c slice: notes,
sustains, chords, strum groups, unison bends and lyrics — everything the default renderer
paints each frame.
━━━ MUTABILITY, NOT LOCATION, DECIDES WHERE A THING BELONGS ━━━
Three per-instance caches came out with this slice, and they are why it needed care:
_frameMismatchWarned a warn-once Set of chord ids (feedBack#88)
_chordRenderInfo a WeakMap of chord -> chain info
_lyricMeasureCache Map<fontSize, Map<text, width>>
All three are MUTATED. Left at module scope they would be SHARED ACROSS PANELS — one
highway's lyric widths and chord chains stomping another's, silently, with nothing throwing.
createHighway() is a factory (the constitution publishes window.createHighway so a plugin can
build a second highway), so they are lifted onto hwState, which is exactly what hwState is for.
The shimmer LUT went the OTHER way — to MODULE scope in highway-geometry.js. It is a
deterministic xorshift table, byte-for-byte identical for every instance, so sharing it is not
merely safe but BETTER: built once for the page rather than once per panel.
Same slice, opposite directions, decided entirely by whether the thing mutates.
━━━ MY SCRIPT WAS WRONG TWICE. THE GATES CAUGHT BOTH. ━━━
1. HAND-LISTED THE MOVE SET. I listed 10 functions and missed six that drawChords needs
(_ensureChordRenderCache, bsearchChords, getChordTemplateInfo, _computeChordBox,
_updateFretLinePreview, _drawFretLineChordPreview). The no-undef gate named every one. The
set is now DERIVED from the dependency closure — 18, not 10.
2. JUDGED PURITY TOO EARLY, and this one is subtle. I classified _computeChordBox as pure
because its ORIGINAL body never mentions hwState. Then the call-site rewriter injected
`fretX(hwState, …)` INTO it — fretX takes hwState now (#916) — leaving a function that
references an hwState it was never given. Purity has to be judged from the body AS IT WILL
BE, so the classifier iterates to a fixed point: a function needs hwState if it mentions it,
OR calls anything that now takes it. That moved _computeChordBox to the stateful side.
VERIFIED. A/B against origin/main: IDENTICAL, zero page errors. The PLUGIN BUNDLE contract is
byte-identical (b.fretX arity 3, b.getNoteState arity 2, both stable references, both correct
under the old calling convention). PERF GATE PASSES AT 1.92ms against its 12ms budget — and
this is the slice that could really have cost something: the ENTIRE per-frame drawing path is
now cross-module. It costs nothing measurable.
TESTS. highway_teaching_marks follows strumGroupBuckets to the new module. The two source-shape
harnesses now read highway.js AND every static/js/highway-*.js, rather than being re-pinned at
whichever file currently holds a function — re-pinning breaks again next time, and a shape
assertion that silently stops finding its target is indistinguishable from one that passes.
node 1045, pytest 2416, ESLint 0, no-undef 0, Codex 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
12eb73aee9
commit
36cf77dc44
@@ -19,8 +19,23 @@ const path = require('node:path');
|
||||
|
||||
const highwayJs = path.join(__dirname, '..', '..', 'static', 'highway.js');
|
||||
|
||||
|
||||
// R3c: highway.js is being carved into modules, so its source is no longer ONE file. Read the
|
||||
// whole set rather than re-pinning at whichever file currently holds a function — re-pinning
|
||||
// just breaks again next time, and a source-shape assertion that silently stops finding its
|
||||
// target is indistinguishable from one that passes.
|
||||
function highwaySources() {
|
||||
const root = path.join(__dirname, '..', '..');
|
||||
const jsDir = path.join(root, 'static', 'js');
|
||||
const parts = [fs.readFileSync(path.join(root, 'static', 'highway.js'), 'utf8')];
|
||||
for (const f of fs.readdirSync(jsDir).sort()) {
|
||||
if (f.startsWith('highway-') && f.endsWith('.js')) parts.push(fs.readFileSync(path.join(jsDir, f), 'utf8'));
|
||||
}
|
||||
return parts.join('\n');
|
||||
}
|
||||
|
||||
test('_ensureChordRenderCache keys off src, _inverted, AND chordTemplates', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
// The cache key triple must include chordTemplates — without it, a
|
||||
// late-arriving `chord_templates` WS message leaves cached
|
||||
// nonZeroNotes / nonZeroFrets stale until the next chord transition.
|
||||
@@ -41,7 +56,7 @@ test('_ensureChordRenderCache keys off src, _inverted, AND chordTemplates', () =
|
||||
});
|
||||
|
||||
test('chordTemplates change resets fretline preview and frame-mismatch warner', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
// The cache-invalidation block must clear both _chordFretLineNotes
|
||||
// (so _updateFretLinePreview re-publishes with corrected isOpen
|
||||
// classification) and _frameMismatchWarned (so a chord ID warned
|
||||
|
||||
@@ -36,13 +36,28 @@ function extractBlock(src, signature) {
|
||||
return src.slice(start, i);
|
||||
}
|
||||
|
||||
|
||||
// R3c: highway.js is being carved into modules, so its source is no longer ONE file. Read the
|
||||
// whole set rather than re-pinning at whichever file currently holds a function — re-pinning
|
||||
// just breaks again next time, and a source-shape assertion that silently stops finding its
|
||||
// target is indistinguishable from one that passes.
|
||||
function highwaySources() {
|
||||
const root = path.join(__dirname, '..', '..');
|
||||
const jsDir = path.join(root, 'static', 'js');
|
||||
const parts = [fs.readFileSync(path.join(root, 'static', 'highway.js'), 'utf8')];
|
||||
for (const f of fs.readdirSync(jsDir).sort()) {
|
||||
if (f.startsWith('highway-') && f.endsWith('.js')) parts.push(fs.readFileSync(path.join(jsDir, f), 'utf8'));
|
||||
}
|
||||
return parts.join('\n');
|
||||
}
|
||||
|
||||
test('highway declares the note-state provider slot', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
assert.match(src, /hwState\._noteStateProvider\s*=\s*null/, 'missing _noteStateProvider (provider slot, null = none)');
|
||||
});
|
||||
|
||||
test('public API exposes setNoteStateProvider / getNoteStateProvider / getNoteState / isDefaultRenderer', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
assert.match(src, /setNoteStateProvider\s*\(\s*fn\s*\)\s*\{[^}]*hwState\._noteStateProvider\s*=/, 'setNoteStateProvider must assign _noteStateProvider');
|
||||
assert.match(src, /setNoteStateProvider\s*\(\s*fn\s*\)\s*\{[^}]*typeof\s+fn\s*===\s*['"]function['"][^}]*:\s*null/, 'setNoteStateProvider must coerce non-functions (incl. null) to null');
|
||||
assert.match(src, /getNoteStateProvider\s*\(\s*\)\s*\{\s*return\s+hwState\._noteStateProvider/, 'getNoteStateProvider must return the slot');
|
||||
@@ -51,7 +66,7 @@ test('public API exposes setNoteStateProvider / getNoteStateProvider / getNoteSt
|
||||
});
|
||||
|
||||
test('_makeBundle exposes getNoteState (stable reference, no per-frame alloc)', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
const fn = extractBlock(src, 'function _makeBundle()');
|
||||
// The bundle field must point straight at _noteState — not a fresh
|
||||
// arrow each frame (the per-frame allocation the review flagged).
|
||||
@@ -67,7 +82,7 @@ test('_makeBundle exposes getNoteState (stable reference, no per-frame alloc)',
|
||||
});
|
||||
|
||||
test('_makeBundle exposes getNoteStateProvider as a stable reference (feedBack#254)', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
const fn = extractBlock(src, 'function _makeBundle()');
|
||||
// Same allocation discipline as getNoteState: highway_3d uses this
|
||||
// bundle field to tell "provider attached" from "no provider but
|
||||
@@ -90,7 +105,7 @@ test('_makeBundle exposes getNoteStateProvider as a stable reference (feedBack#2
|
||||
});
|
||||
|
||||
test('_noteState normalizes provider output as documented', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
const fn = extractBlock(fs.readFileSync(primitivesJs, 'utf8'), 'function _noteState(hwState, note, chartTime)');
|
||||
assert.match(fn, /if\s*\(\s*!hwState\._noteStateProvider\s*\)\s*return\s+null/, 'must short-circuit when no provider is registered');
|
||||
assert.match(fn, /try\s*\{[\s\S]*_noteStateProvider\s*\([\s\S]*catch[\s\S]*return\s+null/, 'must call the provider inside try/catch and return null on throw');
|
||||
@@ -102,9 +117,14 @@ test('_noteState normalizes provider output as documented', () => {
|
||||
});
|
||||
|
||||
test('default 2D renderer threads note state into drawNote / drawSustains / chord path', () => {
|
||||
const src = fs.readFileSync(highwayJs, 'utf8');
|
||||
const src = highwaySources();
|
||||
// drawNote takes the trailing `ns` param.
|
||||
assert.match(src, /function\s+drawNote\(\s*W\s*,\s*H\s*,\s*x\s*,\s*y\s*,\s*scale\s*,\s*string\s*,\s*fret\s*,\s*opts\s*,\s*ns\s*\)/, 'drawNote must accept the trailing ns param');
|
||||
// R3c: drawNote moved to ./static/js/highway-draw.js and gained hwState as its FIRST arg
|
||||
// (createHighway is a factory — a module cannot import per-instance state without two
|
||||
// panels sharing it). The contract asserted here is unchanged: `ns` is still the TRAILING
|
||||
// parameter, which is what the note-state threading depends on.
|
||||
assert.match(src, /function\s+drawNote\(\s*hwState\s*,\s*W\s*,\s*H\s*,\s*x\s*,\s*y\s*,\s*scale\s*,\s*string\s*,\s*fret\s*,\s*opts\s*,\s*ns\s*\)/,
|
||||
'drawNote must take hwState first and keep ns as the trailing param');
|
||||
// drawNotes / drawSustains / drawChords gate the lookup on the provider.
|
||||
// R3c: _noteState gained an explicit hwState first arg (it lives in a module now, and
|
||||
// createHighway is a factory). The CONTRACT here is unchanged and still the point: skip
|
||||
|
||||
@@ -32,7 +32,7 @@ const fingerLabel2D = loadFn('static/js/highway-geometry.js', 'teachingFingerLab
|
||||
const degreeLabel2D = loadFn('static/js/highway-geometry.js', 'teachingDegreeLabel');
|
||||
const fingerLabel3D = loadFn('plugins/highway_3d/screen.js', 'teachingFingerLabel');
|
||||
const degreeLabel3D = loadFn('plugins/highway_3d/screen.js', 'teachingDegreeLabel');
|
||||
const strumGroupBuckets = loadFn('static/highway.js', 'strumGroupBuckets');
|
||||
const strumGroupBuckets = loadFn('static/js/highway-draw.js', 'strumGroupBuckets');
|
||||
|
||||
// ── teachingFingerLabel (fg) ─────────────────────────────────────────────────
|
||||
|
||||
|
||||
Reference in New Issue
Block a user