daniel/reasampler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: close review findings on the velocity-curve deck and bipolar curves

Browse Source 
Cancels the curve-node drag whenever the popup closes so Esc mid-drag can't alias the amp curve; generalizes CurveTarget routing to one switch; fixes stale/overstated comments; clamps a pre-v12 depth fold; adds deck-inertness and filter-fold test coverage.
This commit is contained in:
Daniel Harvey
2026-07-31 19:46:37 -04:00
parent 9d38f87a2d
commit cfb53aade3
22 changed files with 223 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/test_component_state_io.cpp
+35 -6
View File
@@ -859,21 +859,36 @@ static void testPriorPayloadVersionsLiftToAHardSeam() {
in.params.play.pitchVelocityCurve = reasampler::instrument::engine::VelocityCurve::fromPoints(
{VelocityPoint{0.0, 0.5}, VelocityPoint{127.0, 1.0}},
reasampler::instrument::engine::CurveDomain::Bipolar);
// The filter's pre-v12 depth fold (see testPreV12FilterVelocityDepthFoldsIntoTheCurve for
// the single-version proof); planted here too so EVERY version that carries a filter tail
// (v9..v11) is walked, not just v11 — the fold's branch condition is pv < kParamsVelocityVersion.
in.params.play.filter.enabled = true;
in.params.play.filter.modAmount = -0.6251953125; // distinct, exactly representable anchor
const reasampler::instrument::engine::VelocityCurve filterShape =
reasampler::instrument::engine::VelocityCurve::fromPoints(
{VelocityPoint{0.0, 0.0}, VelocityPoint{64.0, 0.25}, VelocityPoint{127.0, 1.0}},
reasampler::instrument::engine::CurveDomain::Bipolar);
in.params.play.filter.velocityCurve = filterShape;
constexpr double kPlantedDepth = -0.75;
struct Case {
std::uint32_t pv;
std::size_t cut;
bool keepsCurveTail;
bool keepsFilterTail;
};
const Case cases[] = {
{11, kVelocityTailBytes, true},
{10, kVelocityTailBytes + kLoopTailBytes, true},
{9, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes, false},
{8, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes + kFilterTailBytes, false},
{11, kVelocityTailBytes, true, true},
{10, kVelocityTailBytes + kLoopTailBytes, true, true},
{9, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes, false, true},
{8, kVelocityTailBytes + kLoopTailBytes + kCurveTailBytes + kFilterTailBytes, false, false},
};
for (const Case& c : cases) {
const ComponentState out =
deserializeComponentState(payloadDowngradedTo(in, c.pv, c.cut), 48000.0);
std::vector<std::uint8_t> bytes = payloadDowngradedTo(in, c.pv, c.cut);
if (c.keepsFilterTail) {
plantPreV12FilterDepth(bytes, in.params.play.filter.modAmount, kPlantedDepth);
}
const ComponentState out = deserializeComponentState(bytes, 48000.0);
// The span itself has been in the format since v2 and must survive untouched.
CHECK(out.params.loopOverride && out.params.loopOverride->hasLoop);
CHECK(out.params.loopOverride && out.params.loopOverride->start == 2000);
@@ -889,6 +904,20 @@ static void testPriorPayloadVersionsLiftToAHardSeam() {
// And the cut landed on the tail boundary the ladder claims, not somewhere inside it.
CHECK(out.params.play.adsr.attackCurve ==
(c.keepsCurveTail ? 4.0 : reasampler::util::kCurveNeutral));
// The filter's own velocity curve: folded by the planted depth where the tail survives
// (v9..v11), the off/neutral default where it was cut away entirely (v8).
if (c.keepsFilterTail) {
CHECK(out.params.play.filter.enabled);
for (int v = 0; v <= 127; ++v) {
CHECK(std::fabs(out.params.play.filter.velocityCurve.eval(v) -
kPlantedDepth * filterShape.eval(v)) < 1e-12);
}
} else {
CHECK(!out.params.play.filter.enabled);
for (int v = 0; v <= 127; ++v) {
CHECK(out.params.play.filter.velocityCurve.eval(v) == 0.0);
}
}
}
}
tests/test_curve_popup.cpp
+15 -7
View File
@@ -8,6 +8,7 @@
#include "../src/core/instrument/engine/velocity_curve.h"
#include <algorithm>
#include <cstdio>
using namespace reasampler;
@@ -89,10 +90,14 @@ static void testOutsideSheetDismissTest() {
// the popup's real geometry, because that is what makes one popup code path legitimate.
static void testTheSameCurveBoxHostsBothDomains() {
const CurvePopupLayout pl = computeCurvePopup(840, 620);
// The shell insets this rect before mapping; the inset is uniform, so any box inside the
// curveBox exercises the same relationship. Use the rect itself.
const VelocityCurve::Box box{pl.curveBox.x, pl.curveBox.y, pl.curveBox.width,
pl.curveBox.height};
// The editor never maps against the raw curveBox — shell/instrument/editor_internal.h's
// curveBoxFromRect insets it first (kVelCurveInset == 14, mirrored here since this pure
// target cannot link the shell). Applying it, not the raw rect, is what exercises the
// actual box shape paint/hit-test/drag agree on.
constexpr int kInset = 14;
const VelocityCurve::Box box{pl.curveBox.x + kInset, pl.curveBox.y + kInset,
std::max(0, pl.curveBox.width - 2 * kInset),
std::max(0, pl.curveBox.height - 2 * kInset)};
CHECK(box.width > 1 && box.height > 1);
const VelocityCurve amp = VelocityCurve::flat();
@@ -105,12 +110,15 @@ static void testTheSameCurveBoxHostsBothDomains() {
CHECK(amp.pixelFromPoint(box, {0.0, 0.0}).y == bottom);
CHECK(mod.pixelFromPoint(box, {0.0, 1.0}).y == top);
CHECK(mod.pixelFromPoint(box, {0.0, -1.0}).y == bottom);
// ...so value 0 is the FLOOR for the amp curve and the MIDLINE for a modulation curve.
CHECK(mod.pixelFromPoint(box, {0.0, 0.0}).y == (top + bottom) / 2);
// ...so value 0 is the FLOOR for the amp curve and the MIDLINE for a modulation curve. The
// expected row replicates valueToY's own HALF-UP rounding on the inverted fraction (velocity_
// curve.cpp) rather than a plain integer bisection of top/bottom: the two agree only when
// box.height-1 is even, so a naive (top+bottom)/2 silently depends on this box's parity.
const int midY = top + static_cast<int>(0.5 * static_cast<double>(box.height - 1) + 0.5);
CHECK(mod.pixelFromPoint(box, {0.0, 0.0}).y == midY);
// And a click at the vertical centre adds a point at 0 in the bipolar editor, at 0.5 in
// the unipolar one — one hit-test path, two correct answers.
const int midY = (top + bottom) / 2;
CHECK(mod.pointFromPixel(box, box.left, midY).value == 0.0);
CHECK(amp.pointFromPixel(box, box.left, midY).value > 0.49);
CHECK(amp.pointFromPixel(box, box.left, midY).value < 0.51);
tests/test_deck_groups.cpp
+18
View File
@@ -459,11 +459,29 @@ static void testOverlayIsInertExactlyWhenItsGroupToggleIsOff() {
CHECK(!overlayEnvInert(OverlayEnv::kNone, false, false));
}
// A deck knob goes inert exactly with its group's own enable toggle — including the filter's
// VELOCITY cell, which sits in the VELOCITY group visually but is a filter parameter and must
// go inert with the rest of the filter (the reachable-through-the-deck route mouseDownDeck
// checks before ever routing a curve-cell click to the popup).
static void testDeckKnobIsInertExactlyWithItsGroupsEnableToggle() {
CHECK(deckKnobInert(DeckParam::kFilterVelCurve, /*pitchEnv=*/true, /*filter=*/false));
CHECK(!deckKnobInert(DeckParam::kFilterVelCurve, true, true));
CHECK(deckKnobInert(DeckParam::kFilterCutoff, true, false));
CHECK(!deckKnobInert(DeckParam::kFilterCutoff, true, true));
CHECK(deckKnobInert(DeckParam::kPitchEnvDepth, /*pitchEnv=*/false, true));
CHECK(!deckKnobInert(DeckParam::kPitchEnvDepth, true, true));
// The amp's own velocity cell and every ordinary control are never inert here — inertness
// is a filter/pitch-env-group-only concept.
CHECK(!deckKnobInert(DeckParam::kAmpVelCurve, false, false));
CHECK(!deckKnobInert(DeckParam::kAttack, false, false));
}
int main() {
testOverlaySelectionIsExclusiveAcrossTheThreeEnvelopeDecks();
testClickingTheActiveOverlayRadioClearsToNone();
testANonRadioIdLeavesTheOverlaySelectionAlone();
testOverlayIsInertExactlyWhenItsGroupToggleIsOff();
testDeckKnobIsInertExactlyWithItsGroupsEnableToggle();
testEveryDeckControlIsClassifiedLiveOrReloading();
testOnlyALiveControlsDragTakesTheLiveTier();
testDeckReadsPitchThenFilterThenAmpLeftToRight();