Partial bank has no layer-local note-on randoms, but the README says it has two #55

New issue

Closed

opened 2026-08-31 00:17:29 +00:00 by bcox · 1 comment

bcox commented

2026-08-31 00:17:29 +00:00

Owner

The partial bank's engine source vocabulary is its two LFOs and nothing else:

// dsp/partial_layer.h:358
// No randoms. Per-note variation on this engine is per *partial* -- a start
// phase drawn per note is what separates a strike from a chime -- and that is
// a patch mode rather than a matrix source, because one shared draw routed to
// thirty-two frequency lanes moves them all by the same amount, which is a
// detune and not a scatter. Appending them later is an append.
enum Source : int {
  kSourceLfo1 = 0,
  kSourceLfo2,
  kSourceCount,
};

The FM pair has them -- kSourceRandomA/kSourceRandomB, drawn from the engine's own LCG in NoteOn (dsp/fm_pair_layer.h:469, :2136), labelled "Random A"/"Random B" in editor/source_labels.h:47. The VA has none either, and doesn't claim any.

The README's Partial Bank section says otherwise:

There's a mod matrix with 64 entries; all continuous parameters as dests and as 1-sample-delayed sources. Two modulatable layer LFOs, two note-on randoms, plus the usual voice note-on randoms and the voice and global LFOs.

(Found while fact-checking the README. The entry count in that same sentence is wrong too -- kPartialModEntries is 32, not 64 -- and that half is a plain doc fix.)

Which way to resolve

The code's stated reason is narrow and correct as far as it goes: one draw routed across the 32 frequency lanes is a detune, not a scatter, and the scatter you actually want at note-on is already PhaseMode::kRandom over start_phase. But that argues only against randoms as a frequency source. A single per-note draw is still the ordinary way to reach:

amplitude and pan, for note-to-note variation in a struck/plucked bank
timeline speed (all four clocks are destinations), so a strike's decay rate varies per note
LFO rate/width/slew, so two notes' motion decorrelates -- the same complaint as #33, one level down
an amount via, to make any other routing note-dependent

The two voice-level randoms (kSharedRandomA/B) are reachable from this engine and cover some of that today, but they are shared across every layer in the patch: two partial-bank layers stacked in one voice get the same pair, so they cannot be decorrelated from each other.

So the options are:

Append the two sources, mirroring the FM pair verbatim -- two Sample members, two draws off an engine-local LCG in NoteOn, two arms in SourceValue, two rows in editor/source_labels.h. The comment above already calls this an append, and it is: a source is a U16 on disk (plugin/patch_format.h:1169) and the new ids land past kSourceLfo2, so every existing patch keeps meaning what it meant. Worth noting the one-way part -- a patch using the new sources, read by a build predating them, falls through SourceValue's switch to zero rather than being refused.
Fix the README instead, and extend the // No randoms. comment to say the decision covers the whole source vocabulary rather than just the frequency lanes.

I lean toward (1): the README is the design intent, the cost is a handful of members, and the decorrelation argument reaches beyond the frequency lanes the existing comment answers. But it is a design call, not a bug.

Related: #33 (shared S&H seed across LFOs), #36 (general-purpose lanes as sources, which would subsume some of this).

The partial bank's engine source vocabulary is its two LFOs and nothing else: ``` // dsp/partial_layer.h:358 // No randoms. Per-note variation on this engine is per *partial* -- a start // phase drawn per note is what separates a strike from a chime -- and that is // a patch mode rather than a matrix source, because one shared draw routed to // thirty-two frequency lanes moves them all by the same amount, which is a // detune and not a scatter. Appending them later is an append. enum Source : int { kSourceLfo1 = 0, kSourceLfo2, kSourceCount, }; ``` The FM pair has them -- `kSourceRandomA`/`kSourceRandomB`, drawn from the engine's own LCG in `NoteOn` (`dsp/fm_pair_layer.h:469`, `:2136`), labelled "Random A"/"Random B" in `editor/source_labels.h:47`. The VA has none either, and doesn't claim any. The README's Partial Bank section says otherwise: > There's a mod matrix with 64 entries; all continuous parameters as dests and as 1-sample-delayed sources. Two modulatable layer LFOs, **two note-on randoms**, plus the usual voice note-on randoms and the voice and global LFOs. (Found while fact-checking the README. The entry count in that same sentence is wrong too -- `kPartialModEntries` is 32, not 64 -- and that half is a plain doc fix.) ## Which way to resolve The code's stated reason is narrow and correct as far as it goes: one draw routed across the 32 frequency lanes is a detune, not a scatter, and the scatter you actually want at note-on is already `PhaseMode::kRandom` over `start_phase`. But that argues only against randoms as a *frequency* source. A single per-note draw is still the ordinary way to reach: - amplitude and pan, for note-to-note variation in a struck/plucked bank - timeline speed (all four clocks are destinations), so a strike's decay rate varies per note - LFO rate/width/slew, so two notes' motion decorrelates -- the same complaint as #33, one level down - an amount `via`, to make any other routing note-dependent The two voice-level randoms (`kSharedRandomA`/`B`) are reachable from this engine and cover some of that today, but they are shared across every layer in the patch: two partial-bank layers stacked in one voice get the same pair, so they cannot be decorrelated from each other. So the options are: 1. **Append the two sources**, mirroring the FM pair verbatim -- two `Sample` members, two draws off an engine-local LCG in `NoteOn`, two arms in `SourceValue`, two rows in `editor/source_labels.h`. The comment above already calls this an append, and it is: a source is a U16 on disk (`plugin/patch_format.h:1169`) and the new ids land past `kSourceLfo2`, so every existing patch keeps meaning what it meant. Worth noting the one-way part -- a patch *using* the new sources, read by a build predating them, falls through `SourceValue`'s switch to zero rather than being refused. 2. **Fix the README instead**, and extend the `// No randoms.` comment to say the decision covers the whole source vocabulary rather than just the frequency lanes. I lean toward (1): the README is the design intent, the cost is a handful of members, and the decorrelation argument reaches beyond the frequency lanes the existing comment answers. But it is a design call, not a bug. Related: #33 (shared S&H seed across LFOs), #36 (general-purpose lanes as sources, which would subsume some of this).

bcox referenced this issue from a commit

2026-09-01 23:52:18 +00:00

Give the partial bank two layer-local note-on randoms

bcox commented

2026-09-10 23:05:59 +00:00

Author

Owner

Fixed on main by 3e6ebe3 ("Give the partial bank two layer-local note-on randoms"), which took option (1). Closing.

What landed:

kSourceRandomA/kSourceRandomB appended past the LFO block in dsp/partial_layer.h, with static_asserts pinning the new layout in place of the two that pinned the vocabulary at "the LFOs and nothing else".
The draw runs off a generator of its own, note_random_, seeded at index kLfoCount + 1 — appended to the flat index space rather than inserted into it. Drawing off random_ would have shifted every start-phase scatter every existing patch produces by two steps.
"Random A"/"Random B" rows in editor/partial_matrix_labels.h, named exactly as editor/source_labels.h names the FM pair's.
README: 32 entries, not 64, and the two note-on randoms stated as fact. It also dropped the "all continuous parameters as 1-sample-delayed sources" claim in the same sentence, which is true of the VA engine and was never true of this one.
No format bump. WriteModMatrix writes a source as a bare U16 and ReadModMatrix reads one back without validating it against the owning engine's vocabulary, so the new ids travel on bytes that were already there.

Re-verified on the current tree (9e6f351) by injecting into code I did not write:

kSourceRandomB's SourceValue arm pointed at random_a_ → TheMatrixTellsTheTwoRandomsApart failed, and nothing else did.
note_random_ dropped from SeedRandom → TwoLayersInOneVoiceDrawSeparateNoteOnRandoms and TwoVoicesDrawSeparateNoteOnRandoms failed, both slots and all four voices landing on the identical pair (A = -246705570, B = -963814283).

Both injections reverted; //dsp:partial_layer_test, //editor:partial_matrix_labels_test and //plugin:patch_format_test pass on a clean tree.

The caveat this issue opened with survives in the enum comment, scoped to what it actually argues: one draw across thirty-two frequency lanes is a detune and not a scatter, and PhaseMode::kRandom over start_phase — per partial, which no single matrix source can be — stays the thing that scatters a bank at note-on. #33 and #36 are untouched.

Fixed on main by 3e6ebe3 ("Give the partial bank two layer-local note-on randoms"), which took option (1). Closing. What landed: - `kSourceRandomA`/`kSourceRandomB` appended past the LFO block in `dsp/partial_layer.h`, with static_asserts pinning the new layout in place of the two that pinned the vocabulary at "the LFOs and nothing else". - The draw runs off a generator of its own, `note_random_`, seeded at index `kLfoCount + 1` — appended to the flat index space rather than inserted into it. Drawing off `random_` would have shifted every start-phase scatter every existing patch produces by two steps. - "Random A"/"Random B" rows in `editor/partial_matrix_labels.h`, named exactly as `editor/source_labels.h` names the FM pair's. - README: 32 entries, not 64, and the two note-on randoms stated as fact. It also dropped the "all continuous parameters as 1-sample-delayed sources" claim in the same sentence, which is true of the VA engine and was never true of this one. - No format bump. `WriteModMatrix` writes a source as a bare U16 and `ReadModMatrix` reads one back without validating it against the owning engine's vocabulary, so the new ids travel on bytes that were already there. Re-verified on the current tree (9e6f351) by injecting into code I did not write: - `kSourceRandomB`'s `SourceValue` arm pointed at `random_a_` → `TheMatrixTellsTheTwoRandomsApart` failed, and nothing else did. - `note_random_` dropped from `SeedRandom` → `TwoLayersInOneVoiceDrawSeparateNoteOnRandoms` and `TwoVoicesDrawSeparateNoteOnRandoms` failed, both slots and all four voices landing on the identical pair (A = -246705570, B = -963814283). Both injections reverted; `//dsp:partial_layer_test`, `//editor:partial_matrix_labels_test` and `//plugin:patch_format_test` pass on a clean tree. The caveat this issue opened with survives in the enum comment, scoped to what it actually argues: one draw across thirty-two frequency lanes is a detune and not a scatter, and `PhaseMode::kRandom` over `start_phase` — per *partial*, which no single matrix source can be — stays the thing that scatters a bank at note-on. #33 and #36 are untouched.

bcox closed this issue

2026-09-10 23:06:03 +00:00