fix(bend): GP8 short-bend curve loss + 2D curve timing + 3D bnv gating (Codex follow-ups)#535
Merged
Merged
Conversation
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 #334. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the bend-curve PRs (#531/#532). An independent post-merge Codex review surfaced three correctness edges (no P1s); this fixes them.
rn.sustain(zeroed for notes ≤0.2s) → short GP8 bends lostbt/bnv(kept scalarbn); GP5 uses rawdurdur(matches GP5)bnvNormalizedPointsmappedxover the curve's[first.t,last.t]not the note span → curves not starting at 0 / ending atsuswere time-distorted[0, sus](clamped); curve-span fallback whensus≤0bn>0→ abnv-with-bn==0note drew no ribbon/markerbnvpresence; chevron steps frommax(bn, bnv peak)The GP5 unit-conversion (the flagged risk) was confirmed correct in review and is untouched.
Review
Codex re-reviewed the fix: clean, no findings (
duris the in-scope note duration; thesusmapping + clamp + fallback are correct and don't affect no-suscallers; the 3D path doesn't double-draw).Tests
+1 JS case (sus-relative mapping +
sus≤0fallback).JS 8/8;250core GP/song tests pass.NB: the GP8 short-bend path still lacks a dedicated synthetic-GPIF fixture (same gap as the GP8 offset-prop-name P3) —
_gpx_bend_shapeunits cover the function; the fix is the one-line caller change.Part of #334.
🤖 Generated with Claude Code