Impulse UGEN sets the presample to the wrong value when frequency equals zero

[cianoc]

Environment

• SuperCollider 3.93
• OSX 10.13.6

Steps to reproduce (for bugs)

(
{
  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)

Graph:

---

Expected Behavior

The two audio signals should be identical.

Current Behavior

Currently the envelope triggered by Impulse.kr is a control period later.

[cianoc]

This is caused by the fact that for Impulse.kr the pre-sample values are always set to 0. An easy solution to this would be simply to change the pre-sample value to 1 when frequency is 0, and also set the phase appropriately.

However this would change the existing behavior of Impulse.kr for existing synthdefs. My personal opinion is that this is acceptable as the behavior above is clearly wrong, is confusing (particularly as Impulse.kr fires an impulse at 0 seconds), inconsistent, un-mathematical, etc. I understand backwards compatibility concerns, but I don't think preserving clearly wrong behavior is the way to go. Obviously others may disagree here.

I know how to fix this, and if there is some consensus that this should be fixed then I'll do so. But I don't want to waste time on this if people want to preserve the existing behavior.

[telephon]

Thanks for clarifying! As you predicted a change of the existing behavior, could you give an example where this would be observable? Then we could gauge the impact better.

[cianoc]

Sorry I should have been clearer. The only change of the existing behavior would be that it would fix the bug. However this bug has been around presumably for ever, so it could change how existing pieces sound if they rely upon this behavior. This feels like a small risk to me, but I know backwards compatibility is important.

So to clarify - my proposed fix wouldn't affect anything except when Impulse is passed a frequency of 0. Currently the envelope only begins on the second control block (e.g. sample 65) - which is clearly wrong. I propose changing this so that the envelope would begin on the first control block (e.g. sample 1) - which is expected behavior.

[telephon]

So to clarify - my proposed fix wouldn't affect anything except when Impulse is passed a frequency of 0.

Yes, definitely. I was really just trying to imagine how you could rely on the bug. Any idea?

Btw. I am the one who implemented the Impulse.ar(0)-feature (and this bug slipped in because the whole initialization was still incoherently treated all over the place).

[cianoc]

Yes, definitely. I was really just trying to imagine how you could rely on the bug. Any idea?

Not really. I'm of the opinion that it should be fixed. I was just acknowledging the fact that in certain cases it might affect the sound of existing pieces slightly. But it's hard for to believe that anyone composed a piece knowing about this bug and deliberately exploiting it.

[cianoc]

Okay, this seems to relate to: Issue #2343. That discussion kind of fizzled out without any resolution. I agree with James the current situation with Impulse is ludicrous, and it makes certain things impossible. But there doesn't seem to be any resolution on what the best way to proceed here is.

Myself I liked James' solution of just fixing Impulse.

[telephon]

It is a complicated discussion and I am not sure I can make up my mind easily. Closest to a conclusion seemed:

The simplest solution is that the UGen expecting triggers inits prevTrig to 0 in the Ctor.

If I understand correctly, the problem should be that EnvGen doesn't initialize its previous gate value to zero? If you could check this, that would be great. If it turns out not to be the case, then we have to rethink the trigger semantics.

[cianoc]

I checked Envelope and envelope is fine. Also Envelope seems to do the right thing with every impulse I could think of except Impulse.

I actually think the problem is that Impulse is a sample behind. Looking at the code, and the description of writing UGens in the SuperCollider book (which I realize isn't canon - but still), I'm reasonably certain that the last line in Impulse_Ctor should be:

	Impulse_next_k(unit, 1);

rather than

	ZOUT0(0) = 0.f;

This fixes the problem, and so far I haven't found any other problems, while the timing of Impulse now seems to be correct.

[telephon]

Agreed. Let's fix the impulse implementation first.

[mtmccrea]

I wanted to raise a cautionary point about using UGen_next_x(unit, 1) to initialize the state of a UGen, and please elaborate if I'm missing something...

Many next_x calc functions are setting member variables of the unit: unit->mPhase = phase; in the case of Impulse_next_k. During the next sample's calculation, this state is expected to have already "happened", i.e. it's now a past state. However, the output of UGen_next_x(unit, 1) isn't actually ever used—it's recalculated at the first sample of the the first full control loop. So the unit's state was moved forward in time t = 1 (unit->mPhase in this case), but once recalculated for true output, the resulting sample represents t = 0;

Please correct me if I'm mistaken, as this has had my brain in knots after reading through some very looong threads (#2333, #2343, #2864).

NOTE this also has implications about how SCUnit's (re-)definition of set_calc_function automatically calls (mCalcFunc)(this, 1);, which may not be best practice, giving all the special care needed for this initial sample calculation!

[mtmccrea]

Also, I think ToggleFF may also require whatever solution is agreed on for Impulse.

[mtmccrea]

I wanted to mention here that I've been working on a rewrite of Impulse and I am progressing, though there are many use cases to account for, especially considering that the phase offset is now modulatable.

It's proven to be a bit tricky, and at this point I'll need to get some input from some other folks on the dev list, but I'll update here if I resolve it.

[mtmccrea]

I'll update here if I resolve it.

Update: first step in resolving this issue has been taken - #4150