mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-10-02 11:11:47 +00:00
fix(h3d): thread maxStrings param through resolveStringCount (Toby r1)
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014uZ169yfoFYArXz962g7KW
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
c7f7c88c62
commit
6050b6262b
@@ -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;
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user