loop: fix the crossfade seam's residual discontinuity, plus six review minors

Normalizes crossfadeWeight over crossfade-1 so the last rendered frame lands at exactly the incoming tap instead of a residual step; corrects the CLAUDE.md invariant and seam test to match. Shares lerpSource/crossfadedSource/maxCrossfade, fixes stale docs/constants, and clears crossfade on the loop-OFF gesture.
2026-07-31 17:59:58 -04:00
parent 0fe4166d7d
commit 3cb22e984d
12 changed files with 172 additions and 83 deletions
@@ -93,18 +93,34 @@ static void testZeroCrossfadeWeighsNothingAnywhere() {
    CHECK(crossfadeWeight(lp, 399.999) == 0.0);
 }

-// The weight rises from exactly 0 at the region's start to exactly 1 at the loop end, which
-// is what makes the seam continuous: at `end` the incoming tap has reached `start`, and the
-// wrap puts the head there.
+// The weight rises from exactly 0 at the region's start to exactly 1 at the LAST rendered
+// frame (end - 1, d == crossfade - 1) — normalizing over crossfade - 1 rather than crossfade is
+// what lands the ceiling exactly there instead of merely approaching it, which is what makes
+// the seam continuous: at that frame the incoming tap has fully replaced the raw read, and the
+// wrap hands over exactly that value.
 static void testWeightRunsZeroToOneAcrossTheFadeRegion() {
-    const ResolvedLoop lp = resolveLoop(span(100, 400), 100, 1000, true);
-    CHECK(lp.crossfade == 100);
-    CHECK(lp.fadeBegin == 300.0);
+    // xf = 101 so xf - 1 = 100, a clean denominator (loopStart 200 keeps the clamp out of the
+    // way: max is min(200, 200)).
+    const ResolvedLoop lp = resolveLoop(span(200, 400), 101, 1000, true);
+    CHECK(lp.crossfade == 101);
+    CHECK(lp.fadeBegin == 299.0);
+    CHECK(crossfadeWeight(lp, 298.0) == 0.0);
    CHECK(crossfadeWeight(lp, 299.0) == 0.0);
-    CHECK(crossfadeWeight(lp, 300.0) == 0.0);
-    CHECK(crossfadeWeight(lp, 350.0) == 0.5);
-    CHECK(crossfadeWeight(lp, 375.0) == 0.75);
-    CHECK(crossfadeWeight(lp, 400.0) == 1.0);
+    CHECK(crossfadeWeight(lp, 349.0) == 0.5);   // d = 50
+    CHECK(crossfadeWeight(lp, 374.0) == 0.75);  // d = 75
+    CHECK(crossfadeWeight(lp, 399.0) == 1.0);   // d = 100 == xf - 1, the ceiling's own threshold
+    CHECK(crossfadeWeight(lp, 400.0) == 1.0);   // past it too (the unwrapped-caller belt)
+}
+
+// xf - 1 == 0 would divide by zero; the guard parks fadeInv at 0 instead. Unreachable via the
+// multiply branch anyway (the region's only frame has d == 0, caught by the d <= 0 check
+// first), but fadeInv must still be a sane value rather than +inf.
+static void testCrossfadeOfOneNeedsNoDivisionGuard() {
+    const ResolvedLoop lp = resolveLoop(span(100, 400), 1, 1000, true);
+    CHECK(lp.crossfade == 1);
+    CHECK(lp.fadeInv == 0.0);
+    CHECK(crossfadeWeight(lp, 399.0) == 0.0);  // d == 0, the region's one frame
+    CHECK(crossfadeWeight(lp, 400.0) == 1.0);  // past it, the ceiling belt still holds
 }

 static void testWeightIsMonotoneAndBoundedAcrossTheRegion() {
@@ -158,6 +174,7 @@ int main() {
    testNegativeCrossfadeIsZero();
    testZeroCrossfadeWeighsNothingAnywhere();
    testWeightRunsZeroToOneAcrossTheFadeRegion();
+    testCrossfadeOfOneNeedsNoDivisionGuard();
    testWeightIsMonotoneAndBoundedAcrossTheRegion();
    testWeightSaturatesPastTheLoopEnd();
    testDefaultBoundsSitInTheLastQuarterAndClearFrameZero();
@@ -810,7 +810,9 @@ static void testCrossfadeBlendsMonotonelyAcrossTheRegion() {
    CHECK(approx(sourceFrameOf(out[51]), 51.0, 1e-3));
    CHECK(approx(sourceFrameOf(out[52]), 52.0, 1e-3));  // weight 0 at the region edge
    for (int n = 52; n < 60; ++n) {
-        const double w = static_cast<double>(n - 52) / 8.0;
+        // Normalized over crossfade - 1 (= 7), not crossfade: weight reaches exactly 1 at
+        // n == 59 (d == 7 == xf - 1), not merely approaching it.
+        const double w = static_cast<double>(n - 52) / 7.0;
        const double want = static_cast<double>(n) * (1.0 - w) + static_cast<double>(n - 20) * w;
        CHECK(approx(sourceFrameOf(out[static_cast<std::size_t>(n)]), want, 1e-3));
    }
@@ -819,25 +821,33 @@ static void testCrossfadeBlendsMonotonelyAcrossTheRegion() {
    }
 }

-// What a longer fade actually buys, measured rather than asserted by adjective. The last
-// rendered frame before the wrap carries weight (xf-1)/xf, not 1 — the weight only reaches 1
-// AT `end`, a position no frame lands on — so a residual step of (seam step)/xf survives.
-// It shrinks in exact proportion to the fade length, which is the property that makes the
-// parameter meaningful and the seam inaudible at any musically useful setting.
-static void testSeamStepShrinksInProportionToTheFadeLength() {
+// What the crossfade actually buys, measured rather than asserted by adjective: normalizing
+// over crossfade - 1 (loop_span.h) lands the weight at exactly 1 on the last rendered frame
+// (end - 1) for every REAL crossfade length (xf >= 2), not merely approaching it — that frame
+// is the incoming tap outright, which is exactly the value the wrap hands over. The step across
+// the seam is therefore the material's own natural one-frame step (indexSample's slope is 1 raw
+// frame per frame), constant for any xf >= 2 — not a residual that merely shrinks with a longer
+// fade. The |out[60]-out[59]| metric is otherwise a trap: it conflates the natural step with any
+// leftover discontinuity, and at xf == loop length the OLD 1/xf normalization happened to score
+// 0 by this same metric — a coincidence of that one ratio, not a property of the fix. Comparing
+// against the natural step rather than "small" or "zero" closes that hole.
+static void testSeamStepMatchesTheNaturalStepForAnyCrossfade() {
    auto seamStep = [](std::int64_t xf) {
        return std::fabs(loopedFrameValue(xf, 60) - loopedFrameValue(xf, 59));
    };
-    const double hard = seamStep(0);
-    CHECK(approx(hard, 19.0, 1e-3));   // 59 -> 40, the whole loop length less one frame
-    CHECK(approx(seamStep(4), 4.0, 1e-3));
-    CHECK(approx(seamStep(8), 1.5, 1e-3));
-    CHECK(approx(seamStep(16), 0.25, 1e-3));
-    // Each doubling roughly halves it, and even the shortest fade tested is a quarter of the
-    // hard seam.
-    CHECK(seamStep(4) < hard * 0.25);
-    CHECK(seamStep(8) < seamStep(4) * 0.5);
-    CHECK(seamStep(16) < seamStep(8) * 0.5);
+    // No crossfade: the seam is the whole loop length less one frame — the wart the fade fixes.
+    CHECK(approx(seamStep(0), 19.0, 1e-3));
+    // xf == 1 has no fractional region to blend — its one frame sits exactly at d == 0, caught
+    // by crossfadeWeight's own d <= 0 floor before the multiply/ceiling ever runs — so it is
+    // still the hard seam, not a one-frame fade.
+    CHECK(approx(seamStep(1), 19.0, 1e-3));
+    // Any REAL crossfade length: the step is exactly the natural one-frame step, not merely
+    // small — and constant regardless of the fade length, unlike the old formula's proportional
+    // shrink.
+    for (std::int64_t xf : {std::int64_t{4}, std::int64_t{8}, std::int64_t{16},
+                            std::int64_t{20}}) {
+        CHECK(approx(seamStep(xf), 1.0, 1e-3));
+    }
 }

 static void testCrossfadeLengthFollowsItsParameter() {
@@ -2810,7 +2820,7 @@ int main() {
    testLoopReadWrapsSampleExactOverManyCycles();
    testZeroCrossfadeLeavesTheSeamHard();
    testCrossfadeBlendsMonotonelyAcrossTheRegion();
-    testSeamStepShrinksInProportionToTheFadeLength();
+    testSeamStepMatchesTheNaturalStepForAnyCrossfade();
    testCrossfadeLengthFollowsItsParameter();
    testCrossfadeIsSuppressedForALoopAtFrameZero();
    testNoteOffDuringLoopSustainRunsTheReleaseAndFreesTheVoice();
@@ -361,6 +361,28 @@ static void testMarkerHandleOnDegenerateAreas() {
    CHECK(markerHandleRect(overlayOf(thin), 1000, 500).height == 4);
 }

+// The shell (editor_input_waveform.cpp) checks the loop crossfade's own grab handle — at
+// loopStart - crossfade — before it iterates the ordinary marker array, because a zero-length
+// fade puts that handle exactly on the loop-start marker's frame. The same coincidence recurs
+// whenever ANY marker shares that frame, most plausibly the START marker dragged up against the
+// fade edge: this module can't exercise the shell's check-order itself, but it can prove the
+// geometric ambiguity that makes the ordering load-bearing — the array's own first-match rule
+// would otherwise resolve the top strip to the START marker, not the fade handle.
+static void testStartMarkerSharesTheHandleStripWhenItSitsAtTheFadeEdge() {
+    const Rect a = wideArea();
+    const std::int64_t loopStart = 400, crossfade = 30;
+    const std::int64_t fadeEdge = loopStart - crossfade;  // where the crossfade handle sits
+    const std::int64_t markers[3] = {fadeEdge, loopStart, loopStart + 100};  // start dialled here
+    const int mx = frameToX(overlayOf(a), 1000, fadeEdge);
+    const int topY = a.y;  // inside the handle's top strip
+    // Without the shell's priority check, the array's own first-match rule already resolves the
+    // column to the start marker (index 0) at this x/y...
+    CHECK(markerAtPoint(overlayOf(a), 1000, markers, 3, mx, topY) == 0);
+    // ...and the fade handle's rect claims the exact same pixel — the ambiguity the shell
+    // resolves by asking the handle first, same as it does for the zero-fade/loop-start case.
+    CHECK(contains(markerHandleRect(overlayOf(a), 1000, fadeEdge), mx, topY));
+}
+
 // --- Per-lane envelope content -------------------------------------------------

 static void testAsymmetricStereoLanesCarryDifferentContent() {
@@ -435,6 +457,7 @@ int main() {
    testCoincidentMarkersStayIndependentlyGrabbable();
    testMarkerHandleClipsIntoTheArea();
    testMarkerHandleOnDegenerateAreas();
+    testStartMarkerSharesTheHandleStripWhenItSitsAtTheFadeEdge();

    testAsymmetricStereoLanesCarryDifferentContent();
    testLaneEnvelopeRejectsOutOfRangeLane();