patternist: fix note-offs that never reach the output

[jcelerier]

Companion to ossia/libossia#914 and ossia/libossia#915.

The Patternist is not dropping note-offs

Reported symptom: a pattern alternating 36 / 38 across two lanes, observed through MIDI to array → Value display, only ever shows [144, 36, 100] [144, 38, 100].

The step logic is correct. Lifting pattern_node's step loop verbatim into a standalone program against the real libremidi gives:

step 0:  [90 36 100]
step 1:  [80 36 0]  [90 38 100]
step 2:  [80 38 0]  [90 36 100]

and from_midi1::note_off(0, 36, 0) round-trips through UMP to 80 24 00. The note-offs are emitted and they are well-formed.

The measurement chain is what loses them. The note-off and the note-on of a step share one timestamp and land in the same tick, in that order. MIDI to array writes both into its value port, in order. Value display then takes a plain (non sample-accurate) ossia::value control input, and avendish samples those with:

// port_run_preprocess.hpp
auto& last = port.data.get_data().back().value;

Only the last value of the tick survives — always the note-on. On top of that ExecutorUpdateControlValueInUi::handle_controls drains its queue keeping only the last tuple per coarseUpdateTimer poll (~60–100ms), so values are lost across ticks too. The chain cannot show a note-off that is immediately followed by a note-on, whatever the source emits.

What this PR actually fixes

None of the below is the cause of the report above; they are real note-off-losing defects found while chasing it.

end_discontinuous stamps its note-offs at 0. That is outside [tick_start; tick_start + frames[ as soon as the interval does not begin on a buffer boundary, so consumers that window on the tick drop them — avendish does exactly this in port_run_preprocess.hpp. in_flight.clear() runs right after, so those notes are never released again. Now stamped at the start of the tick.

all_notes_off() did not clear in_flight, so the same notes were offed a second time on the following step. Both paths now go through release_all().

Channel handling. channelChanged assigned the raw 1–16 model value to the 0–15 wire channel, off by one against the constructor's element.channel() - 1. Changing the channel while notes were held also sent their note-offs on the new channel, stranding them on the old one. The node now remembers which channel the in-flight notes were started on and releases them before switching.

Robustness. pattern.length <= 0 reached current = (current + 1) % pattern.length (modulo by zero); tk.speed == 0 divided by zero when computing the event date.

Default pattern. It declared length = 4 for 16-step lanes, so three quarters of the built-in rhythm never played.

[coveralls]

coverage: 15.319% (-0.002%) from 15.321% — fix/patternist-note-off into master

[jcelerier]

Added tests, plus the refactor they needed.

pattern_node lived inside PatternExecutor.cpp, behind the Execution component machinery, so none of it was reachable from a test. It now lives in Patternist/PatternNode.hpp, which the executor includes — no behaviour change, the class is byte-identical.

tests/unit/PatternistNodeTest.cpp drives it tick by tick. Leaving the musical fields of the token at zero makes get_quantification_date() return prev_date, so one tick is exactly one step — that keeps these tests about note pairing rather than about the quantization arithmetic, which token_request owns and QuantificationTest already covers.

Covered:

• a step releases what the previous one held — single lane, alternating lanes (the 36 38 36 38 case from the report), and a note repeated on one lane
• legato holds a note across a step and releases it when the lane rests; a legato step strikes the note if it was not already held
• end_discontinuous releases inside the tick rather than at 0, and does not release twice
• all_notes_off() clears the in-flight set
• changing channel releases the held notes on the channel they were struck on
• channel conversion and clamping
• a zero pattern length and a zero speed do not divide by zero
• steps past the end of a shorter lane still release what is held
• lanes above the MIDI range (accent 255, slide 254) are not struck

⚠ Ordering with ossia/libossia#914

This commit also updates tests/unit/MidiMessageTest.cpp. Its "ossia midi node emits note-on / note-off" case pinned the old message order — every note-off first, regardless of when it happens — which libossia#914 deliberately changes to chronological order. In the tick it exercises, note B starts at 250 before note A ends at 300, so the note-on now precedes the note-off.

That means this PR needs the libossia submodule bumped to include ossia/libossia#914, or that test fails. Merge #914 first and bump, then this.

[jcelerier]

Second review of the playback path turned up one more, and it is the plainest instance of the reported symptom yet: stopping a pattern never released what it was holding.

node_process::stop() calls all_notes_off(), which wrote the note-offs straight into the outlet. That runs outside of a tick, and graph_util::init_outlet() clears every outlet before the node runs again:

static void init_outlet(outlet& out, execution_state&)
{
  out.visit(clear_data{});   // midi_port -> messages.clear()
  out.pre_process();
}

so nothing ever read them. ossia::nodes::midi escapes this because midi_node_process::stop() requests a tick and raises a flag the node consumes from run(), where the outlet is live:

void stop() override
{
  midi& n = *static_cast<midi*>(node.get());
  n.request(ossia::token_request{});
  n.mustStop = true;
}

Same shape here now: all_notes_off() only raises mustStop, a new pattern_node_process requests the tick, and the flush happens in run(). The requested token is default-constructed (prev_date == date), so mustStop is handled before the empty-tick early return — with a test pinning exactly that, since it is the kind of thing a later reader would move.

Worth noting my earlier change made this worse before it made it better: the original all_notes_off() did not clear in_flight, so the notes stayed queued and the next step would emit real note-offs. Routing it through release_all() cleared them, which removed that accidental safety net. Now the flush is real, so clearing is correct.

Two cases replace the old all_notes_off one: nothing is written outside the tick, the release happens on the next one and only once; and the flag is honoured on an empty tick.

[jcelerier]

Patternist no longer drops steps.

get_quantification_date() reports only the first quantification point of a tick, so every other step in it vanished without a trace. A small division, a large buffer or a high tempo are enough: a quarter note of music at a sixteenth division is four steps, of which one was played.

It now iterates get_quantification_dates() — added in ossia/libossia#916 — and plays them all, each stamped at its own date inside the buffer. The step body moved to play_step() unchanged.

Test: one tick worth a quarter note, two alternating lanes, sixteenth division. Seven messages at 0 / 250 / 250 / 500 / 500 / 750 / 750, note-off before note-on at each shared timestamp. Previously that tick produced one.

This PR now depends on two libossia PRs: ossia/libossia#916 for get_quantification_dates(), and ossia/libossia#914 for the MidiMessageTest ordering expectation. Both need the submodule bumped before this builds.

[jcelerier]

Built and run locally: all 15 Patternist cases pass, 103 assertions, clang 22 on Windows, against a score tree with the three libossia branches applied.

PatternNode.hpp and the test also compile cleanly against the flags score already uses for PatternExecutor.cpp, so the extraction and the switch to get_quantification_dates() are sound.

One thing worth knowing, unrelated to this PR: tests/unit/MidiMessageTest.cpp does not build on Windows at all.

tests/unit/MidiMessageTest.cpp:41:26: error: use of undeclared identifier 'SIGTRAP'
tests/unit/MidiMessageTest.cpp:44:40: error: use of undeclared identifier 'SIGTRAP'

ScopedIgnoreSigtrap uses SIGTRAP, which the mingw/clang64 CRT does not define. Pre-existing — it predates everything here — so the ordering expectation I changed in that file is still only verified by reasoning, and only Linux CI will exercise it. Worth guarding that helper behind a platform check if the Windows job is ever meant to run these.

[jcelerier]

MidiMessageTest builds and passes on Windows now — 202 assertions, 8 cases, so the note ordering expectation is covered somewhere other than Linux CI.

Score already knew how to do this: Debug.hpp maps DEBUG_BREAK to DebugBreak() on Windows and raise(SIGTRAP) elsewhere. The test only mirrored the POSIX half, so SIGTRAP was undeclared and the whole file failed to compile.

The Windows side installs a vectored exception handler that swallows EXCEPTION_BREAKPOINT. One thing worth keeping in the comment: it has to step the instruction pointer over the trap by hand.

const auto at = reinterpret_cast<uintptr_t>(ex->ExceptionRecord->ExceptionAddress);
ex->ContextRecord->Rip = at + 1; // int3

My first version just returned EXCEPTION_CONTINUE_EXECUTION, on the assumption that the reported context already points past the int3 the way it does for a trap on some platforms. It does not: the test spun at 100% CPU until I killed it. Reading would never have settled that one.

Renamed to ScopedIgnoreDebugBreak, since it is no longer about a signal.

[jcelerier]

Superseded by #2163, which carries every commit from this branch plus the backwards-playback work for the plug-in hosts. Nothing here is dropped:

• patternist: fix note-offs that never reach the output
• tests: cover the patternist step engine
• patternist: actually release held notes when stopping
• patternist: play every step a tick covers
• tests: make MidiMessageTest build and run on Windows

#2163 additionally makes the pattern walk backwards when the timeline does, and takes each step's sample from the grid point's musical position rather than its flick-truncated date. Closing in favour of that one.