diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index 0dce045..7b6de16 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -2703,6 +2703,14 @@ // that CSS-box drift and re-frame, instead of the user having to // un/re-maximize the window. let _appliedW = 0, _appliedH = 0; + // True once applySize() has pinned the .h3d-wrap overlay to the + // highway canvas's offset box. Stays false while the canvas has no + // layout yet (init() can run before #highway has a real box, where + // applySize falls back to the parent-panel size and only sets the + // wrap height). The rAF loop re-pins once the canvas lays out even + // when the logical render size is unchanged — otherwise the overlay + // would stay at top:0;left:0;right:0 and expose a strip of #highway. + let _wrapPinned = false; let mBeatM = null, mBeatQ = null; let txtCache = {}; // Cloned sprite materials cached on individual sprite instances @@ -12941,7 +12949,55 @@ const baseDPR = _ssActive() ? Math.min(devicePixelRatio, 1.25) : Math.min(devicePixelRatio, 2); ren.setPixelRatio(_renderScale * baseDPR); ren.setSize(w, h); - wrap.style.height = h + 'px'; + // Pin the overlay to #highway's exact box so it fully covers the + // canvas. The wrap is anchored to top:0/left:0/right:0 of its + // offset parent, which only lines up with #highway when the + // canvas sits at the parent's origin. The v3 player can place + // chrome above the canvas, shifting the wrap up so its lower edge + // falls short of #highway — leaving a strip of the canvas exposed + // (the reported gap, where the previous renderer's frame showed + // through). The wrap is a sibling of highwayCanvas, so they share + // an offset parent; tracking the canvas's box keeps the overlay + // flush in single-player and splitscreen alike. + // + // Derive the box from the SAME getBoundingClientRect measurements + // that drive ren.setSize(w, h) — NOT integer offsetTop/Width — so + // the overlay matches the renderer exactly. Under browser zoom or + // fractional flex layouts the canvas lands on sub-pixel bounds; + // offsetWidth/Top round to whole pixels and would leave the wrap up + // to 1px short of (or shifted from) the canvas, reopening the + // exposed edge strip. Position is taken relative to the containing + // block's padding edge (clientTop/Left strip the parent's border), + // which is what `top`/`left` resolve against for the absolutely + // positioned wrap. Guarded on a laid-out canvas (offsetWidth/Height + // > 0); otherwise fall back to the static top:0/left:0/right:0. + if (highwayCanvas && highwayCanvas.offsetWidth > 0 && highwayCanvas.offsetHeight > 0) { + const _pinParent = wrap.offsetParent || highwayCanvas.parentNode; + const _cr = highwayCanvas.getBoundingClientRect(); + const _pr = _pinParent ? _pinParent.getBoundingClientRect() : { top: 0, left: 0 }; + const _pbTop = _pinParent ? _pinParent.clientTop : 0; + const _pbLeft = _pinParent ? _pinParent.clientLeft : 0; + wrap.style.top = (_cr.top - _pr.top - _pbTop) + 'px'; + wrap.style.left = (_cr.left - _pr.left - _pbLeft) + 'px'; + wrap.style.right = 'auto'; + wrap.style.width = _cr.width + 'px'; + wrap.style.height = _cr.height + 'px'; + _wrapPinned = true; + } else { + // Canvas not laid out (e.g. init ran before #highway had a real + // box, or a panel hide/show where canvasSize() falls back to the + // parent panel). Reset to the static anchor — if we had pinned + // before, the old top/left/right:auto/width would otherwise stay + // and the wrap would reappear at a stale horizontal position on + // the next show. Leave _wrapPinned false so the rAF loop re-pins + // once the canvas materializes again. + wrap.style.top = '0'; + wrap.style.left = '0'; + wrap.style.right = '0'; + wrap.style.width = 'auto'; + wrap.style.height = h + 'px'; + _wrapPinned = false; + } if (lyricsCanvas) { lyricsCanvas.width = w; lyricsCanvas.height = h; } _diagRenderCache.clear(); cam.aspect = w / h; @@ -13320,6 +13376,17 @@ } else if (box.w > 0 && box.h > 0 && (Math.abs(box.w - _appliedW) > 1 || Math.abs(box.h - _appliedH) > 1)) { applySize(box.w, box.h); + } else if (!_wrapPinned && box.w > 0 && box.h > 0 && + highwayCanvas.offsetWidth > 0 && highwayCanvas.offsetHeight > 0) { + // 3. The overlay pin couldn't be applied at init because + // #highway had no layout yet (offsetWidth/Height === 0), + // so applySize() only set the wrap height. The canvas has + // now laid out but to the same logical size, so neither + // drift branch above fires — re-run applySize to pin the + // wrap to the canvas box now that its offsets are real. + // Otherwise the overlay stays at top:0;left:0;right:0 and + // a strip of #highway is exposed on first load / split. + applySize(box.w, box.h); } } update(bundle); @@ -13497,6 +13564,7 @@ _destroyed = true; _isReady = false; _diagChord = null; _diagPrev = null; _diagLastKey = null; _diagRenderCache.clear(); _lastHwW = 0; _lastHwH = 0; _appliedW = 0; _appliedH = 0; + _wrapPinned = false; _unsubscribeFocus(); teardown(); highwayCanvas = null; }, diff --git a/static/highway.js b/static/highway.js index edadca4..7589d87 100644 --- a/static/highway.js +++ b/static/highway.js @@ -973,6 +973,23 @@ function createHighway() { return '2d'; } + // Logical identity of a renderer for swap detection: its viz id, not + // its object reference. app.js stamps `pluginId`/`source` (the viz + // picker id) onto every renderer it installs via _tagVizRenderer, and + // setViz()/_autoMatchViz() build a FRESH renderer object with factory() + // on every (re)apply — even when the selected viz did not change (e.g. + // Auto re-resolving to the same viz across consecutive songs). Keying + // the canvas-replacement decision on object identity would therefore + // treat those benign re-installs as a swap and needlessly clone + // #highway, dropping listeners/expando state attached to the element. + // The default renderer has no id, so it keys on its own singleton; an + // untagged custom renderer falls back to its object reference (the safe + // "treat as a swap" answer). + function _rendererVizKey(r) { + if (r === _defaultRenderer) return _defaultRenderer; + return (r && (r.pluginId || r.source)) || r; + } + // Replace the underlying element with a fresh one because // the next renderer needs a different context type than the current // canvas is locked to. Preserves the DOM position, id, classes, @@ -1038,6 +1055,11 @@ function createHighway() { } function _setRenderer(r) { + // Capture the outgoing renderer before _destroyCurrentIfInited / + // the `_renderer = next` assignment below overwrite it — the + // canvas-replacement decision needs to know whether we're + // actually swapping to a *different* renderer instance. + const prev = _renderer; _destroyCurrentIfInited(); // null/undefined reverts to default. Anything else must provide // at minimum a draw(bundle) function — without it the rAF loop @@ -1070,7 +1092,29 @@ function createHighway() { // destroyed by _destroyCurrentIfInited above, so it's safe to // detach the element from the DOM here. const nextType = _resolveRendererContextType(next); - if (nextType !== _currentCanvasContextType) { + // Replace the underlying when (a) the new renderer needs + // a different context type than the one currently bound, OR (b) + // we're swapping to a *different* visualization while keeping the + // same context type. Case (b) prevents a stale frame from the + // previous renderer bleeding through: renderers that draw into a + // sibling overlay (e.g. 3D Highway's `.h3d-wrap`) never paint + // #highway, so whatever the *previous* renderer left on the shared + // canvas would otherwise stay visible in any area the overlay does + // not cover. The reported symptom was the 3D drum highway (which + // renders directly onto #highway) showing through the gap below + // the 3D guitar highway's overlay after switching between them — + // both are webgl2, so the type-change check alone never fired. A + // fresh canvas starts blank/transparent, so the incoming renderer + // always begins over a clean surface. The swap test keys on the + // viz id (_rendererVizKey), NOT object identity: setViz/_autoMatchViz + // build a fresh renderer object on every apply, so an object-identity + // check would clone #highway on every benign same-viz re-install + // (e.g. Auto re-resolving to the same viz across songs) and drop the + // element's listeners/expando state. Skipped when re-installing the + // same viz, and on the very first install (prev === null) where + // there is no prior frame to clear. + const _vizChanged = prev && _rendererVizKey(next) !== _rendererVizKey(prev); + if (nextType !== _currentCanvasContextType || _vizChanged) { _replaceCanvas(nextType); } const bundle = _makeBundle(); diff --git a/tests/js/highway_3d_overlay_full_cover.test.js b/tests/js/highway_3d_overlay_full_cover.test.js new file mode 100644 index 0000000..bdbbc4b --- /dev/null +++ b/tests/js/highway_3d_overlay_full_cover.test.js @@ -0,0 +1,94 @@ +// Source-level guard for the 3D Highway overlay fully covering #highway. +// +// The `.h3d-wrap` overlay is anchored to top:0/left:0/right:0 of its offset +// parent, which only lines up with #highway when the canvas sits at the +// parent's origin. The v3 player can place chrome above the canvas, shifting +// the wrap up so its lower edge falls short of #highway and exposes a strip +// of the canvas (the reported gap). applySize() must pin the wrap to the +// canvas's actual offset box so it stays flush. createHighway's WebGL +// lifecycle is too heavy for a vm sandbox, so this locks in the wiring. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const screenJs = path.join(__dirname, '..', '..', 'plugins', 'highway_3d', 'screen.js'); + +function extractBlock(src, signature) { + const start = src.indexOf(signature); + assert.ok(start !== -1, `signature '${signature}' not found`); + const openBrace = src.indexOf('{', start); + let depth = 1; + let i = openBrace + 1; + while (i < src.length && depth > 0) { + const ch = src[i]; + if (ch === '{') depth++; + else if (ch === '}') depth--; + i++; + } + assert.ok(depth === 0, `unbalanced braces after '${signature}'`); + return src.slice(start, i); +} + +test('applySize pins the .h3d-wrap overlay to the highway canvas rect box', () => { + const src = fs.readFileSync(screenJs, 'utf8'); + const fn = extractBlock(src, 'function applySize(w, h)'); + // Guarded on a laid-out canvas so we never pin to a zero box. + assert.match( + fn, + /highwayCanvas\s*&&\s*highwayCanvas\.offsetWidth\s*>\s*0\s*&&\s*highwayCanvas\.offsetHeight\s*>\s*0/, + 'must guard the pin on a laid-out canvas (offsetWidth/Height > 0)', + ); + // Size/position must come from getBoundingClientRect (fractional, matches + // ren.setSize), NOT integer offset* props which round and reopen the strip. + assert.match(fn, /highwayCanvas\.getBoundingClientRect\(\)/, 'must measure the canvas via getBoundingClientRect'); + assert.doesNotMatch(fn, /wrap\.style\.width\s*=\s*highwayCanvas\.offsetWidth/, 'must NOT size to integer offsetWidth'); + assert.doesNotMatch(fn, /wrap\.style\.height\s*=\s*highwayCanvas\.offsetHeight/, 'must NOT size to integer offsetHeight'); + // Width/height set from the rect; position is parent-relative (padding edge). + assert.match(fn, /wrap\.style\.width\s*=\s*_cr\.width/, 'must size width to the canvas rect width'); + assert.match(fn, /wrap\.style\.height\s*=\s*_cr\.height/, 'must size height to the canvas rect height'); + assert.match(fn, /wrap\.style\.top\s*=\s*\(\s*_cr\.top\s*-\s*_pr\.top\s*-\s*_pbTop\s*\)/, 'top must be canvas rect relative to the containing block padding edge'); + assert.match(fn, /wrap\.style\.left\s*=\s*\(\s*_cr\.left\s*-\s*_pr\.left\s*-\s*_pbLeft\s*\)/, 'left must be canvas rect relative to the containing block padding edge'); + assert.match(fn, /clientTop/, 'must strip the parent border via clientTop'); + // right:0 must be released so the explicit width takes effect. + assert.match(fn, /wrap\.style\.right\s*=\s*['"]auto['"]/, "must release right:0 (set 'auto') when pinning width"); +}); + +test('applySize fallback resets the static anchor and the computed height', () => { + const src = fs.readFileSync(screenJs, 'utf8'); + const fn = extractBlock(src, 'function applySize(w, h)'); + // The not-laid-out fallback must clear any stale pin styles (a prior pin + // leaves top/left/right:auto/width set) back to the original + // top:0;left:0;right:0;width:auto anchor, or the wrap reappears at a + // stale horizontal position after a panel hide/show. + const fallback = fn.slice(fn.indexOf('} else {')); + assert.match(fallback, /wrap\.style\.top\s*=\s*['"]0['"]/, 'fallback must reset top:0'); + assert.match(fallback, /wrap\.style\.left\s*=\s*['"]0['"]/, 'fallback must reset left:0'); + assert.match(fallback, /wrap\.style\.right\s*=\s*['"]0['"]/, 'fallback must reset right:0'); + assert.match(fallback, /wrap\.style\.width\s*=\s*['"]auto['"]/, 'fallback must reset width:auto'); + assert.match(fallback, /wrap\.style\.height\s*=\s*h\s*\+\s*['"]px['"]/, 'fallback must keep the computed height'); +}); + +test('applySize records whether the overlay pin was applied (_wrapPinned)', () => { + const src = fs.readFileSync(screenJs, 'utf8'); + const fn = extractBlock(src, 'function applySize(w, h)'); + // Pin path sets the flag true; the not-laid-out fallback sets it false + // so the rAF loop knows the pin is still pending. + assert.match(fn, /_wrapPinned\s*=\s*true/, 'pin path must set _wrapPinned = true'); + assert.match(fn, /_wrapPinned\s*=\s*false/, 'fallback path must set _wrapPinned = false'); +}); + +test('the rAF loop re-pins the overlay once the canvas lays out (Codex P1)', () => { + // When init() pins via the parent-panel fallback (offset box still 0) and + // the canvas later lays out to the SAME logical size, neither size-drift + // branch fires. A dedicated branch must re-run applySize so the overlay + // gets pinned to the now-real canvas box instead of leaving the exposed + // strip the fix was meant to close. + const src = fs.readFileSync(screenJs, 'utf8'); + assert.match( + src, + /else if\s*\(\s*!_wrapPinned\s*&&\s*box\.w\s*>\s*0\s*&&\s*box\.h\s*>\s*0\s*&&\s*highwayCanvas\.offsetWidth\s*>\s*0\s*&&\s*highwayCanvas\.offsetHeight\s*>\s*0\s*\)\s*\{\s*[\s\S]*?applySize\(\s*box\.w\s*,\s*box\.h\s*\)\s*;/, + 'must re-pin via applySize when !_wrapPinned and the canvas has laid out', + ); +}); diff --git a/tests/js/highway_renderer_swap_canvas_reset.test.js b/tests/js/highway_renderer_swap_canvas_reset.test.js new file mode 100644 index 0000000..18d4481 --- /dev/null +++ b/tests/js/highway_renderer_swap_canvas_reset.test.js @@ -0,0 +1,91 @@ +// Source-level guard for the renderer-swap canvas reset. +// +// Both 3D visualizations are webgl2 renderers, but they paint to +// different surfaces: the 3D *drum* highway renders directly onto the +// shared #highway canvas, while the 3D *guitar* highway renders into its +// own `.h3d-wrap` sibling overlay and never touches #highway. Switching +// drum -> guitar is webgl2 -> webgl2, so the context-type check alone +// never replaced the canvas — the last drum frame stayed painted on +// #highway and bled through the gap the guitar overlay does not cover. +// +// The fix replaces the underlying on ANY swap to a different +// renderer instance (not just on a context-type change) so the incoming +// renderer always starts over a blank surface. These checks lock in the +// wiring; the createHighway closure owns a WebGL lifecycle too heavy to +// reproduce in a vm sandbox. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const highwayJs = path.join(__dirname, '..', '..', 'static', 'highway.js'); + +function extractBlock(src, signature) { + const start = src.indexOf(signature); + assert.ok(start !== -1, `signature '${signature}' not found`); + const openBrace = src.indexOf('{', start); + assert.ok(openBrace !== -1, `opening brace after '${signature}' not found`); + let depth = 1; + let i = openBrace + 1; + while (i < src.length && depth > 0) { + const ch = src[i]; + if (ch === '{') depth++; + else if (ch === '}') depth--; + i++; + } + assert.ok(depth === 0, `unbalanced braces after '${signature}'`); + return src.slice(start, i); +} + +test('_setRenderer captures the outgoing renderer before overwriting it', () => { + const src = fs.readFileSync(highwayJs, 'utf8'); + const fn = extractBlock(src, 'function _setRenderer(r)'); + // prev must be captured BEFORE _destroyCurrentIfInited and the + // `_renderer = next` assignment, otherwise the swap detection below + // would always compare next against itself. + const prevIdx = fn.search(/const\s+prev\s*=\s*_renderer/); + const destroyIdx = fn.search(/_destroyCurrentIfInited\(\)/); + const assignIdx = fn.search(/^\s*_renderer\s*=\s*next\s*;/m); + assert.ok(prevIdx !== -1, 'must capture `const prev = _renderer`'); + assert.ok(destroyIdx !== -1, 'must call _destroyCurrentIfInited'); + assert.ok(assignIdx !== -1, 'must assign `_renderer = next`'); + assert.ok(prevIdx < destroyIdx, 'prev must be captured before _destroyCurrentIfInited'); + assert.ok(prevIdx < assignIdx, 'prev must be captured before `_renderer = next`'); +}); + +test('_setRenderer replaces the canvas on a context-type change OR a viz change', () => { + const src = fs.readFileSync(highwayJs, 'utf8'); + const fn = extractBlock(src, 'function _setRenderer(r)'); + // The replace guard must fire on EITHER a context-type change OR a + // swap to a different visualization. A regression that drops the + // viz-change clause would let a stale frame bleed through. + assert.match( + fn, + /const\s+_vizChanged\s*=\s*prev\s*&&\s*_rendererVizKey\(next\)\s*!==\s*_rendererVizKey\(prev\)/, + '_vizChanged must compare _rendererVizKey(next) vs _rendererVizKey(prev), guarded by prev', + ); + assert.match( + fn, + /if\s*\(\s*nextType\s*!==\s*_currentCanvasContextType\s*\|\|\s*_vizChanged\s*\)\s*\{\s*_replaceCanvas\(nextType\)/, + 'replace guard must be `nextType !== _currentCanvasContextType || _vizChanged`', + ); +}); + +test('_rendererVizKey keys on the viz id, not object identity (avoids churn on same-viz re-install)', () => { + const src = fs.readFileSync(highwayJs, 'utf8'); + const fn = extractBlock(src, 'function _rendererVizKey(r)'); + // Default renderer keys on its singleton; custom renderers key on the + // viz picker id (pluginId/source) stamped by app.js's _tagVizRenderer, + // falling back to the object reference only when untagged. + assert.match(fn, /r\s*===\s*_defaultRenderer/, 'default renderer must key on its own singleton'); + assert.match(fn, /r\.pluginId\s*\|\|\s*r\.source/, 'custom renderers must key on pluginId/source (the viz id)'); +}); + +test('_setRenderer skips the extra replace on first install (prev === null)', () => { + const src = fs.readFileSync(highwayJs, 'utf8'); + const fn = extractBlock(src, 'function _setRenderer(r)'); + // The `prev &&` guard avoids a needless swap on the very first install + // (prev === null) where there is no prior frame to clear. + assert.match(fn, /_vizChanged\s*=\s*prev\s*&&/, 'must short-circuit _vizChanged when prev is null'); +});