Γ-W1-T1 review fixes: mode-independent taper rounding, sharper drag-step test, reset-sweep verifies stored fields

Swap nearbyint for std::round (MXCSR-independent); derive the finest-drag test
from the editor floor, not the knob; verify resets against fields, not norms;
record the spline-point modifier exclusion.
This commit is contained in:
2026-08-01 20:07:48 -04:00
parent 3eb72d01c4
commit ee8a956fbd
8 changed files with 96 additions and 32 deletions
+1 -1
View File
@@ -334,7 +334,7 @@ anything for a trigger shape.
parameter set does not carry (key-track, voice count, master gain, preview velocity) and the
labels for them.
- `deck_groups` — also home to `isLiveDeckParam` and `liveCommitFor`, the editor's whole commit-tier routing decision (see "Live parameter delivery" above), and to `OverlayEnv` + `nextOverlaySelection`/`overlayEnvEnabled`/`overlayEnvInert`, the whole overlay-selection state machine (exclusivity, the none resting state, and which selections a disabled or DRAWN group makes inert); WHICH groups the Sample face's deck carries, split from `knob_deck`'s HOW they lay out: the `DeckParam` control-id space (the editor's `ParamControl` is an alias of it), the `DeckGroupId` list, `sampleDeckGroups` in signal-flow order (**pitch → filter → amp**, then velocity/voice/master), and the deck's bipolar-knob law. Reads `PlayMode` for the AMP group's Gate/Trigger face, which is why this and not `knob_deck` is the module that touches the engine's value layer. Also home to `CurveTarget` + `curveTargetFor` — the VELOCITY group's three cells are popup openers, not dials, and that predicate is the ONE place they are named, so paint, hit-test routing and the popup's title all agree. MASTER is reserved for post-voice-mixer concerns, which is why the curves sit in their own group immediately left of VOICE rather than there.
- `spline_edit` — THE point-editing grammar, and the one place it is written down: left-click grabs a node and adds one in empty space, right-click deletes, control-click toggles hard/smooth. Both spline consumers — the velocity-curve popup and the spline EG overlay — route their mouse-down through `resolveSplineEdit`, so the two cannot drift into two grammars. The endpoint and point-count rules are NOT restated here: `deletePoint` and `addPoint` own them, and the caller applies the resolved action to the curve. Also home to `splineOverlayBox`, the contour's mapping box inside the waveform overlay — the FULL area, no inset, so the drawn contour stays 1:1 with the sample's time axis.
- `spline_edit` — THE point-editing grammar, and the one place it is written down: left-click grabs a node and adds one in empty space, right-click deletes, control-click toggles hard/smooth. Both spline consumers — the velocity-curve popup and the spline EG overlay — route their mouse-down through `resolveSplineEdit`, so the two cannot drift into two grammars. The endpoint and point-count rules are NOT restated here: `deletePoint` and `addPoint` own them, and the caller applies the resolved action to the curve. Also home to `splineOverlayBox`, the contour's mapping box inside the waveform overlay — the FULL area, no inset, so the drawn contour stays 1:1 with the sample's time axis. Spline points are excluded from `param_taper`'s Shift/Ctrl modifier law like waveform markers are: a point is a normalized position with no displayed unit, and control-click there is already claimed by the hard/smooth toggle above.
- `curve_popup` — pure curve-popup geometry + dismissal test (FB1): centered sheet over the Sample face — width/height clamps, title row, Close button rect, curve-box rect, outside-sheet dismissal test. Mirror of `overflow_menu`; no LICE or REAPER types.
- `envelope_overlay` — pure staged-envelope→polyline geometry for the Sample-view overlay (read from `envelope_overlay.h`): maps a `StageEnvelope` to a polyline inside a rect under whichever of TWO layout policies its `EnvKind` selects — an AHDSR draws a bounded param-domain schematic with its release RIGHT-ANCHORED to the canvas edge, an AHD draws 1:1 over the waveform's own time axis — plus a round mid-segment knot on every sloped stage that has a duration. Every vertex clamped in-canvas. Shares the `EnvNode`/`StageEnvelope`/`timeToX`/`levelToY` vocabulary with `envelope_edit` so the drawn handle and its grab region agree pixel-for-pixel. No VST3/REAPER/LICE types at the boundary.
- `envelope_edit` — pure node hit-test + pixel-delta→clamped-param inverse map for the draggable envelope nodes and their curve knots (read from `envelope_edit.h`): `nodeAtPoint` resolves a grab to the nearest node within a pick radius (Chebyshev distance, draw-order tie-break, knots appended last so a coincident endpoint handle wins); `resolveNodeDrag` maps a pixel delta since grab to a new `StageEnvelope` under the same caller-supplied per-param clamp bounds the knobs use — a drag can never produce a param a knob couldn't. Mirror of `card_drag`/`waveform_view`; the inverse of `envelope_overlay`'s params→polyline forward map, so node-drag, knot-drag and knob-edit read/write one shared model and can never diverge.
+4 -1
View File
@@ -100,4 +100,7 @@ reasampler_test(curve_popup LINK curve_popup velocity_curve)
# envelope_overlay and deck_values all read it — which is exactly why it could not stay inside
# deck_values, which sits above envelope_overlay.
reasampler_pure_library(param_taper SOURCES param_taper.cpp LINK PUBLIC curve_law)
reasampler_test(param_taper LINK param_taper)
# envelope_overlay and sample_bands are linked for the test only: the finest-drag-step assertion
# is judged against the envelope node drag at the editor's own floor width (the sharper of the
# taper's two consumers), read from the allocator/overlay rather than copied as a number.
reasampler_test(param_taper LINK param_taper envelope_overlay sample_bands)
+4 -9
View File
@@ -16,7 +16,6 @@ namespace reasampler::instrument::ui {
using engine::filter::MorphLaw;
using util::clamp01;
double deckParamNorm(DeckParam id, const PlaySeconds& play) {
switch (id) {
case DeckParam::kPlayMode: return play.playMode == PlayMode::Trigger ? 1.0 : 0.0;
@@ -197,12 +196,10 @@ void setDeckParam(DeckParam id, PlaySeconds& play, double value, int segment) {
enforceGateUnavailableWhileDrawn(play);
}
namespace {
// The ADDRESS of the one stored field a knob id owns. deckParamNorm and setDeckParam carry each
// id's MAP — which taper, which clamp; this carries only its LOCATION, which is the whole
// mechanism of the taper-free reset. A toggle, radio or curve cell has no reset gesture and
// resolves to null.
// deckParamNorm and setDeckParam carry each id's MAP — which taper, which clamp; these two carry
// only its LOCATION, which is the whole mechanism of the taper-free reset (see deck_values.h for
// why they are exposed beyond that one caller). A toggle, radio or curve cell has no reset gesture
// and resolves to null.
double* deckDoubleField(DeckParam id, PlaySeconds& p) {
switch (id) {
case DeckParam::kAttack: return &p.adsr.attackSeconds;
@@ -257,8 +254,6 @@ float* deckFloatField(DeckParam id, PlaySeconds& p) {
}
}
} // namespace
void resetDeckParam(DeckParam id, PlaySeconds& play) {
PlaySeconds defaults;
if (double* dst = deckDoubleField(id, play)) {
+10
View File
@@ -48,6 +48,16 @@ void setDeckParam(DeckParam id, PlaySeconds& play, double value, int segment);
// For knob-valued controls — a toggle has no reset gesture.
void resetDeckParam(DeckParam id, PlaySeconds& play);
// The ADDRESS of the one stored field `id` owns — the mechanism resetDeckParam bypasses the taper
// with. Exposed beyond that one caller so a test can verify a reset (or any other mutation)
// against the actual stored field rather than its normalized read-back, which deckParamNorm does
// not guarantee is injective. Null for a control with no reset gesture (a toggle, radio, or
// curve-popup cell) or one whose value lives outside PlaySeconds (master gain, key-track).
double* deckDoubleField(DeckParam id, PlaySeconds& p);
// The filter's four tone controls store their normalized position as float — see deckFloatField's
// definition for why that is a second resolver rather than a widened first one.
float* deckFloatField(DeckParam id, PlaySeconds& p);
// THE snap-unit table: which whole unit Shift snaps each control to. Includes the deck's
// processor-side ids (voice count, master gain), which have no entry in the two functions above
// because their VALUE lives outside the parameter set — the unit does not.
+10 -6
View File
@@ -8,14 +8,18 @@ namespace reasampler::instrument::ui {
namespace {
// The output quanta (header: EXACT PREIMAGE). Powers of TEN on purpose: nearbyint(v*S)/S is the
// The output quanta (header: EXACT PREIMAGE). Powers of TEN on purpose: std::round(v*S)/S is the
// correctly-rounded double of k/S, which is the same double a decimal literal of k/S parses to —
// so a default written as 0.003 or 0.060 lands on the grid exactly. A power-of-two quantum would
// not have that property against decimal literals.
constexpr double kSecondsPerQuantum = 1e9; // 1 ns
constexpr double kSemitonesPerQuantum = 1e6; // 1 micro-semitone
double resolveTo(double value, double perUnit) { return std::nearbyint(value * perUnit) / perUnit; }
// std::nearbyint reads the CURRENT FP rounding mode (MXCSR) — not exclusively ours on a DAW's UI
// thread. Under round-toward-zero it can drop a grid value by a whole quantum, which is exactly
// what the quantization scheme exists to prevent. std::round (half-away-from-zero) is the same
// regardless of that mode, which is what makes the EXACT PREIMAGE guarantee (header) structural.
double resolveTo(double value, double perUnit) { return std::round(value * perUnit) / perUnit; }
// The shifted-log offsets. Both are FITTED AGAINST THE CEILING above them, which is why the
// ceiling could not be raised in a later track: doing the two apart means fitting twice.
@@ -64,19 +68,19 @@ double depthSemitonesFromNorm(double norm, double maxSemitones) {
double snapSecondsToWholeMs(double seconds) {
if (!(seconds > 0.0)) return 0.0;
return std::nearbyint(seconds * 1000.0) / 1000.0;
return std::round(seconds * 1000.0) / 1000.0;
}
double snapFractionToWholePercent(double fraction) {
return std::nearbyint(fraction * 100.0) / 100.0;
return std::round(fraction * 100.0) / 100.0;
}
double snapSemitonesToWhole(double semitones) { return std::nearbyint(semitones); }
double snapSemitonesToWhole(double semitones) { return std::round(semitones); }
// Rounding lands on 1..10; anything under half a unit clamps to the domain floor rather than to
// zero, which is not an exponent. 1.0, the linear neutral, is therefore one snap from centre.
double snapExponentToWhole(double exponent) {
return util::clampCurve(std::nearbyint(util::clampCurve(exponent)));
return util::clampCurve(std::round(util::clampCurve(exponent)));
}
} // namespace reasampler::instrument::ui
+9 -6
View File
@@ -43,12 +43,15 @@ enum class UnitCategory {
// as toPlain(defaultNorm) with no editor-side bypass available, so every default must satisfy
// toPlain(toNormalized(d)) == d BITWISE. No transcendental map delivers that at an arbitrary
// interior point — the image of toPlain is sparser there than the doubles around it — so both
// maps below resolve their output onto a fixed decimal quantum. That turns the guarantee into
// "every value on the quantum grid round-trips exactly" instead of a libm coincidence that a
// compiler upgrade could take away. Both quanta sit four or more orders below the finest
// reachable drag step, so nothing observable is quantized. The converse,
// toNormalized(toPlain(n)) == n at arbitrary n, is NOT required and must not be demanded: no log
// map satisfies it in double, and requiring it would rule out the shape the range needs.
// maps below resolve their output onto a fixed decimal quantum, via std::round rather than
// std::nearbyint: round is half-away-from-zero regardless of the caller's FP rounding mode, so
// the quantization is mode-independent, not just decimal-exact. That turns the guarantee into
// "every value on the quantum grid round-trips exactly" instead of a libm/MXCSR coincidence that
// a compiler upgrade or a host's UI thread could take away. Both quanta sit roughly 3.7-4 orders
// below the finest reachable drag step (time ~3.98, depth ~3.71), so nothing observable is
// quantized. The converse, toNormalized(toPlain(n)) == n at arbitrary n, is NOT required and must
// not be demanded: no log map satisfies it in double, and requiring it would rule out the shape
// the range needs.
// The stage-time domain's upper end — the value at norm 1, and the one home of that number:
// envelope_overlay's kGateStageMaxSeconds and deck_values' kEnvTimeMaxSeconds are both aliases
+19 -6
View File
@@ -175,8 +175,11 @@ static void testResetLandsOnTheStoredDefaultOfEachControl() {
// EVERY knob resets to its own stored default, not just the six dual-ring pairs above. Swept
// over the whole control-id space so a control added later cannot quietly miss the reset table:
// perturb, reset, and require the control to read exactly what a fresh PlaySeconds reads.
// Compared against the STORED FIELD directly (deckDoubleField/deckFloatField), not the
// normalized read-back: deckParamNorm is not guaranteed injective, so a norm match is weaker
// than the criterion — verification against a default-constructed PlaySeconds.
static void testEveryKnobIdResetsToItsDefault() {
const PlaySeconds defaults;
PlaySeconds defaults;
for (int i = 0; i < static_cast<int>(DeckParam::kCount); ++i) {
const DeckParam id = static_cast<DeckParam>(i);
if (deckParamUnit(id) == UnitCategory::None) continue; // no reset gesture
@@ -184,9 +187,17 @@ static void testEveryKnobIdResetsToItsDefault() {
PlaySeconds p;
setDeckParam(id, p, 0.37, 0);
setDeckParam(id, p, 0.83, 0); // two writes: one of the two is off every default
CHECK(deckParamNorm(id, p) != deckParamNorm(id, defaults));
resetDeckParam(id, p);
CHECK(deckParamNorm(id, p) == deckParamNorm(id, defaults));
if (double* pd = deckDoubleField(id, p)) {
CHECK(*pd != *deckDoubleField(id, defaults));
resetDeckParam(id, p);
CHECK(*pd == *deckDoubleField(id, defaults));
} else if (float* pf = deckFloatField(id, p)) {
CHECK(*pf != *deckFloatField(id, defaults));
resetDeckParam(id, p);
CHECK(*pf == *deckFloatField(id, defaults));
} else {
CHECK(false); // every non-None, non-excluded id must own a reset field
}
}
}
@@ -279,8 +290,10 @@ static void testAValueStoredUnderTheOldCeilingIsReadNotRewritten() {
CHECK(p.adsr.releaseSeconds == 2.0);
CHECK(normDecay > 0.0 && normDecay < 1.0); // still on the knob, just at a new angle
CHECK(deckParamNorm(DeckParam::kRelease, p) > normDecay);
// And it survives the norm the knob would hand back, so a no-op touch of the control does
// not quantize a legacy value away.
// And a no-op touch survives the norm the knob would hand back — for THIS value, which is
// exactly on the taper's output quantum grid (1.75 s parses to a grid-aligned double). A
// legacy value off the grid (e.g. 1.2345678912345) WOULD be re-quantized on first touch;
// that is correct, intended behaviour, not a gap this test is claiming to cover.
setDeckParam(DeckParam::kDecay, p, normDecay, 0);
CHECK(p.adsr.decaySeconds == 1.75);
}
+39 -3
View File
@@ -9,6 +9,10 @@
#include "../src/core/instrument/ui/param_taper.h"
#include "../src/core/instrument/ui/envelope_overlay.h" // gateStageSlotPx: finest drag surface
#include "../src/core/instrument/ui/sample_bands.h" // kEditorMinWidth/kPad: the editor floor
#include <cfenv>
#include <cmath>
#include <cstdio>
@@ -78,10 +82,15 @@ static void testStageTimeIsMonotone() {
}
}
// The FINEST drag a user can make — Ctrl's 1/20 rate over the 128 px knob travel — must still
// move the value, or the output quantum would be observable as a dead zone.
// The FINEST drag a user can make on ANY surface this taper serves — not the knob's own 128 px
// travel, which is coarser than the AHDSR schematic's node drag at the editor floor. Derived from
// the floor constant and the overlay's own slot-width formula, so a later floor change sharpens
// (or coarsens) the step this test exercises automatically instead of leaving a copied number
// silently stale.
static void testEveryFinestDragStepMovesTheValue() {
const int steps = static_cast<int>(1.0 / kFineDragScale) * 128;
const Rect floorArea = Rect::ltrb(0, 0, kEditorMinWidth - 2 * kPad, 100);
const double slot = gateStageSlotPx(floorArea);
const int steps = static_cast<int>(slot / kFineDragScale);
for (int i = 0; i < steps; ++i) {
const double lo = timeSecondsFromNorm(static_cast<double>(i) / steps);
const double hi = timeSecondsFromNorm(static_cast<double>(i + 1) / steps);
@@ -108,6 +117,32 @@ static void testEveryWholeMicrosecondRoundTripsExactly() {
}
}
// MODE-INDEPENDENCE, the whole point of resolveTo's std::round over std::nearbyint. First shows
// the defect directly, generically: under round-toward-zero, the RETIRED std::nearbyint reads
// that mode and truncates a value whose fraction is well past half, while std::round (specified
// to round half-away-from-zero REGARDLESS of the current mode) does not. Then proves the
// production round trip itself — not a stand-in — survives the same hostile mode across the grid.
static void testRoundingSurvivesAHostileFpRoundingMode() {
const int saved = std::fegetround();
CHECK(std::fesetround(FE_TOWARDZERO) == 0);
CHECK(std::nearbyint(12.9) == 12.0); // the RETIRED behaviour: mode-dependent, wrong here
CHECK(std::round(12.9) == 13.0); // the fix: mode-independent, rounds to nearest
for (int us = 0; us <= 200000; us += 7) {
const double seconds = static_cast<double>(us) / 1e6;
CHECK(timeSecondsFromNorm(timeNormFromSeconds(seconds)) == seconds);
if (timeSecondsFromNorm(timeNormFromSeconds(seconds)) != seconds) break;
}
for (int milli = -24000; milli <= 24000; milli += 37) {
const double d = static_cast<double>(milli) / 1000.0;
CHECK(depthSemitonesFromNorm(depthNormFromSemitones(d, kDepth), kDepth) == d);
if (depthSemitonesFromNorm(depthNormFromSemitones(d, kDepth), kDepth) != d) break;
}
std::fesetround(saved); // restore — every other test in this binary assumes the default
}
// The converse round trip is NOT required, but its residual is worth pinning: it is bounded by
// the output quantum read back through the map, which stays four orders below one drag pixel.
// Pinned so a future quantum change cannot make the needle visibly lag the hand unnoticed.
@@ -239,6 +274,7 @@ int main() {
testStageTimeIsMonotone();
testEveryFinestDragStepMovesTheValue();
testEveryWholeMicrosecondRoundTripsExactly();
testRoundingSurvivesAHostileFpRoundingMode();
testNormRoundTripResidualStaysBelowOneDragPixel();
testTheStageTimeDefaultsRoundTripExactly();