-
Notifications
You must be signed in to change notification settings - Fork 801
Description
This issue is meant to document and discuss the ongoing effort to fix TriggerUGens init sample calculation, along the lines of supercollider/rfcs#19
part 1: standardize init sample calculation
- completed in plugins: TriggerUGens standardize init sample #6810
This part doesn't change any init sample or output value, so no breaking change. UGens that require a fix that would change either init sample or output values are left for subsequent PRs.
When initializing a UGen, the convention is to reset internal state after the first calc function call in Ctor. UGens changed by #6810 were already calculating init sample and managing internal state correctly, so adding an explicit reset was not functionally necessary. However, it makes it easier to reason about initialization, by aligning with the convention, and provides a better reference to be imitated when writing new UGens.
I made an exception for 3 UGens (see exceptions below), for which I didn't enforce the convention.
Note that this is in principle not just a refactor: the side effect is it forces y(0) recalculation after Ctor, which was not the case before. However, due to the simplicity of these UGens, I argue that the performance cost is negligible (I haven't measured it though).
Affected UGens:
- SetResetFF
- Gate
- Schmidt
- PulseCount
- Stepper
- LastValue
- Latch: I was unsure if making an exception for Latch, but I decided not to.
- LeastChange
- MostChange
Note for MostChange and LeastChange: although #6810 doesn't affect their output or init sample, they behave differently from what the docs say (see #6814).
Exceptions:
These UGens don't call _next in Ctor, and I thought it was for a good reason, so I made an exception and didn't enforce the init convention of calling _next and resetting internal state.
- ZeroCrossing: needs at least 4 samples to output a value
- RunningMin: correctly initialize output to first input value, no calc needed
- RunningMax: correctly initialize output to first input value, no calc needed
Unchanged UGens:
These UGens were doing all right already:
- Trig1: correct y(0), resets
- Trig: correct y(0), resets
- FreeSelf: no output, resets
- PauseSelf: no output, resets (ok to pause twice)
- FreeSelfWhenDone: no internal state
- PauseSelfWhenDone: no internal state
- Done: correct y(0), nothing to reset
- SendTrig: no output, shouldn't call calc in Ctor
- SendReply: no output, shouldn't call calc in Ctor
- Poll: no output, shouldn't call calc in Ctor
- Pause: no calcFunc call, shouldn't call pause in Ctor
- Free: no calcFunc call, shouldn't call NodeEnd in Ctor
- SendPeakRMS: no output, no calcFunc call in Ctor
part 2: minor fixes
The remaining UGens need a fix that would change their init sample or output values.
In #6816 I propose fixes that only introduce limited changes to the UGens output or init sample
- ToggleFF: changes init sample to 1 if UGen has trig on first input sample (x[0] from now on)
- Peak, PeakFollower: changes init sample for negative input on x[0]
- PulseDivider: changes first output sample for x[0] > 0 and div[0] = 1
- Timer: changes output correcting the first measured duration by one sample
part 3: more involved fixes
I'll propose fixes for the three remaining UGens separately, one PR for each, as they need more discussion
- TDelay: plugins: TDelay: fix initialization sample #6832 (changes output: trig at y(0) was not delayed correctly)
- Sweep: plugins: Sweep: apply rate increment after writing to output #6822 (changes output, see issue Sweep starts at a sample count of 1 when beeing triggered by Impulse.ar(0) #6541)
- Phasor: plugins: Phasor: fix init sample: don't ignore trig on first sample #6833 (changes output, see issue plugins: Phasor doesn't start from resetPos on initial trigger #6805)
part 4: testing
we started discussing testing in #6816 (comment). Quoting @mtmccrea:
- As it stands now, no unit tests cover initialization sample issues, so that would be a new suite/category of testing.
- At a minimum, we should aim to have the first "startup samples" tested for UGens,
so we know it outputs what is expected, which is often discernible from the first few samples/blocks. Historically this has meant testing the first x samples against a known output - a [new] UGen that outputs its initialization sample is a nice idea and could be useful for debugging and unit testing
part 5: documentation
As these fixes make the UGens more adherent to their expected behavior, they don't need documentation per-se. However, it would be nice to have a Changelog in schelp to document UGen changes across SC versions. I think we could start it when we have the RC for 3.14.
other issues related to TriggerUGens
- Trigger UGens ak case reads adjacent wires #6803
- Trigger UGens "ka" case redesign #6911
- Add a trigger function to UGen library #6811
- Phasor adds the current input sample when triggered, but the previous input sample when not #4128: Phasor, and in a less impacting way also Sweep, have another issue with subsample interpolation, not being compatible with expected behavior with Impulse-like triggers