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

fix: pin the waveform arbitration in a testable predicate, close round-4 review minors

Browse Source 
Extracts resolveWaveformClaim (core/instrument/ui/spline_edit) so the shell's node/tab/marker click resolution is unit-tested directly, not just its input geometry; folds the staged-envelope node into it; fixes comment accuracy, a cost regression, and test fidelity issues.
This commit is contained in:
Daniel Harvey
2026-08-01 00:17:06 -04:00
parent 757e1585d6
commit aedcc6976c
10 changed files with 316 additions and 179 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/shell/instrument/editor_input_waveform.cpp
+57 -57
View File
@@ -39,88 +39,88 @@ bool ReaSamplerEditor::mouseDownWaveform(const FaceLayout& fl, int x, int y) {
? SplineGesture::kControlLeft
: SplineGesture::kLeft;
// Envelope nodes first (they sit on top of the markers), then the wave markers. With no
// envelope overlay-active — or with its deck group's enable toggle off, which makes the
// same params' knobs inert — there are no grabbable nodes and the markers take every grab.
// The staged envelope's draggable node and the drawn contour's node are mutually exclusive
// (overlayEnvInert flips the staged one inert exactly when its envelope is in Spline mode),
// so at most one of the two hit-tests below is ever live for the same click — both feed the
// SAME arbitration slot below rather than either one getting its own check-order return.
const double rate = liveSampleRate();
const bool nodesLive =
overlayEnv_ != OverlayEnv::kNone && !overlayEnvInert(overlayEnv_, gates);
NodeHit envNodeHit;
StageEnvelope env;
if (rate > 0.0 && nodesLive) {
const std::int64_t startFrame = params_.startPoint.value_or(0);
const StageEnvelope env = packEnvelope(overlayEnv_, params_.play, frames, startFrame);
env = packEnvelope(overlayEnv_, params_.play, frames, startFrame);
const double totalSeconds = static_cast<double>(frames) / rate;
const NodeHit nh = nodeAtPoint(env, overlay, totalSeconds, x, y);
if (nh.hit) {
drag_ = DragKind::kEnvNode;
envNode_ = nh.node;
dragStartX_ = x;
dragStartY_ = y;
dragStartEnv_ = env;
dragSampleFrames_ = frames;
dragStartParams_ = params_;
return true; // node moves once the cursor drags
}
envNodeHit = nodeAtPoint(env, overlay, totalSeconds, x, y);
}
const SetupMarkers m = pickedMarkers(frames);
const std::int64_t markerFrames[3] = {m.start, m.loopStart, m.loopEnd};
// Three affordances can claim the same pixel: a contour node (a small fixed pick box), the
// crossfade tab (a small clipped top-strip tab), and a marker's full-height grab column
// (waveform_view.h's tab-vs-column split already keeps the tab apart from ITS OWN column;
// this is the cross-affordance case on top of that). Resolving by any fixed check order
// shadows whichever one loses the tie — this seam regressed twice from exactly that fix.
// Instead measure each claimant's own target area and let the SMALLEST hit win: the marker
// column is the odd one out (its target is the whole band height), so it only wins where
// nothing narrower also claims the pixel. Never add here (kAdd is only tried once nothing
// else has claimed the click, below).
struct Candidate {
bool hit = false;
std::int64_t area = 0;
};
Candidate node;
if (splineLive) {
// Three affordances can claim the same pixel: a node (the staged envelope's or the drawn
// contour's — a small fixed pick box either way), the crossfade tab (a small clipped
// top-strip tab), and a marker's full-height grab column (waveform_view.h's tab-vs-column
// split already keeps the tab apart from ITS OWN column; this is the cross-affordance case
// on top of that). resolveWaveformClaim (spline_edit.h) is the ONE arbitration: it measures
// each claimant's own NOMINAL target area and lets the smallest hit win, since a fixed check
// order shadows whichever one loses the tie — this seam regressed twice from exactly that
// fix. Never add here (kAdd is only tried once nothing else has claimed the click, below).
WaveformClaim node;
if (envNodeHit.hit) {
constexpr std::int64_t side = 2 * kNodeGrabRadius + 1;
node = {true, side * side};
} else if (splineLive) {
const VelocityCurve::Box box = splineOverlayBox(overlay);
// Also require strict in-box, matching splineOverlayClick's own narrowing (spline_edit.h's
// grammar note) — otherwise this candidate could "win" the arbitration below for a click
// splineOverlayClick would then refuse, silently swallowing it instead of falling through
// to the tab/marker checks.
if (contains(overlay.rect, x, y) && splineFor(overlayEnv_).pointAtPixel(box, x, y) >= 0) {
node.hit = true;
constexpr std::int64_t side = 2 * kCurveNodeGrabRadius + 1;
node.area = side * side;
node = {true, side * side};
}
}
Candidate tab;
const Rect tabRect =
m.hasLoop ? markerHandleRect(overlay, frames, m.loopStart - m.crossfade) : Rect{};
if (m.hasLoop && contains(tabRect, x, y)) {
tab.hit = true;
tab.area = static_cast<std::int64_t>(tabRect.width) * tabRect.height;
}
const WaveformClaim tab = (m.hasLoop && contains(tabRect, x, y))
? WaveformClaim{true, static_cast<std::int64_t>(tabRect.width) *
tabRect.height}
: WaveformClaim{};
// Nominal, not actual: markerAtPoint clips the column at the overlay edges (a marker at
// frame 0 has 6 usable columns, not 11) and the node's fixed side clips too at a pick-box
// corner. Both overestimate in the direction that already produces the intended winner, so
// the arbitration runs on NOMINAL area, not the measured hit-testable pixel count.
const int markerHit = markerAtPoint(overlay, frames, markerFrames, 3, x, y);
Candidate marker;
if (markerHit >= 0) {
marker.hit = true;
marker.area = static_cast<std::int64_t>(2 * kMarkerGrabWidth + 1) * overlay.rect.height;
}
const WaveformClaim marker =
(markerHit >= 0)
? WaveformClaim{true, static_cast<std::int64_t>(2 * kMarkerGrabWidth + 1) *
overlay.rect.height}
: WaveformClaim{};
// Node checked before marker on an area tie so a degenerate (zero-height) overlay still
// prefers the node — unreachable in practice (sample_bands floors the band well above it),
// kept only so this arbitration has one well-defined answer for every input, not just the
// ones the current geometry constants happen to produce.
if (node.hit && (!tab.hit || node.area <= tab.area) && (!marker.hit || node.area <= marker.area)) {
return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false);
}
if (tab.hit && (!marker.hit || tab.area <= marker.area)) {
beginMarkerDrag(WaveMarker::kLoopXfade, m, frames, x);
return true;
}
if (marker.hit) {
beginMarkerDrag(static_cast<WaveMarker>(markerHit), m, frames, x);
return true;
switch (resolveWaveformClaim(node, tab, marker, gesture)) {
case WaveformClaimant::kNode:
if (envNodeHit.hit) {
drag_ = DragKind::kEnvNode;
envNode_ = envNodeHit.node;
dragStartX_ = x;
dragStartY_ = y;
dragStartEnv_ = env;
dragSampleFrames_ = frames;
dragStartParams_ = params_;
return true; // node moves once the cursor drags
}
return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/false);
case WaveformClaimant::kTab:
beginMarkerDrag(WaveMarker::kLoopXfade, m, frames, x);
return true;
case WaveformClaimant::kMarker:
beginMarkerDrag(static_cast<WaveMarker>(markerHit), m, frames, x);
return true;
case WaveformClaimant::kNone:
break;
}
// Nothing else wanted the click: now the drawn contour may take the empty space.
if (splineLive) return splineOverlayClick(overlay, x, y, gesture, /*addOnEmptySpace=*/true);
src/shell/instrument/editor_session.cpp
+6 -2
View File
@@ -186,8 +186,12 @@ ReaSamplerEditor::SetupMarkers ReaSamplerEditor::pickedMarkers(std::int64_t fram
// Seed from the bank's intrinsic loop (fact about the file), then let the parameter set's
// override win (the instrument's performance choice). Read the loop intrinsic from the
// live bank blob (the same path selectSample uses); when that is not readable (extension
// absent / not yet parsed) the instance-owned ref carries the same intrinsics.
if (processor_) {
// absent / not yet parsed) the instance-owned ref carries the same intrinsics. Skipped
// entirely once an override is already set — it would just be overwritten below, and the
// bridge read + JSON parse it costs is real (mouseDownWaveform's arbitration calls this on
// every waveform click, not just marker grabs, to know whether a tab or marker candidate
// hits at all).
if (processor_ && !params_.loopOverride) {
std::optional<SelectedSample> sel;
auto banksJson =
processor_->bridge().readReasamplerExtState(reasampler::kProjExtBanksKey);