diff --git a/docs/TODO.md b/docs/TODO.md index a9c1fdf..53f57e0 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -445,3 +445,41 @@ today. bitmap and confirmed to place the stroke correctly, or it is redesigned to rasterize at physical rather than logical resolution once `IPlugViewContentScaleSupport` (or equivalent) makes scaling real. + +## The loop intrinsic is folded twice: the bank blob and the instance ref can skew + +**Context (what shipped).** Two call sites answer the same question — "does this capture +have a sustain loop, and where?" — by different routes, and both are load-bearing: + +- `ReaSamplerEditor::pickedMarkers` (`shell/instrument/editor_session.cpp`) resolves the + intrinsic from the **live bank blob** first (`selectSample`), falling back to the + instance-owned `SampleRefs` only when the blob is unreadable, then lets + `params_.loopOverride` supersede it. +- `ReaSamplerProcessor::reloadInstrument` (`shell/instrument/processor_reload.cpp`) + resolves it from the **instance ref** via `resolveCapture`, which is the one + override-beats-intrinsic fold, and that is what the bake renders and what + `bakeWindowNeedsHold` is ultimately asked about. + +**The wart.** The two can disagree whenever the bank blob's loop for a capture differs +from the copy in the instance's own refs table — a recapture that moved the loop points, +a hand-edited blob, or an instance that predates the current bank state. The face then +draws (and the Hold predicate answers about) one loop while the engine plays another. + +**Pre-existing.** This split predates the derived-bake-window work; the bake-Hold +predicate is only a new *consumer* of `pickedMarkers`, not the origin of the divergence. + +**Intended fix.** Route `pickedMarkers` through `resolveCapture` so both sites share the +one fold, as the bank/refs paths already do elsewhere. + +**The constraint the fix MUST handle.** `pickedMarkers` runs on the editor's mouse-down +arbitration path (every waveform click, not just marker grabs) and deliberately skips its +bridge read once an override is set; a unified fold must not put a bank read back on that +path. It must also keep the browser-source semantics: the bank is where a *new* capture's +intrinsics come from, the refs table is where the *loaded* one's live. + +**Priority / risk.** Low. Needs a recapture-moved-the-loop scenario to observe, and the +failure is a mis-drawn marker or a spuriously shown/hidden Hold knob, not bad audio. + +**Done looks like.** One fold answers the intrinsic for both the editor's markers and the +engine's reload, with a test that moves the bank's loop out from under a loaded instance +and shows the two agreeing. diff --git a/src/core/instrument/CLAUDE.md b/src/core/instrument/CLAUDE.md index caf06d2..272c14a 100644 --- a/src/core/instrument/CLAUDE.md +++ b/src/core/instrument/CLAUDE.md @@ -281,7 +281,7 @@ anything for a trigger shape. ### `engine/` - The engine is the `sampler_core` CMake target over FOUR headers and TWO TUs, split on its own responsibility seam — cold note routing vs the hot per-sample render: - - `play_params.h` — the value layer: `PlayParams`/`AdsrParams`/`TriggerParams`/`PitchEnvParams`/`FilterParams`, the per-instance mode enums (`ChannelMode`/`VoiceMode`/`MonoTrigger`), and `SampleData` (the ONE loaded capture: decoded PCM + root + loop + start + keyTrack + velocity curve + play params). Shared by the engine, the codec, and the editor, so a UI/codec TU reading a param struct doesn't recompile when a `Voice` member changes. `FilterParams` stores the filter module's own `FilterSettings` by value rather than a parallel copy of its normalized positions. + - `play_params.h` — the value layer: `PlayParams`/`AdsrParams`/`TriggerParams`/`PitchEnvParams`/`FilterParams`, the per-instance mode enums (`ChannelMode`/`VoiceMode`/`MonoTrigger`), and `SampleData` (the ONE loaded capture: decoded PCM + root + loop + start + keyTrack + velocity curve + play params). Shared by the engine, the codec, and the editor, so a UI/codec TU reading a param struct doesn't recompile when a `Voice` member changes. `FilterParams` stores the filter module's own `FilterSettings` by value rather than a parallel copy of its normalized positions. Also the ONE home of the drawn-EG rule family — `splineActive`, `effectivePlayMode`, `enforceGateUnavailableWhileDrawn` and `effectiveLengthFraction` — all templated over the frames and seconds representations, so no consumer of either can re-read the raw fields instead. - `envelopes.h` — the three per-frame evaluators (`AdsrEnvelope` AHDSR, `AhdEnvelope` the sustain-less Attack/Hold/Decay, `PitchEnvelope` the AHD pitch offset), CONCRETE and fully header-inline. Never give them a common base or a virtual `tick()`: they are called per-voice-per-sample. Also home to `fitAhd`/`ahdLevelAt`, THE span split and shape every sustain-less envelope shares. A voice carries two of each shape — the amp's and the filter's — and its play mode picks which pair it reads. `AdsrEnvelope`/`PitchEnvelope` own `applyLive` (the φ-holding mid-stage rule), its fresh-note peer `snapLive`, and `StepSmoother`, the bounded offset that absorbs the level steps φ cannot cover; `AhdEnvelope` is POSITIONAL (evaluated at a source offset, not ticked), so it has no phase to hold and smooths a live reshape instead. - `live_params.h` / `live_params.cpp` — the live-parameter block: `LiveValues` (the plain, trivially-copyable bundle the audio thread observes), the single-writer `LiveParams` seqlock that publishes it without a lock or a torn read, `foldLive` (the ONE derivation from `PlayParams` — every publisher goes through it so the two representations cannot drift), and `ValueRamp`, the per-frame glide whose EXACT termination is what lets the filter's equality-compare cutoff skip re-engage. Links no engine: the block is a value the voice observes, not a thing the engine owns. - `voice.h` / `voice.cpp` — one voice. The per-SAMPLE render half (`advanceFrame` and everything it calls) is INLINE IN THE HEADER by RT constraint; the per-NOTE half (note-on setup incl. the Preserve ring prime, legato retune, gate-off, the off-thread shifter presize) is out of line in the TU. The voice owns its own `VoiceFilter` and filter envelope, run between the pitch stage and the amp multiply — see `engine/filter/CLAUDE.md`. **Documented ~600-line-ceiling exception** (root `CLAUDE.md` structural heuristic 1): `voice.h` sits over the ceiling because `advanceFrame`'s RT-inline constraint forbids the seam a split would need — a documented exception, not silent overshoot. @@ -299,14 +299,14 @@ anything for a trigger shape. - `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. -- `trigger_seam` — the shared Trigger play-SPAN formula: how the stored %-length becomes the source-frame span the voice plays, the overlay draws over and the bake's window holds, threading `startFrame` correctly. Home to `effectiveLengthFraction`, the spline fold every one of those three must apply — the %-knob goes inert but stays STORED while a contour is drawn, so a raw read of it silently shortens whatever reads it. (Its fade frames↔fraction converters retired with the fade pair itself.) +- `trigger_seam` — the shared Trigger play-SPAN formula: how a %-length becomes the source-frame span the overlay draws over and the bake's window holds, threading `startFrame` correctly and clamping the fraction the same way `Voice::start` does (the engine evaluates the same formula inline rather than depending on `map/`). The spline fold every consumer must apply first — `effectiveLengthFraction` — is `play_params.h`'s, beside the rest of that rule family. (Its fade frames↔fraction converters retired with the fade pair itself.) ### `ui/` - `editor_geometry` (`core/instrument/ui`) — the shared geometry VOCABULARY every instrument UI module speaks: the `core::ui::Rect` alias, `contains()`, and `OverlayArea` (a one-field `Rect` wrapper, no implicit conversion from `Rect`). Header-only (an INTERFACE CMake target), so it carries no layout of its own. - `sample_bands` — **THE band-stack allocator**, and the only module that owns the Sample face's vertical inventory — including `kEditorMinWidth`/`kEditorMinHeight`, the editor's client-area floor, which IS its default size (the shell's `checkSizeConstraint` and opening `ViewRect` both read it; the face grows, never shrinks below what the stack is laid out for). Three bands top-to-bottom (CHROME toolbar+control row / WAVEFORM elastic, floored at two stacked lanes / DECKS bottom-anchored at the knob deck's own wrapped height), plus the waveform band's lane split (`waveformLanes` takes a resolved `LaneSplit`, not a raw bool — only `waveformSurface` folds the source-channel-count decision in). A shared READ-ONLY surface for every band owner — a band's interior module lays out inside the rect it is handed and never re-allocates the stack. -- `sample_chrome` — the CHROME band's interior: the toolbar row (title + the whole right-anchored control run — bake Hold cell, bake, preview, velocity knob cell, channel toggle, Browse) over the strip row, which the piano strip owns outright. The title takes what the run leaves; the strip takes its whole row, inset only by the shared band pad so it lines up with the waveform band beneath. Every run member's width is RESERVED unconditionally, the Hold cell included: the run is right-anchored, so laying a member out conditionally would slide its neighbours out from under the pointer. Also `previewGlyph`, the preview button's play triangle — three vertices for one filled-triangle draw, so the button's label needs no font metric and no image asset. -- `bake_hold` — the Hold knob's value domain and nothing else: the knob's normalized [0,1] mapped onto the note-length ladder and back. Split from `sample_chrome` on the same axis `deck_values` was split from `knob_deck` — that says where the cell is, this says what its position means. +- `sample_chrome` — the CHROME band's interior: the toolbar row (title + the whole right-anchored control run — bake Hold cell, bake, preview, velocity knob cell, channel toggle, Browse) over the strip row, which the piano strip owns outright. The title takes what the run leaves; the strip takes its whole row, inset only by the shared band pad so it lines up with the waveform band beneath. Every run member's width is RESERVED unconditionally, the Hold cell included — the only conditionally-drawn one, and the leftmost, so what its reservation buys is a title slot that does not re-measure when a loop is dialled in or out (`sample_chrome.h` records the cost). Also `previewGlyph`, the preview button's play triangle — three vertices for one filled-triangle draw, so the button's label needs no font metric and no image asset. +- `bake_hold` — the Hold knob's value domain and nothing else: the knob's normalized [0,1] mapped onto the note-length ladder and back, ordered by LENGTH rather than by the ladder's presentation order. Split from `sample_chrome` on the same axis `deck_values` was split from `knob_deck` — that says where the cell is, this says what its position means. - `keyboard_strip` — piano-keyboard strip: true white/black key geometry (whites tiled at one width, blacks overlaid at one width and height, straddling their boundary), hit-test resolving black-over-white by zone, root-marker rect, the absolute-position drag resolver, and MIDI note naming under the C4 convention. **Same-class keys are one integer width by construction; the residue of an indivisible band width (`w % 75`, up to 74 px) lands in symmetric end margins, never in a key** — uniform widths and gap-free edge-to-edge tiling cannot both hold, and uniformity wins. - `waveform_view` — the WAVEFORM band's interior: `waveformSurface` resolves the drawn lane(s) (two stacked lanes, L over R, only when the mode is stereo AND the source has a second channel — a mono source under stereo mode is dual-mono and draws one lane) plus **the** overlay area, and `laneEnvelope` splits one multi-channel envelope pass per lane. Also maps frame span linearly across a rect; generic named draggable markers with drag-delta resolver, clamp, and zero-crossing snap, plus `markerHandleRect` — a top-strip grab tab distinct from a marker's full-height column, so two markers that share a frame stay independently grabbable (the column goes to the first in draw order; the tab, asked first, resolves the other). - **Overlay contract (consumed by later waveform work).** `WaveformSurface::overlay` — equivalently the standalone `waveformOverlayArea(band)` — is the FULL band in both modes. Everything riding the waveform (the amp-envelope trace and its node handles, the start/loop markers, the loop region) draws ONCE into it, spanning both stacked lanes; hit-testing resolves against the same area so a grab in the lower lane reaches them. Anything drawn or hit-tested per lane is a duplicate and a defect — structurally enforced: `overlay` is the distinct `OverlayArea` type (`editor_geometry`), not `Rect`, so every overlay-consuming API (`frameToX`/`markerAtPoint`/`resolveDragFrame`, `envelope_edit`'s `nodeAtPoint`/`resolveNodeDrag`, `envelope_overlay`'s `buildEnvelopePolyline`) rejects a lane rect at compile time rather than silently accepting one. diff --git a/src/core/instrument/bake/CLAUDE.md b/src/core/instrument/bake/CLAUDE.md index 5a57e6a..5a9075a 100644 --- a/src/core/instrument/bake/CLAUDE.md +++ b/src/core/instrument/bake/CLAUDE.md @@ -35,11 +35,13 @@ decision about what the render made obsolete. source exhaustion, since the read head frees the voice whether or not the gate is down. `bakeWindowNeedsHold` is the predicate, and it reads the ENGINE's loop fold rather than the loop fields, so the control that collects Hold cannot appear for a loop the voice refuses. -- **Trailing silence is free; truncation is not.** Every derivation rounds outward — the Gate - note rounds UP to a programmable length that outlasts the source, the Varispeed bound takes - the deepest reachable offset, and every path is padded by the voice's terminal declick ramp - (`kDeclickFrames`, unconditionally — not branched on the pitch engine that has the ramp - today). Judge any change to this module against that asymmetry. +- **Trailing silence is free; truncation is not.** Every derivation errs outward — the + Varispeed bound takes the deepest reachable offset the voice can play, and every path is + padded by the voice's terminal declick ramp (`kDeclickFrames`, unconditionally — not branched + on the pitch engine that has the ramp today). Judge any change to this module against that + asymmetry. What it does NOT mean is quantizing: a derived length is an exact duration and a + finite ladder cannot express one (`note/CLAUDE.md`) — rounding up to a rung truncated any + source past the top rung, which is the failure this asymmetry exists to prevent. - **The reset's survive list is written out; everything else defaults.** `resetAfterBake` starts from a default-constructed parameter set and copies back only the mapping facts. A parameter added later therefore resets by default — the safe direction, since diff --git a/src/core/instrument/bake/bake_plan.cpp b/src/core/instrument/bake/bake_plan.cpp index e17a7bf..d991ea5 100644 --- a/src/core/instrument/bake/bake_plan.cpp +++ b/src/core/instrument/bake/bake_plan.cpp @@ -7,7 +7,7 @@ #include "core/instrument/engine/loop/loop_span.h" // resolveLoop (the one sustain-loop fold) #include "core/instrument/engine/voice.h" // kDeclickFrames (the terminal ramp length) -#include "core/instrument/map/trigger_seam.h" // the one Trigger span formula + %-fold +#include "core/instrument/map/trigger_seam.h" // triggerPlayLength (the one span formula) namespace reasampler::instrument::bake { @@ -71,9 +71,8 @@ bool bakeWindowNeedsHold(const SampleData& dialed) { } NoteProgram defaultBakeProgram(const SampleData& dialed, int renderSampleRate, - note::Tempo tempo, note::Division hold, - note::Velocity velocity) { - NoteProgram p; // 1/4 straight, capture opening at note-on + note::Division hold, note::Velocity velocity) { + NoteProgram p; // a quarter note, capture opening at note-on p.velocity = velocity; if (renderSampleRate <= 0) return p; const double rate = static_cast(renderSampleRate); @@ -86,25 +85,24 @@ NoteProgram defaultBakeProgram(const SampleData& dialed, int renderSampleRate, double endOffsetSeconds = 0.0; if (dialed.play.playMode == PlayMode::Trigger) { // Trigger ignores note-off entirely: the sound ends when the read head reaches the - // play span's end, which has nothing to do with the note's length — so the end - // offset is whatever is left after the note, positive or negative. - const std::int64_t span = map::triggerPlayLength( - map::effectiveLengthFraction(dialed.play), frameCount, start); - endOffsetSeconds = static_cast(span) / rate * stretch - - tempo.beatsToSeconds(note::divisionBeats(p.length)); + // play span's end. The note is that span, so the window closes on the sound rather + // than on a length the voice never consulted. + const std::int64_t span = + map::triggerPlayLength(effectiveLengthFraction(dialed.play), frameCount, start); + p.length = note::lengthOfSeconds(static_cast(span) / rate * stretch); } else if (bakeWindowNeedsHold(dialed)) { // The loop cycles for as long as the note is held, so the hold IS the length, and the // release is the one stage that runs after note-off. - p.length = hold; + p.length = note::lengthOfDivision(hold); endOffsetSeconds = releaseSeconds; } else { // Gate with no loop: the read head runs off the source and frees the voice whether or // not the gate is still down, so the maximal sound is the whole post-start span held. - // Rounding the note UP to a programmable length that outlasts it costs nothing — the - // voice is already gone by then — while a shorter note releases mid-attack. + // Exact, not a ladder rung: a source longer than the ladder's top rung would otherwise + // take that rung and release mid-sound, and rounding up to one costs trailing silence + // on every other source. const std::int64_t postStart = (std::max)(std::int64_t{0}, frameCount - start); - const double exhaustSeconds = static_cast(postStart) / rate * stretch; - p.length = note::shortestDivisionAtLeast(tempo.secondsToBeats(exhaustSeconds)); + p.length = note::lengthOfSeconds(static_cast(postStart) / rate * stretch); endOffsetSeconds = releaseSeconds; } // The voice rings its last output out over kDeclickFrames instead of hard-cutting it, and diff --git a/src/core/instrument/bake/bake_plan.h b/src/core/instrument/bake/bake_plan.h index 0731817..b5ea3a1 100644 --- a/src/core/instrument/bake/bake_plan.h +++ b/src/core/instrument/bake/bake_plan.h @@ -31,21 +31,25 @@ bool bakeWindowNeedsHold(const SampleData& dialed); // The bake's programmed note, DERIVED from the dialed sound at `renderSampleRate` (the rate // the bake renders at, which is what the engine's frame counts are consumed against): // -// Trigger — note length is nominal (note-off is ignored); the end offset carries the -// whole play span, stretched by the deepest downward Varispeed offset. +// Trigger — the note IS the play span (note-off is ignored anyway), stretched by the +// deepest downward Varispeed offset. // Gate, loop — `hold` is the note length; the end offset is the release. // Gate, no loop— the read head runs off the source and frees the voice whatever the gate is -// doing, so the note is rounded UP to the shortest programmable length that -// outlasts the source. Holding longer than that sounds identical, which is -// what makes the overshoot trailing silence rather than a different sound. +// doing, so the note is the whole post-start span, stretched the same way. +// +// Both derived lengths are EXACT durations, not ladder rungs: a source longer than the +// ladder's top rung has no rung that covers it, and quantizing up to one overshoots every +// other source (see note/CLAUDE.md). `hold` alone stays musical — it is a picker. // // Every case is padded by the voice's terminal declick ramp (kDeclickFrames): trailing // silence is free, and closing the window on the frame the ramp starts is a hard cut. // `hold` is read only in the Gate-with-loop case; `velocity` is the velocity the note fires // at, and it feeds the Varispeed stretch as well as the render. +// +// Takes no tempo: nothing derived here is beat-denominated. The one field that is — `hold` — +// meets the tempo in resolveNote, with the rest of the program's beat-denominated fields. note::NoteProgram defaultBakeProgram(const SampleData& dialed, int renderSampleRate, - note::Tempo tempo, note::Division hold, - note::Velocity velocity); + note::Division hold, note::Velocity velocity); // The render window in frames. TWO domains meet here: `totalFrames` is the captured FILE's // length, everything else counts RENDER frames from whichever comes first, note-on or the diff --git a/src/core/instrument/engine/play_params.h b/src/core/instrument/engine/play_params.h index 0620566..1ba35f1 100644 --- a/src/core/instrument/engine/play_params.h +++ b/src/core/instrument/engine/play_params.h @@ -195,13 +195,29 @@ bool splineActive(const Play& p) { (p.filter.enabled && p.filterSpline.mode == EnvMode::Spline); } -// The one enforcement of splineActive's rule (see its doc above). Header-inline and -// allocation-free: play_params.h sits on the per-voice-per-sample include path. Both callers — -// resolvePlay (sample_map.cpp) and the editor's applyControl — route through here, so the two -// cannot drift apart. +// The mode the engine will actually run, and the one home of splineActive's rule (see its doc +// above). Header-inline and allocation-free: play_params.h sits on the per-voice-per-sample +// include path. Every caller — resolvePlay (sample_map.cpp), the editor's applyControl, and +// the editor's read-only predicates — routes through one of these two, so none of them can +// drift into a second reading of the fields. +template +PlayMode effectivePlayMode(const Play& p) { + return splineActive(p) ? PlayMode::Trigger : p.playMode; +} + template void enforceGateUnavailableWhileDrawn(Play& p) { - if (splineActive(p)) p.playMode = PlayMode::Trigger; + p.playMode = effectivePlayMode(p); +} + +// The %-length the voice ACTUALLY plays. Same rule family, same reason it is templated: a drawn +// contour is a pure time function over the full sample length, so any active spline EG folds +// the fraction to 1.0 while the stored knob goes inert — but the stored value survives, so a +// pre-spline setting is still there to be read. Every consumer of the Trigger span must fold it +// here or it silently plays/draws/bakes a fraction of the take. +template +double effectiveLengthFraction(const Play& p) { + return splineActive(p) ? 1.0 : p.trigger.lengthFraction; } // [start, end) frames, half-open. A zero-length loop (start == end) is the "no sustain loop" diff --git a/src/core/instrument/engine/voice.cpp b/src/core/instrument/engine/voice.cpp index fc980b1..b64fd72 100644 --- a/src/core/instrument/engine/voice.cpp +++ b/src/core/instrument/engine/voice.cpp @@ -6,8 +6,6 @@ #include -#include "core/instrument/map/trigger_seam.h" // effectiveLengthFraction (the one %-length rule) - namespace reasampler { void Voice::presizePreserveShifters(std::int64_t windowFrames) { @@ -103,12 +101,13 @@ void Voice::start(int note, int velocity, const SampleData& sample, bool declick env_.noteOn(); playEnd_ = 0; // unused in Gate } else { - // Trigger: play [start, playEnd) where playEnd = start + round(frac*(frames-start)). - // The spline fold lives in effectiveLengthFraction (trigger_seam.h), which the bake's - // window derivation reads too — a second copy of it here is what let a stored-but-inert - // %-knob shorten the bake while the voice played the whole take. - double frac = instrument::map::effectiveLengthFraction(p); - if (frac <= 0.0) frac = 0.0; // %=0 -> zero play length (finishes immediately) + // Trigger: play [start, playEnd) where playEnd = start + round(frac*(frames-start)) — + // map/trigger_seam.h's formula, evaluated inline because the engine does not depend on + // map/. The spline fold is effectiveLengthFraction (play_params.h); a second copy of it + // here is what let a stored-but-inert %-knob shorten the bake while the voice played + // the whole take. + double frac = effectiveLengthFraction(p); + if (!(frac > 0.0)) frac = 0.0; // %=0 (or a corrupt NaN) -> finishes immediately if (frac > 1.0) frac = 1.0; std::int64_t playLen = static_cast( static_cast(postStart) * frac + 0.5); // round diff --git a/src/core/instrument/map/CMakeLists.txt b/src/core/instrument/map/CMakeLists.txt index f775684..91e80d7 100644 --- a/src/core/instrument/map/CMakeLists.txt +++ b/src/core/instrument/map/CMakeLists.txt @@ -2,9 +2,9 @@ reasampler_pure_library(bridge_marshal SOURCES bridge_marshal.cpp) reasampler_test(bridge_marshal LINK bridge_marshal) reasampler_pure_library(trigger_seam SOURCES trigger_seam.cpp) -# The %-length fold lives beside the span formula, so the header reads play_params' value -# layer; the test links that set and nothing else — still no engine, no editor geometry. -reasampler_test(trigger_seam LINK trigger_seam velocity_curve peaks filter curve_law) +# Plain frame arithmetic over doubles: no engine, no value layer, no editor geometry. The +# %-length fold it used to host lives with its siblings in play_params.h. +reasampler_test(trigger_seam LINK trigger_seam) reasampler_pure_library(bank_sync SOURCES bank_sync.cpp diff --git a/src/core/instrument/map/params_payload.cpp b/src/core/instrument/map/params_payload.cpp index c2655ac..c3251e3 100644 --- a/src/core/instrument/map/params_payload.cpp +++ b/src/core/instrument/map/params_payload.cpp @@ -94,24 +94,41 @@ void liftTriggerFades(std::int64_t fadeInFrames, std::int64_t fadeOutFrames, dou out.decayCurve = kTriggerFadeLiftDecayCurve; } +// A wire double whose consumers assume a domain they cannot check: the seconds fields reach +// resolvePlay's static_cast and peakSemitones reaches the bake's pow() and the +// voice's ratio multiply — both undefined or poisoning on NaN. Degrades to the field's own +// construction default, so a damaged blob loses that field rather than the record. +double finiteOr(double v, double fallback) { return std::isfinite(v) ? v : fallback; } + +// A root-note override off the wire. Clamped HERE and not only where it is consumed: planBake +// clamps the note it renders at into MIDI range while the SampleData keeps the raw override as +// its root, and the two disagreeing makes the read rate something other than 1 — which +// mis-sizes the bake's window in the truncating direction. +int clampMidiNote(int note) { return (std::max)(0, (std::min)(127, note)); } + // Read the play tail (v5 shape onward) into `p`. Shared by the legacy zone reader and the // v8 single-record reader so the two can never disagree about field order. void readSecondsPlayTail(ByteReader& r, InstrumentParams& p, double projectRate) { + const PlaySeconds fallback; // the construction defaults, read rather than restated p.play.playMode = (r.u8() != 0) ? PlayMode::Trigger : PlayMode::Gate; - p.play.adsr.holdSeconds = bitsToDouble(r.u64()); - p.play.trigger.lengthFraction = bitsToDouble(r.u64()); + p.play.adsr.holdSeconds = finiteOr(bitsToDouble(r.u64()), fallback.adsr.holdSeconds); + p.play.trigger.lengthFraction = + finiteOr(bitsToDouble(r.u64()), fallback.trigger.lengthFraction); const std::int64_t fadeIn = r.i64(); const std::int64_t fadeOut = r.i64(); liftTriggerFades(fadeIn, fadeOut, projectRate, p.play.trigAhd); p.play.pitchEngine = (r.u8() != 0) ? PitchEngine::Preserve : PitchEngine::Varispeed; p.play.pitchEnv.enabled = (r.u8() != 0); - p.play.pitchEnv.shape.attackSeconds = bitsToDouble(r.u64()); - p.play.pitchEnv.shape.decaySeconds = bitsToDouble(r.u64()); - p.play.pitchEnv.peakSemitones = bitsToDouble(r.u64()); - p.play.adsr.attackSeconds = bitsToDouble(r.u64()); - p.play.adsr.decaySeconds = bitsToDouble(r.u64()); - p.play.adsr.sustainLevel = bitsToDouble(r.u64()); - p.play.adsr.releaseSeconds = bitsToDouble(r.u64()); + p.play.pitchEnv.shape.attackSeconds = + finiteOr(bitsToDouble(r.u64()), fallback.pitchEnv.shape.attackSeconds); + p.play.pitchEnv.shape.decaySeconds = + finiteOr(bitsToDouble(r.u64()), fallback.pitchEnv.shape.decaySeconds); + p.play.pitchEnv.peakSemitones = + finiteOr(bitsToDouble(r.u64()), fallback.pitchEnv.peakSemitones); + p.play.adsr.attackSeconds = finiteOr(bitsToDouble(r.u64()), fallback.adsr.attackSeconds); + p.play.adsr.decaySeconds = finiteOr(bitsToDouble(r.u64()), fallback.adsr.decaySeconds); + p.play.adsr.sustainLevel = finiteOr(bitsToDouble(r.u64()), fallback.adsr.sustainLevel); + p.play.adsr.releaseSeconds = finiteOr(bitsToDouble(r.u64()), fallback.adsr.releaseSeconds); } // Read a velocity curve tail into `curve`, interpreting its y values in `domain` — the domain @@ -170,6 +187,14 @@ void readSplineEnv(ByteReader& r, SplineEnv& s) { reasampler::instrument::engine::CurveDomain::Unipolar); } +// A block whose declared length overruns what the blob still holds has no realignment point: +// every byte after it belongs to a block that was truncated, so a later tail that reads them +// as its own gets an ARBITRARY value — and a tail that clamps (the bake Hold does) turns that +// into a legal-looking fabrication rather than an obvious one. Draining is what makes the +// stream's end honest: each later tail then reads past it and degrades to absent through its +// own revive, while the record that parsed cleanly ahead of the damage survives. +void drainUnaligned(ByteReader& r) { r.pos = r.bytes.size(); } + // Apply a hard-flag tail to an already-read velocity curve. A count that disagrees with the // 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 @@ -185,7 +210,10 @@ void readHardFlags(ByteReader& r, VelocityCurve& curve) { return; } const std::size_t remaining = r.bytes.size() > r.pos ? r.bytes.size() - r.pos : 0; - if (count > remaining) return; // bound-and-skip: cannot safely reserve/read this many + if (count > remaining) { + drainUnaligned(r); // the flags this count promised are not all there + return; + } std::vector flags; flags.reserve(count); for (std::uint32_t i = 0; i < count; ++i) flags.push_back(r.u8()); @@ -306,7 +334,7 @@ PayloadRead readLegacyZonePayload(ByteReader& r, std::uint32_t pv, double projec r.i32(); // lowNote — the retired key range; read to keep the record walk aligned r.i32(); // highNote const std::uint8_t hasOverride = r.u8(); - if (hasOverride) p.rootOverride = r.i32(); + if (hasOverride) p.rootOverride = clampMidiNote(r.i32()); if (extended) { const std::uint8_t hasLoop = r.u8(); if (hasLoop) { @@ -325,9 +353,11 @@ PayloadRead readLegacyZonePayload(ByteReader& r, std::uint32_t pv, double projec // are source-timeline, read as-is. A/D/S/R are ABSENT in v3 -> keep the defaults. assert(projectRate > 0.0 && "readLegacyZonePayload: projectRate must be > 0 for v3 lift"); const double liftRate = projectRate > 0.0 ? projectRate : 1.0; // avoids div-by-zero; assert fires first + const PlaySeconds fallback; // same guard as readSecondsPlayTail's peer fields p.play.playMode = (r.u8() != 0) ? PlayMode::Trigger : PlayMode::Gate; p.play.adsr.holdSeconds = static_cast(r.i64()) / liftRate; - p.play.trigger.lengthFraction = bitsToDouble(r.u64()); + p.play.trigger.lengthFraction = + finiteOr(bitsToDouble(r.u64()), fallback.trigger.lengthFraction); const std::int64_t fadeIn = r.i64(); const std::int64_t fadeOut = r.i64(); liftTriggerFades(fadeIn, fadeOut, liftRate, p.play.trigAhd); @@ -335,7 +365,8 @@ PayloadRead readLegacyZonePayload(ByteReader& r, std::uint32_t pv, double projec p.play.pitchEnv.enabled = (r.u8() != 0); p.play.pitchEnv.shape.attackSeconds = static_cast(r.i64()) / liftRate; p.play.pitchEnv.shape.decaySeconds = static_cast(r.i64()) / liftRate; - p.play.pitchEnv.peakSemitones = bitsToDouble(r.u64()); + p.play.pitchEnv.peakSemitones = + finiteOr(bitsToDouble(r.u64()), fallback.pitchEnv.peakSemitones); } else if (secondsPlay) { readSecondsPlayTail(r, p, projectRate); } @@ -457,7 +488,7 @@ PayloadRead readParamsPayload(ByteReader& r, double projectRate) { PayloadRead out; InstrumentParams& p = out.params; const std::uint8_t hasRoot = r.u8(); - if (hasRoot) p.rootOverride = r.i32(); + if (hasRoot) p.rootOverride = clampMidiNote(r.i32()); const std::uint8_t hasLoop = r.u8(); if (hasLoop) { SampleLoop lp; diff --git a/src/core/instrument/map/trigger_seam.cpp b/src/core/instrument/map/trigger_seam.cpp index c9a439c..1963bb2 100644 --- a/src/core/instrument/map/trigger_seam.cpp +++ b/src/core/instrument/map/trigger_seam.cpp @@ -10,8 +10,10 @@ std::int64_t triggerPlayLength(double lengthFraction, std::int64_t frameCount, std::int64_t startFrame) { const std::int64_t postStart = (std::max)(std::int64_t{0}, frameCount - startFrame); - if (postStart <= 0 || lengthFraction <= 0.0) return 0; - return static_cast(lengthFraction * static_cast(postStart) + 0.5); + if (postStart <= 0 || !(lengthFraction > 0.0)) return 0; // also catches NaN + const double frac = (std::min)(1.0, lengthFraction); + const auto len = static_cast(frac * static_cast(postStart) + 0.5); + return (std::min)(postStart, (std::max)(std::int64_t{0}, len)); } } // namespace reasampler::instrument::map diff --git a/src/core/instrument/map/trigger_seam.h b/src/core/instrument/map/trigger_seam.h index f795d10..8356b97 100644 --- a/src/core/instrument/map/trigger_seam.h +++ b/src/core/instrument/map/trigger_seam.h @@ -1,32 +1,23 @@ -// trigger_seam — the shared Trigger play-span formula: how the stored %-length becomes the -// source-frame span the voice plays, the overlay draws over, and the bake's window holds. -// One home so those three cannot disagree about where a Trigger note ends. +// trigger_seam — the shared Trigger play-span formula: how a %-length becomes the source-frame +// span the voice plays, the overlay draws over, and the bake's window holds. One home so those +// cannot disagree about where a Trigger note ends. // -// playLengthFrames = round(effectiveLengthFraction * (frameCount - startFrame)) +// playLengthFrames = round(clamp01(lengthFraction) * (frameCount - startFrame)) #pragma once #include -#include "core/instrument/engine/play_params.h" // splineActive (the %-knob's inert rule) - namespace reasampler::instrument::map { -// The %-length the voice ACTUALLY plays. A drawn contour is a pure time function over the -// full sample length, so any active spline EG folds the fraction to 1.0 and the stored knob -// goes inert (splineActive, play_params.h) — but the stored value survives, so a pre-spline -// setting is still there to be read. Every consumer of the span must fold it here or it -// silently plays/draws/bakes a fraction of the take. +// `startFrame` is the effective start point (0 when absent), `lengthFraction` the EFFECTIVE one +// — fold it through effectiveLengthFraction (play_params.h) first, or a stored-but-inert %-knob +// shortens the span. Returns 0 for an empty post-start span and for any fraction that is not +// above zero, NaN included; the fraction is clamped to 1.0 and the result to the span, so a +// corrupt stored value cannot reach past the source. // -// Templated over the two parameter representations for the same reason splineActive is. -template -double effectiveLengthFraction(const Play& p) { - return splineActive(p) ? 1.0 : p.trigger.lengthFraction; -} - -// postStart = max(0, frameCount - startFrame); playLength = round(lengthFraction * postStart). -// `startFrame` is the effective start point (0 when absent), `lengthFraction` the EFFECTIVE -// one above. Returns 0 when postStart == 0 or lengthFraction <= 0. +// Voice::start evaluates this same clamped formula inline rather than calling it: the engine +// does not depend on `map/`, in either direction. std::int64_t triggerPlayLength(double lengthFraction, std::int64_t frameCount, std::int64_t startFrame); diff --git a/src/core/instrument/note/CLAUDE.md b/src/core/instrument/note/CLAUDE.md index 7a7e903..a457c7a 100644 --- a/src/core/instrument/note/CLAUDE.md +++ b/src/core/instrument/note/CLAUDE.md @@ -13,7 +13,8 @@ is a performance *description* plus its arithmetic, read by two consumers that m diverge: the bake that renders it, and any surface that comes to edit it. Today the bake DERIVES the whole record from the dialed sound rather than asking for it — the one field a derivation cannot supply is the Gate-over-a-loop note length, and that arrives as a single -`Division` (see `bake/CLAUDE.md`), not as a hand-programmed record. +`Division` (see `bake/CLAUDE.md`), not as a hand-programmed record. Everything else it +derives is an exact duration; see the note-length invariant below. ## Invariants @@ -26,9 +27,17 @@ derivation cannot supply is the Gate-over-a-loop note length, and that arrives a directory — `Tempo` has no default and cannot be constructed without one. - **Resolved times are rate-free seconds.** The standing ruling: no sample rate appears here; the caller converts seconds to frames against the live rate. -- **Note length is musical-division-only.** Offsets carry the ms/beats duality; the note - length does not. A free-duration note length would make two records describe the same - performance at one tempo and different performances at another. +- **A note length is EITHER a musical division or an exact duration, and which one says who + produced it.** *(Amends the earlier "musical-division-only" rule, which was settled when + every length was hand-programmed through a picker.)* A PICKED length stays a `Division`: a + picker's rungs are the point, and the tempo-relative reading — the same record meaning a + different duration at a different tempo — is what the user asked for. A DERIVED length is + exact seconds, because the ladder is finite: a source longer than its top rung has no rung + that covers it, so quantizing up saturates and releases the note mid-sound, and on every + shorter source it buys trailing silence for nothing. `NoteLength` holds one or the other and + `noteLengthSeconds` resolves both, so no reader can pick the wrong denomination. Today the + bake is the only producer of each: its window derivations are exact, and the Gate-over-a-loop + Hold knob is the one picker. - **A division persists as its `{quarterExponent, modifier}` pair, never as its picker index.** The index is presentation order and would silently re-map every saved record if the ladder ever gained a rung or a modifier. @@ -64,9 +73,9 @@ derivation cannot supply is the Gate-over-a-loop note length, and that arrives a to. `fromBpm` validates by running the extreme conversions rather than by testing the `60/bpm` reciprocal they start from — that reciprocal stays finite well past the point the multiply after it overflows. -- `note_program` — `Velocity` (clamped 1..127), the denominated `OffsetAmount` and its unit - toggle, the anchored `StartOffset` / `EndOffset`, the `NoteProgram` record, and - `resolveNote`. +- `note_program` — `Velocity` (clamped 1..127), the two-denomination `NoteLength` and its + resolver, the denominated `OffsetAmount` and its unit toggle, the anchored `StartOffset` / + `EndOffset`, the `NoteProgram` record, and `resolveNote`. ## Gotchas diff --git a/src/core/instrument/note/musical_division.cpp b/src/core/instrument/note/musical_division.cpp index 716f959..deec8ab 100644 --- a/src/core/instrument/note/musical_division.cpp +++ b/src/core/instrument/note/musical_division.cpp @@ -68,7 +68,10 @@ Division shortestDivisionAtLeast(double beats) { if (b > longestBeats) { longest = d; longestBeats = b; } if (b >= beats && (!found || b < bestBeats)) { best = d; bestBeats = b; found = true; } } - if (!(beats > 0.0)) return divisionAt(0); // also catches NaN + // NaN names no length to be at least, so it takes the never-short direction rather than + // the bottom rung: a corrupt value must not resolve to a near-instant note. + if (std::isnan(beats)) return longest; + if (!(beats > 0.0)) return divisionAt(0); return found ? best : longest; } diff --git a/src/core/instrument/note/musical_division.h b/src/core/instrument/note/musical_division.h index 9bda5a1..e2257b1 100644 --- a/src/core/instrument/note/musical_division.h +++ b/src/core/instrument/note/musical_division.h @@ -69,10 +69,11 @@ double divisionBeats(Division d); Division divisionAt(int index); int divisionIndex(Division d); -// The shortest ladder length that is at least `beats` — the rung a caller reaches for when a -// derived duration has to be expressed as a programmable note and overshooting is the safe -// direction. Nothing long enough (or a non-finite `beats`) yields the top rung; a -// non-positive one yields the bottom. +// The shortest ladder length that is at least `beats` — for a caller quantizing a duration ONTO +// the ladder, where overshooting is the safe direction. It is NOT how a derived duration reaches +// the bake: nothing on a finite ladder covers an arbitrarily long source (see note/CLAUDE.md). +// Nothing long enough, or a non-finite `beats`, yields the top rung; a non-positive one yields +// the bottom. Division shortestDivisionAtLeast(double beats); // The notation divisions are named in: "1/16", "1/8.", "1/4t", "4/1". diff --git a/src/core/instrument/note/note_program.cpp b/src/core/instrument/note/note_program.cpp index f2b1d73..c8df434 100644 --- a/src/core/instrument/note/note_program.cpp +++ b/src/core/instrument/note/note_program.cpp @@ -31,6 +31,26 @@ OffsetAmount offsetOf(double magnitude, Denomination denomination) { return OffsetAmount(bounded, named ? denomination : Denomination::Milliseconds); } +NoteLength lengthOfDivision(Division division) { return NoteLength(division, 0.0, false); } + +NoteLength lengthOfSeconds(double seconds) { + const double bounded = std::isnan(seconds) + ? 0.0 + : (std::max)(0.0, (std::min)(kMaxLengthSeconds, seconds)); + return NoteLength(Division{}, bounded, true); +} + +bool operator==(NoteLength a, NoteLength b) { + if (a.exact_ != b.exact_) return false; + return a.exact_ ? a.seconds_ == b.seconds_ : a.division_ == b.division_; +} + +bool operator!=(NoteLength a, NoteLength b) { return !(a == b); } + +double noteLengthSeconds(NoteLength length, Tempo tempo) { + return length.exact_ ? length.seconds_ : tempo.beatsToSeconds(divisionBeats(length.division_)); +} + OffsetAmount offsetFromMs(double ms) { return offsetOf(ms, Denomination::Milliseconds); } OffsetAmount offsetFromBeats(double beats) { return offsetOf(beats, Denomination::Beats); } @@ -94,7 +114,7 @@ bool operator!=(const NoteProgram& a, const NoteProgram& b) { return !(a == b); ResolvedNote resolveNote(const NoteProgram& program, Tempo tempo) { ResolvedNote out; - out.noteOffSeconds = tempo.beatsToSeconds(divisionBeats(program.length)); + out.noteOffSeconds = noteLengthSeconds(program.length, tempo); out.captureStartSeconds = offsetSeconds(program.start.amount(), tempo); const double rawEndSeconds = out.noteOffSeconds + offsetSeconds(program.end.amount(), tempo); // An inverted window has no meaning to a renderer, so a far-negative end offset yields a diff --git a/src/core/instrument/note/note_program.h b/src/core/instrument/note/note_program.h index 148f53b..eca29e1 100644 --- a/src/core/instrument/note/note_program.h +++ b/src/core/instrument/note/note_program.h @@ -85,6 +85,47 @@ OffsetAmount redenominate(OffsetAmount amount, Denomination to, Tempo tempo); OffsetAmount withMsView(OffsetAmount amount, double ms, Tempo tempo); OffsetAmount withBeatsView(OffsetAmount amount, double beats, Tempo tempo); +// The largest exact note length, in seconds — the ms ceiling restated in the unit an exact +// length is entered in, so a length and an offset cap at the same instant. +inline constexpr double kMaxLengthSeconds = msToSeconds(kMaxConvertibleMagnitude); + +class NoteLength; + +// The two doors. A DERIVED length is exact: `lengthOfSeconds` is what every computed duration +// takes, and rounding one onto the ladder is what truncates a source longer than the ladder's +// top rung. A PICKED length is a `Division`, because a picker's rungs are the point. Both +// clamp: a negative or NaN duration names no length and becomes zero, a magnitude past +// kMaxLengthSeconds clamps to it, and `makeDivision` already holds the rung's own domain. +NoteLength lengthOfDivision(Division division); +NoteLength lengthOfSeconds(double seconds); + +// One length, in whichever denomination its door established. There is no accessor per +// denomination: `noteLengthSeconds` resolves both, so no reader can pick the wrong one. +class NoteLength { +public: + NoteLength() = default; // a quarter note — Division's own default + +private: + NoteLength(Division division, double seconds, bool exact) + : division_(division), seconds_(seconds), exact_(exact) {} + friend NoteLength lengthOfDivision(Division division); + friend NoteLength lengthOfSeconds(double seconds); + friend double noteLengthSeconds(NoteLength length, Tempo tempo); + friend bool operator==(NoteLength a, NoteLength b); + + Division division_{}; + double seconds_ = 0.0; + bool exact_ = false; +}; + +static_assert(!std::is_constructible_v, + "lengthOfDivision must be the only way to give a NoteLength a rung"); + +bool operator==(NoteLength a, NoteLength b); +bool operator!=(NoteLength a, NoteLength b); + +double noteLengthSeconds(NoteLength length, Tempo tempo); + // Two types rather than one carrying an anchor field: the anchor is then unswappable at // compile time. Sign is uniform — positive is later in time — so a capture that opens before // the note is a negative start offset, and a negative end offset truncates before release. @@ -114,7 +155,7 @@ static_assert(!std::is_convertible_v, "the anchor constructor must stay explicit"); struct NoteProgram { - Division length{}; + NoteLength length{}; StartOffset start{}; EndOffset end{}; Velocity velocity{}; diff --git a/src/core/instrument/ui/bake_hold.cpp b/src/core/instrument/ui/bake_hold.cpp index 6d1efad..208c37f 100644 --- a/src/core/instrument/ui/bake_hold.cpp +++ b/src/core/instrument/ui/bake_hold.cpp @@ -3,23 +3,58 @@ #include "core/instrument/ui/bake_hold.h" #include +#include namespace reasampler::instrument::ui { namespace { -constexpr int kLastIndex = note::kDivisionCount - 1; + +constexpr int kLastSlot = note::kDivisionCount - 1; + +using Order = std::array; + +// Slot -> picker index, sorted by LENGTH. The ladder's own order is presentation order, in +// which a rung's triplet is shorter than the previous rung's dotted (musical_division.cpp) — +// so addressing it directly makes a knob whose whole meaning is duration shorten the note at +// every rung boundary. Built once; every length on the ladder is distinct, so the sort is total. +const Order& bySlot() { + static const Order order = [] { + Order a{}; + for (int i = 0; i < note::kDivisionCount; ++i) a[static_cast(i)] = i; + std::sort(a.begin(), a.end(), [](int l, int r) { + return note::divisionBeats(note::divisionAt(l)) < + note::divisionBeats(note::divisionAt(r)); + }); + return a; + }(); + return order; +} + +// The inverse: picker index -> slot. +const Order& toSlot() { + static const Order inverse = [] { + Order a{}; + const Order& forward = bySlot(); + for (int slot = 0; slot < note::kDivisionCount; ++slot) + a[static_cast(forward[static_cast(slot)])] = slot; + return a; + }(); + return inverse; +} + } // namespace note::Division bakeHoldFromNorm(double norm) { - if (!(norm > 0.0)) return note::divisionAt(0); // also catches NaN - if (norm >= 1.0) return note::divisionAt(kLastIndex); - // Round to nearest so each rung owns an equal slice of the knob's travel; divisionAt - // clamps, so the +0.5 landing on kDivisionCount at norm just under 1 is harmless. - return note::divisionAt(static_cast(norm * kLastIndex + 0.5)); + if (!(norm > 0.0)) return note::divisionAt(bySlot()[0]); // also catches NaN + if (norm >= 1.0) return note::divisionAt(bySlot()[kLastSlot]); + // Round to nearest so each rung owns an equal slice of the knob's travel. + const int slot = static_cast(norm * kLastSlot + 0.5); + return note::divisionAt(bySlot()[static_cast(slot)]); } double bakeHoldNorm(note::Division hold) { - return static_cast(note::divisionIndex(hold)) / static_cast(kLastIndex); + const int slot = toSlot()[static_cast(note::divisionIndex(hold))]; + return static_cast(slot) / static_cast(kLastSlot); } } // namespace reasampler::instrument::ui diff --git a/src/core/instrument/ui/bake_hold.h b/src/core/instrument/ui/bake_hold.h index a4ef834..a2120b0 100644 --- a/src/core/instrument/ui/bake_hold.h +++ b/src/core/instrument/ui/bake_hold.h @@ -1,14 +1,16 @@ #pragma once // bake_hold — how one radial knob addresses the note-length ladder: the Hold control's -// normalized [0,1] position mapped onto a Division and back. The ladder and its picker order -// are musical_division's; nothing about them is restated here. +// normalized [0,1] position mapped onto a Division and back. The ladder itself is +// musical_division's; only the knob's travel ORDER is decided here. #include "core/instrument/note/musical_division.h" namespace reasampler::instrument::ui { -// [0,1] across the ladder in picker order (shortest rung first). Out-of-range or non-finite -// input clamps to an end rather than wrapping — a knob cannot express anything else. +// [0,1] across the ladder ordered by LENGTH, shortest first — NOT the ladder's picker order, +// which is presentation order and would shorten the note at every rung boundary. Out-of-range +// or non-finite input clamps to an end rather than wrapping — a knob cannot express anything +// else. note::Division bakeHoldFromNorm(double norm); // The inverse: the position that reproduces `hold` exactly, so a knob painted from a stored diff --git a/src/core/instrument/ui/sample_chrome.h b/src/core/instrument/ui/sample_chrome.h index 1c5ae00..bf66034 100644 --- a/src/core/instrument/ui/sample_chrome.h +++ b/src/core/instrument/ui/sample_chrome.h @@ -19,9 +19,11 @@ struct ChromeRects { Rect toolbar; // full-width top row Rect title; // the title text slot: the toolbar left of the control run // The bake Hold cell. Its width is RESERVED unconditionally even though the control is - // only load-bearing for a looped Gate sound (bake_plan.h's bakeWindowNeedsHold): the run - // is right-anchored, so laying it out conditionally would slide Bake and Preview out from - // under the pointer whenever a loop is dialled in or out. + // only drawn for a looped Gate sound (bake_plan.h's bakeWindowNeedsHold). It is the + // LEFTMOST member of the right-anchored run, so no other member moves either way — what + // the reservation buys is a title slot that holds still: dialling a loop in or out would + // otherwise re-measure and re-ellipsize the capture name. The cost is ~62 px of dead + // toolbar in the common non-looped case. Rect holdCell; // ---- the right-anchored run, left to right ---- Rect holdKnob; Rect holdLabel; diff --git a/src/shell/instrument/editor_input.cpp b/src/shell/instrument/editor_input.cpp index ae7c8db..56d7b09 100644 --- a/src/shell/instrument/editor_input.cpp +++ b/src/shell/instrument/editor_input.cpp @@ -118,6 +118,13 @@ void ReaSamplerEditor::onMouseUp(int x, int y) { invalidate(); return; } + // A bare click on the Hold cell — no drag, no value change — would otherwise buy a bridge + // read, a WAV re-decode and an engine rebuild for a parameter the engine never reads. + if (kind == DragKind::kDeckKnob && paramId == kBakeHoldKnobId && + params_.bakeHold == dragStartParams_.bakeHold) { + invalidate(); + return; + } // A live control already reached the voices during the drag; its release commits the // final value through the same tier. if (dragCommitsLive(kind, paramId)) { diff --git a/src/shell/instrument/editor_input_chrome.cpp b/src/shell/instrument/editor_input_chrome.cpp index 599b6eb..fc9fa1b 100644 --- a/src/shell/instrument/editor_input_chrome.cpp +++ b/src/shell/instrument/editor_input_chrome.cpp @@ -109,9 +109,13 @@ bool ReaSamplerEditor::doubleClickChrome(const FaceLayout& fl, int x, int y) { if (selectedId_.empty() || !processor_) return false; if (bakeHoldNeeded_ && inKnobFace(fl.chrome.holdKnob, x, y)) { // The default is READ off a default-constructed parameter set, so there is no second - // table of defaults to drift from the codec's own lift. - params_.bakeHold = InstrumentParams{}.bakeHold; - commitAndReload(); + // table of defaults to drift from the codec's own lift. A reset that changes nothing + // commits nothing — the same rule the release path applies. + const instrument::note::Division reset = InstrumentParams{}.bakeHold; + if (params_.bakeHold != reset) { + params_.bakeHold = reset; + commitAndReload(); + } invalidate(); return true; } diff --git a/src/shell/instrument/editor_paint_chrome.cpp b/src/shell/instrument/editor_paint_chrome.cpp index e4653a4..0abb6ba 100644 --- a/src/shell/instrument/editor_paint_chrome.cpp +++ b/src/shell/instrument/editor_paint_chrome.cpp @@ -156,9 +156,11 @@ void ReaSamplerEditor::paintChrome(LICE_IBitmap* bmp, const FaceLayout& fl, bool : (hov ? InteractionState::Hover : InteractionState::Rest); drawKnobFace(bmp, cr.holdKnob, bakeHoldNorm(), st); + // "Bake Hold", not "Hold": the AMP deck's AHDSR Hold knob is on screen in the same + // frame, and two knobs labelled the same are two knobs the user has to guess between. const std::string label = dragging || hov ? instrument::note::divisionLabel(params_.bakeHold) - : std::string("Hold"); + : std::string("Bake Hold"); kitTextCentered(bmp, cr.holdLabel, label.c_str(), Font::Micro, Role::TextDim); } diff --git a/src/shell/instrument/editor_platform.cpp b/src/shell/instrument/editor_platform.cpp index 4ff47ef..2406806 100644 --- a/src/shell/instrument/editor_platform.cpp +++ b/src/shell/instrument/editor_platform.cpp @@ -24,9 +24,11 @@ constexpr const wchar_t* kChildClassName = L"ReaSampler9000VstEditor"; // The change-detection poll (WM_TIMER on the child window). A low-frequency UI-thread // timer: responsive enough that a recapture/ingest/assign refreshes within a bounded -// cadence, yet cheap — three small ext-state reads per tick, coalescing many bumps -// between ticks into one reload. 500 ms is a deliberate build-time residual. The id is a -// per-window SetTimer id (any nonzero). +// cadence, yet cheap — three small ext-state reads per tick in the steady state, coalescing +// many bumps between ticks into one reload. Anything costlier a tick answers (the bake-Hold +// predicate's bank parse) is memoized against its inputs, so keep it that way rather than +// letting a per-tick full read back in. 500 ms is a deliberate build-time residual. The id is +// a per-window SetTimer id (any nonzero). constexpr UINT_PTR kSyncTimerId = 1; constexpr UINT kSyncTimerIntervalMs = 500; } // namespace diff --git a/src/shell/instrument/editor_session.cpp b/src/shell/instrument/editor_session.cpp index d0eee4e..7c51cad 100644 --- a/src/shell/instrument/editor_session.cpp +++ b/src/shell/instrument/editor_session.cpp @@ -57,6 +57,7 @@ void ReaSamplerEditor::refreshFromBank() { // Main/UI thread only — reads the live bank over the bridge (allocates, calls REAPER). thumbCache_.clear(); // a bank edit may have re-captured/removed a sample; drop stale peaks pcmCache_.clear(); // and its decoded PCM (the waveform + snap source) + holdNeedValid_ = false; // …and the bake-Hold answer derived from the bank's loop intrinsic channelPcmId_.clear(); channelPcm_ = ChannelPcm{}; if (!processor_) { @@ -262,16 +263,43 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram return m; } +bool ReaSamplerEditor::HoldNeedKey::operator==(const HoldNeedKey& o) const { + if (sampleId != o.sampleId || crossfade != o.crossfade) return false; + if (loopOverride.has_value() != o.loopOverride.has_value()) return false; + if (!loopOverride) return true; + return loopOverride->hasLoop == o.loopOverride->hasLoop && + loopOverride->start == o.loopOverride->start && + loopOverride->end == o.loopOverride->end; +} + bool ReaSamplerEditor::resolveBakeHoldNeeded() { - // The mode test comes first so the common Trigger case never pays pickedMarkers' bridge - // read. `m.hasLoop` alone is not the answer — the engine's fold also refuses a span that - // reaches outside the PCM, which is exactly what the pure predicate is asked for. - if (selectedId_.empty() || params_.play.playMode != PlayMode::Gate) return false; + // effectivePlayMode, not the raw field: the bake reads the mode AFTER the drawn-EG fold + // (resolvePlay, via bakeSnapshot), so a restored or foreign blob carrying Gate together + // with an active spline would otherwise paint a control for a sound that bakes as Trigger. + // The mode test comes first so the common Trigger case never pays the resolve below. + const PlayMode mode = effectivePlayMode(params_.play); + if (selectedId_.empty() || mode != PlayMode::Gate) return false; + + // Everything past here costs a WAV decode and — with no loop override set — a bridge read + // plus a bank parse, and the sync tick asks twice a second for as long as an editor is + // open. Answer once per distinct input; refreshFromBank drops the memo along with the rest + // of the bank-derived caches, which is the same edge a bank generation bump would give. + // The mode is not part of the key: the resolve below always asks about Gate, and the test + // above is what keeps a Trigger sound from reaching it. + const HoldNeedKey key{selectedId_, params_.loopOverride, params_.loopCrossfadeFrames}; + if (holdNeedValid_ && key == holdNeedKey_) return holdNeedAnswer_; + holdNeedValid_ = true; + holdNeedKey_ = key; + holdNeedAnswer_ = false; + const auto frames = static_cast(monoPcmFor(selectedId_).size()); if (frames <= 0) return false; + // `m.hasLoop` alone is not the answer — the engine's fold also refuses a span that reaches + // outside the PCM, which is exactly what the pure predicate is asked for. const SetupMarkers m = pickedMarkers(frames); - return instrument::bake::bakeWindowNeedsHold( + holdNeedAnswer_ = instrument::bake::bakeWindowNeedsHold( PlayMode::Gate, SampleLoop{m.hasLoop, m.loopStart, m.loopEnd}, m.crossfade, frames); + return holdNeedAnswer_; } void ReaSamplerEditor::applyMarkers(const SetupMarkers& m) { diff --git a/src/shell/instrument/instrument_bake.cpp b/src/shell/instrument/instrument_bake.cpp index 3612c61..fc8d2ab 100644 --- a/src/shell/instrument/instrument_bake.cpp +++ b/src/shell/instrument/instrument_bake.cpp @@ -132,8 +132,7 @@ BakeChainResult runBake(ReaSamplerProcessor& processor) { // are live, so a sound dialed at 127 does not bake as one dialed at 40. const Velocity velocity = Velocity::of(processor.previewVelocity()); const instrument::bake::PlannedBake planned = planBake( - resolveNote(defaultBakeProgram(*snapshot, sampleRate, *tempo, dialed.bakeHold, - velocity), + resolveNote(defaultBakeProgram(*snapshot, sampleRate, dialed.bakeHold, velocity), *tempo), sampleRate, rootNote); if (!planned.plan) { diff --git a/src/shell/instrument/reasampler_editor.h b/src/shell/instrument/reasampler_editor.h index d557acb..635068e 100644 --- a/src/shell/instrument/reasampler_editor.h +++ b/src/shell/instrument/reasampler_editor.h @@ -342,9 +342,22 @@ private: // Whether the loaded sound's bake window needs the user's Hold — the pure predicate // (bake_plan.h) answered against the markers this face is showing. Decodes and reads the - // bank, so it is called on the sync tick, not per paint. + // bank, so it is called on the sync tick, not per paint, and memoized against the inputs + // below on top of that. bool resolveBakeHoldNeeded(); + // What that answer was last computed against. Invalidated wholesale by refreshFromBank, + // which is where the bank half of the input changes. + struct HoldNeedKey { + std::string sampleId; + std::optional loopOverride; + std::int64_t crossfade = 0; + bool operator==(const HoldNeedKey& other) const; + }; + HoldNeedKey holdNeedKey_; + bool holdNeedValid_ = false; + bool holdNeedAnswer_ = false; + // Writes `m` into params_ as the loop/start override. Does NOT call commitAndReload — // callers decide live-drag vs final commit. void applyMarkers(const SetupMarkers& m); diff --git a/tests/test_bake_hold.cpp b/tests/test_bake_hold.cpp index 334f4a2..c39287c 100644 --- a/tests/test_bake_hold.cpp +++ b/tests/test_bake_hold.cpp @@ -2,7 +2,8 @@ // note-length ladder. No VST3, no REAPER, no framework. // // Covers: both ends of the knob, the round trip from every rung, out-of-range and non-finite -// input, and that every rung is reachable (no rung is skipped by the rounding). +// input, that every rung is reachable (no rung is skipped by the rounding), that the travel is +// monotone in DURATION, and that each rung owns an equal slice of it. #include "../src/core/instrument/ui/bake_hold.h" @@ -17,15 +18,37 @@ static int g_fail = 0; #define CHECK(cond) do { if(!(cond)) { \ std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) +namespace { + +// The shortest and longest lengths ON THE LADDER, found by scanning rather than by indexing an +// end of the picker order — which is exactly the assumption under test. +Division shortestRung() { + Division best = divisionAt(0); + for (int i = 1; i < kDivisionCount; ++i) + if (divisionBeats(divisionAt(i)) < divisionBeats(best)) best = divisionAt(i); + return best; +} + +Division longestRung() { + Division best = divisionAt(0); + for (int i = 1; i < kDivisionCount; ++i) + if (divisionBeats(divisionAt(i)) > divisionBeats(best)) best = divisionAt(i); + return best; +} + +} // namespace + int main() { - // --- The ends of the travel are the ends of the ladder --------------------------- - CHECK(bakeHoldFromNorm(0.0) == divisionAt(0)); - CHECK(bakeHoldFromNorm(1.0) == divisionAt(kDivisionCount - 1)); + // --- The ends of the travel are the SHORTEST and LONGEST notes ------------------- + // Not divisionAt(0) / divisionAt(kDivisionCount - 1): those are the ends of the picker's + // presentation order, which is not length order. + CHECK(bakeHoldFromNorm(0.0) == shortestRung()); + CHECK(bakeHoldFromNorm(1.0) == longestRung()); // --- Out of range clamps rather than wrapping ------------------------------------ - CHECK(bakeHoldFromNorm(-3.0) == divisionAt(0)); - CHECK(bakeHoldFromNorm(9.5) == divisionAt(kDivisionCount - 1)); - CHECK(bakeHoldFromNorm(std::nan("")) == divisionAt(0)); + CHECK(bakeHoldFromNorm(-3.0) == shortestRung()); + CHECK(bakeHoldFromNorm(9.5) == longestRung()); + CHECK(bakeHoldFromNorm(std::nan("")) == shortestRung()); // --- Round trip: a knob painted from a stored rung and released reproduces it ----- for (int i = 0; i < kDivisionCount; ++i) { @@ -44,13 +67,34 @@ int main() { for (int i = 0; i < kDivisionCount; ++i) CHECK(seen[static_cast(i)]); } - // --- The map is monotone: turning the knob up never shortens the note ------------- + // --- The map is monotone IN DURATION: turning the knob up never shortens the note -- + // Asserted over divisionBeats, not divisionIndex: the index is presentation order, in which + // a rung's triplet sits after (and is shorter than) the previous rung's dotted — so an + // index sweep can be non-decreasing while the note it selects gets shorter. { - int previous = -1; + double previous = 0.0; for (int step = 0; step <= 4000; ++step) { - const int index = divisionIndex(bakeHoldFromNorm(static_cast(step) / 4000.0)); - CHECK(index >= previous); - previous = index; + const double beats = + divisionBeats(bakeHoldFromNorm(static_cast(step) / 4000.0)); + CHECK(beats >= previous); + previous = beats; + } + } + + // --- Each rung owns an EQUAL slice, centred on its own position ------------------- + // A floor-based map passes every property above while giving each rung the slice ABOVE its + // position; only the boundaries tell the two apart. Slice k spans [(k-0.5)/L, (k+0.5)/L). + { + constexpr int last = kDivisionCount - 1; + const double slice = 1.0 / static_cast(last); + const double eps = slice / 1000.0; + for (int k = 1; k < last; ++k) { + const double centre = static_cast(k) * slice; + CHECK(bakeHoldFromNorm(centre) == bakeHoldFromNorm(centre - slice * 0.5 + eps)); + CHECK(bakeHoldFromNorm(centre) == bakeHoldFromNorm(centre + slice * 0.5 - eps)); + // …and one step past the upper boundary is already the NEXT rung. + CHECK(divisionBeats(bakeHoldFromNorm(centre + slice * 0.5 + eps)) > + divisionBeats(bakeHoldFromNorm(centre))); } } diff --git a/tests/test_bake_plan.cpp b/tests/test_bake_plan.cpp index 29082cc..2770f70 100644 --- a/tests/test_bake_plan.cpp +++ b/tests/test_bake_plan.cpp @@ -2,7 +2,8 @@ // framework. Same fast assert loop as the sibling pure tests. // // Covers: the default program's window derived from the dialed sound (Gate's hold to source -// exhaustion plus its release, a Trigger play span, and the Varispeed read-stretch bound); +// exhaustion plus its release, a start point, a Trigger play span, and the Varispeed +// read-stretch bound on both branches that take it); // the frame window and both event frames against hand-computed values; a capture opening // BEFORE note-on and one opening AFTER it; the refusals and what each one reports — a // collapsed window, a non-positive rate, a window that rounds to nothing, and one past the @@ -45,8 +46,10 @@ SampleData dialedSample(std::size_t frames = 96000) { // The Hold default; read only where the window is underivable, which is nowhere in this file. Division oneBar() { return makeDivision(2, DivisionModifier::Straight); } -NoteProgram derived(const SampleData& s, Tempo t) { - return defaultBakeProgram(s, kRate, t, oneBar(), Velocity{}); +// `t` is unused by the derivation itself — it takes no tempo — but every caller here resolves +// against the same one, so threading it keeps each case's two halves visibly paired. +NoteProgram derived(const SampleData& s, Tempo) { + return defaultBakeProgram(s, kRate, oneBar(), Velocity{}); } bool near(double a, double b) { return a > b - 1e-6 && a < b + 1e-6; } @@ -60,8 +63,8 @@ int main() { s.play.playMode = PlayMode::Gate; s.play.adsr.releaseFrames = kRate * 3 / 2; // 1.5 s — past any fixed tail const ResolvedNote r = resolveNote(derived(s, at(120.0)), at(120.0)); - // The note is rounded up to the shortest length outlasting the source — 2 s exactly, - // one bar at 120 BPM — instead of the constant quarter note that released it early. + // The note is held to exhaustion — 2 s exactly — instead of the constant quarter note + // that released it early. CHECK(near(r.noteOffSeconds, 2.0)); CHECK(r.captureStartSeconds == 0.0); CHECK(near(r.captureEndSeconds, 2.0 + 1.5 + kPadSeconds)); @@ -72,14 +75,50 @@ int main() { const ResolvedNote shorter = resolveNote(derived(s, at(120.0)), at(120.0)); CHECK(near(shorter.captureEndSeconds, 2.0 + 0.1 + kPadSeconds)); - // A source that does not land on a rung rounds UP. 2.5 s is 5 beats, and the shortest - // rung at or above that is the 2/1 triplet (8 * 2/3 == 5.33 beats) — NOT the dotted - // half above it, which is why picker order is not length order. + // The length is EXACT, not rounded onto the ladder: 2.5 s is 5 beats, which no rung + // hits — the ladder's nearest never-short answer is the 2/1 triplet at 5.33 beats, and + // taking it would buy a third of a second of silence for nothing. The tempo is not an + // input to a derived length at all, so the same sound derives the same seconds at any. SampleData odd = dialedSample(static_cast(kRate * 5 / 2)); // 2.5 s odd.play.playMode = PlayMode::Gate; - const ResolvedNote up = resolveNote(derived(odd, at(120.0)), at(120.0)); - CHECK(up.noteOffSeconds >= 2.5); // the property that matters: never short - CHECK(near(up.noteOffSeconds, at(120.0).beatsToSeconds(8.0 * 2.0 / 3.0))); + CHECK(near(resolveNote(derived(odd, at(120.0)), at(120.0)).noteOffSeconds, 2.5)); + CHECK(near(resolveNote(derived(odd, at(97.0)), at(97.0)).noteOffSeconds, 2.5)); + } + + // --- Gate with no loop, a START POINT set: only the post-start span is held ---------- + // effectiveStart's whole reason to exist. A start marker means the head begins there, so + // holding the note for the WHOLE source would pad the window with silence the voice + // already finished; holding it for the source minus the start is exact. + { + SampleData s = dialedSample(/*frames=*/kRate * 2); // 2 s + s.play.playMode = PlayMode::Gate; + s.play.adsr.releaseFrames = 0; + s.startFrame = kRate / 2; // start half a second in -> 1.5 s of sound left + CHECK(near(resolveNote(derived(s, at(120.0)), at(120.0)).noteOffSeconds, 1.5)); + + // Voice::start's own degenerate rule: a start at or past the end plays from the top, + // so the window covers the whole source rather than nothing. + s.startFrame = kRate * 2; + CHECK(near(resolveNote(derived(s, at(120.0)), at(120.0)).noteOffSeconds, 2.0)); + s.startFrame = -1; + CHECK(near(resolveNote(derived(s, at(120.0)), at(120.0)).noteOffSeconds, 2.0)); + } + + // --- Gate with no loop under Varispeed: the read-stretch bound applies here too ------- + // The Trigger branch has its own coverage below; this pins that the Gate exhaustion length + // takes the same bound, which is the branch a downward offset would otherwise truncate. + { + SampleData s = dialedSample(/*frames=*/kRate); // 1 s + s.play.playMode = PlayMode::Gate; + s.play.adsr.releaseFrames = 0; + s.play.pitchEngine = PitchEngine::Varispeed; + s.play.pitchEnv.enabled = true; + s.play.pitchEnv.peakSemitones = -12.0; // an octave down == half speed at the peak + CHECK(near(resolveNote(derived(s, at(120.0)), at(120.0)).noteOffSeconds, 2.0)); + + // Preserve reads at the source rate, so the same dial bounds nothing. + s.play.pitchEngine = PitchEngine::Preserve; + CHECK(near(resolveNote(derived(s, at(120.0)), at(120.0)).noteOffSeconds, 1.0)); } // --- Trigger: the window is the play span, which ignores the note's length ---------- @@ -91,8 +130,8 @@ int main() { CHECK(r.captureStartSeconds == 0.0); CHECK(near(r.captureEndSeconds, 1.5 + kPadSeconds)); - // A span SHORTER than the quarter note closes the window early rather than padding - // it out to note-off — the sound is over, and a negative end offset is legal. + // A shorter %-length is a shorter window, with no floor under it: the sound is over + // when the span is, and note-off means nothing to a Trigger voice. s.play.trigger.lengthFraction = 0.1; // 0.2 s const ResolvedNote brief = resolveNote(derived(s, at(120.0)), at(120.0)); CHECK(!brief.windowCollapsed); @@ -127,9 +166,9 @@ int main() { {{0.0, -0.5}, {127.0, 0.0}}, reasampler::instrument::engine::CurveDomain::Bipolar); const NoteProgram soft = - defaultBakeProgram(s, kRate, at(120.0), oneBar(), Velocity::of(1)); + defaultBakeProgram(s, kRate, oneBar(), Velocity::of(1)); const NoteProgram hard = - defaultBakeProgram(s, kRate, at(120.0), oneBar(), Velocity::of(127)); + defaultBakeProgram(s, kRate, oneBar(), Velocity::of(127)); CHECK(soft.velocity.value() == 1); CHECK(hard.velocity.value() == 127); const ResolvedNote softR = resolveNote(soft, at(120.0)); @@ -148,9 +187,9 @@ int main() { CHECK(bakeWindowNeedsHold(s)); // The window follows Hold rather than the source, so a longer Hold is a longer file. const ResolvedNote bar = resolveNote( - defaultBakeProgram(s, kRate, at(120.0), oneBar(), Velocity{}), at(120.0)); + defaultBakeProgram(s, kRate, oneBar(), Velocity{}), at(120.0)); const ResolvedNote twoBars = resolveNote( - defaultBakeProgram(s, kRate, at(120.0), + defaultBakeProgram(s, kRate, makeDivision(3, DivisionModifier::Straight), Velocity{}), at(120.0)); CHECK(near(bar.noteOffSeconds, 2.0)); diff --git a/tests/test_bake_window.cpp b/tests/test_bake_window.cpp index c8720ad..02e43b9 100644 --- a/tests/test_bake_window.cpp +++ b/tests/test_bake_window.cpp @@ -10,6 +10,7 @@ #include #include +#include using namespace reasampler; using namespace reasampler::instrument::bake; @@ -34,12 +35,14 @@ constexpr std::int64_t kPad = kDeclickFrames; // expectations below read as arithmetic rather than as magic. Division oneBar() { return makeDivision(2, DivisionModifier::Straight); } -Tempo tempo() { - const std::optional t = Tempo::fromBpm(kBpm); - if (!t) { std::printf("FAIL: fixture tempo rejected\n"); ++g_fail; } +Tempo tempoOf(double bpm) { + const std::optional t = Tempo::fromBpm(bpm); + if (!t) { std::printf("FAIL: fixture tempo %f rejected\n", bpm); ++g_fail; } return t.value_or(Tempo::fromBpm(120.0).value()); } +Tempo tempo() { return tempoOf(kBpm); } + // Flat DC so a level reading is unambiguous: any departure from 0.5 is the envelope, the // filter or a ring-out, never the source's own shape. SampleData dcSample(std::size_t frames) { @@ -66,7 +69,7 @@ double peakAt(const BakeAudio& audio, std::int64_t from, std::int64_t to) { // comparison a measurement of the derived end rather than of a second derivation. NoteProgram derivedProgram(const SampleData& s, double extraMs, Division hold = oneBar(), int velocity = 100) { - NoteProgram p = defaultBakeProgram(s, kRate, tempo(), hold, Velocity::of(velocity)); + NoteProgram p = defaultBakeProgram(s, kRate, hold, Velocity::of(velocity)); if (extraMs != 0.0) p.end = EndOffset(offsetFromMs(offsetMs(p.end.amount(), tempo()) + extraMs)); return p; @@ -136,6 +139,63 @@ int main() { CHECK(peakAt(wide, 192000, wide.frameCount()) == 0.0); } + // --- Gate, no loop, a source LONGER than the note-length ladder --------------------- + // The regression the exact-duration seam exists for. A derived length used to be rounded + // onto the musical-division ladder, whose longest rung is kMaxDivisionBeats (384 beats): + // a source past that took the top rung, so note-off — and the window with it — landed + // INSIDE the sound. Rendered at a low rate and a fast tempo so the case is 400 beats long + // without being twenty million frames; nothing here depends on either number but the + // beats it puts the source at. + { + constexpr int kSlowRate = 8000; + const Tempo fast = tempoOf(480.0); // 384 beats == 48 s at this tempo + constexpr std::int64_t kFrames = kSlowRate * 50; // 50 s == 400 beats: past the ladder + SampleData s = dcSample(static_cast(kFrames)); + s.sampleRate = kSlowRate; + s.play.playMode = PlayMode::Gate; + s.play.adsr.releaseFrames = 0; + + const NoteProgram p = + defaultBakeProgram(s, kSlowRate, oneBar(), Velocity::of(100)); + const ResolvedNote r = resolveNote(p, fast); + CHECK(r.noteOffSeconds > 49.9 && r.noteOffSeconds < 50.1); // 50 s, not the 48 s rung + const std::optional plan = planBake(r, kSlowRate, 60).plan; + CHECK(plan.has_value()); + if (plan) { + CHECK(plan->totalFrames == kFrames + kPad); + const BakeAudio whole = renderBake(s, *plan, kUnity); + // Full level across the two seconds the saturated rung used to cut, and the file + // still ends on the declick ramp rather than on a hard edge. + CHECK(peakAt(whole, kSlowRate * 48, kFrames) > 0.4); + CHECK(lastFrameLevel(whole) < 1e-3); + } + } + + // --- Gate, no loop, a START POINT under Varispeed: both terms of the exhaustion ------ + // The two derivation terms this branch has that the cases above do not exercise: the + // window covers frameCount MINUS the start, and that remainder is stretched by the + // deepest downward offset. Getting either wrong shortens the window. + { + SampleData s = dcSample(48000); // 1 s of source + s.play.playMode = PlayMode::Gate; + s.play.adsr.releaseFrames = 0; + s.startFrame = 24000; // half of it left to play + s.play.pitchEngine = PitchEngine::Varispeed; + // A flat velocity->pitch curve at half depth: an octave down (the range is 24 + // semitones) for the note's whole lifetime, so the read really does run at half rate + // to the end rather than for one envelope stage. + s.play.pitchVelocityCurve = VelocityCurve::fromPoints( + {{0.0, -0.5}, {127.0, -0.5}}, reasampler::instrument::engine::CurveDomain::Bipolar); + + // (48000 - 24000) source frames at half rate == 48000 output frames. Reading either + // term wrong halves or doubles this. + CHECK(derivedFrames(s) == 48000 + kPad); + const BakeAudio derived = bakeWith(s, 0.0); + CHECK(peakAt(derived, 47000, 48000) > 0.4); // still sounding at the derived end + const BakeAudio wide = bakeWith(s, /*extraMs=*/200.0); + CHECK(peakAt(wide, 48000 + kPad, wide.frameCount()) == 0.0); // and nothing past it + } + // --- Gate WITH a sustain loop: Hold is the note length, and the ONLY user input ------ // A looped Gate voice sounds for as long as it is held, so no derivation supplies a // duration — this is the one case the predicate names, and the window follows Hold. @@ -254,7 +314,7 @@ int main() { // --- Trigger + a DRAWN amp EG: the window holds the WHOLE take ----------------------- // A drawn contour covers the full sample length, so the engine folds lengthFraction to - // 1.0 (effectiveLengthFraction, trigger_seam.h). The stored %-knob is inert but still + // 1.0 (effectiveLengthFraction, play_params.h). The stored %-knob is inert but still // saved, and reading it raw here cut this window to a quarter of the take. { SampleData s = dcSample(48000); diff --git a/tests/test_component_state_io.cpp b/tests/test_component_state_io.cpp index 92fe5c1..202e76c 100644 --- a/tests/test_component_state_io.cpp +++ b/tests/test_component_state_io.cpp @@ -793,11 +793,15 @@ static void testNonFiniteAhdSecondsLiftToZero() { static constexpr std::size_t kHardFlagTailBytes = 4 + 2 + 4 + 2 + 4 + 2; static constexpr std::size_t kBakeHoldTailBytes = 4 + 1; -// The v14 tail at its one-bar default, re-appended after a splice so the record still ends -// where the reader expects it to. +// The v14 tail, re-appended after a splice so the record still ends where the reader expects. +static void putBakeHoldTail(std::vector& out, int quarterExponent, + note::DivisionModifier modifier) { + legacy::u32v(out, static_cast(static_cast(quarterExponent))); + legacy::u8v(out, static_cast(modifier)); +} + static void putDefaultBakeHoldTail(std::vector& out) { - legacy::u32v(out, 2); // quarterExponent 2 == 1/1 - legacy::u8v(out, 0); // Straight + putBakeHoldTail(out, 2, note::DivisionModifier::Straight); // 1/1, the field's default } // A hard-flag COUNT that disagrees with the curve fromPoints already built, but is still @@ -866,11 +870,10 @@ static void testV13HardFlagInBoundsMismatchDropsFlagsOnly() { 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. +// A hard-flag COUNT that exceeds what its own tail carries is a genuinely corrupt count: the +// record parsed AHEAD of it survives (never the old "reset everything to defaults"), and the +// stream is drained rather than guessed at — see the stranding test below for why guessing is +// worse. Here nothing follows that the drain can cost, so the two behaviours coincide. static void testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord() { ComponentState in; in.selectionId = "pad"; @@ -884,20 +887,12 @@ static void testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord() { CHECK(bytes.size() >= kHardFlagTailBytes + kBakeHoldTailBytes); bytes.resize(bytes.size() - kHardFlagTailBytes - kBakeHoldTailBytes); 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); - putDefaultBakeHoldTail(bytes); + // …and nothing at all after it, so the blob simply ends inside the v13 tail. 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. + // the hard-flag tail (the three spline EGs). Only the hard-flag applications and the tail + // that never arrived are lost. CHECK(out.selectionId == "pad"); CHECK(out.params.rootOverride && *out.params.rootOverride == 44); CHECK(out.params.play.adsr.releaseSeconds == 0.44); @@ -907,6 +902,82 @@ static void testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord() { 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); + CHECK(out.params.bakeHold == InstrumentParams{}.bakeHold); +} + +// The stranding case, and the reason a bogus count DRAINS rather than skipping in place: the +// blob keeps going after the corrupt block, so "skip nothing and read on" starts every later +// tail mid-block. The bake Hold is the tail that makes it visible — it CLAMPS whatever it +// reads, so a misaligned read installs a legal-looking division rather than failing loudly. +// The bar is that it degrades to ABSENT (the field's own default), never to a fabricated value +// — and in particular never to the top rung the misread count used to clamp to. +static void testV13HardFlagCountThatStrandsAlignmentLeavesTheHoldAbsentNotFabricated() { + ComponentState in; + in.selectionId = "pad"; + in.params.rootOverride = 44; + in.params.play.adsr.releaseSeconds = 0.44; + in.params.loopCrossfadeFrames = 321; + in.params.bakeHold = note::makeDivision(-2, note::DivisionModifier::Triplet); + + // A THREE-point amp curve, so its flag block is three bytes rather than two: the + // misaligned reads below then land on bytes that decode to something other than the + // default, which is what makes "absent" and "fabricated" distinguishable at all. + in.params.velocityCurve = reasampler::instrument::engine::VelocityCurve::fromPoints( + {VelocityPoint{0.0, 0.0}, VelocityPoint{64.0, 0.5, /*hard=*/true}, + VelocityPoint{127.0, 1.0}}, + reasampler::instrument::engine::CurveDomain::Unipolar); + + // A REAL blob with exactly ONE corrupt field: the amp hard-flag count, patched in place. + // Everything after it — the amp flags, both well-formed neighbour blocks, and the Hold — + // is exactly what the serializer wrote, which is the whole hazard. + constexpr std::size_t kThreePointFlagTail = (4 + 3) + (4 + 2) + (4 + 2); + std::vector bytes = serializeComponentState(in); + CHECK(bytes.size() >= kThreePointFlagTail + kBakeHoldTailBytes); + const std::size_t ampCountAt = bytes.size() - kThreePointFlagTail - kBakeHoldTailBytes; + for (std::size_t i = 0; i < 4; ++i) bytes[ampCountAt + i] = i == 0 ? 0x00 : 0xFF; + + const ComponentState out = deserializeComponentState(bytes, 48000.0); + CHECK(out.selectionId == "pad"); + CHECK(out.params.rootOverride && *out.params.rootOverride == 44); + CHECK(out.params.play.adsr.releaseSeconds == 0.44); + CHECK(out.params.loopCrossfadeFrames == 321); + CHECK(out.params.play.ampSpline.mode == EnvMode::Staged); + // Absent, not the stored value (its tail is past the damage and cannot be trusted)… + CHECK(out.params.bakeHold == InstrumentParams{}.bakeHold); + CHECK(out.params.bakeHold != note::makeDivision(-2, note::DivisionModifier::Triplet)); + // …and above all not the division a misaligned read manufactures: the flag bytes read as + // the next count, and the next-but-one block's bytes read as the Hold, whose exponent + // clamps to the top rung. + CHECK(out.params.bakeHold != + note::makeDivision(note::kMaxQuarterExponent, note::DivisionModifier::Straight)); +} + +// Numeric domains are established at the DOOR, not at each consumer. A NaN pitch depth reaches +// the bake's pow() and the voice's ratio multiply; a NaN %-length and a NaN stage time reach +// narrowing casts that are undefined on one; and a root override outside MIDI range makes the +// bake's render note and the sample's own root disagree, which is a read rate other than 1 and +// therefore a window sized in the truncating direction. +static void testOutOfDomainWireValuesAreBoundedAtTheCodec() { + const PlaySeconds defaults; + ComponentState in; + in.selectionId = "pad"; + in.params.keyTrack = 0.5; // a neighbouring field, to show the guards are per-field + in.params.rootOverride = 9999; + in.params.play.pitchEnv.peakSemitones = std::numeric_limits::quiet_NaN(); + in.params.play.trigger.lengthFraction = std::numeric_limits::quiet_NaN(); + in.params.play.adsr.releaseSeconds = std::numeric_limits::infinity(); + + const ComponentState out = deserializeComponentState(serializeComponentState(in), 48000.0); + CHECK(out.params.rootOverride && *out.params.rootOverride == 127); + CHECK(out.params.play.pitchEnv.peakSemitones == defaults.pitchEnv.peakSemitones); + CHECK(out.params.play.trigger.lengthFraction == defaults.trigger.lengthFraction); + CHECK(out.params.play.adsr.releaseSeconds == defaults.adsr.releaseSeconds); + CHECK(out.params.keyTrack == 0.5); + + ComponentState low = in; + low.params.rootOverride = -5; + const ComponentState lowOut = deserializeComponentState(serializeComponentState(low), 48000.0); + CHECK(lowOut.params.rootOverride && *lowOut.params.rootOverride == 0); } // A hard-flag tail truncated mid-COUNT-FIELD (only 2 of its 4 length bytes present, and @@ -1862,6 +1933,8 @@ int main() { testNonFiniteAhdSecondsLiftToZero(); testV13HardFlagInBoundsMismatchDropsFlagsOnly(); testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord(); + testV13HardFlagCountThatStrandsAlignmentLeavesTheHoldAbsentNotFabricated(); + testOutOfDomainWireValuesAreBoundedAtTheCodec(); testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord(); testBakeHoldRoundTripsAndDisturbsNothingElse(); testV13BlobLiftsToTheDefaultHold(); diff --git a/tests/test_musical_division.cpp b/tests/test_musical_division.cpp index b555d99..c308928 100644 --- a/tests/test_musical_division.cpp +++ b/tests/test_musical_division.cpp @@ -9,7 +9,9 @@ #include "../src/core/instrument/note/musical_division.h" +#include #include +#include using namespace reasampler::instrument::note; @@ -240,6 +242,11 @@ static void testShortestAtLeastDegenerateInputs() { const Division longest = makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted); CHECK(almostEqual(divisionBeats(longest), kMaxDivisionBeats)); CHECK(shortestDivisionAtLeast(kMaxDivisionBeats * 2.0) == longest); + // A non-finite request names no length to be at least, so it takes the SAME never-short + // answer as one nothing covers. Landing on the bottom rung instead would turn a corrupt + // value into a near-instant note. + CHECK(shortestDivisionAtLeast(std::nan("")) == longest); + CHECK(shortestDivisionAtLeast(std::numeric_limits::infinity()) == longest); } int main() { diff --git a/tests/test_note_program.cpp b/tests/test_note_program.cpp index 4bf496a..35949ac 100644 --- a/tests/test_note_program.cpp +++ b/tests/test_note_program.cpp @@ -1,7 +1,8 @@ // Standalone tests for reasampler::instrument::note::note_program — no VST3, no REAPER, no // framework. Same fast assert loop as the sibling pure tests. // -// Covers: velocity clamping; the ms/beats denomination seam and its round-trip; anchoring +// Covers: velocity clamping; the picked/exact note-length seam and its bounded door; the +// ms/beats denomination seam and its round-trip; anchoring // (start to note-on, end to note-off); the resolved window against hand-computed values and // its windowCollapsed flag, including the zero-length window the flag exists to distinguish; // every division resolving to its duration in seconds; proportionality across two tempos; @@ -39,7 +40,7 @@ static const double kStraightBeats[kRungCount] = { static NoteProgram program(Division length, OffsetAmount start, OffsetAmount end, int velocity) { NoteProgram p; - p.length = length; + p.length = lengthOfDivision(length); p.start = StartOffset(start); p.end = EndOffset(end); p.velocity = Velocity::of(velocity); @@ -194,6 +195,52 @@ static void testNoteLengthIsProportionalToTempo() { } } +// --- NoteLength: the two denominations ----------------------------------------- + +// A picked length follows the tempo (that is what a picker means); a derived one does not, +// because it was computed against a concrete sound and a tempo change did not lengthen that +// sound. Both resolve through the one function, so no reader chooses. +static void testAnExactLengthIsTempoIndependentAndAPickedOneIsNot() { + const NoteLength picked = lengthOfDivision(makeDivision(0, DivisionModifier::Straight)); + const NoteLength exact = lengthOfSeconds(0.5); + CHECK(almostEqual(noteLengthSeconds(picked, at(120.0)), 0.5)); + CHECK(almostEqual(noteLengthSeconds(picked, at(60.0)), 1.0)); + CHECK(almostEqual(noteLengthSeconds(exact, at(120.0)), 0.5)); + CHECK(almostEqual(noteLengthSeconds(exact, at(60.0)), 0.5)); + // Equal at one tempo is not equal as records — the denomination IS part of the value. + CHECK(picked != exact); + CHECK(lengthOfSeconds(0.5) == exact); + CHECK(lengthOfSeconds(0.5) != lengthOfSeconds(0.5000001)); +} + +// The whole point of the exact denomination: a duration past the longest ladder rung is +// representable, where quantizing onto the ladder would saturate at kMaxDivisionBeats. +static void testAnExactLengthCarriesDurationsPastTheLaddersTopRung() { + const Tempo t = at(120.0); + const double pastTheLadder = t.beatsToSeconds(kMaxDivisionBeats) * 3.0; + CHECK(almostEqual(noteLengthSeconds(lengthOfSeconds(pastTheLadder), t), pastTheLadder)); + // …and the ladder really does saturate there, which is what makes the seam load-bearing. + CHECK(almostEqual(divisionBeats(shortestDivisionAtLeast(kMaxDivisionBeats * 3.0)), + kMaxDivisionBeats)); +} + +// The door establishes the domain, like every other value type here: a corrupt or absurd +// duration becomes a representable one rather than reaching resolveNote as a poison value. +static void testTheExactLengthDoorBoundsItsDomain() { + const Tempo t = at(120.0); + CHECK(almostEqual(noteLengthSeconds(lengthOfSeconds(-4.0), t), 0.0)); + CHECK(almostEqual(noteLengthSeconds(lengthOfSeconds(std::nan("")), t), 0.0)); + CHECK(almostEqual(noteLengthSeconds(lengthOfSeconds(kMaxLengthSeconds * 10.0), t), + kMaxLengthSeconds)); + CHECK(std::isfinite( + noteLengthSeconds(lengthOfSeconds(std::numeric_limits::infinity()), t))); + // And the whole resolve stays finite over it, which is the module's headline claim. + NoteProgram p; + p.length = lengthOfSeconds(std::numeric_limits::infinity()); + const ResolvedNote r = resolveNote(p, t); + CHECK(std::isfinite(r.noteOffSeconds) && std::isfinite(r.captureEndSeconds)); +} + // --- The resolved window ------------------------------------------------------- static void testWindowAnchorsStartToNoteOnAndEndToNoteOff() { @@ -279,7 +326,7 @@ static void testRecordRoundTripsAsAWhole() { offsetFromMs(-20.0), offsetFromBeats(2.0), 96); const NoteProgram copy = original; CHECK(copy == original); - CHECK(copy.length == makeDivision(-1, DivisionModifier::Dotted)); + CHECK(copy.length == lengthOfDivision(makeDivision(-1, DivisionModifier::Dotted))); CHECK(copy.start.amount() == offsetFromMs(-20.0)); CHECK(copy.end.amount() == offsetFromBeats(2.0)); CHECK(copy.velocity.value() == 96); @@ -526,6 +573,9 @@ int main() { testEveryDivisionResolvesToItsDuration(); testExtremeAndNamedDivisionsInSeconds(); testNoteLengthIsProportionalToTempo(); + testAnExactLengthIsTempoIndependentAndAPickedOneIsNot(); + testAnExactLengthCarriesDurationsPastTheLaddersTopRung(); + testTheExactLengthDoorBoundsItsDomain(); testWindowAnchorsStartToNoteOnAndEndToNoteOff(); testEndOffsetMovesWithTheNoteLength(); diff --git a/tests/test_sample_chrome.cpp b/tests/test_sample_chrome.cpp index 51b272e..264f40e 100644 --- a/tests/test_sample_chrome.cpp +++ b/tests/test_sample_chrome.cpp @@ -128,9 +128,9 @@ static void testVelocityKnobIsCentredInItsCellAboveTheLabel() { CHECK(r.velLabel.x == r.velCell.x && r.velLabel.right() == r.velCell.right()); } -// The Hold cell reserves its width whether or not the control is drawn — a run laid out -// conditionally would slide Bake and Preview out from under the pointer whenever a loop is -// dialled in or out. Its interior follows the velocity cell's grammar exactly. +// The Hold cell reserves its width whether or not the control is drawn, so the title slot +// beside it holds still (sample_chrome.h owns the reasoning). Its interior follows the +// velocity cell's grammar exactly. static void testHoldCellIsReservedAndFollowsTheVelocityCellGrammar() { const ChromeRects r = chromeRects(chromeBand(), kKnob); CHECK(r.holdCell.width == r.velCell.width); diff --git a/tests/test_spline_egs.cpp b/tests/test_spline_egs.cpp index e977d85..2c83fb5 100644 --- a/tests/test_spline_egs.cpp +++ b/tests/test_spline_egs.cpp @@ -491,6 +491,52 @@ static void testEnforceGateUnavailableWhileDrawnForcesTriggerOnBothRepresentatio CHECK(frames.playMode == PlayMode::Trigger); } +// effectiveLengthFraction is the third member of the same rule family, and the one every +// consumer of the Trigger span (the voice, the overlay, the bake's window) must fold through: +// a drawn contour covers the FULL sample length, so the stored %-knob goes inert. It is NOT +// cleared — a pre-spline value survives in the record, and a reader that takes it raw plays, +// draws or bakes a fraction of the take. Gated on the same per-envelope `enabled` flags for the +// same reason the predicate above is. +static void testEffectiveLengthFractionFoldsToOneOnlyWhileAnEgIsActuallyDrawn() { + PlayParams p; + p.trigger.lengthFraction = 0.25; + CHECK(effectiveLengthFraction(p) == 0.25); // staged: the knob is what it says + + p.ampSpline.mode = EnvMode::Spline; + CHECK(effectiveLengthFraction(p) == 1.0); // drawn: the whole take + CHECK(p.trigger.lengthFraction == 0.25); // …and the stored value is untouched + + // Flipping back restores the stored fraction, which is why the fold cannot be a write. + p.ampSpline.mode = EnvMode::Staged; + CHECK(effectiveLengthFraction(p) == 0.25); + + p.pitchSpline.mode = EnvMode::Spline; + CHECK(effectiveLengthFraction(p) == 0.25); // pitchEnv.enabled is still false + p.pitchEnv.enabled = true; + CHECK(effectiveLengthFraction(p) == 1.0); + + p.pitchEnv.enabled = false; + p.pitchSpline.mode = EnvMode::Staged; + p.filterSpline.mode = EnvMode::Spline; + CHECK(effectiveLengthFraction(p) == 0.25); // filter.enabled is still false + p.filter.enabled = true; + CHECK(effectiveLengthFraction(p) == 1.0); +} + +// effectivePlayMode is what a READ-ONLY consumer asks instead of re-testing the fields — the +// editor's bake-Hold predicate is one. It must answer exactly what the enforcement writes. +static void testEffectivePlayModeAgreesWithTheEnforcementItShares() { + PlaySeconds stored; + stored.playMode = PlayMode::Gate; + CHECK(effectivePlayMode(stored) == PlayMode::Gate); + stored.ampSpline.mode = EnvMode::Spline; + CHECK(effectivePlayMode(stored) == PlayMode::Trigger); + CHECK(stored.playMode == PlayMode::Gate); // a read, never a write + PlaySeconds enforced = stored; + enforceGateUnavailableWhileDrawn(enforced); + CHECK(enforced.playMode == effectivePlayMode(stored)); +} + // --- 11. The velocity->amp curve is the same grammar ------------------------- static void testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete() { @@ -563,6 +609,8 @@ int main() { testAFreshSplineEgDefaultsToTheSmoothDownwardSlope(); testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards(); testEnforceGateUnavailableWhileDrawnForcesTriggerOnBothRepresentations(); + testEffectiveLengthFractionFoldsToOneOnlyWhileAnEgIsActuallyDrawn(); + testEffectivePlayModeAgreesWithTheEnforcementItShares(); testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete(); testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead(); if (g_fail == 0) std::printf("spline_egs: all tests passed\n"); diff --git a/tests/test_trigger_seam.cpp b/tests/test_trigger_seam.cpp index 9ff736a..a66b4d3 100644 --- a/tests/test_trigger_seam.cpp +++ b/tests/test_trigger_seam.cpp @@ -2,13 +2,16 @@ // Same fast assert loop as the sibling pure tests. // // Covers triggerPlayLength: zero play length, startFrame set, startFrame past frameCount, -// rounding, and the Finding 1 regression (start-point set — the case that was broken before -// this module existed); plus effectiveLengthFraction, the spline fold every consumer of the -// span must go through. +// rounding, the out-of-domain clamps that keep it identical to Voice::start's inline copy of +// the same formula, and the Finding 1 regression (start-point set — the case that was broken +// before this module existed). The spline fold that used to live here is now beside its +// siblings in play_params.h, and pinned with them in test_spline_egs. #include "../src/core/instrument/map/trigger_seam.h" +#include #include +#include using namespace reasampler; using namespace reasampler::instrument::map; @@ -58,46 +61,19 @@ static void testPlayLengthRounding() { CHECK(triggerPlayLength(0.6, 3, 0) == 2); } -// --- effectiveLengthFraction -------------------------------------------------- - -// A drawn contour covers the FULL sample length, so the stored %-knob goes inert. It is not -// cleared, though — a pre-spline value survives in the record, and every reader that takes it -// raw plays, draws or bakes a fraction of the take. -static void testDrawnEnvelopeFoldsTheFractionToOne() { - PlayParams p; - p.trigger.lengthFraction = 0.25; - CHECK(effectiveLengthFraction(p) == 0.25); // staged: the knob is what it says - - p.ampSpline.mode = EnvMode::Spline; - CHECK(effectiveLengthFraction(p) == 1.0); // drawn: the whole take - CHECK(p.trigger.lengthFraction == 0.25); // …and the stored value is untouched - - // Flipping back restores the stored fraction, which is why the fold cannot be a write. - p.ampSpline.mode = EnvMode::Staged; - CHECK(effectiveLengthFraction(p) == 0.25); -} - -// The fold reads the SAME predicate the engine's Gate refusal does, gating flags included: a -// Spline mode on a DISABLED pitch/filter envelope binds no cursor, so it must not fold. -static void testDisabledEnvelopesDoNotFoldTheFraction() { - PlayParams p; - p.trigger.lengthFraction = 0.5; - - p.pitchSpline.mode = EnvMode::Spline; - CHECK(effectiveLengthFraction(p) == 0.5); // pitchEnv.enabled is still false - p.pitchEnv.enabled = true; - CHECK(effectiveLengthFraction(p) == 1.0); - - p.pitchEnv.enabled = false; - p.filterSpline.mode = EnvMode::Spline; - CHECK(effectiveLengthFraction(p) == 0.5); // filter.enabled is still false - p.filter.enabled = true; - CHECK(effectiveLengthFraction(p) == 1.0); +// The stored fraction is a wire double with no codec-side range check beyond finiteness, and +// Voice::start clamps every one of these the same way. A span that ran past the source would +// read off the end of the PCM; one derived from NaN would reach an undefined narrowing. +static void testOutOfDomainFractionsClampToTheSpan() { + CHECK(triggerPlayLength(1.5, 1000, 0) == 1000); // never past the post-start span + CHECK(triggerPlayLength(1.5, 1000, 200) == 800); + CHECK(triggerPlayLength(-0.5, 1000, 0) == 0); + CHECK(triggerPlayLength(std::nan(""), 1000, 0) == 0); + CHECK(triggerPlayLength(std::numeric_limits::infinity(), 1000, 0) == 1000); } int main() { - testDrawnEnvelopeFoldsTheFractionToOne(); - testDisabledEnvelopesDoNotFoldTheFraction(); + testOutOfDomainFractionsClampToTheSpan(); testPlayLengthNoStartPoint(); testPlayLengthWithStartPoint(); testPlayLengthZeroFrameCount();