From cfb53aade3de264bc75f96b485ad1d784cefb041 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Fri, 31 Jul 2026 19:46:37 -0400 Subject: [PATCH] fix: close review findings on the velocity-curve deck and bipolar curves Cancels the curve-node drag whenever the popup closes so Esc mid-drag can't alias the amp curve; generalizes CurveTarget routing to one switch; fixes stale/overstated comments; clamps a pre-v12 depth fold; adds deck-inertness and filter-fold test coverage. --- src/core/instrument/CLAUDE.md | 2 +- src/core/instrument/engine/velocity_curve.cpp | 6 ++- src/core/instrument/engine/velocity_curve.h | 17 ++++++-- src/core/instrument/map/component_state_io.h | 19 +++++---- src/core/instrument/map/params_payload.cpp | 7 +++- src/core/instrument/ui/deck_groups.cpp | 30 ++++++++++++++ src/core/instrument/ui/deck_groups.h | 13 +++++- src/core/instrument/ui/sample_bands.h | 2 +- src/shell/instrument/editor_controls.cpp | 36 +++++++++++++--- src/shell/instrument/editor_input_browse.cpp | 7 +++- src/shell/instrument/editor_input_curve.cpp | 4 +- src/shell/instrument/editor_input_deck.cpp | 31 ++------------ src/shell/instrument/editor_paint_chrome.cpp | 11 ++--- src/shell/instrument/editor_paint_curve.cpp | 15 +------ src/shell/instrument/editor_paint_deck.cpp | 2 +- src/shell/instrument/editor_session.cpp | 2 +- src/shell/instrument/reasampler_editor.h | 21 +++++++--- src/shell/panel/draw_kit.cpp | 11 ++--- src/shell/panel/draw_kit.h | 5 +++ tests/test_component_state_io.cpp | 41 ++++++++++++++++--- tests/test_curve_popup.cpp | 22 ++++++---- tests/test_deck_groups.cpp | 18 ++++++++ 22 files changed, 223 insertions(+), 99 deletions(-) diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index 80d8650..ea4b95e 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -262,7 +262,7 @@ anything for a trigger shape. - `voice_engine.h` / `voice_engine.cpp` — `VoiceEngine`: note routing, bounded-stealing allocation, user-parameterized voice count (1–32, default 16), `VoiceMode` Poly/Mono (last-note held-note stack, `MonoTrigger` Retrigger/Legato), two-tier panic (CC 123 = all-notes-off release, CC 120 = immediate hard-stop including Trigger one-shots), and the block render loops. Preview injects a synthetic note-on at the loaded capture's root note into the main `VoiceEngine` — no dedicated `PreviewCard`; preview obeys polyphony/mono/voice-stealing/envelopes. - `engine/loop/` — the sustain loop's ONE validity/clamp fold (`resolveLoop`) plus its pre-seam crossfade geometry and the editor's default handle span; see `engine/loop/CLAUDE.md`. The voice folds it once at note-on; the crossfade weight is header-inline because it rides the per-sample read. - `pitch_shift` — hand-rolled **correlation-aligned SOLA** (splice-overlap-add) pitch shifter for the Preserve playback mode: one active read tap chases the write head at the shift ratio; each splice jump is refined by a cross-correlation search so the new read point is waveform-aligned, then old and new taps are crossfaded (raised-cosine, amplitude-complementary). Replaces the prior dual-tap OLA whose fixed half-window tap offset caused anti-phase cancellation on many source frequencies. **GA2:** ring buffer **primed with the actual upcoming source** at note-on (was zero-filled) → gap-free frame-0 onset, ~25 ms Preserve onset latency eliminated (Preserve now speaks on frame 0, matching Varispeed), and real-content-bounded tail (last-window tail-truncation gone). No third-party dependencies; RT-discipline: no allocation in `process()`. -- `velocity_curve` — the pure velocity transfer curve shared by all THREE destinations: `VelocityCurve` evaluated by a Fritsch–Carlson monotone cubic Hermite spline (no overshoot). `eval(velocity)` called once per note-on. It carries its own y `CurveDomain`: UNIPOLAR [0,1] is the amp's GAIN, defaulting to `flat()` (y=1, every velocity→unity — a deliberate non-back-compat replacement of the old fixed `velocity/127` path, Daniel-approved); BIPOLAR [−1,1] is the signed modulation depth for pitch and filter, defaulting to `zero()` so velocity modulates neither until a curve is drawn. A bipolar curve is BOTH the shape and the amount — there is no depth control behind it, which is why the filter's `velAmount` retired into it. eval is HOMOGENEOUS in y (scaling every knot by k scales the curve by k exactly); the codec's pre-v12 lift rests on that. +- `velocity_curve` — the pure velocity transfer curve shared by all THREE destinations: `VelocityCurve` evaluated by a Fritsch–Carlson monotone cubic Hermite spline (no overshoot). `eval(velocity)` called once per note-on. It carries its own y `CurveDomain`: UNIPOLAR [0,1] is the amp's GAIN, defaulting to `flat()` (y=1, every velocity→unity — a deliberate non-back-compat replacement of the old fixed `velocity/127` path, Daniel-approved); BIPOLAR [−1,1] is the signed modulation depth for pitch and filter, defaulting to `zero()` so velocity modulates neither until a curve is drawn. A bipolar curve is BOTH the shape and the amount — there is no depth control behind it, which is why the filter's `velAmount` retired into it. eval's homogeneity in y (see `velocity_curve.h`) is what the codec's pre-v12 lift rests on. - `master_gain` — pure dB↔linear taper math (FB1): normalized [0,1] ↔ dB ↔ linear for the post-mixer master gain control (−∞…+24 dB, norm 0 = true silence, unity ≈ 0.714). Shared by the editor knob and the processor multiply so the needle, persisted value, and audio multiply cannot drift. ### `map/` diff --git a/src/core/instrument/engine/velocity_curve.cpp b/src/core/instrument/engine/velocity_curve.cpp index 37a4d73..5eb1079 100644 --- a/src/core/instrument/engine/velocity_curve.cpp +++ b/src/core/instrument/engine/velocity_curve.cpp @@ -43,7 +43,8 @@ int valueToY(const VelocityCurve::Box& box, double value, CurveDomain d) { VelocityCurve VelocityCurve::flat() { VelocityCurve c; - c.points_ = {{kVelMin, kCurveYMax}, {kVelMax, kCurveYMax}}; + const double n = curveNeutral(CurveDomain::Unipolar); + c.points_ = {{kVelMin, n}, {kVelMax, n}}; return c; } @@ -56,7 +57,8 @@ VelocityCurve VelocityCurve::linear() { VelocityCurve VelocityCurve::zero() { VelocityCurve c; c.domain_ = CurveDomain::Bipolar; - c.points_ = {{kVelMin, 0.0}, {kVelMax, 0.0}}; + const double n = curveNeutral(CurveDomain::Bipolar); + c.points_ = {{kVelMin, n}, {kVelMax, n}}; return c; } diff --git a/src/core/instrument/engine/velocity_curve.h b/src/core/instrument/engine/velocity_curve.h index 12fd9b9..54ec54b 100644 --- a/src/core/instrument/engine/velocity_curve.h +++ b/src/core/instrument/engine/velocity_curve.h @@ -25,8 +25,10 @@ enum class CurveDomain { Unipolar, Bipolar }; constexpr double curveYMin(CurveDomain d) { return d == CurveDomain::Bipolar ? -1.0 : 0.0; } -// The value that changes nothing in each domain — unity gain, or zero modulation. Every -// defensive fallback lands here so a corrupt blob loses the shaping rather than inventing one. +// The value that changes nothing in each domain — unity gain, or zero modulation. THE one home +// for that value: eval()'s own empty-curve fallback reads it directly, and flat()/zero() (what +// fromPoints' sub-2-point fallback constructs) are built from it too, so a corrupt blob always +// loses the shaping rather than inventing one, however the fallback is reached. constexpr double curveNeutral(CurveDomain d) { return d == CurveDomain::Bipolar ? 0.0 : 1.0; } // A raw-constructed point is NOT auto-clamped (the mutators own that invariant) — build curves @@ -45,7 +47,10 @@ inline constexpr int kCurveNodeGrabRadius = 6; // slope, so the spline reproduces linear()'s straight line to within ~1e-15. The two endpoints // (velocity 0 and 127) are load-bearing: they keep eval total over the domain and are never // deletable. eval is HOMOGENEOUS in y — scaling every knot's value by k scales the whole curve -// by k exactly, which is what lets the codec fold a retired depth control into stored knots. +// by k TO WITHIN DOUBLE ROUNDING (the Hermite basis and the Fritsch-Carlson tangent are exactly +// degree-1 homogeneous in real arithmetic; `fl(k*b) - fl(k*a)` isn't bit-identical to +// `k*(b-a)`), which is what lets the codec (component_state_io.h's v12 pre-lift) fold a retired +// depth control into stored knots and still sound identical. class VelocityCurve { public: // flat() (endpoints (0,1)/(127,1), every velocity -> unity) is the unipolar default — see @@ -117,6 +122,12 @@ public: bool equals(const VelocityCurve& other, double eps = 1e-9) const; private: + // Private: an implicit-default curve is empty (no endpoints) and Unipolar, so a stray + // default-construction wouldn't fail loudly — it would eval() to unity gain everywhere, + // or a full +/-1 (a full-scale transpose / wide-open filter) if ever read as bipolar. Build + // through flat()/linear()/zero()/fromPoints(), all of which establish the endpoint invariant. + VelocityCurve() = default; + // Always X-ordered with an endpoint at 0 and 127; constructors + deserialize establish the // invariant, mutators preserve it. std::vector points_; diff --git a/src/core/instrument/map/component_state_io.h b/src/core/instrument/map/component_state_io.h index 3472735..db71549 100644 --- a/src/core/instrument/map/component_state_io.h +++ b/src/core/instrument/map/component_state_io.h @@ -90,18 +90,21 @@ namespace reasampler::instrument::map { // prefix and lifts to 0 — the hard seam it always played. // // v12 (CURRENT WRITE FORMAT) is v11 PLUS the velocity->PITCH transfer curve (count + points, -// the same shape as v7's), appended after the loop crossfade. It also RE-INTERPRETS two frozen -// slots inside the v9 filter tail — the byte shape is untouched, only the meaning at v12+: +// the same shape as v7's), appended after the loop crossfade. Its y is a normalized fraction +// of kVelocityPitchRangeSemitones (play_params.h) — a full-scale constant that lives OUTSIDE +// this frozen ladder, so retuning it re-tunes every saved v12 project's pitch-curve throw. It +// also RE-INTERPRETS two frozen slots inside the v9 filter tail — the byte shape is untouched, +// only the meaning at v12+: // * the filter's velocity curve is now BIPOLAR [-1,+1] and is the whole velocity->cutoff // amount, not a [0,1] shape scaled by a separate depth; // * the retired filter velAmount slot is written as a constant 1.0 and ignored on read. // PRE-v12 LIFT: the stored [0,1] filter curve has every knot's y multiplied by that blob's -// velAmount and is re-read as bipolar. eval is homogeneous in y, so the lifted curve evaluates -// to exactly velAmount * oldCurve(v) — the product the voice used to compute per note — and a -// pre-v12 project sounds identical. A pre-v12 blob carries no pitch curve at all and lifts to -// the bipolar flat-at-zero default, which transposes nothing. A DOWNGRADE to a pre-v12 binary -// reads the constant 1.0 depth against a curve whose negative half clamps away, so it -// reproduces the curve's positive half only. +// velAmount and is re-read as bipolar. eval is homogeneous in y to within double rounding (see +// velocity_curve.h), so the lifted curve evaluates to velAmount * oldCurve(v) — the product the +// voice used to compute per note — and a pre-v12 project sounds identical. A pre-v12 blob +// carries no pitch curve at all and lifts to the bipolar flat-at-zero default, which transposes +// nothing. A DOWNGRADE to a pre-v12 binary reads the constant 1.0 depth against a curve whose +// negative half clamps away, so it reproduces the curve's positive half only. // // The two int64 slots the v5 play tail spends on the RETIRED Trigger fade pair are frozen in // shape and still read: a pre-v10 blob's fade-in/fade-out become the Trigger AHD that replaced diff --git a/src/core/instrument/map/params_payload.cpp b/src/core/instrument/map/params_payload.cpp index a465735..fdf8144 100644 --- a/src/core/instrument/map/params_payload.cpp +++ b/src/core/instrument/map/params_payload.cpp @@ -140,8 +140,13 @@ void readFilterTail(ByteReader& r, InstrumentParams& p, bool preVelocityVersion) f.env.decaySeconds = bitsToDouble(r.u64()); f.env.sustainLevel = bitsToDouble(r.u64()); f.env.releaseSeconds = bitsToDouble(r.u64()); + // velAmount feeds a MULTIPLIER on the stored curve's knots (below), not a param the engine + // clamps on its own — the UI never dials it outside [-1,1] (deckBipolarFromNorm), so a + // corrupt-but-finite blob value outside that range must clamp here rather than silently + // scaling the lifted curve past what fromPoints' own [-1,1] box-clamp would then truncate. const double velFold = - preVelocityVersion ? (std::isfinite(velAmount) ? velAmount : 0.0) : 1.0; + preVelocityVersion ? std::clamp(std::isfinite(velAmount) ? velAmount : 0.0, -1.0, 1.0) + : 1.0; readCurveTail(r, f.velocityCurve, reasampler::instrument::engine::CurveDomain::Bipolar, velFold); } diff --git a/src/core/instrument/ui/deck_groups.cpp b/src/core/instrument/ui/deck_groups.cpp index 4b2517e..667be3b 100644 --- a/src/core/instrument/ui/deck_groups.cpp +++ b/src/core/instrument/ui/deck_groups.cpp @@ -238,6 +238,36 @@ bool overlayEnvInert(OverlayEnv env, bool pitchEnvEnabled, bool filterEnabled) { return false; // unreachable for a valid enumerator; silences a warning. } +bool deckKnobInert(DeckParam id, bool pitchEnvEnabled, bool filterEnabled) { + switch (id) { + case DeckParam::kPitchEnvAttack: + case DeckParam::kPitchEnvHold: + case DeckParam::kPitchEnvDecay: + case DeckParam::kPitchEnvDepth: + return !pitchEnvEnabled; + case DeckParam::kFilterMorph: + case DeckParam::kFilterCutoff: + case DeckParam::kFilterQ: + case DeckParam::kFilterDrive: + case DeckParam::kFilterModAmt: + case DeckParam::kFilterKeyTrack: + // The filter's velocity curve sits in the VELOCITY group but is a filter parameter: + // it goes inert with every other one, so no surface can reach a param the knobs can't. + case DeckParam::kFilterVelCurve: + case DeckParam::kFilterEnvAttack: + case DeckParam::kFilterEnvHold: + case DeckParam::kFilterEnvDecay: + case DeckParam::kFilterEnvSustain: + case DeckParam::kFilterEnvRelease: + case DeckParam::kFilterTrigAttack: + case DeckParam::kFilterTrigHold: + case DeckParam::kFilterTrigDecay: + return !filterEnabled; + default: + return false; + } +} + bool liveCommitFor(LiveDragKind kind, int paramId) { switch (kind) { case LiveDragKind::kDeckKnob: diff --git a/src/core/instrument/ui/deck_groups.h b/src/core/instrument/ui/deck_groups.h index 4741a03..2aa2fd3 100644 --- a/src/core/instrument/ui/deck_groups.h +++ b/src/core/instrument/ui/deck_groups.h @@ -96,8 +96,10 @@ enum DeckGroupId { }; // Which velocity curve a deck cell edits, or kNone when the control is an ordinary knob. THE -// one place the three curve cells are named, so paint (draw a curve thumbnail, not a dial), -// hit-test (open a popup, not start a drag) and the popup's own title all read from it. +// one place a control id resolves to a curve target — paint (draw a curve thumbnail, not a +// dial) and hit-test (open a popup, not start a drag) both read this predicate rather than +// re-deriving which ids are curve cells. What each resolved target then shows (which stored +// curve, which title) is a separate switch — see the shell's curveFor/curveTitle. enum class CurveTarget { kNone, kAmp, kPitch, kFilter }; CurveTarget curveTargetFor(int controlId); @@ -163,6 +165,13 @@ OverlayEnv nextOverlaySelection(OverlayEnv current, int radioId); // a param a knob couldn't (envelope_edit.h). Amp has no enable toggle and is never inert. bool overlayEnvInert(OverlayEnv env, bool pitchEnvEnabled, bool filterEnabled); +// Whether a deck knob cell is drawn-but-dead: the pitch envelope's four knobs while it is +// disabled, and the filter group's tone/modulation knobs (plus its VELOCITY cell, a filter +// parameter that just sits in that group) while the filter is disabled. Every other id is +// always live. Mirrors overlayEnvInert's group-toggle-gates-its-knobs shape for the deck's own +// mouse-down/paint (the shell's deckKnobDisabled is a thin int-id wrapper over this). +bool deckKnobInert(DeckParam id, bool pitchEnvEnabled, bool filterEnabled); + // The deck's BIPOLAR knob law: 0.5 of the knob's travel is zero depth, the ends are -1 and // +1. Exact inverses, and exact at the centre detent (0.5 -> 0 -> 0.5), so a knob parked at // centre can never persist a hair of modulation. Out-of-range norm clamps to the endpoints. diff --git a/src/core/instrument/ui/sample_bands.h b/src/core/instrument/ui/sample_bands.h index 69de15a..ef5c812 100644 --- a/src/core/instrument/ui/sample_bands.h +++ b/src/core/instrument/ui/sample_bands.h @@ -20,7 +20,7 @@ inline constexpr int kEditorMinWidth = 840; inline constexpr int kEditorMinHeight = 620; // Chrome band: the toolbar row (title + nav) stacked over the control row (piano strip, -// preview, velocity knob, curve button, channel toggle). sample_chrome partitions it. +// preview, velocity knob, channel toggle). sample_chrome partitions it. inline constexpr int kTitleHeight = 26; inline constexpr int kChromeRowHeight = 52; diff --git a/src/shell/instrument/editor_controls.cpp b/src/shell/instrument/editor_controls.cpp index 868d1a7..708be0e 100644 --- a/src/shell/instrument/editor_controls.cpp +++ b/src/shell/instrument/editor_controls.cpp @@ -53,7 +53,9 @@ namespace { // The ceiling is READ from the overlay's schematic scale rather than restated: the AHDSR // schematic anchors a maxed knob at the canvas edge, which only holds while the two agree. constexpr double kEnvTimeMaxSeconds = instrument::ui::kGateStageMaxSeconds; -constexpr double kPitchDepthMaxSemis = 24.0; // pitch depth throw: +/-24 st, centered +// Pitch depth throw: +/-kVelocityPitchRangeSemitones, centered. The one throw the pitch +// envelope's peak and the velocity->pitch curve's full scale both speak (play_params.h). +constexpr double kPitchDepthMaxSemis = kVelocityPitchRangeSemitones; constexpr double kKeyTrackMax = 2.0; // key-track slider ceiling (0..200%) // The raw stored curve exponent for a curve-dial control id, read DIRECTLY off the field — @@ -510,21 +512,43 @@ void ReaSamplerEditor::unpackEnvelope(OverlayEnv which, const StageEnvelope& env } } -const VelocityCurve& ReaSamplerEditor::editedCurve() const { - switch (curvePopup_) { +const VelocityCurve& ReaSamplerEditor::curveFor(CurveTarget target) const { + switch (target) { case CurveTarget::kPitch: return params_.play.pitchVelocityCurve; case CurveTarget::kFilter: return params_.play.filter.velocityCurve; case CurveTarget::kAmp: case CurveTarget::kNone: - // kNone only reaches here from a paint/hover racing the close; the amp curve is a - // valid, harmless read rather than a branch every caller would have to repeat. + // kNone reads as amp — a valid, harmless choice for a target-agnostic caller (a + // disabled deck cell still needs SOME curve to draw). editedCurve()'s mutable + // overload below refuses kNone rather than relying on this fallback to protect amp. return params_.velocityCurve; } return params_.velocityCurve; } +VelocityCurve& ReaSamplerEditor::curveFor(CurveTarget target) { + return const_cast(std::as_const(*this).curveFor(target)); +} + +const VelocityCurve& ReaSamplerEditor::editedCurve() const { return curveFor(curvePopup_); } + VelocityCurve& ReaSamplerEditor::editedCurve() { - return const_cast(std::as_const(*this).editedCurve()); + // Refuses kNone rather than aliasing amp (see curvePopup_'s declaration for why); should be + // unreachable now that every writer of kNone also cancels the drag, but this is the second, + // independent line of defense. + if (curvePopup_ == CurveTarget::kNone) { + static VelocityCurve sink = VelocityCurve::flat(); + return sink; + } + return curveFor(curvePopup_); +} + +void ReaSamplerEditor::closeCurvePopup() { + curvePopup_ = CurveTarget::kNone; + if (drag_ == DragKind::kCurveNode) { + drag_ = DragKind::kNone; + curvePointIndex_ = -1; + } } void ReaSamplerEditor::applyParamControl(int id, double value, int segment) { diff --git a/src/shell/instrument/editor_input_browse.cpp b/src/shell/instrument/editor_input_browse.cpp index d871d79..50932ce 100644 --- a/src/shell/instrument/editor_input_browse.cpp +++ b/src/shell/instrument/editor_input_browse.cpp @@ -131,9 +131,12 @@ void ReaSamplerEditor::onMouseWheel(int delta) { void ReaSamplerEditor::onSearchChar(unsigned int ch) { // The curve popup: Esc dismisses (checked first — the popup is modal over the face, and - // the Browse search cannot hold focus under it). + // the Browse search cannot hold focus under it). Esc reaches here unconditionally + // (editor_platform's WM_CHAR routing), including mid-drag on a curve node — closeCurvePopup + // cancels that drag too, or a subsequent WM_MOUSEMOVE would resolve editedCurve() with the + // popup already closed. if (curvePopup_ != CurveTarget::kNone && ch == 27) { - curvePopup_ = CurveTarget::kNone; + closeCurvePopup(); invalidate(); return; } diff --git a/src/shell/instrument/editor_input_curve.cpp b/src/shell/instrument/editor_input_curve.cpp index 9a0686e..8990c5e 100644 --- a/src/shell/instrument/editor_input_curve.cpp +++ b/src/shell/instrument/editor_input_curve.cpp @@ -23,7 +23,7 @@ bool ReaSamplerEditor::handlePopupMouseDown(int w, int h, int x, int y) { if (curvePopup_ == CurveTarget::kNone) return false; const CurvePopupLayout pl = computeCurvePopup(w, h); if (contains(pl.close, x, y)) { - curvePopup_ = CurveTarget::kNone; + closeCurvePopup(); invalidate(); return true; } @@ -32,7 +32,7 @@ bool ReaSamplerEditor::handlePopupMouseDown(int w, int h, int x, int y) { return true; } if (popupOutsideSheet(pl, x, y) && drag_ == DragKind::kNone) { - curvePopup_ = CurveTarget::kNone; + closeCurvePopup(); invalidate(); } return true; diff --git a/src/shell/instrument/editor_input_deck.cpp b/src/shell/instrument/editor_input_deck.cpp index 8dd5aa0..41a36e0 100644 --- a/src/shell/instrument/editor_input_deck.cpp +++ b/src/shell/instrument/editor_input_deck.cpp @@ -17,33 +17,10 @@ using namespace reasampler::ui; using namespace reasampler::instrument::ui; bool ReaSamplerEditor::deckKnobDisabled(int id) const { - switch (static_cast(id)) { - case ParamControl::kPitchEnvAttack: - case ParamControl::kPitchEnvHold: - case ParamControl::kPitchEnvDecay: - case ParamControl::kPitchEnvDepth: - return !params_.play.pitchEnv.enabled; - case ParamControl::kFilterMorph: - case ParamControl::kFilterCutoff: - case ParamControl::kFilterQ: - case ParamControl::kFilterDrive: - case ParamControl::kFilterModAmt: - case ParamControl::kFilterKeyTrack: - // The filter's velocity curve sits in the VELOCITY group but is a filter parameter: - // it goes inert with every other one, so no surface can reach a param the knobs can't. - case ParamControl::kFilterVelCurve: - case ParamControl::kFilterEnvAttack: - case ParamControl::kFilterEnvHold: - case ParamControl::kFilterEnvDecay: - case ParamControl::kFilterEnvSustain: - case ParamControl::kFilterEnvRelease: - case ParamControl::kFilterTrigAttack: - case ParamControl::kFilterTrigHold: - case ParamControl::kFilterTrigDecay: - return !params_.play.filter.enabled; - default: - return false; - } + // Thin int-id wrapper over the pure, CTest-covered predicate — see deckKnobInert + // (deck_groups.h) for which cells go inert and why. + return deckKnobInert(static_cast(id), params_.play.pitchEnv.enabled, + params_.play.filter.enabled); } bool ReaSamplerEditor::mouseDownDeck(const FaceLayout& fl, int x, int y) { diff --git a/src/shell/instrument/editor_paint_chrome.cpp b/src/shell/instrument/editor_paint_chrome.cpp index 4f1e9a9..81ad6e8 100644 --- a/src/shell/instrument/editor_paint_chrome.cpp +++ b/src/shell/instrument/editor_paint_chrome.cpp @@ -1,7 +1,7 @@ // editor_paint_chrome.cpp — the CHROME band's painter: the toolbar row (product title + -// live readout, then the control run — preview, preview-velocity knob, curve button, -// Mono|Stereo, Browse) over the strip row, which the piano strip has to itself. Windows-only; -// all rects come from the pure sample_chrome interior and the pure keyboard_strip geometry. +// live readout, then the control run — preview, preview-velocity knob, Mono|Stereo, Browse) +// over the strip row, which the piano strip has to itself. Windows-only; all rects come from +// the pure sample_chrome interior and the pure keyboard_strip geometry. #include "shell/instrument/reasampler_editor.h" @@ -140,8 +140,9 @@ void ReaSamplerEditor::paintChrome(LICE_IBitmap* bmp, const FaceLayout& fl, bool drawButton(bmp, box, nullptr, st, /*warn=*/false); const PreviewGlyph g = previewGlyph(cr.preview); if (!g.empty()) { - const LICE_pixel ink = - toLice(roleColor(active ? Role::BgBase : Role::TextPrimary)); + // Same surface as the button label, so the glyph tracks drawButton's own rule + // (buttonLabelRole) rather than a second copy of it that could desync from the kit. + const LICE_pixel ink = toLice(roleColor(buttonLabelRole(st))); LICE_FillTriangle(bmp, g.leftX, g.topY, g.leftX, g.bottomY, g.apexX, g.apexY, ink, 1.0f, 0); } diff --git a/src/shell/instrument/editor_paint_curve.cpp b/src/shell/instrument/editor_paint_curve.cpp index 8988fd1..c695a3e 100644 --- a/src/shell/instrument/editor_paint_curve.cpp +++ b/src/shell/instrument/editor_paint_curve.cpp @@ -17,19 +17,6 @@ using namespace reasampler::instrument::ui; // popup geometry namespace { -// The curve a VELOCITY cell shows. Mirrors editedCurve's routing for the popup's own target; -// a cell draws whichever curve it opens, whether or not the popup is up. -const VelocityCurve& curveFor(const InstrumentParams& p, CurveTarget target) { - switch (target) { - case CurveTarget::kPitch: return p.play.pitchVelocityCurve; - case CurveTarget::kFilter: return p.play.filter.velocityCurve; - case CurveTarget::kAmp: - case CurveTarget::kNone: - return p.velocityCurve; - } - return p.velocityCurve; -} - const char* curveTitle(CurveTarget target) { switch (target) { case CurveTarget::kPitch: return "VELOCITY -> PITCH"; @@ -56,7 +43,7 @@ void ReaSamplerEditor::paintCurveButton(LICE_IBitmap* bmp, const Rect& r, CurveT ? roleColor(Role::AccentPrimary) : roleColor(Role::LineHairline); LICE_DrawRect(bmp, r.x, r.y, r.width - 1, r.height - 1, toLice(border), 1.0f, 0); - const VelocityCurve& curve = curveFor(params_, target); + const VelocityCurve& curve = curveFor(target); const int inset = 3; const VelocityCurve::Box mini{r.x + inset, r.y + inset, r.width - 2 * inset, r.height - 2 * inset}; diff --git a/src/shell/instrument/editor_paint_deck.cpp b/src/shell/instrument/editor_paint_deck.cpp index cab8a3f..c9fcdd3 100644 --- a/src/shell/instrument/editor_paint_deck.cpp +++ b/src/shell/instrument/editor_paint_deck.cpp @@ -179,7 +179,7 @@ void ReaSamplerEditor::paintDeck(LICE_IBitmap* bmp, const FaceLayout& fl) { paintCurveButton(bmp, c.knob, curveCell, disabled, !disabled && isHovered(HoverKind::kControl, c.id)); kitTextCentered(bmp, c.label, knobName(static_cast(c.id)), - Font::Micro, disabled ? Role::LineHairline : Role::TextDim); + Font::Micro, Role::TextDim); continue; } const bool dragging = (drag_ == DragKind::kDeckKnob && dragParamId_ == c.id); diff --git a/src/shell/instrument/editor_session.cpp b/src/shell/instrument/editor_session.cpp index 518ef6c..478178c 100644 --- a/src/shell/instrument/editor_session.cpp +++ b/src/shell/instrument/editor_session.cpp @@ -72,7 +72,7 @@ void ReaSamplerEditor::refreshFromBank() { monoTrigger_ = processor_->monoTrigger(); // A refresh that emptied the selection closes the curve popup — an open-but-invisible // modal would otherwise swallow clicks on the empty state. - if (selectedId_.empty()) curvePopup_ = CurveTarget::kNone; + if (selectedId_.empty()) closeCurvePopup(); // Drop a filter that names a bank no longer present. if (!activeFilterBankId_.empty()) { bool found = false; diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index 797d549..07a53ba 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -150,7 +150,7 @@ private: // --- Band painters (one TU each, mirroring the input side) --- // Chrome: title band + Browse nav + the control row (root strip, preview, velocity knob, - // curve button, channel toggle). + // channel toggle). void paintChrome(LICE_IBitmap* bmp, const FaceLayout& fl, bool empty); // The hovered piano key's note-name chip. Drawn after every band — it overhangs the // chrome into whatever is below it. @@ -459,11 +459,22 @@ private: // delta from this anchor, so a grab never jumps the value. double dragKnobStartValue_ = 0.0; - // Which velocity curve the popup is editing; kNone = closed. Never persisted. + // Which velocity curve the popup is editing; kNone = closed. Never persisted. Every writer + // of kNone must also cancel a live curve-node drag (closeCurvePopup does both) — an Esc + // mid-drag that closed the popup without cancelling the drag used to leave editedCurve()'s + // mutable overload aliasing the amp curve underneath an in-flight pitch/filter drag. CurveTarget curvePopup_ = CurveTarget::kNone; - // The parameter-set curve `curvePopup_` names. Both overloads exist because every edit - // path needs the mutable one and paint needs the const one; routing through this ONE - // switch is what keeps the three curves on a single popup/draw/hit-test code path. + void closeCurvePopup(); + + // THE one switch from a CurveTarget to the parameter-set curve it names — paint (button + // thumbnails, for every target) and edit (editedCurve, for curvePopup_ specifically) both + // route through it, so a fourth curve or a moved field is a one-place edit. + const VelocityCurve& curveFor(CurveTarget target) const; + VelocityCurve& curveFor(CurveTarget target); + + // The curve curvePopup_ names — curveFor(curvePopup_), typed as its own pair because every + // edit path needs the mutable overload and paint needs the const one. The mutable overload + // refuses kNone (a closed popup has nothing open to edit) rather than aliasing amp. VelocityCurve& editedCurve(); const VelocityCurve& editedCurve() const; diff --git a/src/shell/panel/draw_kit.cpp b/src/shell/panel/draw_kit.cpp index 888eba0..4b516e7 100644 --- a/src/shell/panel/draw_kit.cpp +++ b/src/shell/panel/draw_kit.cpp @@ -214,14 +214,15 @@ void drawButton(LICE_IBitmap* bmp, const KitButtonBox& button, const char* label radius, toLice(borderCol), drawAlpha(borderCol), 0, true); if (label && *label) { - // Active fill is the accent — label goes in bg/base for contrast; else text/primary. - const Role textRole = (state == InteractionState::Active) - ? Role::BgBase - : Role::TextPrimary; - text(bmp, b, label, Font::Label, textRole, Align::Center); + text(bmp, b, label, Font::Label, buttonLabelRole(state), Align::Center); } } +Role buttonLabelRole(InteractionState state) { + // Active fill is the accent — the mark goes in bg/base for contrast; else text/primary. + return state == InteractionState::Active ? Role::BgBase : Role::TextPrimary; +} + void drawSlider(LICE_IBitmap* bmp, const SliderGeometry& geom, InteractionState state) { if (!bmp || geom.track.empty()) return; diff --git a/src/shell/panel/draw_kit.h b/src/shell/panel/draw_kit.h index a48bd2b..ed1bb02 100644 --- a/src/shell/panel/draw_kit.h +++ b/src/shell/panel/draw_kit.h @@ -87,6 +87,11 @@ void fillSurface(LICE_IBitmap* bmp, const KitBox& box, Role role, InteractionSta void drawButton(LICE_IBitmap* bmp, const KitButtonBox& button, const char* label, InteractionState state, bool warn); +// THE ink role drawButton's own label draws in, for `state` (see draw_kit.cpp). A non-text +// button mark (e.g. a drawn glyph, not text()) that needs to sit legibly on a drawButton +// surface should call this rather than re-deriving the rule. +Role buttonLabelRole(InteractionState state); + // A horizontal slider: track groove, accent-filled portion up to the handle, and the // handle itself. `geom` is the pure SliderGeometry the caller computed. void drawSlider(LICE_IBitmap* bmp, const SliderGeometry& geom, InteractionState state); diff --git a/tests/test_component_state_io.cpp b/tests/test_component_state_io.cpp index fbeb72f..58c9a9e 100644 --- a/tests/test_component_state_io.cpp +++ b/tests/test_component_state_io.cpp @@ -859,21 +859,36 @@ static void testPriorPayloadVersionsLiftToAHardSeam() { in.params.play.pitchVelocityCurve = reasampler::instrument::engine::VelocityCurve::fromPoints( {VelocityPoint{0.0, 0.5}, VelocityPoint{127.0, 1.0}}, reasampler::instrument::engine::CurveDomain::Bipolar); + // The filter's pre-v12 depth fold (see testPreV12FilterVelocityDepthFoldsIntoTheCurve for + // the single-version proof); planted here too so EVERY version that carries a filter tail + // (v9..v11) is walked, not just v11 — the fold's branch condition is pv < kParamsVelocityVersion. + in.params.play.filter.enabled = true; + in.params.play.filter.modAmount = -0.6251953125; // distinct, exactly representable anchor + const reasampler::instrument::engine::VelocityCurve filterShape = + reasampler::instrument::engine::VelocityCurve::fromPoints( + {VelocityPoint{0.0, 0.0}, VelocityPoint{64.0, 0.25}, VelocityPoint{127.0, 1.0}}, + reasampler::instrument::engine::CurveDomain::Bipolar); + in.params.play.filter.velocityCurve = filterShape; + constexpr double kPlantedDepth = -0.75; struct Case { std::uint32_t pv; std::size_t cut; bool keepsCurveTail; + bool keepsFilterTail; }; const Case cases[] = { - {11, kVelocityTailBytes, true}, - {10, kVelocityTailBytes + kLoopTailBytes, true}, - {9, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes, false}, - {8, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes + kFilterTailBytes, false}, + {11, kVelocityTailBytes, true, true}, + {10, kVelocityTailBytes + kLoopTailBytes, true, true}, + {9, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes, false, true}, + {8, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes + kFilterTailBytes, false, false}, }; for (const Case& c : cases) { - const ComponentState out = - deserializeComponentState(payloadDowngradedTo(in, c.pv, c.cut), 48000.0); + std::vector bytes = payloadDowngradedTo(in, c.pv, c.cut); + if (c.keepsFilterTail) { + plantPreV12FilterDepth(bytes, in.params.play.filter.modAmount, kPlantedDepth); + } + const ComponentState out = deserializeComponentState(bytes, 48000.0); // The span itself has been in the format since v2 and must survive untouched. CHECK(out.params.loopOverride && out.params.loopOverride->hasLoop); CHECK(out.params.loopOverride && out.params.loopOverride->start == 2000); @@ -889,6 +904,20 @@ static void testPriorPayloadVersionsLiftToAHardSeam() { // And the cut landed on the tail boundary the ladder claims, not somewhere inside it. CHECK(out.params.play.adsr.attackCurve == (c.keepsCurveTail ? 4.0 : reasampler::util::kCurveNeutral)); + // The filter's own velocity curve: folded by the planted depth where the tail survives + // (v9..v11), the off/neutral default where it was cut away entirely (v8). + if (c.keepsFilterTail) { + CHECK(out.params.play.filter.enabled); + for (int v = 0; v <= 127; ++v) { + CHECK(std::fabs(out.params.play.filter.velocityCurve.eval(v) - + kPlantedDepth * filterShape.eval(v)) < 1e-12); + } + } else { + CHECK(!out.params.play.filter.enabled); + for (int v = 0; v <= 127; ++v) { + CHECK(out.params.play.filter.velocityCurve.eval(v) == 0.0); + } + } } } diff --git a/tests/test_curve_popup.cpp b/tests/test_curve_popup.cpp index b85a07f..feb6ae2 100644 --- a/tests/test_curve_popup.cpp +++ b/tests/test_curve_popup.cpp @@ -8,6 +8,7 @@ #include "../src/core/instrument/engine/velocity_curve.h" +#include #include using namespace reasampler; @@ -89,10 +90,14 @@ static void testOutsideSheetDismissTest() { // the popup's real geometry, because that is what makes one popup code path legitimate. static void testTheSameCurveBoxHostsBothDomains() { const CurvePopupLayout pl = computeCurvePopup(840, 620); - // The shell insets this rect before mapping; the inset is uniform, so any box inside the - // curveBox exercises the same relationship. Use the rect itself. - const VelocityCurve::Box box{pl.curveBox.x, pl.curveBox.y, pl.curveBox.width, - pl.curveBox.height}; + // The editor never maps against the raw curveBox — shell/instrument/editor_internal.h's + // curveBoxFromRect insets it first (kVelCurveInset == 14, mirrored here since this pure + // target cannot link the shell). Applying it, not the raw rect, is what exercises the + // actual box shape paint/hit-test/drag agree on. + constexpr int kInset = 14; + const VelocityCurve::Box box{pl.curveBox.x + kInset, pl.curveBox.y + kInset, + std::max(0, pl.curveBox.width - 2 * kInset), + std::max(0, pl.curveBox.height - 2 * kInset)}; CHECK(box.width > 1 && box.height > 1); const VelocityCurve amp = VelocityCurve::flat(); @@ -105,12 +110,15 @@ static void testTheSameCurveBoxHostsBothDomains() { CHECK(amp.pixelFromPoint(box, {0.0, 0.0}).y == bottom); CHECK(mod.pixelFromPoint(box, {0.0, 1.0}).y == top); CHECK(mod.pixelFromPoint(box, {0.0, -1.0}).y == bottom); - // ...so value 0 is the FLOOR for the amp curve and the MIDLINE for a modulation curve. - CHECK(mod.pixelFromPoint(box, {0.0, 0.0}).y == (top + bottom) / 2); + // ...so value 0 is the FLOOR for the amp curve and the MIDLINE for a modulation curve. The + // expected row replicates valueToY's own HALF-UP rounding on the inverted fraction (velocity_ + // curve.cpp) rather than a plain integer bisection of top/bottom: the two agree only when + // box.height-1 is even, so a naive (top+bottom)/2 silently depends on this box's parity. + const int midY = top + static_cast(0.5 * static_cast(box.height - 1) + 0.5); + CHECK(mod.pixelFromPoint(box, {0.0, 0.0}).y == midY); // And a click at the vertical centre adds a point at 0 in the bipolar editor, at 0.5 in // the unipolar one — one hit-test path, two correct answers. - const int midY = (top + bottom) / 2; CHECK(mod.pointFromPixel(box, box.left, midY).value == 0.0); CHECK(amp.pointFromPixel(box, box.left, midY).value > 0.49); CHECK(amp.pointFromPixel(box, box.left, midY).value < 0.51); diff --git a/tests/test_deck_groups.cpp b/tests/test_deck_groups.cpp index 96117bb..11e04d0 100644 --- a/tests/test_deck_groups.cpp +++ b/tests/test_deck_groups.cpp @@ -459,11 +459,29 @@ static void testOverlayIsInertExactlyWhenItsGroupToggleIsOff() { CHECK(!overlayEnvInert(OverlayEnv::kNone, false, false)); } +// A deck knob goes inert exactly with its group's own enable toggle — including the filter's +// VELOCITY cell, which sits in the VELOCITY group visually but is a filter parameter and must +// go inert with the rest of the filter (the reachable-through-the-deck route mouseDownDeck +// checks before ever routing a curve-cell click to the popup). +static void testDeckKnobIsInertExactlyWithItsGroupsEnableToggle() { + CHECK(deckKnobInert(DeckParam::kFilterVelCurve, /*pitchEnv=*/true, /*filter=*/false)); + CHECK(!deckKnobInert(DeckParam::kFilterVelCurve, true, true)); + CHECK(deckKnobInert(DeckParam::kFilterCutoff, true, false)); + CHECK(!deckKnobInert(DeckParam::kFilterCutoff, true, true)); + CHECK(deckKnobInert(DeckParam::kPitchEnvDepth, /*pitchEnv=*/false, true)); + CHECK(!deckKnobInert(DeckParam::kPitchEnvDepth, true, true)); + // The amp's own velocity cell and every ordinary control are never inert here — inertness + // is a filter/pitch-env-group-only concept. + CHECK(!deckKnobInert(DeckParam::kAmpVelCurve, false, false)); + CHECK(!deckKnobInert(DeckParam::kAttack, false, false)); +} + int main() { testOverlaySelectionIsExclusiveAcrossTheThreeEnvelopeDecks(); testClickingTheActiveOverlayRadioClearsToNone(); testANonRadioIdLeavesTheOverlaySelectionAlone(); testOverlayIsInertExactlyWhenItsGroupToggleIsOff(); + testDeckKnobIsInertExactlyWithItsGroupsEnableToggle(); testEveryDeckControlIsClassifiedLiveOrReloading(); testOnlyALiveControlsDragTakesTheLiveTier(); testDeckReadsPitchThenFilterThenAmpLeftToRight();