From 6050b6262b20dd4cbb45781d79290f58d0656271 Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Sat, 5 Sep 2026 08:02:00 +0200 Subject: [PATCH] fix(h3d): thread maxStrings param through resolveStringCount (Toby r1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toby r1 on c7f7c88: MAX_RENDER_STRINGS=6 hardcoded in utils.js is a stale compile-time copy — if S_COL grows to 7 entries, resolveStringCount silently clamps a 7-string chart to 6 and no test fails. Fix: delegator-param pattern (mirrors fretX / geoFretX): - Remove const MAX_RENDER_STRINGS from utils.js - resolveStringCount(bundle, maxStrings) — param replaces the copy - _openStringPitchLabelsForTuning(bundle, songInfo, n, maxStrings) — same - screen.js import aliases (_resolveStringCountBase, _openStringPitchLabelsForTuningBase) - 1-line delegators in IIFE supply MAX_RENDER_STRINGS (= S_COL.length); zero call sites change - NSTR=6 kept (it is a fixed semantic fact about standard guitar, not a palette ceiling) New test: 'maxStrings param is authoritative, not a hardcoded 6' — resolveStringCount({stringCount:7}, 7)=7; re-hardcode mutation → RED. Wiring tests: delegator lines asserting _resolveStringCountBase/ _openStringPitchLabelsForTuningBase each receive MAX_RENDER_STRINGS. Mutation-verified: re-hardcode 6 → 1 RED; original → 210/210 GREEN. Suite: node --test tests/js/highway_3d*.test.js plugins/highway_3d/tests/*.test.js 207→210/210 pass. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_014uZ169yfoFYArXz962g7KW --- plugins/highway_3d/screen.js | 4 ++- plugins/highway_3d/src/utils.js | 16 ++++++----- tests/js/highway_3d_utils.test.js | 45 ++++++++++++++++++++++++------- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index e89e17f..83bc014 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -8,7 +8,7 @@ import { geoFretX, dZ, slideTrailEnd, camBaseDistU, camLowFretPullbackU, computeBPM, _makeGaussTex, RENDER_ORDER_LAYER_STACK, RENDER_ORDER_LAYER_INDEX, RENDER_ORDER_AT_Z_ZERO, RENDER_ORDER_FAR_CLAMP, renderOrderForLayerAtZ, _noteKey, lowerBoundT, hwyFirstRelevantFrettedTime, geoFretMid } from './src/geometry.js'; // h3d-carve-1b import { loadThree, T } from './src/three-loader.js'; // h3d-carve-2 -import { _h3dHexToInt, _clampByteI, _darkenInt, _lightenInt, resolveStringCount, _NOTE_NAMES_SHARP, _BASE_OPEN_MIDI_BASS4, _BASE_OPEN_MIDI_BASS5, _BASE_OPEN_MIDI_GUITAR6, _BASE_OPEN_MIDI_GUITAR7, _BASE_OPEN_MIDI_GUITAR8, _baseOpenStringMidis, _midiToPitchLabel, _openStringPitchLabelsForTuning, _ssActive, _ssIsCanvasFocused } from './src/utils.js'; // h3d-carve-3 +import { _h3dHexToInt, _clampByteI, _darkenInt, _lightenInt, resolveStringCount as _resolveStringCountBase, _NOTE_NAMES_SHARP, _BASE_OPEN_MIDI_BASS4, _BASE_OPEN_MIDI_BASS5, _BASE_OPEN_MIDI_GUITAR6, _BASE_OPEN_MIDI_GUITAR7, _BASE_OPEN_MIDI_GUITAR8, _baseOpenStringMidis, _midiToPitchLabel, _openStringPitchLabelsForTuning as _openStringPitchLabelsForTuningBase, _ssActive, _ssIsCanvasFocused } from './src/utils.js'; // h3d-carve-3 (function () { 'use strict'; @@ -782,10 +782,12 @@ import { _h3dHexToInt, _clampByteI, _darkenInt, _lightenInt, resolveStringCount, const MAX_RENDER_STRINGS = S_COL.length; // resolveStringCount — moved to src/utils.js (h3d-carve-3). + const resolveStringCount = bundle => _resolveStringCountBase(bundle, MAX_RENDER_STRINGS); // h3d-carve-3: delegator (1 beyond-subst; passes authoritative S_COL.length ceiling) // _NOTE_NAMES_SHARP, _BASE_OPEN_MIDI_BASS4/5, _BASE_OPEN_MIDI_GUITAR6/7/8 — moved to src/utils.js (h3d-carve-3). // _baseOpenStringMidis, _midiToPitchLabel, _openStringPitchLabelsForTuning — moved to src/utils.js (h3d-carve-3). + const _openStringPitchLabelsForTuning = (bundle, songInfo, n) => _openStringPitchLabelsForTuningBase(bundle, songInfo, n, MAX_RENDER_STRINGS); // h3d-carve-3: delegator (1 beyond-subst) const STR_THICK = 0.25 * K; diff --git a/plugins/highway_3d/src/utils.js b/plugins/highway_3d/src/utils.js index 6cb8233..5beaf41 100644 --- a/plugins/highway_3d/src/utils.js +++ b/plugins/highway_3d/src/utils.js @@ -7,9 +7,13 @@ * PALETTES.default / S_COL layout). */ -// ── Compile-time copies of IIFE constants ───────────────────────────────────── +// ── Compile-time IIFE constant ──────────────────────────────────────────────── +// NSTR: standard guitar default (not a palette ceiling — safe as a fixed fact). +// MAX_RENDER_STRINGS is NOT duplicated here; it is the authority of S_COL.length +// in screen.js and flows in via the maxStrings parameter of resolveStringCount +// and _openStringPitchLabelsForTuning. screen.js keeps 1-line delegators that +// supply MAX_RENDER_STRINGS so zero call-sites change. const NSTR = 6; -const MAX_RENDER_STRINGS = 6; // S_COL.length = PALETTES.default.length // ── Color utilities ─────────────────────────────────────────────────────────── @@ -47,10 +51,10 @@ export function _lightenInt(hex, t) { * Clamped to MAX_RENDER_STRINGS so a malformed bundle doesn't index past * the per-string material arrays. */ -export function resolveStringCount(bundle) { +export function resolveStringCount(bundle, maxStrings) { const sc = bundle && bundle.stringCount; if (Number.isFinite(sc) && sc >= 1) { - return Math.min(Math.trunc(sc), MAX_RENDER_STRINGS); + return Math.min(Math.trunc(sc), maxStrings); } return /bass/i.test(bundle?.songInfo?.arrangement || '') ? 4 : NSTR; } @@ -102,8 +106,8 @@ export function _midiToPitchLabel(midi) { * @param {object} songInfo WS song_info blob (arrangement, tuning, capo) * @param {number} nEffective String count clamped like nStr / resolveStringCount */ -export function _openStringPitchLabelsForTuning(bundle, songInfo, nEffective) { - const n = Number.isFinite(nEffective) ? Math.min(Math.max(1, Math.trunc(nEffective)), MAX_RENDER_STRINGS) : resolveStringCount(bundle); +export function _openStringPitchLabelsForTuning(bundle, songInfo, nEffective, maxStrings) { + const n = Number.isFinite(nEffective) ? Math.min(Math.max(1, Math.trunc(nEffective)), maxStrings) : resolveStringCount(bundle, maxStrings); // bundle first: chart-transform substitutes tuning/capo there, while // songInfo keeps the chart's originals by contract. A malformed // (non-array) bundle.tuning falls back to songInfo instead of diff --git a/tests/js/highway_3d_utils.test.js b/tests/js/highway_3d_utils.test.js index 51bbc0d..147ab47 100644 --- a/tests/js/highway_3d_utils.test.js +++ b/tests/js/highway_3d_utils.test.js @@ -96,25 +96,34 @@ test('_lightenInt: mixing pure black toward white by 1.0 yields white', () => { // ── resolveStringCount ──────────────────────────────────────────────────────── -test('resolveStringCount: uses bundle.stringCount and clamps to 6', () => { +test('resolveStringCount: uses bundle.stringCount and clamps to maxStrings', () => { // Mutation: remove Math.min → returns 8 for a chart that declares 8 strings; - // _NOTE_NAMES_SHARP lookup and per-string material arrays index OOB. - assert.strictEqual(fns().resolveStringCount({ stringCount: 8 }), 6, - 'stringCount=8 exceeds MAX_RENDER_STRINGS=6; must clamp to 6'); - assert.strictEqual(fns().resolveStringCount({ stringCount: 4 }), 4); + // per-string material arrays index OOB. + assert.strictEqual(fns().resolveStringCount({ stringCount: 8 }, 6), 6, + 'stringCount=8 exceeds maxStrings=6; must clamp'); + assert.strictEqual(fns().resolveStringCount({ stringCount: 4 }, 6), 4); +}); + +test('resolveStringCount: maxStrings param is authoritative, not a hardcoded 6', () => { + // Mutation: re-hardcode maxStrings=6 inside utils.js → resolveStringCount({stringCount:7}, 7) + // returns 6; 7th-string notes are silently never drawn and no test fails. + assert.strictEqual(fns().resolveStringCount({ stringCount: 7 }, 7), 7, + 'maxStrings=7 must allow stringCount=7 through without clamping to a hardcoded 6'); + assert.strictEqual(fns().resolveStringCount({ stringCount: 10 }, 7), 7, + 'stringCount exceeding maxStrings must clamp to maxStrings, not 6'); }); test('resolveStringCount: falls back to 4 for bass arrangement', () => { // Mutation: remove /bass/i test → bass charts get 6 strings; 5th/6th string // material slots are undefined and T.WebGLRenderer calls throw. assert.strictEqual( - fns().resolveStringCount({ songInfo: { arrangement: 'Bass' } }), + fns().resolveStringCount({ songInfo: { arrangement: 'Bass' } }, 6), 4, 'arrangement containing "Bass" must fall back to 4 strings'); }); test('resolveStringCount: defaults to NSTR=6 when bundle has no string info', () => { - assert.strictEqual(fns().resolveStringCount({}), 6); + assert.strictEqual(fns().resolveStringCount({}, 6), 6); }); // ── _NOTE_NAMES_SHARP ───────────────────────────────────────────────────────── @@ -155,11 +164,13 @@ test('_midiToPitchLabel: MIDI 60 = C4, MIDI 69 = A4', () => { // ── _openStringPitchLabelsForTuning ────────────────────────────────────────── test('_openStringPitchLabelsForTuning: standard guitar in E returns correct labels', () => { - // Smoke: 6 zero-offset strings with guitar6 MIDI base. + // Smoke: 6 zero-offset strings with guitar6 MIDI base. maxStrings=6 passed explicitly + // (mirrors the delegator in screen.js which supplies MAX_RENDER_STRINGS). const labels = fns()._openStringPitchLabelsForTuning( { tuning: [0, 0, 0, 0, 0, 0], capo: 0, stringCount: 6 }, { arrangement: 'Lead' }, 6, + 6, // maxStrings ); assert.deepStrictEqual(labels, ['E2', 'A2', 'D3', 'G3', 'B3', 'E4'], 'standard guitar open-string labels must be E2-A2-D3-G3-B3-E4'); @@ -188,12 +199,26 @@ test('screen.js imports all Cut 3 utils from src/utils.js', () => { assert.match(src, /import\s+\{[^}]*_ssActive[^}]*\}\s+from\s+['"]\.\/src\/utils\.js['"]/, 'screen.js must import _ssActive (and other utils) from ./src/utils.js'); - assert.match(src, /resolveStringCount/, - 'resolveStringCount must appear in the utils.js import line'); + assert.match(src, /_resolveStringCountBase/, + 'resolveStringCount must be imported with an alias so the delegator can shadow it'); assert.match(src, /_h3dHexToInt/, '_h3dHexToInt must appear in the utils.js import line'); }); +test('screen.js delegator passes MAX_RENDER_STRINGS to resolveStringCount', () => { + // Mutation: delegator omits MAX_RENDER_STRINGS → resolveStringCount called with + // maxStrings=undefined; Math.min(sc, undefined)=NaN; string count is always NaN. + const src = fs.readFileSync(SCREEN_JS, 'utf8'); + assert.match(src, /_resolveStringCountBase\s*\(.*MAX_RENDER_STRINGS/, + 'delegator must supply MAX_RENDER_STRINGS so palette growth is auto-respected'); +}); + +test('screen.js delegator passes MAX_RENDER_STRINGS to _openStringPitchLabelsForTuning', () => { + const src = fs.readFileSync(SCREEN_JS, 'utf8'); + assert.match(src, /_openStringPitchLabelsForTuningBase\s*\(.*MAX_RENDER_STRINGS/, + 'delegator must forward MAX_RENDER_STRINGS as the maxStrings argument'); +}); + test('screen.js IIFE no longer declares the moved symbols', () => { // Strip import lines first so we only scan the IIFE body. const src = fs.readFileSync(SCREEN_JS, 'utf8');