fix: close review findings on spline EGs — engine, codec, and popup/overlay UI grammar
Live pitch depth, Gate/Spline enable-rule agreement, inert kTrigLength, NaN wire guards, hard-flag-tail corruption no longer wipes the record, RT/cold spline tie-break, retired alt-click, marker-shadow fix, plus new test coverage.
This commit is contained in:
@@ -780,6 +780,141 @@ static void testNonFiniteAhdSecondsLiftToZero() {
|
||||
CHECK(out.params.play.filter.trigEnv.decaySeconds == 0.0);
|
||||
}
|
||||
|
||||
// --- The v13 hard-flag tail: corruption must never widen past its own three curves -----------
|
||||
|
||||
// A hard-flag COUNT that disagrees with the curve fromPoints already built, but is still
|
||||
// IN-BOUNDS (the blob really does carry that many bytes) — the documented promise
|
||||
// (component_state_io.h) is that the tail is dropped, never misapplied, and nothing else in
|
||||
// the record is disturbed. Corrupts only the AMP curve's tail; FILTER/PITCH follow at their
|
||||
// normal, byte-precise offsets, proving a mismatch on one curve does not cascade to its
|
||||
// neighbours.
|
||||
static void testV13HardFlagInBoundsMismatchDropsFlagsOnly() {
|
||||
ComponentState in;
|
||||
in.selectionId = "pad";
|
||||
in.params.rootOverride = 44;
|
||||
in.params.keyTrack = 0.6;
|
||||
in.params.play.adsr.attackSeconds = 0.12;
|
||||
in.params.play.adsr.sustainLevel = 0.55;
|
||||
in.params.play.filter.enabled = true;
|
||||
in.params.play.filter.settings.cutoffNorm = 0.4f;
|
||||
in.params.play.filter.modAmount = -0.3;
|
||||
in.params.play.pitchEnv.enabled = true;
|
||||
in.params.play.pitchEnv.peakSemitones = 5.0;
|
||||
in.params.loopCrossfadeFrames = 777;
|
||||
in.params.play.pitchVelocityCurve = reasampler::instrument::engine::VelocityCurve::fromPoints(
|
||||
{VelocityPoint{0.0, -0.4}, VelocityPoint{127.0, 0.4}},
|
||||
reasampler::instrument::engine::CurveDomain::Bipolar);
|
||||
// Every velocity curve left at its DEFAULT 2-point shape, so the v13 hard-flag tail's byte
|
||||
// layout (three 4-byte-count + N-byte blocks, amp/filter/pitch order — putHardFlags' call
|
||||
// order in params_payload.cpp) is deterministic and this test can splice it exactly.
|
||||
|
||||
std::vector<std::uint8_t> bytes = serializeComponentState(in);
|
||||
CHECK(bytes.size() >= 18);
|
||||
bytes.resize(bytes.size() - 18); // drop the three well-formed 4+2-byte blocks
|
||||
legacy::u32v(bytes, 5); // amp: bogus count...
|
||||
for (int i = 0; i < 5; ++i) legacy::u8v(bytes, 0); // ...with 5 REAL bytes, so nothing shifts
|
||||
legacy::u32v(bytes, 2); // filter: correct count, unchanged
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u32v(bytes, 2); // pitch: correct count, unchanged
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u8v(bytes, 0);
|
||||
|
||||
const ComponentState out = deserializeComponentState(bytes, 48000.0);
|
||||
// Every param preceding AND following the corrupted amp tail survives untouched.
|
||||
CHECK(out.selectionId == "pad");
|
||||
CHECK(out.params.rootOverride && *out.params.rootOverride == 44);
|
||||
CHECK(out.params.keyTrack == 0.6);
|
||||
CHECK(out.params.play.adsr.attackSeconds == 0.12);
|
||||
CHECK(out.params.play.adsr.sustainLevel == 0.55);
|
||||
CHECK(out.params.play.filter.enabled);
|
||||
CHECK(out.params.play.filter.settings.cutoffNorm == 0.4f);
|
||||
CHECK(out.params.play.filter.modAmount == -0.3);
|
||||
CHECK(out.params.play.pitchEnv.enabled);
|
||||
CHECK(out.params.play.pitchEnv.peakSemitones == 5.0);
|
||||
CHECK(out.params.loopCrossfadeFrames == 777);
|
||||
CHECK(out.params.play.pitchVelocityCurve.eval(0.0) == -0.4);
|
||||
CHECK(out.params.play.pitchVelocityCurve.eval(127.0) == 0.4);
|
||||
// The mismatched (amp) curve keeps its points; the flags are dropped, never misapplied.
|
||||
CHECK(out.params.velocityCurve.size() == 2);
|
||||
CHECK(!out.params.velocityCurve.points()[0].hard);
|
||||
CHECK(!out.params.velocityCurve.points()[1].hard);
|
||||
CHECK(out.params.velocityCurve.equals(reasampler::instrument::engine::VelocityCurve::flat()));
|
||||
}
|
||||
|
||||
// A hard-flag COUNT that exceeds what its OWN tail carries — a genuinely corrupt/out-of-bounds
|
||||
// count — must be BOUND-AND-SKIPPED without consuming any of the following bytes, so the
|
||||
// FILTER/PITCH tails immediately after the AMP block still parse at their correct offset. The
|
||||
// old behavior (r.ok = false) reset the ENTIRE params record to defaults on this path, which is
|
||||
// strictly worse than the documented "drops only the hard points" promise.
|
||||
static void testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord() {
|
||||
ComponentState in;
|
||||
in.selectionId = "pad";
|
||||
in.params.rootOverride = 44;
|
||||
in.params.play.adsr.releaseSeconds = 0.44;
|
||||
in.params.play.filter.enabled = true;
|
||||
in.params.play.filter.settings.resonanceNorm = 0.9f;
|
||||
in.params.loopCrossfadeFrames = 321;
|
||||
|
||||
std::vector<std::uint8_t> bytes = serializeComponentState(in);
|
||||
CHECK(bytes.size() >= 18);
|
||||
bytes.resize(bytes.size() - 18); // drop the three well-formed hard-flag blocks
|
||||
legacy::u32v(bytes, 1000); // amp: a count its own tail cannot possibly carry
|
||||
// No amp flag bytes follow — bound-and-skip must consume none, so the well-formed
|
||||
// filter/pitch blocks right after it land exactly where they belong.
|
||||
legacy::u32v(bytes, 2); // filter: correct count, unchanged
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u32v(bytes, 2); // pitch: correct count, unchanged
|
||||
legacy::u8v(bytes, 0);
|
||||
legacy::u8v(bytes, 0);
|
||||
|
||||
const ComponentState out = deserializeComponentState(bytes, 48000.0);
|
||||
// The whole record survives — including everything the v13 section itself carries ahead of
|
||||
// the hard-flag tail (the three spline EGs) and the two well-formed tails after the
|
||||
// corrupted one — only the AMP curve's hard-flag application is lost.
|
||||
CHECK(out.selectionId == "pad");
|
||||
CHECK(out.params.rootOverride && *out.params.rootOverride == 44);
|
||||
CHECK(out.params.play.adsr.releaseSeconds == 0.44);
|
||||
CHECK(out.params.play.filter.enabled);
|
||||
CHECK(out.params.play.filter.settings.resonanceNorm == 0.9f);
|
||||
CHECK(out.params.loopCrossfadeFrames == 321);
|
||||
CHECK(out.params.play.ampSpline.mode == EnvMode::Staged);
|
||||
CHECK(out.params.velocityCurve.size() == 2); // unaffected: not misapplied, not discarded
|
||||
CHECK(!out.params.velocityCurve.points()[0].hard);
|
||||
}
|
||||
|
||||
// A hard-flag tail truncated mid-COUNT-FIELD (only 2 of its 4 length bytes present, and
|
||||
// nothing else after) is a different failure shape than a declared-huge count: the u32 read
|
||||
// itself fails, tripping r.ok inside readHardFlags rather than its own bound check. That must
|
||||
// be revived the same way — the whole record survives, only the hard-flag applications (on
|
||||
// all three curves, since the truncation strands every one of them) are lost.
|
||||
static void testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord() {
|
||||
ComponentState in;
|
||||
in.selectionId = "pad";
|
||||
in.params.rootOverride = 21;
|
||||
in.params.play.adsr.decaySeconds = 0.08;
|
||||
in.params.play.pitchEnv.enabled = true;
|
||||
in.params.play.pitchEnv.peakSemitones = -3.0;
|
||||
in.params.loopCrossfadeFrames = 5;
|
||||
|
||||
std::vector<std::uint8_t> bytes = serializeComponentState(in);
|
||||
CHECK(bytes.size() >= 18);
|
||||
bytes.resize(bytes.size() - 18); // drop the three well-formed hard-flag blocks
|
||||
legacy::u8v(bytes, 0x02); // half of the amp tail's 4-byte LE count, then nothing
|
||||
legacy::u8v(bytes, 0x00);
|
||||
|
||||
const ComponentState out = deserializeComponentState(bytes, 48000.0);
|
||||
CHECK(out.selectionId == "pad");
|
||||
CHECK(out.params.rootOverride && *out.params.rootOverride == 21);
|
||||
CHECK(out.params.play.adsr.decaySeconds == 0.08);
|
||||
CHECK(out.params.play.pitchEnv.enabled);
|
||||
CHECK(out.params.play.pitchEnv.peakSemitones == -3.0);
|
||||
CHECK(out.params.loopCrossfadeFrames == 5);
|
||||
CHECK(out.params.velocityCurve.size() == 2);
|
||||
CHECK(!out.params.velocityCurve.points()[0].hard);
|
||||
}
|
||||
|
||||
// --- The loop tail (payload v11) ---------------------------------------------
|
||||
|
||||
// The loop span and its crossfade survive a save/reload intact, alongside the two overrides
|
||||
@@ -1605,6 +1740,9 @@ int main() {
|
||||
testPitchVelocityCurveRoundTripsIndependently();
|
||||
testNonFiniteFilterFieldsLiftToTheNeutralDefault();
|
||||
testNonFiniteAhdSecondsLiftToZero();
|
||||
testV13HardFlagInBoundsMismatchDropsFlagsOnly();
|
||||
testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord();
|
||||
testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord();
|
||||
if (failures == 0) {
|
||||
std::printf("component_state_io_tests: all tests passed\n");
|
||||
return 0;
|
||||
|
||||
@@ -205,6 +205,41 @@ static void testInnerDialHit() {
|
||||
CHECK(h.kind == DeckHitKind::Knob && h.id == c.id && !h.inner);
|
||||
}
|
||||
|
||||
// captionToggle2 sits immediately left of captionToggle when both are present (no overlap, and
|
||||
// the caption text stops before the LEFTMOST one), and takes captionToggle's own slot when
|
||||
// captionToggle is absent — the shipped FILTER ENV group's exact shape (deck_groups.cpp).
|
||||
static void testCaptionToggle2() {
|
||||
const DeckGroupDesc both{9, 40, {}, {300, 30}, {301, 20}, {1, 2, 3}, {}};
|
||||
std::vector<DeckGroupDesc> g{both};
|
||||
const DeckLayout dl = layoutDeck(g, 0, 0, 800);
|
||||
const DeckGroupLayout& lay = dl.groups[0];
|
||||
CHECK(lay.captionToggle.id == 300);
|
||||
CHECK(lay.captionToggle2.id == 301);
|
||||
CHECK(lay.captionToggle2.seg0.width == 20 && lay.captionToggle2.seg1.width == 20);
|
||||
// Left of the first, with exactly one gap between — no overlap by construction.
|
||||
CHECK(lay.captionToggle2.seg1.right() == lay.captionToggle.seg0.x - kDeckToggleGap);
|
||||
// Caption text stops before the LEFTMOST toggle (toggle2), not just the first-placed one.
|
||||
CHECK(lay.caption.right() <= lay.captionToggle2.seg0.x);
|
||||
|
||||
DeckHit h = hitTestDeck(dl, lay.captionToggle2.seg0.right() - 1,
|
||||
lay.captionToggle2.seg0.y + 1);
|
||||
CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 301 && h.segment == 0);
|
||||
h = hitTestDeck(dl, lay.captionToggle2.seg1.x, lay.captionToggle2.seg1.y + 1);
|
||||
CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 301 && h.segment == 1);
|
||||
|
||||
// FILTER ENV's real shape: captionToggle absent, captionToggle2 present with a radio — it
|
||||
// takes the first (rightmost) slot rather than leaving a gap where captionToggle would sit.
|
||||
const DeckGroupDesc filterEnvLike{10, 66, {200}, {}, {302, 23}, {1, 2, 3, 4, 5}, {}};
|
||||
std::vector<DeckGroupDesc> g2{filterEnvLike};
|
||||
const DeckLayout dl2 = layoutDeck(g2, 0, 0, 800);
|
||||
const DeckGroupLayout& fe = dl2.groups[0];
|
||||
CHECK(fe.captionToggle.id == -1);
|
||||
CHECK(fe.captionToggle2.id == 302);
|
||||
CHECK(fe.captionToggle2.seg1.right() == fe.captionRadio.box.x - kDeckToggleGap);
|
||||
h = hitTestDeck(dl2, fe.captionToggle2.seg1.x, fe.captionToggle2.seg1.y + 1);
|
||||
CHECK(h.kind == DeckHitKind::CaptionToggle && h.id == 302 && h.segment == 1);
|
||||
}
|
||||
|
||||
static void testEmptyDeck() {
|
||||
const std::vector<DeckGroupDesc> none;
|
||||
CHECK(deckRowCount(none, 800) == 0);
|
||||
@@ -221,6 +256,7 @@ int main() {
|
||||
testHitTest();
|
||||
testCaptionRadioGeometryAndHit();
|
||||
testInnerDialHit();
|
||||
testCaptionToggle2();
|
||||
testEmptyDeck();
|
||||
if (g_fail) {
|
||||
std::printf("%d FAILURE(S)\n", g_fail);
|
||||
|
||||
@@ -341,21 +341,46 @@ static void testAFreshSplineEgDefaultsToTheSmoothDownwardSlope() {
|
||||
|
||||
// The rule has one home (splineActive) and one enforcement point on the way to the engine
|
||||
// (resolvePlay). The editor's Gate segment refuses and paints Disabled off the same predicate.
|
||||
//
|
||||
// Pitch and filter additionally gate on their own `enabled` flag, matching Voice::start's
|
||||
// binder (voice.cpp only binds pitchSplineCur_/filterSplineCur_ under that same condition): a
|
||||
// Spline mode flip on a still-disabled envelope produces no modulation, so it must not cost
|
||||
// Gate either — the predicate and the binder must agree on one enable rule. Amp has no such
|
||||
// flag and counts on its mode alone.
|
||||
static void testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards() {
|
||||
PlaySeconds stored;
|
||||
stored.playMode = PlayMode::Gate;
|
||||
CHECK(!splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
|
||||
for (EnvMode* slot : {&stored.ampSpline.mode, &stored.pitchSpline.mode,
|
||||
&stored.filterSpline.mode}) {
|
||||
*slot = EnvMode::Spline;
|
||||
CHECK(splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger);
|
||||
*slot = EnvMode::Staged;
|
||||
CHECK(!splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
}
|
||||
stored.ampSpline.mode = EnvMode::Spline;
|
||||
CHECK(splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger);
|
||||
stored.ampSpline.mode = EnvMode::Staged;
|
||||
CHECK(!splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
|
||||
stored.pitchSpline.mode = EnvMode::Spline;
|
||||
CHECK(!splineActive(stored)); // pitchEnv.enabled is still false: no modulation, no cost
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
stored.pitchEnv.enabled = true;
|
||||
CHECK(splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger);
|
||||
stored.pitchSpline.mode = EnvMode::Staged;
|
||||
stored.pitchEnv.enabled = false;
|
||||
CHECK(!splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
|
||||
stored.filterSpline.mode = EnvMode::Spline;
|
||||
CHECK(!splineActive(stored)); // filter.enabled is still false: the filter is fully off
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
stored.filter.enabled = true;
|
||||
CHECK(splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Trigger);
|
||||
stored.filterSpline.mode = EnvMode::Staged;
|
||||
stored.filter.enabled = false;
|
||||
CHECK(!splineActive(stored));
|
||||
CHECK(resolvePlay(stored, 48000).playMode == PlayMode::Gate);
|
||||
|
||||
// And the staged knobs of a drawn envelope go inert — drawn-but-dead, not removed — while
|
||||
// its depth knob, which scales either shape, stays live.
|
||||
@@ -407,6 +432,32 @@ static void testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete() {
|
||||
CHECK(!amp.deletePoint(1));
|
||||
}
|
||||
|
||||
// --- 12. SplineCursor's binary-search branch agrees with the cold reader ------
|
||||
|
||||
// Test 8 only walks a monotone forward read, which never leaves SplineCursor::locate's
|
||||
// select(seg_+1) fast path. A backwards/jumping read forces the actual binary search — and at
|
||||
// a duplicate-X knot (a drawn step) the RT cursor must resolve to the SAME point the cold
|
||||
// VelocityCurve::eval() would, or a backwards read audibly steps to the wrong side of the step.
|
||||
static void testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead() {
|
||||
// A step at x=64: two knots sharing an X but different Y.
|
||||
VelocityCurve c = VelocityCurve::fromPoints(
|
||||
{{0.0, 0.1}, {32.0, 0.3}, {64.0, 0.9}, {64.0, 0.2}, {96.0, 0.6}, {127.0, 0.4}},
|
||||
CurveDomain::Unipolar);
|
||||
SplineCursor cur;
|
||||
cur.bind(c);
|
||||
|
||||
// Deliberately out of order, so every eval but the first forces locate()'s binary search
|
||||
// rather than the forward-walk fast path.
|
||||
const double xs[] = {100.0, 10.0, 64.0, 40.0, 64.0, 5.0, 127.0, 20.0, 0.0, 90.0};
|
||||
for (double x : xs) {
|
||||
const double phase = x / kCurveXMax;
|
||||
CHECK(near(cur.eval(phase), c.eval(x), 1e-6));
|
||||
}
|
||||
// The duplicate knot itself: both readers resolve to the SAME one (the first, per
|
||||
// VelocityCurve::eval's "first containing segment" rule).
|
||||
CHECK(near(cur.eval(64.0 / kCurveXMax), 0.9, 1e-6));
|
||||
}
|
||||
|
||||
int main() {
|
||||
testHardPointGivesDifferingOneSidedSlopes();
|
||||
testNoOvershootBetweenAnyAdjacentPairOnARiseAndFallContour();
|
||||
@@ -419,6 +470,7 @@ int main() {
|
||||
testAFreshSplineEgDefaultsToTheSmoothDownwardSlope();
|
||||
testGateIsUnavailableWhileASplineEgIsActiveAndReturnsAfterwards();
|
||||
testTheVelocityAmpCurveGainsTheToggleAndKeepsItsDelete();
|
||||
testSplineCursorBinarySearchAgreesWithTheColdReaderOnAJumpingRead();
|
||||
if (g_fail == 0) std::printf("spline_egs: all tests passed\n");
|
||||
return g_fail == 0 ? 0 : 1;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user