From a858617d711e4e02b582dbdeaf88d9da47fdaa9a Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Sun, 21 Jun 2026 00:46:36 +0200 Subject: [PATCH] fix(bend): GP8 short-bend curve loss + 2D curve timing + 3D bnv gating (#535) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge Codex review of the bend-curve PRs (#531/#532) surfaced edge cases: - GP8 (#531 P2): bnv timing used rn.sustain, which is zeroed for notes <= 0.2s, so short GP8 bends kept the scalar bn but lost bt/bnv. Use the beat duration `dur` (matching the GP5 path) so the curve survives. - 2D highway (#532 P2): bnvNormalizedPoints mapped x over the curve's own t-range [first,last] instead of the note span, so curves not starting at 0 / ending at sus were time-distorted. Now maps over [0, sus] (clamped), with a curve-span fallback when sus<=0 (existing no-sus callers unaffected). - 3D highway (#532 P3): the sustain ribbon + bend chevron were gated on bn>0, so a note carrying an authoritative bnv with bn==0 drew no ribbon/marker. Both now also fire on bnv presence; chevron steps derived from max(bn, bnv peak). Codex-reviewed: clean (no findings). +1 JS test (sus-relative mapping + fallback). JS 8/8, 250 core GP/song tests pass. NB: GP8's short-bend path still lacks a dedicated synthetic-GPIF fixture (same gap as the GP8 offset-prop-names P3) — _gpx_bend_shape units cover the function; the fix is the one-line caller change. Part of got-feedback/feedback#334. Co-authored-by: Claude Opus 4.8 (1M context) --- lib/gp2rs_gpx.py | 7 ++++++- plugins/highway_3d/screen.js | 11 +++++++++-- static/highway.js | 11 +++++++++-- tests/js/highway_bend_curve.test.js | 15 ++++++++++++++- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/lib/gp2rs_gpx.py b/lib/gp2rs_gpx.py index ecd6a93..a161aea 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -1621,8 +1621,13 @@ def convert_file( # the shape over time (§6.2.1). value/divisor # = semitones (scale auto-detected per file). if 'Bended' in _tp: + # Use the beat duration `dur`, not + # `rn.sustain` (zeroed for notes <= 0.2s), + # so short bends keep their bnv curve — + # matching the GP5 path, which maps over + # the raw note duration. _peak, _intent, _curve = _gpx_bend_shape( - _tp, _bend_divisor, rn.sustain) + _tp, _bend_divisor, dur) if _peak > 0: rn.bend = _peak rn.bend_intent = _intent diff --git a/plugins/highway_3d/screen.js b/plugins/highway_3d/screen.js index 29d7b11..99a23b1 100644 --- a/plugins/highway_3d/screen.js +++ b/plugins/highway_3d/screen.js @@ -11949,6 +11949,7 @@ const ribbonSusTrail = !!( (slideSt && n.f > 0 && (n.sus || 0) > 1e-4) || (Number(n.bn) > 0) + || (Array.isArray(n.bnv) && n.bnv.length > 0) || n.tr || hasTechniqueVibrato ); @@ -12119,11 +12120,17 @@ arrow.material.opacity = 1; } } - if (n.bn > 0) { + // Derive the peak from bn OR the bnv curve: a note may carry an + // authoritative curve with bn left at 0 (bn SHOULD be the peak + // whenever bnv exists — this is the robustness fallback). + const _bnvPeak = (Array.isArray(n.bnv) && n.bnv.length) + ? n.bnv.reduce((m, p) => Math.max(m, Number(p.v) || 0), 0) : 0; + const _bendPeak = Math.max(Number(n.bn) || 0, _bnvPeak); + if (_bendPeak > 0) { // Bend chevron stack — PlaneGeometry mesh so it tilts with // the gem (approachRot). Fixed world size so it perspective- // shrinks naturally without distFactor compensation. - const steps = Math.max(1, Math.min(4, Math.round(n.bn))); + const steps = Math.max(1, Math.min(4, Math.round(_bendPeak))); const bendSm = bendChevronMat(steps, activePalette[s] || 0xffffff); const l = pTechPlane.get(); l.material = _spriteMat2MeshMat(l, bendSm); diff --git a/static/highway.js b/static/highway.js index 4f40136..fd8871a 100644 --- a/static/highway.js +++ b/static/highway.js @@ -471,8 +471,15 @@ function createHighway() { /** Map a bend curve [{t, v}] (§6.2.1) to [{x, v}] with x normalized to * 0..1 across the curve's time span (0 when the span is degenerate). * Pure — drives the 2D bend-shape glyph. */ - function bnvNormalizedPoints(bnv) { + function bnvNormalizedPoints(bnv, sus) { if (!Array.isArray(bnv) || bnv.length === 0) return []; + // Map each point's time over the NOTE's span [0, sus] so it sits at its + // real fraction of the note (a bend that completes before the note ends + // draws short of the glyph's right edge). Fall back to the curve's own + // t-range only when the note has no usable sustain. + if (Number.isFinite(sus) && sus > 0) { + return bnv.map(p => ({ x: Math.min(Math.max(p.t / sus, 0), 1), v: p.v })); + } const t0 = bnv[0].t; const span = bnv[bnv.length - 1].t - t0; return bnv.map(p => ({ x: span > 0 ? (p.t - t0) / span : 0, v: p.v })); @@ -1625,7 +1632,7 @@ function createHighway() { // Bend curve (§6.2.1): trace the real shape as a contour above // the gem (round-trip rises then falls, pre-bend starts high, // release descends, …) — `bt` is implicit in the point shape. - const pts = bnvNormalizedPoints(bnv); + const pts = bnvNormalizedPoints(bnv, opts?.sus); const gw = sz * 0.6; const x0 = x - gw / 2; ctx.beginPath(); diff --git a/tests/js/highway_bend_curve.test.js b/tests/js/highway_bend_curve.test.js index a20e164..5b051e9 100644 --- a/tests/js/highway_bend_curve.test.js +++ b/tests/js/highway_bend_curve.test.js @@ -30,13 +30,26 @@ const bnvSampleAt = loadFn('plugins/highway_3d/screen.js', 'bnvSampleAt'); // ── bnvNormalizedPoints (2D) ───────────────────────────────────────────────── -test('bnvNormalizedPoints normalizes t to 0..1 across the span', () => { +test('bnvNormalizedPoints normalizes t to 0..1 across the curve span (no sus)', () => { const pts = bnvNormalizedPoints([ { t: 0.5, v: 0 }, { t: 1.0, v: 2 }, { t: 1.5, v: 0 }]); assert.deepEqual(pts, [ { x: 0, v: 0 }, { x: 0.5, v: 2 }, { x: 1, v: 0 }]); }); +test('bnvNormalizedPoints maps t over the note sus span when given', () => { + // A bend that completes at t=0.4 of a 0.5s note draws to x=0.8, not x=1 — + // i.e. it stops short of the glyph's right edge (correct timing shape). + assert.deepEqual( + bnvNormalizedPoints([{ t: 0, v: 0 }, { t: 0.25, v: 1 }, { t: 0.4, v: 0 }], 0.5), + [{ x: 0, v: 0 }, { x: 0.5, v: 1 }, { x: 0.8, v: 0 }]); + // Points beyond sus clamp to 1; sus<=0 falls back to curve-span mapping. + assert.deepEqual(bnvNormalizedPoints([{ t: 0, v: 0 }, { t: 1, v: 2 }], 0.5), + [{ x: 0, v: 0 }, { x: 1, v: 2 }]); + assert.deepEqual(bnvNormalizedPoints([{ t: 0, v: 0 }, { t: 1, v: 2 }], 0), + [{ x: 0, v: 0 }, { x: 1, v: 2 }]); +}); + test('bnvNormalizedPoints handles degenerate/empty input', () => { assert.deepEqual(bnvNormalizedPoints([]), []); assert.deepEqual(bnvNormalizedPoints(null), []);