From 1c774226d353c6cef87ec09b61b6bfb8ac467070 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 31 Jul 2026 22:27:41 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20close=20review=20findings=20on=20spline?= =?UTF-8?q?=20EGs=20=E2=80=94=20engine,=20codec,=20and=20popup/overlay=20U?= =?UTF-8?q?I=20grammar?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live pitch depth, Gate/Spline enable-rule agreement, inert kTrigLength, NaN wire guards, hard-flag-tail corruption no longer wipes the record, RT/cold spline tie-break, retired alt-click, marker-shadow fix, plus new test coverage. --- src/core/instrument/CLAUDE.md | 2 +- src/core/instrument/engine/play_params.h | 10 +- src/core/instrument/engine/velocity_curve.h | 24 ++- src/core/instrument/engine/voice.cpp | 14 +- src/core/instrument/engine/voice.h | 14 +- src/core/instrument/map/params_payload.cpp | 35 ++++- src/core/instrument/map/sample_map.cpp | 2 +- src/core/instrument/ui/deck_groups.cpp | 10 +- src/core/instrument/ui/knob_deck.h | 5 +- src/shell/instrument/editor_controls.cpp | 13 +- src/shell/instrument/editor_input.cpp | 17 ++- src/shell/instrument/editor_input_curve.cpp | 13 +- src/shell/instrument/editor_input_deck.cpp | 12 +- .../instrument/editor_input_waveform.cpp | 30 +++- src/shell/instrument/editor_models.cpp | 17 --- src/shell/instrument/editor_paint_deck.cpp | 6 +- .../instrument/editor_paint_waveform.cpp | 19 ++- src/shell/instrument/editor_session.cpp | 8 + src/shell/instrument/reasampler_editor.h | 7 +- tests/test_component_state_io.cpp | 138 ++++++++++++++++++ tests/test_knob_deck.cpp | 36 +++++ tests/test_spline_egs.cpp | 70 +++++++-- 22 files changed, 414 insertions(+), 88 deletions(-) diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index f0782f3..68cbcfc 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -289,7 +289,7 @@ anything for a trigger shape. ### `map/` - `sample_map` — the bank blob → selected capture resolve, the channel policy (downmix / dual-mono / L-R split), `InstrumentParams` (the ONE parameter set: root/loop/start overrides, keyTrack, velocity curve, `PlaySeconds`), the single override-beats-intrinsic fold (`resolveCapture`, shared by the bank and refs paths so they cannot drift), and the `SampleData` build. **Wall-clock times stored as rate-free SECONDS, resolved against the live project rate — NO hardcoded sample rates in `src/`** (Daniel's standing ruling, load-bearing). Deliberately does NOT link the voice engine: the build's product is plain `SampleData`. -- `component_state_io` (`core/instrument/map`) — the `ComponentState` envelope + params-payload binary codec (envelope v1…v11, params payload v1…v12), split out of `sample_map` (Q-W2v, T4-13 ≡ T2-07) so BOTH artifacts can link the codec without the extension pulling in the whole voice engine to serialize one preset blob — the extension's `instrument_drop` and the instrument's processor read/write the identical bytes, so the cross-artifact contract cannot drift. Payload v1…v7 are the RETIRED per-zone lists: still read, lifting by adopting zone one's capture + parameters (that first zone is what the old first-match resolve actually played, so it is also what supersedes the envelope's stored selection id). Payload v9 appends the per-voice filter tail; a v8 blob is a strict prefix of it and lifts to the off/neutral filter default. Every tail since is a strict suffix on the same discipline — v10 the staged curves, v11 the loop crossfade, v12 the velocity→pitch curve, v13 the dual Staged/Spline state (the three contours, plus hard-flag tails for the three velocity curves — their v7/v9/v12 blocks are frozen at 16 bytes/point and had no room for a per-point flag). v12 also RE-TAGS the y DOMAIN of one frozen slot inside the v9 filter tail — its velocity curve reads bipolar from v12 on, unipolar before — which needs no version branch, because a pre-v12 curve's y values are already valid bipolar ones; every other filter slot, `velAmount` included, keeps its meaning. +- `component_state_io` (`core/instrument/map`) — the `ComponentState` envelope + params-payload binary codec (envelope v1…v11, params payload v1…v13), split out of `sample_map` (Q-W2v, T4-13 ≡ T2-07) so BOTH artifacts can link the codec without the extension pulling in the whole voice engine to serialize one preset blob — the extension's `instrument_drop` and the instrument's processor read/write the identical bytes, so the cross-artifact contract cannot drift. Payload v1…v7 are the RETIRED per-zone lists: still read, lifting by adopting zone one's capture + parameters (that first zone is what the old first-match resolve actually played, so it is also what supersedes the envelope's stored selection id). Payload v9 appends the per-voice filter tail; a v8 blob is a strict prefix of it and lifts to the off/neutral filter default. Every tail since is a strict suffix on the same discipline — v10 the staged curves, v11 the loop crossfade, v12 the velocity→pitch curve, v13 the dual Staged/Spline state (the three contours, plus hard-flag tails for the three velocity curves — their v7/v9/v12 blocks are frozen at 16 bytes/point and had no room for a per-point flag). v12 also RE-TAGS the y DOMAIN of one frozen slot inside the v9 filter tail — its velocity curve reads bipolar from v12 on, unipolar before — which needs no version branch, because a pre-v12 curve's y values are already valid bipolar ones; every other filter slot, `velAmount` included, keeps its meaning. - `params_payload` — the PARAMS-PAYLOAD half of that codec, split from the envelope half on the axis the format already has: the payload carries its own version and grows independently, so the two version ladders are two responsibilities. An INTERNAL seam — the public entry points stay `serialize`/`deserializeComponentState`. The prose ladder and every version constant stay in `component_state_io.h`, their one home. - `bank_sync` — generation change-detection + assignment-request consume: owns the yes/no decision logic so the rules are provable without a host. The processor shell owns cadence and side effects. - `bridge_marshal` — pure marshalling helper for the REAPER VST-host bridge read: interprets the `GetProjExtState` int return against its filled buffer. diff --git a/src/core/instrument/engine/play_params.h b/src/core/instrument/engine/play_params.h index 586c842..896a8fe 100644 --- a/src/core/instrument/engine/play_params.h +++ b/src/core/instrument/engine/play_params.h @@ -183,10 +183,16 @@ struct PlayParams { // sample length, which IS the Trigger/one-shot playback model — so Gate is not available while // any spline EG is active. resolvePlay enforces it on the way to the engine; the editor's // play-mode toggle refuses the Gate segment so the two agree. +// +// The pitch/filter terms are gated on their own `enabled` flag to match Voice::start's binder +// (voice.cpp only binds pitchSplineCur_/filterSplineCur_ when that flag is set): without this, +// a Spline mode flip on a disabled pitch/filter envelope would cost Gate for zero modulation, +// since the binder would never actually engage. Amp has no such flag, so it counts unconditionally. template bool splineActive(const Play& p) { - return p.ampSpline.mode == EnvMode::Spline || p.pitchSpline.mode == EnvMode::Spline || - p.filterSpline.mode == EnvMode::Spline; + return p.ampSpline.mode == EnvMode::Spline || + (p.pitchEnv.enabled && p.pitchSpline.mode == EnvMode::Spline) || + (p.filter.enabled && p.filterSpline.mode == EnvMode::Spline); } // [start, end) frames, half-open. A zero-length loop (start == end) is the "no sustain loop" diff --git a/src/core/instrument/engine/velocity_curve.h b/src/core/instrument/engine/velocity_curve.h index 67e2219..24642db 100644 --- a/src/core/instrument/engine/velocity_curve.h +++ b/src/core/instrument/engine/velocity_curve.h @@ -23,8 +23,8 @@ inline constexpr double kCurveYMax = 1.0; // Point-count ceiling. A MUSICAL bound, not a performance one: long rhythmic phrases need the // resolution, and at roughly two points per articulation event 128 is about four bars of 16ths. -// Segment lookup is logarithmic and the editor's node separation is the real density limit, so -// there is nothing to buy by lowering it. DO NOT LOWER. +// Segment lookup is logarithmic (<=7 steps at this ceiling), so there is no performance case for +// lowering it. DO NOT LOWER. inline constexpr std::size_t kMaxCurvePoints = 128; // The curve's Y range. UNIPOLAR [0,1] is a GAIN — the amp's domain, where the do-nothing @@ -229,6 +229,11 @@ public: } void clear() { pts_ = nullptr; n_ = 0; } bool active() const { return n_ >= 2; } + // True once the cursor has settled on the contour's LAST segment: past this point there is + // no further point to rise into, so a value read here that reaches 0 is a genuine permanent + // terminus (Voice::tickAmplitude's early-free), unlike a 0 touched mid-contour, which a + // later segment may still rise out of (the spline is deliberately not globally monotone). + bool onFinalSegment() const { return seg_ + 2 == n_; } // `phase` is normalized position over the contour's whole span, [0,1]; out-of-range clamps // to the terminal values (a note past its span holds the contour's last level). @@ -238,7 +243,12 @@ public: : kCurveXMin + phase * (kCurveXMax - kCurveXMin); if (x <= x0_ && seg_ == 0) return y0_; if (x >= x1_ && seg_ + 2 == n_) return y1_; - if (x < x0_ || x > x1_) locate(x); + // x <= x0_ (not just <): landing exactly on the cached segment's LEFT edge normally + // reproduces y0_ either way, but at a duplicate-X step (coincident knots with + // DIFFERENT Y) the cached segment may be the LATER of the two — re-locate so a query + // sitting exactly on the shared X always resolves through locate()'s tie-break, which + // agrees with the cold VelocityCurve::eval's first-containing-segment rule. + if (x <= x0_ || x > x1_) locate(x); if (span_ <= 0.0) return y1_; // coincident-X knots: a step, no interior to blend return hermiteAt(y0_, y1_, span_, mA_, mB_, (x - x0_) / span_); } @@ -248,10 +258,14 @@ private: // binary search over the X-ordered array. void locate(double x) { if (x > x1_ && seg_ + 2 < n_ && x <= pts_[seg_ + 2].velocity) { select(seg_ + 1); return; } + // Leftmost segment containing x: smallest lo with pts_[lo+1].velocity >= x. At + // coincident-X knots (a drawn step) this picks the FIRST segment ending at the shared X, + // matching VelocityCurve::eval's cold linear walk — the two readers must agree here or a + // backwards/jumping read can return a different knot's Y than a forward one would. std::size_t lo = 0, hi = n_ - 2; while (lo < hi) { - const std::size_t mid = lo + (hi - lo + 1) / 2; - if (pts_[mid].velocity <= x) lo = mid; else hi = mid - 1; + const std::size_t mid = lo + (hi - lo) / 2; + if (pts_[mid + 1].velocity < x) lo = mid + 1; else hi = mid; } select(lo); } diff --git a/src/core/instrument/engine/voice.cpp b/src/core/instrument/engine/voice.cpp index d95c9e2..01376bb 100644 --- a/src/core/instrument/engine/voice.cpp +++ b/src/core/instrument/engine/voice.cpp @@ -102,8 +102,12 @@ void Voice::start(int note, int velocity, const SampleData& sample, bool declick playEnd_ = 0; // unused in Gate } else { // Trigger: play [start, playEnd) where - // playEnd = start + round(lengthFraction*(frames-start)). - double frac = p.trigger.lengthFraction; + // playEnd = start + round(lengthFraction*(frames-start)) — except kTrigLength is INERT + // while any spline EG is active (splineActive, play_params.h): a contour is a pure + // function over the FULL sample length, so truncating playEnd_ to a %-length would + // hard-cut it mid-shape. The staged Trigger AHD below (trigSpan) plays the same full + // span in that case, matching the "drawn-but-dead" treatment of the other staged knobs. + double frac = splineActive(p) ? 1.0 : p.trigger.lengthFraction; if (frac <= 0.0) frac = 0.0; // %=0 -> zero play length (finishes immediately) if (frac > 1.0) frac = 1.0; std::int64_t playLen = static_cast( @@ -258,6 +262,12 @@ void Voice::applyLive(const instrument::engine::LiveValues& live, bool snap) { else ampAhd_.applyLive(sourceOffset(), live.ampAhd); pitchEnv_.applyLive(live.pitchEnv); } + // The pitch DEPTH knob stays live under a spline (core/instrument/CLAUDE.md), but + // pitchSplineDepth_ is a plain member latched at note-on — unlike filter's modAmount_, + // which already glides through rModAmount_'s live ramp regardless of spline state (below), + // this is the one place a live pitch-depth move must be re-applied by hand. Only meaningful + // while pitchSplineCur_ is bound; harmless (and cheap) to set otherwise. + pitchSplineDepth_ = live.pitchEnv.peakSemitones; if (!filterOn_) return; // filter enable is a discrete toggle: it travels by reload if (snap) { diff --git a/src/core/instrument/engine/voice.h b/src/core/instrument/engine/voice.h index b3d6293..fd094e0 100644 --- a/src/core/instrument/engine/voice.h +++ b/src/core/instrument/engine/voice.h @@ -178,10 +178,20 @@ private: // the voice. double tickAmplitude() { double amp; - if (ampSplineCur_.active()) { + // playMode_ is Trigger whenever a spline is genuinely reachable (resolvePlay forces it — + // splineActive, play_params.h); the guard is a pure-core defense against a hand-built + // SampleData pairing Gate with an amp spline, which would otherwise bypass env_ + // entirely — release() then has no envelope to end, and an active sustain loop rings + // forever. + if (ampSplineCur_.active() && playMode_ == PlayMode::Trigger) { // A contour covers the sample end to end, so the head leaving the span IS the end of - // the note — the exhaustion path in advanceFrame is what frees the voice. + // the note — the exhaustion path in advanceFrame is what frees the voice. A contour + // that flatlines at 0 across its FINAL segment is a permanent terminus (no later + // segment to rise out of), so that case frees early too, the spline analogue of a + // staged AHD's finished() — mid-contour dips do not, since the spline is deliberately + // not globally monotone. amp = ampSplineCur_.eval(splinePhase()); + if (amp == 0.0 && ampSplineCur_.onFinalSegment()) amplitudeDone_ = true; } else if (playMode_ == PlayMode::Gate) { amp = env_.tick(); if (env_.finished()) amplitudeDone_ = true; diff --git a/src/core/instrument/map/params_payload.cpp b/src/core/instrument/map/params_payload.cpp index 1ce5e27..cc48022 100644 --- a/src/core/instrument/map/params_payload.cpp +++ b/src/core/instrument/map/params_payload.cpp @@ -128,7 +128,11 @@ void readCurveTail(ByteReader& r, VelocityCurve& curve, for (std::uint32_t i = 0; i < ptCount && r.ok; ++i) { const double vel = bitsToDouble(r.u64()); const double value = bitsToDouble(r.u64()); - pts.push_back(VelocityPoint{vel, value}); + // A NaN velocity breaks fromPoints' stable_sort (not a strict weak ordering with NaN + // present); a NaN value reaches the RT eval's multiply. Same non-finite-falls-back-to-0 + // guard as every other wire double this codec reads. + pts.push_back(VelocityPoint{std::isfinite(vel) ? vel : 0.0, + std::isfinite(value) ? value : 0.0}); } if (r.ok) { curve = reasampler::instrument::engine::VelocityCurve::fromPoints(std::move(pts), domain); @@ -149,24 +153,43 @@ void readSplineEnv(ByteReader& r, SplineEnv& s) { const double x = bitsToDouble(r.u64()); const double y = bitsToDouble(r.u64()); const bool hard = (r.u8() != 0); - pts.push_back(VelocityPoint{x, y, hard}); + // Same NaN guard as readCurveTail: an x NaN breaks fromPoints' sort, a y NaN reaches + // SplineCursor::eval's multiply into the per-sample amp gain. + pts.push_back(VelocityPoint{std::isfinite(x) ? x : 0.0, std::isfinite(y) ? y : 0.0, hard}); } if (!r.ok) return; s.mode = spline ? EnvMode::Spline : EnvMode::Staged; + if (pts.size() < 2) { + // fromPoints' own sub-2-point fallback is flat()/zero() by DOMAIN — the neutral velocity + // curve response (a full-open gate). A spline EG's documented neutral is y = 1 - x + // instead, so a malformed/short block substitutes that rather than fromPoints' default. + s.contour = VelocityCurve::rampDown(); + return; + } s.contour = VelocityCurve::fromPoints(std::move(pts), reasampler::instrument::engine::CurveDomain::Unipolar); } // Apply a hard-flag tail to an already-read velocity curve. A count that disagrees with the -// curve fromPoints actually produced is dropped rather than applied to shifted knots. +// curve fromPoints actually produced — including an out-of-bounds or truncated one — is +// dropped rather than applied to shifted knots, and the whole params record parsed ahead of +// this tail survives (component_state_io.h's documented promise): if THIS call is what tripped +// r.ok (a truncated count field), it is revived before returning. An r.ok already false on +// entry (an earlier, unrelated field genuinely truncated) is left alone — that failure is not +// this tail's to forgive. void readHardFlags(ByteReader& r, VelocityCurve& curve) { + const bool enteredOk = r.ok; const std::uint32_t count = r.u32(); + if (!r.ok) { + if (enteredOk) r.ok = true; // a truncated count field: nothing to apply + return; + } const std::size_t remaining = r.bytes.size() > r.pos ? r.bytes.size() - r.pos : 0; - if (count > remaining) { r.ok = false; return; } + if (count > remaining) return; // bound-and-skip: cannot safely reserve/read this many std::vector flags; flags.reserve(count); - for (std::uint32_t i = 0; i < count && r.ok; ++i) flags.push_back(r.u8()); - if (!r.ok || flags.size() != curve.size()) return; + for (std::uint32_t i = 0; i < count; ++i) flags.push_back(r.u8()); + if (flags.size() != curve.size()) return; for (std::size_t i = 0; i < flags.size(); ++i) curve.setHard(i, flags[i] != 0); } diff --git a/src/core/instrument/map/sample_map.cpp b/src/core/instrument/map/sample_map.cpp index f8b91d4..62c1b77 100644 --- a/src/core/instrument/map/sample_map.cpp +++ b/src/core/instrument/map/sample_map.cpp @@ -259,7 +259,7 @@ PlayParams resolvePlay(const PlaySeconds& stored, int sampleRate) { // The three drawn contours are normalized over the sample's own length, so no rate resolves // them — they carry through verbatim, which is also what makes a different-length capture // replay the same shape proportionally. - out.ampSpline = stored.ampSpline; + out.ampSpline = stored.ampSpline; out.pitchSpline = stored.pitchSpline; out.filterSpline = stored.filterSpline; // Gate is unavailable while any EG is drawn — see splineActive (play_params.h) for why. diff --git a/src/core/instrument/ui/deck_groups.cpp b/src/core/instrument/ui/deck_groups.cpp index 6ff04ab..7798f99 100644 --- a/src/core/instrument/ui/deck_groups.cpp +++ b/src/core/instrument/ui/deck_groups.cpp @@ -36,9 +36,8 @@ std::vector sampleDeckGroups(PlayMode playMode) { penv.captionWidth = 58; penv.captionRadio = {id(DeckParam::kPitchEnvSelect)}; penv.captionToggle = {id(DeckParam::kPitchEnvEnable), 32}; - // The mode toggle rides the caption slack rather than the knob row: every env group's - // knob row is wider than its caption row, so this costs no group width — and the deck - // has six pixels of headroom on its first row at the editor's floor width. + // The mode toggle rides the caption slack rather than the knob row — costs no group + // width; see this module's CLAUDE.md bullet (knob_deck) for the headroom this relies on. penv.captionToggle2 = {id(DeckParam::kPitchEnvMode), kEnvModeSegW}; penv.cellIds = {id(DeckParam::kPitchEnvAttack), id(DeckParam::kPitchEnvHold), @@ -289,6 +288,11 @@ bool deckKnobInert(DeckParam id, const DeckEnableState& state) { case DeckParam::kTrigHold: case DeckParam::kTrigDecay: return state.ampSpline; + // The Trigger %-length knob is inert whenever ANY spline is active (not just the amp's): + // a drawn contour always covers the full sample length (splineActive, play_params.h), so + // the engine ignores lengthFraction in that case regardless of which EG is drawn. + case DeckParam::kTrigLength: + return state.ampSpline || state.pitchSpline || state.filterSpline; case DeckParam::kPitchEnvAttack: case DeckParam::kPitchEnvHold: case DeckParam::kPitchEnvDecay: diff --git a/src/core/instrument/ui/knob_deck.h b/src/core/instrument/ui/knob_deck.h index b28c7ce..4014770 100644 --- a/src/core/instrument/ui/knob_deck.h +++ b/src/core/instrument/ui/knob_deck.h @@ -65,9 +65,8 @@ struct DeckGroupDesc { DeckRadioDesc captionRadio; // the caption row's far corner; id -1 = none DeckToggleDesc captionToggle; // caption row, left of the radio; id -1 = none // A second caption toggle, placed immediately left of the first (or in its place when the - // first is absent). Exists because a group whose knob row is wider than its caption row has - // caption slack a toggle can occupy for free — a rowToggle would widen the GROUP, and the - // deck has six pixels of headroom on its first row at the editor's floor width. + // first is absent) — why this exists rather than a rowToggle is recorded once, at this + // module's CLAUDE.md bullet. DeckToggleDesc captionToggle2; std::vector cellIds; // knob cells; -1 = blank reserve DeckToggleDesc rowToggle; // in the knob row after the cells; id -1 = none diff --git a/src/shell/instrument/editor_controls.cpp b/src/shell/instrument/editor_controls.cpp index 4f35729..57327c6 100644 --- a/src/shell/instrument/editor_controls.cpp +++ b/src/shell/instrument/editor_controls.cpp @@ -11,12 +11,10 @@ #include #include // snprintf (deck value labels) #include -#include // std::as_const (the const/non-const editedCurve pair) #include #include "core/instrument/engine/filter/filter_params.h" // the filter's own control laws #include "core/instrument/engine/master_gain.h" // master-gain dB<->linear<->knob taper -#include "core/instrument/map/trigger_seam.h" // triggerPlayLength (the Trigger play span) #include "core/instrument/ui/deck_groups.h" // sampleDeckGroups (the deck's composition) #include "core/instrument/ui/knob_deck.h" // deckHeight / kDeckKnobSize (the band's own height) #include "core/util/clamp01.h" @@ -26,7 +24,7 @@ namespace reasampler::vst { -using namespace reasampler::instrument::map; // PlaySeconds vocabulary + trigger_seam +using namespace reasampler::instrument::map; // PlaySeconds vocabulary using instrument::ui::computeSampleBands; using instrument::ui::chromeRects; using instrument::ui::deckHeight; @@ -299,6 +297,15 @@ double ReaSamplerEditor::deckControlNorm(int id) const { } } +instrument::ui::DeckEnableState ReaSamplerEditor::deckEnableState() const { + const PlaySeconds& play = params_.play; + return instrument::ui::DeckEnableState{ + play.pitchEnv.enabled, play.filter.enabled, + play.ampSpline.mode == EnvMode::Spline, + play.pitchSpline.mode == EnvMode::Spline, + play.filterSpline.mode == EnvMode::Spline}; +} + void ReaSamplerEditor::applyDeckKnob(int id, double norm) { if (!processor_) return; norm = clamp01(norm); diff --git a/src/shell/instrument/editor_input.cpp b/src/shell/instrument/editor_input.cpp index e89d4a3..2396a14 100644 --- a/src/shell/instrument/editor_input.cpp +++ b/src/shell/instrument/editor_input.cpp @@ -112,17 +112,22 @@ void ReaSamplerEditor::onMouseUp(int x, int y) { invalidate(); return; } - // Drag-off delete: releasing a curve-node drag well outside the box removes the dragged - // point (deletePoint refuses the two endpoints, so an endpoint drag-off is a plain move — - // its amp keeps the last clamped drag value). - if (kind == DragKind::kCurveNode && curveIdx >= 0) { + // Drag-off delete: releasing a curve-node drag well outside its box removes the dragged + // point on EITHER spline surface — the popup's kCurveNode and the overlay's kSplineNode + // share this grammar, not just their click grammar (deletePoint refuses the two endpoints, + // so an endpoint drag-off is a plain move — its amp keeps the last clamped drag value). + if ((kind == DragKind::kCurveNode || kind == DragKind::kSplineNode) && curveIdx >= 0) { const bool off = x < curveRect.x - kCurveDragOffMargin || x > curveRect.right() + kCurveDragOffMargin || y < curveRect.y - kCurveDragOffMargin || y > curveRect.bottom() + kCurveDragOffMargin; if (off) { - editedCurve().deletePoint(static_cast(curveIdx)); - hover_ = HoverTarget{}; // stale kCurveNode index would light a shifted node + if (kind == DragKind::kCurveNode) { + editedCurve().deletePoint(static_cast(curveIdx)); + } else { + splineFor(overlayEnv_).deletePoint(static_cast(curveIdx)); + } + hover_ = HoverTarget{}; // stale node index would light a shifted node } } commitAndReload(); diff --git a/src/shell/instrument/editor_input_curve.cpp b/src/shell/instrument/editor_input_curve.cpp index 6f51743..02908fa 100644 --- a/src/shell/instrument/editor_input_curve.cpp +++ b/src/shell/instrument/editor_input_curve.cpp @@ -45,17 +45,8 @@ void ReaSamplerEditor::handleCurveMouseDown(const Rect& r, int x, int y) { const VelocityCurve::Box box = curveBoxFromRect(r); if (box.width <= 0 || box.height <= 1) return; - // Alt-click delete predates the converged grammar and is kept as a landed alternate; every - // other gesture routes through the shared resolver so both spline consumers stay one - // grammar. - if ((GetKeyState(VK_MENU) & 0x8000) != 0) { - const int alt = editedCurve().pointAtPixel(box, x, y); - if (alt >= 0 && editedCurve().deletePoint(static_cast(alt))) { - hover_ = HoverTarget{}; // a stale kCurveNode index would light a shifted node - commitAndReload(); - } - return; - } + // Alt-click delete is retired (the spec's right-click supersedes it — one grammar, no + // migration on either side): every gesture here routes through the shared resolver. const bool ctrl = (GetKeyState(VK_CONTROL) & 0x8000) != 0; const SplineEdit edit = resolveSplineEdit( editedCurve(), box, ctrl ? SplineGesture::kControlLeft : SplineGesture::kLeft, x, y); diff --git a/src/shell/instrument/editor_input_deck.cpp b/src/shell/instrument/editor_input_deck.cpp index d398b8b..46f1b3e 100644 --- a/src/shell/instrument/editor_input_deck.cpp +++ b/src/shell/instrument/editor_input_deck.cpp @@ -63,11 +63,19 @@ bool ReaSamplerEditor::mouseDownDeck(const FaceLayout& fl, int x, int y) { applyParamControl(hit.id, 0.0, hit.segment); commitAndReload(); break; - default: - // Parameter-set toggles (play mode / pitch engine / pitch-env + filter enable). + default: { + // Parameter-set toggles (play mode / pitch engine / pitch-env + filter enable, + // and the three env-mode toggles). applyParamControl(hit.id, 0.0, hit.segment); commitAndReload(); + // Flipping an EG's Staged|Spline toggle makes THAT envelope's overlay active, + // so the contour (or the staged shape you just returned to) is what's drawn. + // overlayEnvForModeToggle answers kNone for every other toggle this default + // case handles, which is why the assignment is conditional. + const OverlayEnv modeEnv = overlayEnvForModeToggle(hit.id); + if (modeEnv != OverlayEnv::kNone) overlayEnv_ = modeEnv; break; + } } return true; } diff --git a/src/shell/instrument/editor_input_waveform.cpp b/src/shell/instrument/editor_input_waveform.cpp index cd48ded..3093c38 100644 --- a/src/shell/instrument/editor_input_waveform.cpp +++ b/src/shell/instrument/editor_input_waveform.cpp @@ -31,17 +31,16 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) { if (frames <= 0) return false; const OverlayArea overlay = waveformOverlayArea(fl.bands.waveform); - // A drawn EG's own point actions come first, for the same reason the staged nodes do — - // they sit on top of the markers. Its ADD, which any empty-space click satisfies, is held - // back until after the markers below, or a spline overlay would make them unreachable. + // A drawn EG's contour is evaluated below, AFTER the markers: pointAtPixel's pick radius + // has no box check of its own, so a coincident contour node would otherwise shadow a + // marker's own dedicated grab rect (the loop-crossfade tab most sharply, since it is the + // ONLY affordance at zero crossfade) — marker reachability wins on any pixel overlap. The + // staged envelope-node pass just below is unaffected (its own, longer-standing ordering). const DeckEnableState gates = deckEnableState(); const bool splineLive = overlayIsSpline() && overlayEnvEnabled(overlayEnv_, gates); const SplineGesture gesture = (GetKeyState(VK_CONTROL) & 0x8000) != 0 ? SplineGesture::kControlLeft : SplineGesture::kLeft; - if (splineLive && splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false)) { - return true; - } // Envelope nodes first (they sit on top of the markers), then the wave markers. With no // envelope overlay-active — or with its deck group's enable toggle off, which makes the @@ -83,6 +82,11 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) { beginMarkerDrag(static_cast(hit), m, frames, x); return true; } + // No marker wanted the click: a drawn EG's own node grab/toggle (never add — empty space is + // tried last, below). + if (splineLive && splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false)) { + return true; + } // Nothing else wanted the click: now the drawn contour may take the empty space. if (splineLive) return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/true); return false; @@ -92,7 +96,15 @@ bool ReaSamplerEditor::splineOverlayClick(const OverlayArea& waveArea, int x, in SplineGesture gesture, bool addOnEmptySpace) { const VelocityCurve::Box box = splineOverlayBox(waveArea); VelocityCurve& contour = splineFor(overlayEnv_); - const SplineEdit edit = resolveSplineEdit(contour, box, gesture, x, y); + SplineEdit edit = resolveSplineEdit(contour, box, gesture, x, y); + // resolveSplineEdit's outside-box grab/delete/toggle allowance (pointAtPixel's radius has + // no box check of its own) was designed for the popup's inset ring; the overlay box has NO + // inset (splineOverlayBox), so honoring it here would extend the grab halo 6px into the + // inter-band pad. kAdd already requires in-box (resolveSplineEdit's own check). + if (edit.kind != SplineEditKind::kNone && edit.kind != SplineEditKind::kAdd && + !(x >= box.left && x < box.left + box.width && y >= box.top && y < box.top + box.height)) { + edit = SplineEdit{}; + } if (edit.kind == SplineEditKind::kAdd && !addOnEmptySpace) return false; switch (edit.kind) { case SplineEditKind::kNone: @@ -125,6 +137,10 @@ bool ReaSamplerEditor::splineOverlayClick(const OverlayArea& waveArea, int x, in drag_ = DragKind::kSplineNode; curvePointIndex_ = index; dragStartCurve_ = contour; // AFTER the add — resolvePointDrag's delta base + // The release-time drag-off-delete bound (onMouseUp), matching the popup's kCurveNode use + // of the same field — the two spline surfaces share one drag-off grammar, not just the + // click grammar. + dragCurveRect_ = waveArea.rect; dragStartX_ = x; dragStartY_ = y; invalidate(); // live feedback; the commit lands on WM_LBUTTONUP diff --git a/src/shell/instrument/editor_models.cpp b/src/shell/instrument/editor_models.cpp index 8128f8e..4c51136 100644 --- a/src/shell/instrument/editor_models.cpp +++ b/src/shell/instrument/editor_models.cpp @@ -112,15 +112,6 @@ void ReaSamplerEditor::unpackEnvelope(OverlayEnv which, const StageEnvelope& env } } -instrument::ui::DeckEnableState ReaSamplerEditor::deckEnableState() const { - const PlaySeconds& play = params_.play; - return instrument::ui::DeckEnableState{ - play.pitchEnv.enabled, play.filter.enabled, - play.ampSpline.mode == EnvMode::Spline, - play.pitchSpline.mode == EnvMode::Spline, - play.filterSpline.mode == EnvMode::Spline}; -} - const VelocityCurve& ReaSamplerEditor::splineFor(OverlayEnv which) const { switch (which) { case OverlayEnv::kPitch: return params_.play.pitchSpline.contour; @@ -177,12 +168,4 @@ VelocityCurve& ReaSamplerEditor::editedCurve() { return curveFor(curvePopup_); } -void ReaSamplerEditor::closeCurvePopup() { - curvePopup_ = CurveTarget::kNone; - if (drag_ == DragKind::kCurveNode) { - drag_ = DragKind::kNone; - curvePointIndex_ = -1; - } -} - } // namespace reasampler::vst diff --git a/src/shell/instrument/editor_paint_deck.cpp b/src/shell/instrument/editor_paint_deck.cpp index fd73177..3436162 100644 --- a/src/shell/instrument/editor_paint_deck.cpp +++ b/src/shell/instrument/editor_paint_deck.cpp @@ -159,9 +159,13 @@ void ReaSamplerEditor::paintDeck(LICE_IBitmap* bmp, const FaceLayout& fl) { drawToggle(t, "Off", "On", play.filter.enabled, false); break; case ParamControl::kAmpEnvMode: + drawToggle(t, "Stg", "Spl", play.ampSpline.mode == EnvMode::Spline, false); + break; case ParamControl::kPitchEnvMode: + drawToggle(t, "Stg", "Spl", play.pitchSpline.mode == EnvMode::Spline, false); + break; case ParamControl::kFilterEnvMode: - drawToggle(t, "Stg", "Spl", deckControlNorm(t.id) > 0.5, false); + drawToggle(t, "Stg", "Spl", play.filterSpline.mode == EnvMode::Spline, false); break; default: break; } diff --git a/src/shell/instrument/editor_paint_waveform.cpp b/src/shell/instrument/editor_paint_waveform.cpp index 37c0c0b..432dc0f 100644 --- a/src/shell/instrument/editor_paint_waveform.cpp +++ b/src/shell/instrument/editor_paint_waveform.cpp @@ -141,7 +141,10 @@ void ReaSamplerEditor::paintSplineOverlay(LICE_IBitmap* bmp, const OverlayArea& // handles share the coordinate system the hit-test resolves against. const LICE_pixel line = toLice(roleColor(Role::OverlayTrace)); int prevX = 0, prevY = 0; - for (int px = 0; px <= box.width; ++px) { + // < not <=: box.left + box.width is the overlay's own EXCLUSIVE right edge (the box has no + // inset, unlike the popup's), so a <= column paints one pixel into the next band's pad — + // and it is redundant with the clamped endpoint handle below anyway. + for (int px = 0; px < box.width; ++px) { const int cx = box.left + px; const double t = curve.pointFromPixel(box, cx, box.top).velocity; const int cy = curve.pixelFromPoint(box, {t, curve.eval(t)}).y; @@ -153,9 +156,18 @@ void ReaSamplerEditor::paintSplineOverlay(LICE_IBitmap* bmp, const OverlayArea& // Handles carry two independent states on the same mark, so they use two independent // channels: SIZE is the grab (the staged painter's grammar — a hotter hue reads as lower // contrast over the lime, see its note), and HOLLOW is hard. The corner itself is the - // primary hard cue, but a corner between two near-collinear segments has none to show. + // primary hard cue, but a corner between two near-collinear segments has none to show. A + // grabbed node past the drag-off margin draws WARN — the popup's same "release will + // delete" cue (editor_paint_curve.cpp), since the two spline surfaces share the drag-off + // grammar, not just the click grammar. const LICE_pixel handle = toLice(roleColor(Role::OverlayTrace)); + const LICE_pixel handleWarn = toLice(roleColor(Role::Warn)); const LICE_pixel core = toLice(roleColor(Role::BgBase)); + const bool dragOffArmed = drag_ == DragKind::kSplineNode && + (dragCurX_ < box.left - kCurveDragOffMargin || + dragCurX_ > box.left + box.width + kCurveDragOffMargin || + dragCurY_ < box.top - kCurveDragOffMargin || + dragCurY_ > box.top + box.height + kCurveDragOffMargin); const std::vector& pts = curve.points(); for (std::size_t i = 0; i < pts.size(); ++i) { const auto np = curve.pixelFromPoint(box, pts[i]); @@ -167,7 +179,8 @@ void ReaSamplerEditor::paintSplineOverlay(LICE_IBitmap* bmp, const OverlayArea& (std::min)(box.left + box.width - 1 - r, np.x)); const int hy = (std::max)(box.top + r, (std::min)(box.top + box.height - 1 - r, np.y)); - LICE_FillRect(bmp, hx - r, hy - r, 2 * r, 2 * r, handle, 1.0f, 0); + const LICE_pixel fill = (grabbed && dragOffArmed) ? handleWarn : handle; + LICE_FillRect(bmp, hx - r, hy - r, 2 * r, 2 * r, fill, 1.0f, 0); if (pts[i].hard && ir > 0) { LICE_FillRect(bmp, hx - ir, hy - ir, 2 * ir, 2 * ir, core, 1.0f, 0); } diff --git a/src/shell/instrument/editor_session.cpp b/src/shell/instrument/editor_session.cpp index 478178c..d93890c 100644 --- a/src/shell/instrument/editor_session.cpp +++ b/src/shell/instrument/editor_session.cpp @@ -157,6 +157,14 @@ bool ReaSamplerEditor::dragCommitsLive(DragKind kind, int paramId) const { return instrument::ui::liveCommitFor(k, paramId); } +void ReaSamplerEditor::closeCurvePopup() { + curvePopup_ = CurveTarget::kNone; + if (drag_ == DragKind::kCurveNode) { + drag_ = DragKind::kNone; + curvePointIndex_ = -1; + } +} + void ReaSamplerEditor::loadSelection(const std::string& id) { // A load REPLACES the loaded sound. The shaping parameters (play mode, envelopes, pitch // engine, key-track, velocity curve) are NOT reset — the one set governs whatever is diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index c0da07b..99b55c7 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -216,8 +216,7 @@ private: // Mouse-down inside curve-editor box `r`, resolved through the shared point-editing // grammar (spline_edit): a node grab starts a kCurveNode drag, an empty-space click adds a - // point and grabs it, control-click toggles a node hard/smooth. Alt-click delete predates - // that grammar and is kept as a landed alternate. + // point and grabs it, control-click toggles a node hard/smooth. void handleCurveMouseDown(const Rect& r, int x, int y); // Left-click while the curve popup is open (modal over the Sample face): Close / @@ -270,8 +269,8 @@ private: void commitLive(); // Whether an in-flight drag commits live rather than through a reload. A deck knob is - // live per isLiveDeckParam; an envelope-node drag is live only in Gate, where it edits the - // AHDSR — in Trigger the same drag rewrites the play span, which is not a live control. + // live per isLiveDeckParam; an envelope-node drag is live in EITHER mode — see + // liveCommitFor (deck_groups.h) for why. bool dragCommitsLive(DragKind kind, int paramId = -1) const; // Commits `id` as the loaded capture. The one parameter set carries over — it governs diff --git a/tests/test_component_state_io.cpp b/tests/test_component_state_io.cpp index b468e5a..75da1fd 100644 --- a/tests/test_component_state_io.cpp +++ b/tests/test_component_state_io.cpp @@ -780,6 +780,141 @@ static void testNonFiniteAhdSecondsLiftToZero() { CHECK(out.params.play.filter.trigEnv.decaySeconds == 0.0); } +// --- The v13 hard-flag tail: corruption must never widen past its own three curves ----------- + +// A hard-flag COUNT that disagrees with the curve fromPoints already built, but is still +// IN-BOUNDS (the blob really does carry that many bytes) — the documented promise +// (component_state_io.h) is that the tail is dropped, never misapplied, and nothing else in +// the record is disturbed. Corrupts only the AMP curve's tail; FILTER/PITCH follow at their +// normal, byte-precise offsets, proving a mismatch on one curve does not cascade to its +// neighbours. +static void testV13HardFlagInBoundsMismatchDropsFlagsOnly() { + ComponentState in; + in.selectionId = "pad"; + in.params.rootOverride = 44; + in.params.keyTrack = 0.6; + in.params.play.adsr.attackSeconds = 0.12; + in.params.play.adsr.sustainLevel = 0.55; + in.params.play.filter.enabled = true; + in.params.play.filter.settings.cutoffNorm = 0.4f; + in.params.play.filter.modAmount = -0.3; + in.params.play.pitchEnv.enabled = true; + in.params.play.pitchEnv.peakSemitones = 5.0; + in.params.loopCrossfadeFrames = 777; + in.params.play.pitchVelocityCurve = reasampler::instrument::engine::VelocityCurve::fromPoints( + {VelocityPoint{0.0, -0.4}, VelocityPoint{127.0, 0.4}}, + reasampler::instrument::engine::CurveDomain::Bipolar); + // Every velocity curve left at its DEFAULT 2-point shape, so the v13 hard-flag tail's byte + // layout (three 4-byte-count + N-byte blocks, amp/filter/pitch order — putHardFlags' call + // order in params_payload.cpp) is deterministic and this test can splice it exactly. + + std::vector bytes = serializeComponentState(in); + CHECK(bytes.size() >= 18); + bytes.resize(bytes.size() - 18); // drop the three well-formed 4+2-byte blocks + legacy::u32v(bytes, 5); // amp: bogus count... + for (int i = 0; i < 5; ++i) legacy::u8v(bytes, 0); // ...with 5 REAL bytes, so nothing shifts + legacy::u32v(bytes, 2); // filter: correct count, unchanged + legacy::u8v(bytes, 0); + legacy::u8v(bytes, 0); + legacy::u32v(bytes, 2); // pitch: correct count, unchanged + legacy::u8v(bytes, 0); + legacy::u8v(bytes, 0); + + const ComponentState out = deserializeComponentState(bytes, 48000.0); + // Every param preceding AND following the corrupted amp tail survives untouched. + CHECK(out.selectionId == "pad"); + CHECK(out.params.rootOverride && *out.params.rootOverride == 44); + CHECK(out.params.keyTrack == 0.6); + CHECK(out.params.play.adsr.attackSeconds == 0.12); + CHECK(out.params.play.adsr.sustainLevel == 0.55); + CHECK(out.params.play.filter.enabled); + CHECK(out.params.play.filter.settings.cutoffNorm == 0.4f); + CHECK(out.params.play.filter.modAmount == -0.3); + CHECK(out.params.play.pitchEnv.enabled); + CHECK(out.params.play.pitchEnv.peakSemitones == 5.0); + CHECK(out.params.loopCrossfadeFrames == 777); + CHECK(out.params.play.pitchVelocityCurve.eval(0.0) == -0.4); + CHECK(out.params.play.pitchVelocityCurve.eval(127.0) == 0.4); + // The mismatched (amp) curve keeps its points; the flags are dropped, never misapplied. + CHECK(out.params.velocityCurve.size() == 2); + CHECK(!out.params.velocityCurve.points()[0].hard); + CHECK(!out.params.velocityCurve.points()[1].hard); + CHECK(out.params.velocityCurve.equals(reasampler::instrument::engine::VelocityCurve::flat())); +} + +// A hard-flag COUNT that exceeds what its OWN tail carries — a genuinely corrupt/out-of-bounds +// count — must be BOUND-AND-SKIPPED without consuming any of the following bytes, so the +// FILTER/PITCH tails immediately after the AMP block still parse at their correct offset. The +// old behavior (r.ok = false) reset the ENTIRE params record to defaults on this path, which is +// strictly worse than the documented "drops only the hard points" promise. +static void testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord() { + ComponentState in; + in.selectionId = "pad"; + in.params.rootOverride = 44; + in.params.play.adsr.releaseSeconds = 0.44; + in.params.play.filter.enabled = true; + in.params.play.filter.settings.resonanceNorm = 0.9f; + in.params.loopCrossfadeFrames = 321; + + std::vector bytes = serializeComponentState(in); + CHECK(bytes.size() >= 18); + bytes.resize(bytes.size() - 18); // drop the three well-formed hard-flag blocks + legacy::u32v(bytes, 1000); // amp: a count its own tail cannot possibly carry + // No amp flag bytes follow — bound-and-skip must consume none, so the well-formed + // filter/pitch blocks right after it land exactly where they belong. + legacy::u32v(bytes, 2); // filter: correct count, unchanged + legacy::u8v(bytes, 0); + legacy::u8v(bytes, 0); + legacy::u32v(bytes, 2); // pitch: correct count, unchanged + legacy::u8v(bytes, 0); + legacy::u8v(bytes, 0); + + const ComponentState out = deserializeComponentState(bytes, 48000.0); + // The whole record survives — including everything the v13 section itself carries ahead of + // the hard-flag tail (the three spline EGs) and the two well-formed tails after the + // corrupted one — only the AMP curve's hard-flag application is lost. + CHECK(out.selectionId == "pad"); + CHECK(out.params.rootOverride && *out.params.rootOverride == 44); + CHECK(out.params.play.adsr.releaseSeconds == 0.44); + CHECK(out.params.play.filter.enabled); + CHECK(out.params.play.filter.settings.resonanceNorm == 0.9f); + CHECK(out.params.loopCrossfadeFrames == 321); + CHECK(out.params.play.ampSpline.mode == EnvMode::Staged); + CHECK(out.params.velocityCurve.size() == 2); // unaffected: not misapplied, not discarded + CHECK(!out.params.velocityCurve.points()[0].hard); +} + +// A hard-flag tail truncated mid-COUNT-FIELD (only 2 of its 4 length bytes present, and +// nothing else after) is a different failure shape than a declared-huge count: the u32 read +// itself fails, tripping r.ok inside readHardFlags rather than its own bound check. That must +// be revived the same way — the whole record survives, only the hard-flag applications (on +// all three curves, since the truncation strands every one of them) are lost. +static void testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord() { + ComponentState in; + in.selectionId = "pad"; + in.params.rootOverride = 21; + in.params.play.adsr.decaySeconds = 0.08; + in.params.play.pitchEnv.enabled = true; + in.params.play.pitchEnv.peakSemitones = -3.0; + in.params.loopCrossfadeFrames = 5; + + std::vector bytes = serializeComponentState(in); + CHECK(bytes.size() >= 18); + bytes.resize(bytes.size() - 18); // drop the three well-formed hard-flag blocks + legacy::u8v(bytes, 0x02); // half of the amp tail's 4-byte LE count, then nothing + legacy::u8v(bytes, 0x00); + + const ComponentState out = deserializeComponentState(bytes, 48000.0); + CHECK(out.selectionId == "pad"); + CHECK(out.params.rootOverride && *out.params.rootOverride == 21); + CHECK(out.params.play.adsr.decaySeconds == 0.08); + CHECK(out.params.play.pitchEnv.enabled); + CHECK(out.params.play.pitchEnv.peakSemitones == -3.0); + CHECK(out.params.loopCrossfadeFrames == 5); + CHECK(out.params.velocityCurve.size() == 2); + CHECK(!out.params.velocityCurve.points()[0].hard); +} + // --- The loop tail (payload v11) --------------------------------------------- // The loop span and its crossfade survive a save/reload intact, alongside the two overrides @@ -1605,6 +1740,9 @@ int main() { testPitchVelocityCurveRoundTripsIndependently(); testNonFiniteFilterFieldsLiftToTheNeutralDefault(); testNonFiniteAhdSecondsLiftToZero(); + testV13HardFlagInBoundsMismatchDropsFlagsOnly(); + testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord(); + testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord(); if (failures == 0) { std::printf("component_state_io_tests: all tests passed\n"); return 0; diff --git a/tests/test_knob_deck.cpp b/tests/test_knob_deck.cpp index e155065..d6fa6fa 100644 --- a/tests/test_knob_deck.cpp +++ b/tests/test_knob_deck.cpp @@ -205,6 +205,41 @@ static void testInnerDialHit() { CHECK(h.kind == DeckHitKind::Knob && h.id == c.id && !h.inner); } +// captionToggle2 sits immediately left of captionToggle when both are present (no overlap, and +// the caption text stops before the LEFTMOST one), and takes captionToggle's own slot when +// captionToggle is absent — the shipped FILTER ENV group's exact shape (deck_groups.cpp). +static void testCaptionToggle2() { + const DeckGroupDesc both{9, 40, {}, {300, 30}, {301, 20}, {1, 2, 3}, {}}; + std::vector g{both}; + const DeckLayout dl = layoutDeck(g, 0, 0, 800); + const DeckGroupLayout& lay = dl.groups[0]; + CHECK(lay.captionToggle.id == 300); + CHECK(lay.captionToggle2.id == 301); + CHECK(lay.captionToggle2.seg0.width == 20 && lay.captionToggle2.seg1.width == 20); + // Left of the first, with exactly one gap between — no overlap by construction. + CHECK(lay.captionToggle2.seg1.right() == lay.captionToggle.seg0.x - kDeckToggleGap); + // Caption text stops before the LEFTMOST toggle (toggle2), not just the first-placed one. + CHECK(lay.caption.right() <= lay.captionToggle2.seg0.x); + + DeckHit h = hitTestDeck(dl, lay.captionToggle2.seg0.right() - 1, + lay.captionToggle2.seg0.y + 1); + CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 301 && h.segment == 0); + h = hitTestDeck(dl, lay.captionToggle2.seg1.x, lay.captionToggle2.seg1.y + 1); + CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 301 && h.segment == 1); + + // FILTER ENV's real shape: captionToggle absent, captionToggle2 present with a radio — it + // takes the first (rightmost) slot rather than leaving a gap where captionToggle would sit. + const DeckGroupDesc filterEnvLike{10, 66, {200}, {}, {302, 23}, {1, 2, 3, 4, 5}, {}}; + std::vector g2{filterEnvLike}; + const DeckLayout dl2 = layoutDeck(g2, 0, 0, 800); + const DeckGroupLayout& fe = dl2.groups[0]; + CHECK(fe.captionToggle.id == -1); + CHECK(fe.captionToggle2.id == 302); + CHECK(fe.captionToggle2.seg1.right() == fe.captionRadio.box.x - kDeckToggleGap); + h = hitTestDeck(dl2, fe.captionToggle2.seg1.x, fe.captionToggle2.seg1.y + 1); + CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 302 && h.segment == 1); +} + static void testEmptyDeck() { const std::vector none; CHECK(deckRowCount(none, 800) == 0); @@ -221,6 +256,7 @@ int main() { testHitTest(); testCaptionRadioGeometryAndHit(); testInnerDialHit(); + testCaptionToggle2(); testEmptyDeck(); if (g_fail) { std::printf("%d FAILURE(S)\n", g_fail); diff --git a/tests/test_spline_egs.cpp b/tests/test_spline_egs.cpp index 4d82990..5e52b60 100644 --- a/tests/test_spline_egs.cpp +++ b/tests/test_spline_egs.cpp @@ -341,21 +341,46 @@ static void testAFreshSplineEgDefaultsToTheSmoothDownwardSlope() { // The rule has one home (splineActive) and one enforcement point on the way to the engine // (resolvePlay). The editor's Gate segment refuses and paints Disabled off the same predicate. +// +// Pitch and filter additionally gate on their own `enabled` flag, matching Voice::start's +// binder (voice.cpp only binds pitchSplineCur_/filterSplineCur_ under that same condition): a +// Spline mode flip on a still-disabled envelope produces no modulation, so it must not cost +// Gate either — the predicate and the binder must agree on one enable rule. Amp has no such +// flag and counts on its mode alone. static void testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards() { PlaySeconds stored; stored.playMode = PlayMode::Gate; CHECK(!splineActive(stored)); CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); - for (EnvMode* slot : {&stored.ampSpline.mode, &stored.pitchSpline.mode, - &stored.filterSpline.mode}) { - *slot = EnvMode::Spline; - CHECK(splineActive(stored)); - CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger); - *slot = EnvMode::Staged; - CHECK(!splineActive(stored)); - CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); - } + stored.ampSpline.mode = EnvMode::Spline; + CHECK(splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger); + stored.ampSpline.mode = EnvMode::Staged; + CHECK(!splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); + + stored.pitchSpline.mode = EnvMode::Spline; + CHECK(!splineActive(stored)); // pitchEnv.enabled is still false: no modulation, no cost + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); + stored.pitchEnv.enabled = true; + CHECK(splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger); + stored.pitchSpline.mode = EnvMode::Staged; + stored.pitchEnv.enabled = false; + CHECK(!splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); + + stored.filterSpline.mode = EnvMode::Spline; + CHECK(!splineActive(stored)); // filter.enabled is still false: the filter is fully off + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); + stored.filter.enabled = true; + CHECK(splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger); + stored.filterSpline.mode = EnvMode::Staged; + stored.filter.enabled = false; + CHECK(!splineActive(stored)); + CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate); // And the staged knobs of a drawn envelope go inert — drawn-but-dead, not removed — while // its depth knob, which scales either shape, stays live. @@ -407,6 +432,32 @@ static void testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete() { CHECK(!amp.deletePoint(1)); } +// --- 12. SplineCursor's binary-search branch agrees with the cold reader ------ + +// Test 8 only walks a monotone forward read, which never leaves SplineCursor::locate's +// select(seg_+1) fast path. A backwards/jumping read forces the actual binary search — and at +// a duplicate-X knot (a drawn step) the RT cursor must resolve to the SAME point the cold +// VelocityCurve::eval() would, or a backwards read audibly steps to the wrong side of the step. +static void testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead() { + // A step at x=64: two knots sharing an X but different Y. + VelocityCurve c = VelocityCurve::fromPoints( + {{0.0, 0.1}, {32.0, 0.3}, {64.0, 0.9}, {64.0, 0.2}, {96.0, 0.6}, {127.0, 0.4}}, + CurveDomain::Unipolar); + SplineCursor cur; + cur.bind(c); + + // Deliberately out of order, so every eval but the first forces locate()'s binary search + // rather than the forward-walk fast path. + const double xs[] = {100.0, 10.0, 64.0, 40.0, 64.0, 5.0, 127.0, 20.0, 0.0, 90.0}; + for (double x : xs) { + const double phase = x / kCurveXMax; + CHECK(near(cur.eval(phase), c.eval(x), 1e-6)); + } + // The duplicate knot itself: both readers resolve to the SAME one (the first, per + // VelocityCurve::eval's "first containing segment" rule). + CHECK(near(cur.eval(64.0 / kCurveXMax), 0.9, 1e-6)); +} + int main() { testHardPointGivesDifferingOneSidedSlopes(); testNoOvershootBetweenAnyAdjacentPairOnARiseAndFallContour(); @@ -419,6 +470,7 @@ int main() { testAFreshSplineEgDefaultsToTheSmoothDownwardSlope(); testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards(); testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete(); + testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead(); if (g_fail == 0) std::printf("spline_egs: all tests passed\n"); return g_fail == 0 ? 0 : 1; }