Topic/impulse rewrite

[mtmccrea]

Purpose and Motivation

This PR addresses a number of longstanding issues with Impulse that I've aggregated from issues, comments, etc. which are highlighted below.

The primary goal was to initialize the UGen properly and support new frequency and phase argument ranges.

Part fo the initialization sample fix project, specifically in the LFUGens subset.

Types of changes

Added features

• Supports positive and negative frequencies.
• Phase is wrapped internally, so any phase offset is valid.
• Correctly sets the initialization sample and first output sample.

Fixes

#4752, #4075, likely more trigger-related issues

Implementation notes to review

1. Added a wrapping function.
   I chose to use a function to wrap the phase, taking advantage of the fact that the range is [0,1].
   This could be useful for other UGens with a phasor in this range.
   https://github.com/mtmccrea/supercollider/blob/36cadad0eef499a8122a9136fe2f17acace566ca/server/plugins/LFUGens.cpp#L915-L937

2. Trigger logic: freq drives impulse triggering, not phase offset.
   Phase offset is applied, then phase is wrapped, then phase increment (freq) is applied, then the condition is checked for whether an impulse fires. In this way, phase changes are "instantaneous" and always in-range internally. So phase increment (the frequency control) primarily determines when impulses fire.

In other words, if you advance the phase offset by 3 periods, it's a relative phase offset of 0, so that won't cause an impulse to fire.

3. Argument input rates aren't limited.
   You can read in audio rate arguments even if the UGen running at control rate.
   If it's advisable to limit kr ugens to kr arguments, that can be added to the class.

4. A k-rate frequency argument is now interpolated when running at a-rate.
   This wasn't the case before. My understanding is that all k-rate args should be interpolated when used at a-rate, unless there's a particular reason not to.

5. phOffChanged flag added to Impulse_next_ak.
   This flag checks if k-rate phase offset input changes, and if not, skips adding a slope increment of 0 in the sample loop.
   Are these kind of optimizations worth it?... benchmarks seem to indicate yes.
   https://github.com/mtmccrea/supercollider/blob/36cadad0eef499a8122a9136fe2f17acace566ca/server/plugins/LFUGens.cpp#L963-L973
   Aside: we might consider adding support for this kind of optimization in SCUnit::SlopeSignal

6. Variable names are changed
   To more precisely reflect what they are. E.g. unit->mFreq wasn't actually a frequency, but rather a phase increment (unit->mPhaseIncrement).

Bug fixes #

1. One of the primary issues resolved is the Ctor set the initialization sample to 0, incorrectly setting the initial state of downstream UGens. This bug is expressed in the initial state of, e.g., ToggleFF, RLPF, etc.

2. Phase offset isn't wrapped - fixed.
   Ex: Phase is out of range for 3 k-periods, so erroneously outputs 1s for those 3 control samples.
   plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)

{Impulse.kr(10, 3.8)}.plot(duration:0.03).plotMode_(\points);

3. Negative phase offset isn't handle properly - fixed.
   Ex: A phase offset of -1 is equivalent to phase offset of 1, which should trigger an impulse (should output 1 on first sample).

{Impulse.kr(10, -1)}.plot(duration:0.3);

4. First output sample set incorrectly - fixed.
   See Impulse UGEN sets the presample to the wrong value when frequency equals zero #4075

( // envelope triggers one control period late
{
  var sig1 = EnvGen.kr(Env.perc(0.01, 0.04), 1) * SinOsc.ar(440);
  var sig2 = EnvGen.kr(Env.perc(0.01, 0.04), Impulse.kr(0)) * SinOsc.ar(440);
  [sig1,sig2]
}.plot
)

5. Negative frequencies aren't supported - fixed.
   Ex: There's no impulse after first sample:

{Impulse.kr(-10, 0)}.plot(duration:0.3);

6. Inconsistent behavior with Lag - resolved.
   See: plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)
   This behaves properly now.

{Lag.ar(Impulse.ar(1, [0, 0.995]), 0.003)}.plot;

7. Interaction with RLPF - resolved
   plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)

(
s.waitForBoot({
b = Buffer.alloc(s, 256, 1);
0.3.wait;
a = {
  var sig = RLPF.ar(Impulse.ar(0), 1200, 0.1);
  RecordBuf.ar(sig, b, loop: 0, doneAction: 2);
  sig
}.play;
0.3.wait;
// to inspect the IR
b.getToFloatArray(wait: -1, action: { |data| d = data.postln });
})
)
// before (note impulse is broken also in this signal chain):
// -> FloatArray[ 0.0072281970642507, 0.028581265360117, 0.055974174290895, 0.081284336745739, 0.10381526499987, 0.12296265363693, 0.1382302492857, 0.14924238622189, ...
// after this change, with corrected Impulse:
// -> FloatArray[ 0.035809461027384, 0.084555439651012, 0.13725851476192, 0.18509960174561, 0.22677791118622, 0.26119288802147, 0.28747263550758, 0.30499523878098, ...
// after this change AND after fixing RLPF Ctor:
// -> FloatArray[ 0.0072281970642507, 0.028581265360117, 0.055974174290895, 0.081284336745739, 0.10381526499987, 0.12296265363693, 0.1382302492857, 0.14924238622189...

// see first 100 samples of the result
d[0..99].plot

Note, the original IR in the above comment is "erroneously correct".
This correction to Impulse appeared to "break" this RLPF case, but the problem is to do with an initialization bug in RLPF (#4127), causing the appearance of an impulse double in size and off in phase (coefficients are improperly initialized in RLPF). Once this is corrected, it works again, which coincidentally matches the erroneous IR.

Tests

UPDATE: Newly added unit tests will confirm the above issues are fixed as well as the rate-equivalence tests below.

UnitTest.runTest("TestCoreUGens:test_impulse")

I made numerous tests for the above changes, though here's a basic test comparing the output from all variations of argument rates and output rates:

// test for equal output at different input rates
// Note, this doesn't test modulated parameters, as
// results will vary because of k>a interpolation
(
var funcs;
{
	var frq = -32.9327;
	var phs = -0;
	var rate = \ar; // \ar or \kr

	funcs = [
		{Impulse.perform(rate, DC.ar(frq), DC.ar(phs))},
		{Impulse.perform(rate, DC.ar(frq), DC.kr(phs))},
		{Impulse.perform(rate, DC.ar(frq), phs)},
		{Impulse.perform(rate, DC.kr(frq), DC.ar(phs))},
		{Impulse.perform(rate, DC.kr(frq), DC.kr(phs))},
		{Impulse.perform(rate, DC.kr(frq), phs)},
		{Impulse.perform(rate, frq, DC.ar(phs))},
		{Impulse.perform(rate, frq, DC.kr(phs))},
		{Impulse.perform(rate, frq, phs)},
	];
}.value;

~res = Array.newClear(funcs.size);

fork{
	var cond = Condition();
	funcs.postln;
	funcs.do{ |f, i|
		f.loadToFloatArray(duration: 0.15, action: { |arr| ~res[i] = arr; i.post; " done".postln; cond.test_(true).signal; });
		cond.wait;
		cond.test_(false)
	};
	postf("result: %\n", ~res.every(_ == ~res[0]));
}
)
// inspect one
~res[0].plot.plotMode_(\plines);

Checklist

• All previous tests are passing
  • 3 tests fail which previously did not (in TestCoreUGens:test_ugen_generator_equivalences), all relating to Trig and Trig1. After applying init sample bug fixes to both UGens, the tests pass.
  • A change was made to TestFilterUGens:test_time_invariance to accommodate initialization sample bugs that pervade FilterUGens
• Tests were updated or created to address changes in this PR, and tests are passing
• Updated documentation, if necessary
• This PR is ready for review

[mtmccrea]

Update: this PR has been updated with clang-format.

[jamshark70]

The logic is a massive improvement. Let's please get this into 3.11. There are concrete bugs with this UGen that are fixed here.

I'm going to say "comment" for now rather than "approve" because my comments are questions, not "things to change":

• How many calculation functions do we need?
• What should be the behavior of a 0-frequency Impulse with a moving phase offset?

[jamshark70]

I don't know how to address it, but when this reaches the stage of c++ style review, I expect there to be a comment about the number of calculation functions. There have been some discussions lately about the topic of handling the permutations.

If you remove all the i functions, then we go from 9 down to 4 -- reducing the maintenance load by a factor of 2.22, while the performance gain of the i functions would be minuscule. So I'd start there.

[mtmccrea]

Yes, 9 is overkill. I've trimmed it to 7 (dropping a-rate phase offset for k- and i-rate frequencies). If needed we could also drop a-rate phase offset altogether. I kept it because I recall a user on the scsynth forums being surprised it wasn't supported.
I agree we could drop i-rate altogether, falling back on a k-rate minimum for both args. Although one of the goals of this PR was support more rates. If maintenance is the issue though, i-rate is the easiest to introspect.

[jamshark70]

This logic is sound, but I admit, I really had to scratch my head to understand it. Could this comment perhaps be expanded?

A particular sticking point is that it isn't immediately obvious why both phase and phaseOffset are initialized to the phase input. I get it now -- the phase adjustment is based on input phase - previous phase (so, it's 0 if the phase input is constant).

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen. The workaround of course would be HPZ1.ar(Phasor.ar(...)) < 0 instead of Impulse but I guess some users may be surprised by this.

[mtmccrea]

This logic is sound, but I admit, I really had to scratch my head to understand it. Could this comment perhaps be expanded?

Agreed. I've updated the comment to read more clearly.

A particular sticking point is that it isn't immediately obvious why both phase and
phaseOffset are initialized to the phase input.

Part of the confusion may come from the fact that the UGen argument for
phase offset on the sclang side is just called phase. Whereas internally, phase is
the actual phase state value. Throughout the UGen I've renamed local vars
phaseInc and phaseOff to simply inc and off, hopefully disambiguating
it from phase.

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) ...

Great point, I was also surprised the Impulse doesn't support driving by the
phase(offset). Based on your comment I've added this to the Discussion in
the Help file, as well as suggesting the HPZ solution for generating an
impulse train with a Phasor.

[muellmusik]

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen.

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME. Don't know if this changes that, but if so it could break code. But maybe that's not an issue here?

[jamshark70]

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME. Don't know if this changes that, but if so it could break code. But maybe that's not an issue here?

I didn't test it (I should -- this is a very important use case!) but I believe it will be fine, based on lines 1023-1025 in the revision:

    if (initPhase == 0.0 && initPhaseInc >= 0.0) {
        initPhase = 1.0; // positive frequency trigger/wrap position
    }

EDIT: though I believe && initPhaseInc >= 0.0 is unnecessary...?

[muellmusik]

I didn't test it (I should -- this is a very important use case!) but I believe it will be fine, based on lines 1023-1025 in the revision:

Cool. Sorry if I'm adding noise, just thought it good to check!

[mtmccrea]

Thanks @jamshark70 and @muellmusik for having a look at this.
It's been a very long time since I wrote this and I remember it being quite involved and a lot of testing. I'm short on time at the moment but I'd love to move this along. Is there a rough timeline for 3.11?

Briefly,

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME.

I do remember testing this, and based on a quick glance at the additions to the help docs, this is a supported case. But, I'd have to rebuild this and test with the case that @jamshark70 brought up:

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen.

I'll try to get back into this headspace and test soon. And yes, c++ style review is welcomed! This was a long time ago, before the style guidelines were set, and some of the style here was based on the previous Impulse code, which looked a bit strange.

[mossheim]

Is there a rough timeline for 3.11?

3.11 beta should be released on Tuesday; this will probably go into 3.12+.

[jamshark70]

I just (accidentally) discovered some fallout from this:

(
{
	var trig = Impulse.kr(1);
	Sweep.kr(1).poll(trig);
	FreeSelf.kr(PulseCount.kr(trig) >= 3);
	Silent.ar(1)
}.play;
)

// current SC version
UGen(Sweep): 0.00145125
UGen(Sweep): 1.00281
UGen(Sweep): 2.00272

// with this PR: Poll doesn't respond to the initial trigger
// (but, PulseCount *did* catch the first one!)
UGen(Sweep): 1.00281
UGen(Sweep): 2.00272

These changes to Impulse mean that the Ctor initializes the "pre-sample" to 1 when the Impulse is starting at phase 0. I've argued elsewhere that this is correct -- e.g. Lag something that depends on an initial impulse, and in current SC, the lag will start at the wrong value (but it's fixed with this PR): { Lag.ar(Trig1.ar(Impulse.ar(0), 0.002), 0.005) }.plot;.

It also makes sense that inputs expecting a trigger should initialize their unit->m_trig to 0.

But many trigger inputs, e.g. Poll_Ctor, do unit->m_trig = IN0(0);. Now, if you think about it, that's nonsense -- but somehow, it appears in multiple places among the UGens.

So we have two choices:

• Revert Impulse's pre-sample to 0. That's the easy way -- but (and I'm willing to argue until I'm blue in the face about it) this is logically incorrect.

• Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

(When I raised this initialization issue before, the decision at that time was to avoid breaking existing code. But that has a maintenance cost too, which is now coming due -- if we had fixed it properly back when I brought it up, then this PR would be straightforward.)

[mtmccrea]

Thanks @jamshark70 for keeping an eye on this.

It also makes sense that inputs expecting a trigger should initialize their unit->m_trig to 0.

I strongly agree!

So we have two choices:
Revert Impulse's pre-sample to 0. That's the easy way -- but (and I'm willing to argue until I'm blue in the face about it) this is logically incorrect.

I'm strongly against this! I'll get blue in the face about it too ;)

Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

This is the right idea.

I've documented the initialization problem and explained in detail how I think this kind of thing should be resolved:
#4127

[geoffroymontel]

I guess this is related to #4976

[jamshark70]

Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

This is the right idea.

FWIW: I just grepped 'plugins/' for the string trig.* = Z. Almost all occurrences are temporary variables defined within next functions, except:

TRand_Ctor
TExpRand_Ctor
TIRand_Ctor
CoinGate_Ctor

... these 4 include lines like unit->m_trig = ZIN0(2);. By contrast, there are many many more like unit->m_prevtrig = 0.f; -- so the number of fixes may be small.

Also checking for trig.* = I finds only one: Poll (noted above). So, maybe only 5 wrong trigger inits.

[mtmccrea]

Back at it again, and hopefully this time for the win :) I've reviewed all comments and updated the OP with clarifications and updates.

^^^ Inline reviews are addressed above ^^^

Some other notable updates:

• As requested: trimmed the number of calc functions (still at 7 but could keep cutting...)
• renamed variables and made clarifying changes to comments to hopefully make the logic more intelligible
• made unit tests for nearly all of the cases on the "bug fixes" list above (as well as updated other unit tests affected by Impulse)

@muellmusik

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g.
to excite a resonator) is fairly well established IME. Don't know if this
changes that, but if so it could break code.

No problem, this works. :)

@jamshark70

I just (accidentally) discovered some fallout from this: ...
These changes to Impulse mean that the Ctor initializes the "pre-sample" to 1 when the Impulse is starting at phase 0.

This is actually a problem with Poll incorrectly initializing it's trigger state.
After quickly patching Poll's Ctor for the sake of testing, I get closer to the correct
behavior:

UGen(Sweep): 0            // << on initialization
UGen(Sweep): 0.00133333   // << first sample
UGen(Sweep): 1.00267
UGen(Sweep): 2.00267

The first 2 polls happen on the initialization sample (which it ultimately shouldn't), and the second is the first sample. In any case I'm confident Impulse is doing the right thing here.
(Aside: this also reveals is Sweep is likely also not initialized correctly—we should see 0 on the init AND first sample).

FWIW: I just grepped 'plugins/' for the string trig.* = Z. ...

Great, this is a useful approach in addressing the init sample issue. I'll reference this in that thread.

[mtmccrea]

Just a ping on this that this PR is ready for final review ☝️

[joshpar]

This looks good to me! Can you please add documentation for our change log to pull in that describes any breakage from the past, and how to migrate old code in case of breakage? If that is no longer a concern, please let me know and I'll check this to approve.

[mtmccrea]

Thanks @joshpar !

Given that this fix could affect the initial state of a number of UGen configurations, we could say something like:

"
Impulse is now initialized correctly such that:

• it will fire on the first sample, given the default phase of 0 (or multiple of 1).
• a frequency of 0 fires once and only once on the first sample (unless the frequency subsequently changes).
• negative frequencies and phases are now supported and phase of any value is wrapped into range.

These are intended and documented behaviors, but which failed in various UGen configurations previously. Therefore, users may observe changes to the initial state of synth graphs that use Impulse. (Especially triggered UGens.)
"

If it's not too verbose, we could also consider providing one concise example of changed/fixed behavior:

( // Previously: EnvGen using Impulse triggered one control period late. Now fixed.
{
  var sig1 = EnvGen.kr(Env.perc(0.01, 0.04), 1) * SinOsc.ar(440);
  var sig2 = EnvGen.kr(Env.perc(0.01, 0.04), Impulse.kr(0)) * SinOsc.ar(440);
  [sig1, sig2]
}.plot
)

... or if that's not appropriate for a changelog, we could just link to this detailed list of resolved behaviors (subsection of this PR, which lists numerous examples): #4150 (comment)

Is that helpful?
Did you want me to put this in the changelog wiki?

[joshpar]

@mtmccrea - I think adding it to the changelog wiki is good idea. thanks!

[mtmccrea]

@mtmccrea - I think adding it to the changelog wiki is good idea. thanks!

Done! Thanks, @joshpar!