Repository navigation
Conversation
Sustain scales with velocity, so a soft note does not swell up to the sustain level. Stagger is an entry: each player's attack, filter and scoop wait on a gate a timer envelope opens Stagger times its share after the key, rather than the attack being stretched and the scoop lengthened. Needs env::adsr to wait for a wired trigger (#337); until then every player enters with the first.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Short attacks can consume the initial trigger edge twice, shifting the envelope by one sample.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates env::adsr so wired triggers control every envelope phase while preserving legacy constant-trigger behavior.
Changes:
- Adds edge-driven attack, release, and retrigger handling.
- Preserves the existing free-running path for constant triggers.
- Adds gated-envelope and window-consistency tests.
| File | Description |
|---|---|
plugins/env/adsr.cpp |
Adds separate gated and legacy envelope paths. |
scripts/statecheck.cpp |
Tests wired triggers, release behavior, compatibility, and state consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sustain scales with velocity, so a soft note does not swell up to the sustain level. Stagger is an entry: each player's attack, filter and scoop wait on a gate a timer envelope opens Stagger times its share after the key, rather than the attack being stretched and the scoop lengthened. Needs env::adsr to wait for a wired trigger (#337); until then every player enters with the first.
| } | ||
|
|
||
| out[i] = level; | ||
| play[i] = (phase == DONE || phase == WAITING) ? 0 : 1; |
There was a problem hiding this comment.
This is intended, and I've added a comment saying why in ef015f2. A waiting envelope isn't sounding, and its gate may never open: a note shorter than the delay it was waiting out releases first. If WAITING reported play = 1, any graph that put such an envelope on its io node would hold the voice forever on every short note. A voice's own lifetime belongs to an envelope on the voice's trigger, which rises with the note on the first sample, so it never waits. A delayed envelope adds to that rather than replacing it, which is what dsp/violins.dsp and dsp/horns.dsp do with play = max(...) over their players, player 1 never being delayed. A graph whose only envelope waits on a gate has nothing that says the note has started, and retiring that voice is the right answer.
| windowsAgree(pluginPath, adsrGraph("sq", sr * 0.2f, sr * 0.1f, 0.6f, | ||
| sr * 0.1f), |
There was a problem hiding this comment.
Fixed in ef015f2. The check now drives the envelope with a 50 Hz square (up 441 samples, down 441) and short segments, so the 2,000 samples it compares hold several rises, attacks, decays, releases and a release cut short by the next rise. As a mutation check, I dropped the release's starting level between windows, and the test fails at sample 883.
With the trigger wired to anything, the envelope waits for it to rise, releases on a fall from wherever it is (attack and decay included) and from the level it reached, and retriggers only on a rising edge, from that level. It used to start when its voice did, play out the attack and decay of a note let go during them, release from the sustain level whatever it was at, and restart every sample of a held trigger once a decay reached a sustain of 0. A trigger written as a number, or not at all, keeps the old code path to the bit: the shipped one-shots (brass's bend, bd10, dxbell) are built on it. A note held past its decay comes out sample for sample as before.
The voice's first rise is spent on the sample it starts the attack: with an attack of a sample or less the envelope has reached the decay by the end of that sample, and the end-of-sample edge check took the same rise again, emitting the peak twice and shifting the decay. Only that edge is suppressed now, so a rise during an attack `reset' started still retriggers.
The window-agreement check drives the envelope with a 50 Hz square and short segments, so the two thousand samples it compares hold rises, attacks, decays, releases and a release cut short, where a 2 Hz square left them all in WAITING. Losing the release's starting level between windows now fails it. And why a waiting envelope reports play = 0 is said where it does.
env::adsrhad three bugs whenever its trigger was wired:s = 0under a held key looped.The change
The first test is whether the trigger is wired or written as a constant (read from the node's own arg type).
trigger = 0(brass's bend, bd10, dxbell, …) are built on it.What changes audibly
I rendered every piece (60 s) and every instrument graph (a 30 ms note, then a 1.3 s note) with the old and new plugin:
bed(8 s attack),juno/strings/supersaw/section(a short note no longer swells to full), andepiano(2.6 s decay, so held notes shorter than that now release).The shorter piece renders are the voices ending when their notes do.
These are the intended consequences: notes now last as long as they're played. Pieces voiced against the old behavior may want their pad holds or attacks revisited. Old/new previews of pearl, boombox, sunrise and anthem are available locally.
Testing
ctest: 46/46 pass.statecheckcases for the wired trigger: it waits for the rise; a fall mid-attack releases from the level reached; the release runs out; a decay to 0 under a held trigger ends; an unwired trigger still runs once; window agreement.