From 0d4d8229c7f2b1d3a92fa71b1a6206931b24cd10 Mon Sep 17 00:00:00 2001 From: Kris Anderson Date: Thu, 9 Jul 2026 16:15:29 -0400 Subject: [PATCH] =?UTF-8?q?fix(highways):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20bg-key=20alias,=20drum=20cam=20guard,=20resolver=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the per-panel camera work: - highway_3d: _bgPanelKey() resolved splitscreen via window.feedBackSplitscreen only, while _freeCamFor() uses the feedBackSplitscreen||slopsmithSplitscreen alias it claims to "mirror". If the rename lands, per-panel background settings would silently stop being per-panel while the camera stayed per-panel. Resolve the alias the same way in _bgPanelKey. - drum_highway_3d: applyCamera()'s "before first positionCamera()" guard tested `_camBaseH == null`, but _camBaseH/_camBaseD were initialized to 0, so the guard never fired (and could apply a base-0 pose for a frame). Initialize to null. - keys + drum: the PR claimed the Camera Director resolver was unit-checked, but nothing exercised it. Extract the resolver into pure, exported helpers (_resolveFreeCam + _ssApi), delegate the per-instance _freeCamFor to them, and add tests/camera_bridge.test.js covering per-panel select, global fallback, null-when-absent, throw-safety, and the slopsmith-alias resolution. Drum 15→21, keys 50→56, all pass; behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Kris Anderson --- plugins/drum_highway_3d/screen.js | 48 ++++++++----- .../tests/camera_bridge.test.js | 68 +++++++++++++++++++ plugins/highway_3d/screen.js | 4 +- plugins/keys_highway_3d/screen.js | 46 ++++++++----- .../tests/camera_bridge.test.js | 68 +++++++++++++++++++ 5 files changed, 200 insertions(+), 34 deletions(-) create mode 100644 plugins/drum_highway_3d/tests/camera_bridge.test.js create mode 100644 plugins/keys_highway_3d/tests/camera_bridge.test.js diff --git a/plugins/drum_highway_3d/screen.js b/plugins/drum_highway_3d/screen.js index 8a694c9..d2daac0 100644 --- a/plugins/drum_highway_3d/screen.js +++ b/plugins/drum_highway_3d/screen.js @@ -1361,6 +1361,29 @@ } catch (_) { /* dispatch unavailable — persisted value applies next init */ } }; + /* ====================================================================== + * Camera Director bridge resolver (pure — exported via createFactory.__test) + * ====================================================================== */ + + // Resolve the active splitscreen API, defensive on the global-name rename in + // flight (feedBackSplitscreen is canonical; slopsmithSplitscreen is the legacy + // alias). Returns null when splitscreen isn't present. + function _ssApi() { return window.feedBackSplitscreen || window.slopsmithSplitscreen || null; } + + // Given the splitscreen API, THIS window's per-panel camera map, and the + // global camera, return this panel's camera under splitscreen, else the + // global, else null (Camera Director absent → stock framing). Throw-safe on + // panelIndexFor so a misbehaving splitscreen build can't break framing. + function _resolveFreeCam(canvas, ss, panelsMap, globalCam) { + if (panelsMap && ss && typeof ss.panelIndexFor === 'function') { + try { + const i = ss.panelIndexFor(canvas); + if (i != null && panelsMap[i]) return panelsMap[i]; + } catch (e) { /* ignore */ } + } + return globalCam || null; + } + /* ====================================================================== * Renderer factory * ====================================================================== */ @@ -1414,7 +1437,7 @@ let _sparkPts = null, _sparkPos = null, _sparkCol = null, _sparkVel = null, _sparkLife = null; let _fxLastWall = 0; // wall clock for FX integration (sparks, pulse decay) let _kickPulse = 0; // kick-hit camera-dip + floor-wash envelope - let _camBaseH = 0, _camBaseD = 0; // positionCamera's unpulsed pose + let _camBaseH = null, _camBaseD = null; // positionCamera's unpulsed pose (null until it first runs; applyCamera's guard depends on this) let _gaussTex = null; // shared soft-falloff texture for flash quads let _laneFlashQuads = []; // pooled additive quad per hand lane (z=0) let _kickFlashQuad = null; // full-width flash quad for the kick bar @@ -2651,23 +2674,12 @@ cam.lookAt(0, 0, -AHEAD * TS * 0.45); } - // Camera Director bridge resolver. Prefers THIS panel's per-panel camera - // under splitscreen (window.__h3dCamCtlPanels[panelIndex]) and falls back - // to the single global (window.__h3dCamCtl); null when Camera Director is - // absent → 100% stock framing. Defensive on the splitscreen global name - // (rename in flight: feedBackSplitscreen vs slopsmithSplitscreen). + // Camera Director bridge for THIS panel — delegates to the pure, unit- + // tested _resolveFreeCam / _ssApi (see the resolver block above the + // factory). Reads the live globals: per-panel map __h3dCamCtlPanels → + // this panel's camera, else the global __h3dCamCtl, else null (stock). function _freeCamFor(canvas) { - const map = window.__h3dCamCtlPanels; - if (map) { - const ss = window.feedBackSplitscreen || window.slopsmithSplitscreen; - if (ss && typeof ss.panelIndexFor === 'function') { - try { - const i = ss.panelIndexFor(canvas); - if (i != null && map[i]) return map[i]; - } catch (e) { /* ignore */ } - } - } - return window.__h3dCamCtl || null; + return _resolveFreeCam(canvas, _ssApi(), window.__h3dCamCtlPanels, window.__h3dCamCtl); } // Per-frame camera write: static base pose (positionCamera) + kick-pulse Y @@ -3617,6 +3629,8 @@ // vm-loaded with no DOM/WebGL; everything here must stay side-effect // free to call). window.slopsmithViz_drum_highway_3d.__test = { + _resolveFreeCam, + _ssApi, _variantForHit, _classifyTiming, readFxSettings, diff --git a/plugins/drum_highway_3d/tests/camera_bridge.test.js b/plugins/drum_highway_3d/tests/camera_bridge.test.js new file mode 100644 index 0000000..5921849 --- /dev/null +++ b/plugins/drum_highway_3d/tests/camera_bridge.test.js @@ -0,0 +1,68 @@ +// Camera Director bridge resolver tests: per-panel select, global fallback, +// null-when-absent, throw-safety, and the splitscreen global-name alias. Loads +// screen.js in a bare vm window and exercises the __test exports (no DOM/WebGL). +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +function load() { + const window = { + console, + location: { protocol: 'http:', host: 'localhost' }, + slopsmith: {}, + }; + window.window = window; + window.globalThis = window; + const context = vm.createContext(window); + const src = fs.readFileSync(path.join(__dirname, '..', 'screen.js'), 'utf8'); + vm.runInContext(src, context, { filename: 'screen.js' }); + return { window, __test: window.slopsmithViz_drum_highway_3d.__test }; +} + +test('_resolveFreeCam: per-panel camera under splitscreen', () => { + const { __test } = load(); + const c0 = {}, c1 = {}; + const ss = { panelIndexFor: (c) => (c === c0 ? 0 : 1) }; + const map = { 0: { id: 'p0' }, 1: { id: 'p1' } }; + assert.equal(__test._resolveFreeCam(c0, ss, map, { id: 'g' }).id, 'p0'); + assert.equal(__test._resolveFreeCam(c1, ss, map, { id: 'g' }).id, 'p1'); +}); + +test('_resolveFreeCam: falls back to global when there is no panel map', () => { + const { __test } = load(); + const g = { id: 'global' }; + assert.equal(__test._resolveFreeCam({}, { panelIndexFor: () => 0 }, null, g), g); +}); + +test('_resolveFreeCam: falls back to global when the panel has no map entry', () => { + const { __test } = load(); + const g = { id: 'global' }; + const ss = { panelIndexFor: () => 3 }; // index 3 absent from map + assert.equal(__test._resolveFreeCam({}, ss, { 0: {} }, g), g); +}); + +test('_resolveFreeCam: null when Camera Director is absent (no global)', () => { + const { __test } = load(); + assert.equal(__test._resolveFreeCam({}, null, null, null), null); + assert.equal(__test._resolveFreeCam({}, { panelIndexFor: () => 0 }, {}, undefined), null); +}); + +test('_resolveFreeCam: throw-safe on panelIndexFor → falls back to global', () => { + const { __test } = load(); + const g = { id: 'global' }; + const ss = { panelIndexFor: () => { throw new Error('boom'); } }; + assert.equal(__test._resolveFreeCam({}, ss, { 0: {} }, g), g); +}); + +test('_ssApi: null when neither global set; slopsmith alias; feedBack canonical wins', () => { + const { window, __test } = load(); + assert.equal(__test._ssApi(), null); + const legacy = { panelIndexFor: () => 0 }; + window.slopsmithSplitscreen = legacy; + assert.equal(__test._ssApi(), legacy); // legacy alias picked up + const current = { panelIndexFor: () => 1 }; + window.feedBackSplitscreen = current; + assert.equal(__test._ssApi(), current); // canonical name takes precedence +}); diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index 65e6fef..f5351a0 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -2596,7 +2596,9 @@ const FRET_NUMBER_GHOST_SCOPE_IDS = ['chords', 'all']; function _bgPanelKey(canvas) { - const ss = window.feedBackSplitscreen; + // Defensive on the splitscreen global name (rename in flight) so per-panel + // background settings keep resolving the same panel as _freeCamFor(). + const ss = window.feedBackSplitscreen || window.slopsmithSplitscreen; const idx = (ss && typeof ss.panelIndexFor === 'function') ? ss.panelIndexFor(canvas) : null; return (idx == null) ? 'main' : 'panel' + idx; } diff --git a/plugins/keys_highway_3d/screen.js b/plugins/keys_highway_3d/screen.js index 0df538d..238bc8f 100644 --- a/plugins/keys_highway_3d/screen.js +++ b/plugins/keys_highway_3d/screen.js @@ -1600,6 +1600,29 @@ _aiRegisteredCount = 0; } + /* ====================================================================== + * Camera Director bridge resolver (pure — exported via createFactory.__test) + * ====================================================================== */ + + // Resolve the active splitscreen API, defensive on the global-name rename in + // flight (feedBackSplitscreen is canonical; slopsmithSplitscreen is the legacy + // alias). Returns null when splitscreen isn't present. + function _ssApi() { return window.feedBackSplitscreen || window.slopsmithSplitscreen || null; } + + // Given the splitscreen API, THIS window's per-panel camera map, and the + // global camera, return this panel's camera under splitscreen, else the + // global, else null (Camera Director absent → 100% stock framing). Throw-safe + // on panelIndexFor so a misbehaving splitscreen build can't break framing. + function _resolveFreeCam(canvas, ss, panelsMap, globalCam) { + if (panelsMap && ss && typeof ss.panelIndexFor === 'function') { + try { + const i = ss.panelIndexFor(canvas); + if (i != null && panelsMap[i]) return panelsMap[i]; + } catch (e) { /* ignore */ } + } + return globalCam || null; + } + /* ====================================================================== * Renderer factory * ====================================================================== */ @@ -1850,23 +1873,12 @@ return _rigOut; } - // Camera Director bridge resolver. Prefers THIS panel's per-panel camera - // under splitscreen (window.__h3dCamCtlPanels[panelIndex]) and falls back - // to the single global (window.__h3dCamCtl); null when Camera Director is - // absent → 100% stock framing. Defensive on the splitscreen global name - // (rename in flight: feedBackSplitscreen vs slopsmithSplitscreen). + // Camera Director bridge for THIS panel — delegates to the pure, unit- + // tested _resolveFreeCam / _ssApi (see the resolver block above the + // factory). Reads the live globals: per-panel map __h3dCamCtlPanels → + // this panel's camera, else the global __h3dCamCtl, else null (stock). function _freeCamFor(canvas) { - const map = window.__h3dCamCtlPanels; - if (map) { - const ss = window.feedBackSplitscreen || window.slopsmithSplitscreen; - if (ss && typeof ss.panelIndexFor === 'function') { - try { - const i = ss.panelIndexFor(canvas); - if (i != null && map[i]) return map[i]; - } catch (e) { /* ignore */ } - } - } - return window.__h3dCamCtl || null; + return _resolveFreeCam(canvas, _ssApi(), window.__h3dCamCtlPanels, window.__h3dCamCtl); } // Per-key approach glow: a key lights in its pitch-class color ONLY while a // note is heading for it, ramping up the closer that note gets to the hit-line. @@ -4005,6 +4017,8 @@ }; // Pure data-layer + scoring hooks for headless tests. window.slopsmithViz_keys_highway_3d.__test = { + _resolveFreeCam, + _ssApi, beatDurSec, flattenNotation, keyRange, diff --git a/plugins/keys_highway_3d/tests/camera_bridge.test.js b/plugins/keys_highway_3d/tests/camera_bridge.test.js new file mode 100644 index 0000000..efaa0c9 --- /dev/null +++ b/plugins/keys_highway_3d/tests/camera_bridge.test.js @@ -0,0 +1,68 @@ +// Camera Director bridge resolver tests: per-panel select, global fallback, +// null-when-absent, throw-safety, and the splitscreen global-name alias. Loads +// screen.js in a bare vm window and exercises the __test exports (no DOM/WebGL). +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +function load() { + const window = { + console, + location: { protocol: 'http:', host: 'localhost' }, + slopsmith: {}, + }; + window.window = window; + window.globalThis = window; + const context = vm.createContext(window); + const src = fs.readFileSync(path.join(__dirname, '..', 'screen.js'), 'utf8'); + vm.runInContext(src, context, { filename: 'screen.js' }); + return { window, __test: window.slopsmithViz_keys_highway_3d.__test }; +} + +test('_resolveFreeCam: per-panel camera under splitscreen', () => { + const { __test } = load(); + const c0 = {}, c1 = {}; + const ss = { panelIndexFor: (c) => (c === c0 ? 0 : 1) }; + const map = { 0: { id: 'p0' }, 1: { id: 'p1' } }; + assert.equal(__test._resolveFreeCam(c0, ss, map, { id: 'g' }).id, 'p0'); + assert.equal(__test._resolveFreeCam(c1, ss, map, { id: 'g' }).id, 'p1'); +}); + +test('_resolveFreeCam: falls back to global when there is no panel map', () => { + const { __test } = load(); + const g = { id: 'global' }; + assert.equal(__test._resolveFreeCam({}, { panelIndexFor: () => 0 }, null, g), g); +}); + +test('_resolveFreeCam: falls back to global when the panel has no map entry', () => { + const { __test } = load(); + const g = { id: 'global' }; + const ss = { panelIndexFor: () => 3 }; // index 3 absent from map + assert.equal(__test._resolveFreeCam({}, ss, { 0: {} }, g), g); +}); + +test('_resolveFreeCam: null when Camera Director is absent (no global)', () => { + const { __test } = load(); + assert.equal(__test._resolveFreeCam({}, null, null, null), null); + assert.equal(__test._resolveFreeCam({}, { panelIndexFor: () => 0 }, {}, undefined), null); +}); + +test('_resolveFreeCam: throw-safe on panelIndexFor → falls back to global', () => { + const { __test } = load(); + const g = { id: 'global' }; + const ss = { panelIndexFor: () => { throw new Error('boom'); } }; + assert.equal(__test._resolveFreeCam({}, ss, { 0: {} }, g), g); +}); + +test('_ssApi: null when neither global set; slopsmith alias; feedBack canonical wins', () => { + const { window, __test } = load(); + assert.equal(__test._ssApi(), null); + const legacy = { panelIndexFor: () => 0 }; + window.slopsmithSplitscreen = legacy; + assert.equal(__test._ssApi(), legacy); // legacy alias picked up + const current = { panelIndexFor: () => 1 }; + window.feedBackSplitscreen = current; + assert.equal(__test._ssApi(), current); // canonical name takes precedence +});