Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UGens init to unexpected values #2343

Open
jamshark70 opened this issue Sep 17, 2016 · 42 comments
Open

UGens init to unexpected values #2343

jamshark70 opened this issue Sep 17, 2016 · 42 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: server plugins

Comments

@jamshark70
Copy link
Contributor

jamshark70 commented Sep 17, 2016

Another UGen init case. I would expect the first plot to hold 880 for 0.1 seconds, and then lag down to 220, and I'd expect the second to do the opposite.

That is, expected initial state: Impulse == 1.0 --> Trig1 == 1.0 --> Select initially outputs the second array item.

In fact, what happens is that both Lags seem to be initialized with the first array items. The first plot ramps up from 220 to 880, and back down.

{
    var trig = Trig1.kr(Impulse.kr(0), 0.1),
    f = Select.kr(trig, [220, 880]);
    Lag.kr(f, 0.2)
}.plot(duration: 0.2);

{
    var trig = Trig1.kr(Impulse.kr(0), 0.1),
    f = Select.kr(trig, [880, 220]);
    Lag.kr(f, 0.2)
}.plot(duration: 0.2);

Moreover, if you reverse the array items and invert the trigger, it still starts with the wrong item -- this plot is identical to the first above.

{
    var trig = Trig1.kr(Impulse.kr(0), 0.1),
    f = Select.kr(trig <= 0, [880, 220]);
    Lag.kr(f, 0.2)
}.plot(duration: 0.2);

The most likely explanation is: I expect, initially, Impulse == 1.0 --> Trig1 == 1.0, but Impulse_Ctor is actually setting ZOUT0(0) = 0.f; -- so Lag then initializes as if the gate were off, even though the gate is open during the first calculation cycle. Impulse must init to 0, or triggers won't work -- so Lag is making a mistake to initialize unit->m_y1 = ZIN0(0); in its Ctor.

Oh well, another one for the "everything I try in SuperCollider is cursed" file. I only wanted to pass in the previous frequency and get Lag behavior at the beginning of a Synth. Well, SC says I can't use Lag for this. I have to find something else (that will sound different). (Remember, this is me backing away from exotic techniques and sticking to what I know... and I'm still screwed.)

@jamshark70 jamshark70 added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: server plugins labels Sep 17, 2016
@jamshark70
Copy link
Contributor Author

It appears that I can work around it like this:

{
    var sig = Duty.kr(Dseq([ControlDur.ir, Dseq([1], inf)]), 0, Dseq([1, Dseq([0], inf)])),
    f = Select.kr(sig, [880, 220]);
    Lag.kr(f, 0.2)
}.plot(duration: 0.2);

But... come on. That's crazy. (Please, somebody tell me I'm not the only person who thinks this is lunacy.)

@Sciss
Copy link
Contributor

Sciss commented Sep 17, 2016

I think the step toward a real solution is to make a sort of "written
spec" for each UGen that states what is the supposed initial states, and
so one can at least deduce the interaction with others. What makes the
situation complicated is that the compound behaviour depends on the
interaction between the initial states of the UGens, and the most
involved thing seems to be defining what is a trigger at time zero.

A lot of the problems seem to come from Impulse.kr/ar(0).

best, .h.h.

On 17/09/16 10:23, jamshark70 wrote:

It appears that I can work around it like this:

|{ var sig = Duty.kr(Dseq([ControlDur.ir, Dseq([1], inf)]), 0, Dseq([1,
Dseq([0], inf)])), f = Select.kr(sig, [880, 220]); Lag.kr(f, 0.2)
}.plot(duration: 0.2); |

But... come on. That's crazy. (Please, somebody tell me I'm not the only
person who thinks this is lunacy.)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2343 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIA5I73tp85k2eJtUfhF4Rz8wihU6LDks5qq6NngaJpZM4J_gTP.

@nhthn
Copy link
Contributor

nhthn commented Sep 17, 2016

This behavior makes sense if you work out what the Ctor is doing. The Trig1 produces a signal which is zero from negative to positive infinity except for a rectangular window of 1's, and Lag1 is filtering that signal into a smoother window. So to solve this issue you need to find a way to change that Ctor sample to 1, so you can use... uh...

@jamshark70
Copy link
Contributor Author

I think the step toward a real solution is to make a sort of "written spec" for each UGen that states what is the supposed initial states, and so one can at least deduce the interaction with others. What makes the situation complicated is that the compound behaviour depends on the interaction between the initial states of the UGens, and the most involved thing seems to be defining what is a trigger at time zero.

Right, and the lack of a formal spec meant that we didn't notice a contradiction: UGens that respond to triggers assume that the Ctor will "prime the pump" with 0 (so that the subsequent nonzero will be recognized as a trigger), while UGens like Lag (perhaps filters in general?) assume the pump will be primed with the same value that the input UGen will produce in its first calculation cycle.

In a way, this suggests a solution: UGens shouldn't make any assumption at all about the upstream input's initialization. If the UGen expects a trigger-style input, it should prime its own (input) pump with 0. If it's the latter type, it should wait until the first calculation cycle to initialize. We have tended to resist delayed-initialization because you would need either an "if" in the calculation function (performance hit), or switch the calculation function on-the-fly (and have some duplicated code -- sorry... I know that's confusing, but I have to go right now and I don't have time to explain that further). But, the optimization means relying on an assumption that simply cannot be made safe. So, the optimization must be sacrificed.

@jamshark70
Copy link
Contributor Author

Or, simpler, mandate that all UGens must initialize to the first true output sample. Then, trigger-receiving units may ignore the value from the input's Ctor and initialize to 0 instead.

There was discussion about whether the Ctor should initialize to y(0) or y(-1). I'm kind of thinking:

  • If it's y(0), it's fairly easy for units to ignore the incoming value if they need to (e.g. to respond to a trigger when calculating the first real output sample).
  • If it's y(-1), then some units will have to wait until the first calculation block to get the initial value. That's more complicated and easier to break.

@Sciss
Copy link
Contributor

Sciss commented Sep 19, 2016

Or, simpler, mandate that all UGens must initialize to the first true output sample. Then, trigger-receiving units may ignore the value from the input's Ctor and initialize to 0 instead.

It sounds reasonable to me. I'm only worried that nobody else perhaps with deeper knowledge about the original decision - perhaps @audiosynth - has joined in. It is well possible that we overlook some implication that was addressed with the current behaviour?

here was discussion about whether the Ctor should initialize to y(0) or y(-1)

I think the fallacy is to believe that there is such thing as y(-1). For example: Let's say the SinOsc is corrected and so the first output sample is zero. Then what is y(-1), is it sin(-1 * 2 * Pi * f / fs + phase)? But the frequency can be modulated. So what is the value of the frequency parameter at y(-1)? I would conclude that one should go for a conservative view and simply say, signals that have time varying parameters didn't exists and must therefore be zero. How about Line? It makes no sense to "extrapolate" to y(-1). So one can only reasonably conclude that signals must be zero/undefined for negative times.

@Sciss
Copy link
Contributor

Sciss commented Sep 19, 2016

Thinking about it, perhaps I got it wrong. Reading again over all, is the purpose of this first special sample to repeat the first output sample? That is, it doesn't calculate y(-1) but y(0) "ahead of time"?

Then I see only two solutions:

  • change the architecture to completely undo this first sample calculation; it would imply that trigger UGens must now assume internal state of zero.
  • leave everything as is, but elaborate all this in the documentation. And add a helper UGen that "clears" the history. Like a "multiplication by a step function" where the multiplier goes from zero to one. So you could do
// doesn't work
play {
    var f = \freq.ar(120);
    f.poll(f > 100, "test");
   nil
};

// works (hypothetically)
play {
    var f = \freq.ar(120);
    f.poll(Step.ar(f > 100), "test");
   nil
}

@jamshark70
Copy link
Contributor Author

Thinking about it, perhaps I got it wrong. Reading again over all, is the purpose of this first special sample to repeat the first output sample? That is, it doesn't calculate y(-1) but y(0) "ahead of time"?

As far as I understood it, that's what it's supposed to do, but even this simple idea got corrupted somewhere. What might have happened is, somebody (or some people) got confused by the trigger-detection expression curtrig > 0.f && prevtrig <= 0.f and thought, well, if prevtrig is supposed to be 0, and we initialize the trigger input to ZIN(something), then we had better make sure trigger-generator units initialize to 0.f. This, in fact, is exactly what Impulse does. But already, that's broken the contract: Impulse does initialize to y(-1)! But other UGens init to y(0).

This means there is no consistency about the meaning of the ZOUT set in the Ctor. Downstream UGens, reading the initial value, can make no meaningful assumption about this incoming value. Well, no wonder we have a boatload of problems about this.

So, AFAICS, the first step of any solution is to make a simple, inviolable rule: the Ctor sets y(0). Period. End of discussion.

Then I see only two solutions:

  • change the architecture to completely undo this first sample calculation; it would imply that trigger UGens must now assume internal state of zero.

To be more precise, "UGens which receive triggers must now assume internal state of zero" initially. UGens such as filters need to start off with y(0). For that, there are two solutions:

  • Remove the "pump-priming" from the Ctor. Trigger-receiving units can initialize to 0. Others will have to initialize to some flag that says "I'm not initialized yet," so that the calculation function can detect this situation. (It might be possible to optimize that out in some cases, but I bet there will be some where we need the more complicated logic.)
  • Or, declare that pump-priming must be y(0), and any UGens (like Impulse, currently) that don't do this are violating the spec. Then, trigger-receiving units can ignore ZIN(whatever) and init to 0, while others can use ZIN in the Ctor.

The second of these would simplify the logic of calculation functions, I think.

The main point is, the UGen initializing now does not know anything about how the initial value will be used downstream. So it should not do anything special with triggers or non-triggers. The only units that know whether 0 init or y(0) init is appropriate are the downstream units. So, put the logic there and take it out of units like Impulse.

@Sciss
Copy link
Contributor

Sciss commented Sep 19, 2016

Hi James,

sounds reasonable to me. Perhaps post that last message on sc-dev, I'm
not sure people are following.

On 19/09/16 15:30, jamshark70 wrote:
[...]

The main point is, the UGen initializing now does not know anything
about how the initial value will be used downstream. So it should not do
anything special with triggers or non-triggers. The only units that know
whether 0 init or y(0) init is appropriate are the downstream units. So,
put the logic there and take it out of units like Impulse.

@vivid-synth
Copy link
Member

To be honest, it seems like part of the problem is that we're expecting Impulse to do something deeply magical: we allow it to have a y(0) value of 1 that triggers the action of "when a nonpositive to positive transition occurs."

I'm not proposing we change Impulse to output one sample of 0 first, but that's how I always imagined it worked.

@telephon
Copy link
Member

telephon commented Oct 5, 2016

Yes, it should be the receiver's responsibility to decide what a trigger is. I agree with @jamshark70 (2nd case) that the limit case of being in the first sample ever needs to be implemented by the trigger UGens.

@vivid-synth
Copy link
Member

vivid-synth commented Oct 5, 2016

Out of curiosity, other than breaking existing code, how much havoc would it cause to change the first sample of Impulse to 0?

@Sciss
Copy link
Contributor

Sciss commented Oct 5, 2016

That wouldn't make any sense. If you want the first sample to be zero, you can fiddle around with the phase parameter.

How much havoc - well, it will break about everything that relies on triggering something at init time.

deeply magical: we allow it to have a y(0) value of 1 that triggers the action of "when a nonpositive to positive transition occurs."

There is nothing magical. The value at y(0) is positive, and the proposal is that trigger inputs of UGens assume initial state of zero, thus detecting a trigger here is not magical but logical.

@vivid-synth
Copy link
Member

How much havoc - well, it will break about everything that relies on triggering something at init time.

Fair. We of course shouldn't change something so prevalent but I was wondering about how important trigger-at-start behavior was.

deeply magical: we allow it to have a y(0) value of 1 that triggers
the action of "when a nonpositive to positive transition occurs."

There is nothing magical. The value at y(0) is positive, and the proposal is that trigger inputs of UGens assume initial state of zero, thus detecting a trigger here is not magical but logical.

But the fact that there is a value before y(0) is itself not part of the simple mental model we're taught about signals flowing through the UGen graph. The proposal makes sense but we've been, I'm arguing, depending on "magical" behavior without noticing that we are.

@vivid-synth
Copy link
Member

vivid-synth commented Jan 2, 2017

(For people coming to this in the future: there's more discussion at #2333 )

@nhthn
Copy link
Contributor

nhthn commented Apr 23, 2017

Fixing this issue will take a lot of work and a lot of API changes. Furthermore, there are still plenty of absent and inconsistent initialization issues in sc3-plugins and other third-party plugins.

We can introduce a workaround UGen that allows manipulation of the initialization sample only. Just a UGen like "PreTrig" that outputs an initial sample of 1 and then outputs silence would work. For example, you could set the initial sample to an arbitrary value with (1 - PreTrig.ar) * sig + (PreTrig.ar * initValue).

@jamshark70
Copy link
Contributor Author

If this is tested and proven to work, I'd be OK with that. The use case I presented initially is a bit of a corner case.

@jamshark70
Copy link
Contributor Author

Amending my comment from last week... it's a corner case that I keep running into, somehow (new variant reported on the mailing list).

I understand the difficulty of a proper fix. Nonetheless, the behavior is objectively incorrect, and the incorrectness stems from a functional spec that is too loosely defined, and it makes an unknown number of scenarios involving trigger ugens unreliable.

@jamshark70
Copy link
Contributor Author

It's also possible that the incorrect filter initialization from before was hiding the wrong trigger behavior, and that fixing filter init might have made other matters worse (because it was only half the job, thereby making filters' concept of initialization incompatible with triggers' concept of it).

@jamshark70
Copy link
Contributor Author

jamshark70 commented May 3, 2017

FWIW: I just tried a tweak to Impulse_Ctor, which sets the Ctor initial sample to 1 if iphase == 0, and... both the Lag case, and the Impulse --> Latch --> BPF case that I posted on the mailing list, work correctly.

{
	var initTrig = Impulse.ar(0),
	sig = Select.ar(initTrig, DC.ar([0, 1]));
	Lag.ar(sig, 0.005)
}.plot;

Without the proposed fix below, the graph peaks around 0.03. With the fix, it begins at 1 -- as it should, because the initTrig-ged first sample going into Lag is 1. (EDIT: Checked with Impulse.ar(100, 0.8) too, and the Impulse Ctor starts with 0.0, also as it should.)

Even if we do nothing else, I would like to propose this specific change because:

  • Impulse's default iphase is 0 -- meaning that the current behavior is wrong in the normal case and this will specifically fix the normal case;

  • Having an impulse at the start of a synth is a very common case;

  • grep tells me that we already do unit->m_prevtrig = 0.f; in trigger UGen Ctors, so we should not have problems with first-sample triggers being missed. EDIT: I grepped unit->m_prevtrig = and found only one = ZIN(...), and that appears to be a mistake -- go ahead, take a look at the preceding line here -- it's hilarious 🤣 https://github.com/supercollider/supercollider/blob/master/server/plugins/Convolution.cpp#L339

So, we probably don't need a massive API review -- but it would be worth fixing a typical, default case that we currently handle badly. (Side benefit: We could use Impulse to drive PV_ units, which is currently impossible.)

void Impulse_Ctor(Impulse* unit)
{

	unit->mPhase = ZIN0(1);

	bool initialPulse = unit->mPhase == 0.f;

	if (INRATE(0) == calc_FullRate) {
		if(INRATE(1) != calc_ScalarRate) {
			SETCALC(Impulse_next_ak);
			unit->mPhase = 1.f;
		} else {
			SETCALC(Impulse_next_a);
		}
	} else {
		if(INRATE(1) != calc_ScalarRate) {
			SETCALC(Impulse_next_kk);
			unit->mPhase = 1.f;
		} else {
			SETCALC(Impulse_next_k);
		}
	}

	unit->mPhaseOffset = 0.f;
	unit->mFreqMul = unit->mRate->mSampleDur;
	if (initialPulse) {
		unit->mPhase = 1.f;
		ZOUT0(0) = 1.f;
	} else
		ZOUT0(0) = 0.f;
}

@jamshark70
Copy link
Contributor Author

Perhaps a better alternate version, because there are multiple paths by which unit->mPhase may be set to be guaranteed to wrap around in the first block. I don't get the INRATE logic here, but this should cover all possible cases where the first "real" output sample will be an impulse.

void Impulse_Ctor(Impulse* unit)
{

	unit->mPhase = ZIN0(1);

	if (INRATE(0) == calc_FullRate) {
		if(INRATE(1) != calc_ScalarRate) {
			SETCALC(Impulse_next_ak);
			unit->mPhase = 1.f;
		} else {
			SETCALC(Impulse_next_a);
		}
	} else {
		if(INRATE(1) != calc_ScalarRate) {
			SETCALC(Impulse_next_kk);
			unit->mPhase = 1.f;
		} else {
			SETCALC(Impulse_next_k);
		}
	}

	unit->mPhaseOffset = 0.f;
	unit->mFreqMul = unit->mRate->mSampleDur;

	if(unit->mPhase == 0.f)
		unit->mPhase = 1.f;

	if (unit->mPhase == 1.f)
		ZOUT0(0) = 1.f;
	else
		ZOUT0(0) = 0.f;
}

@jamshark70
Copy link
Contributor Author

I am experimenting with Nathan(?)'s suggestion of a pre-trigger unit. However, this diff doesn't compile.

diff --git a/server/plugins/LFUGens.cpp b/server/plugins/LFUGens.cpp
index cf63253..3f0dcf7 100644
--- a/server/plugins/LFUGens.cpp
+++ b/server/plugins/LFUGens.cpp
@@ -79,6 +79,11 @@ struct Impulse : public Unit
 	float mFreqMul;
 };
 
+struct PreTrig : public Unit
+{
+
+};
+
 struct VarSaw : public Unit
 {
 	double mPhase;
@@ -240,6 +245,9 @@ extern "C"
 	void Impulse_next_k(Impulse *unit, int inNumSamples);
 	void Impulse_Ctor(Impulse* unit);
 
+	void PreTrig_next(Impulse *unit, int inNumSamples);
+	void PreTrig_Ctor(Impulse* unit);
+
 	void SyncSaw_next_aa(SyncSaw *unit, int inNumSamples);
 	void SyncSaw_next_ak(SyncSaw *unit, int inNumSamples);
 	void SyncSaw_next_ka(SyncSaw *unit, int inNumSamples);
@@ -1039,6 +1047,21 @@ void Impulse_Ctor(Impulse* unit)
 
 //////////////////////////////////////////////////////////////////////////////////////////////////
 
+void PreTrig_Ctor(PreTrig* unit)
+{
+  SETCALC(PreTrig_next);
+  ZOUT0(0) = 1.f;
+}
+
+void PreTrig_next(PreTrig* unit, int inNumSamples)
+{
+	float *out = ZOUT(0);
+  LOOP1(inNumSamples,
+	ZXP(out) = 0.f;
+	);
+}
+//////////////////////////////////////////////////////////////////////////////////////////////////
+
 void VarSaw_next_a(VarSaw *unit, int inNumSamples)
 {
 	float *out = ZOUT(0);
@@ -3693,6 +3716,7 @@ PluginLoad(LF)
 	DefineSimpleUnit(LFTri);
 	DefineSimpleUnit(LFGauss);
 	DefineSimpleUnit(Impulse);
+	DefineSimpleUnit(PreTrig);
 	DefineSimpleUnit(VarSaw);
 	DefineSimpleUnit(SyncSaw);
 	registerUnit<K2A>( ft, "K2A" );

Replacing DefineSimpleUnit(PreTrig); with registerUnit<PreTrig>( ft, "PreTrig" ); makes it compile, but then booting the server does:

*** ERROR: dlopen '/usr/local/lib/SuperCollider/plugins/LFUGens.so' err '/usr/local/lib/SuperCollider/plugins/LFUGens.so: undefined symbol: PreTrig_next'

So I'm missing something.

@mossheim
Copy link
Contributor

There's a typo--the added lines around L247 should use PreTrig, not Impulse, in their function declarations.

@jamshark70
Copy link
Contributor Author

Ah, right you are.

I'm still not sure which is better: to bundle the pre-sample up with Impulse, or to separate it. I lean toward separating them -- if they're separate, it's easy to make a pseudo-UGen that adds the pretrigger to an Impulse, but if they're unified, you can't easily split them. But independent influence over the pre-sample is another way to shoot oneself in the foot.

I kind of want to live with the PreTrig unit for awhile and see if anything awful shakes out.

@jamshark70
Copy link
Contributor Author

OK, PreTrig compiles and runs now. If I have time later, I might try to steal some of the SIMD code from another UGen to speed it up at audio rate, but as a proof of concept, it definitely does the job. And it has the advantage that the user can decide which behavior is appropriate for a given case.

{
	Lag.ar(
		Impulse.ar(200, add: [PreTrig.ar, 0]),
		0.003
	)
}.plot;

One possible change might be to pass in a value, so that a user could choose to suppress the pre-trigger for nonzero phase, e.g.

{ |freq = 10, phase = 0.5| Impulse.ar(freq, phase, add: PreTrig.ar(BinaryOpUGen('==', phase, 0)) }
// or
{ |freq = 10, phase = 0.5| Impulse.ar(freq, phase, add: PreTrig.ar((phase % 1.0) <= 0)) }

So then it would be necessary to document the issue. I would probably write a "Guide"-style document about triggers in general -- it's not necessarily an obvious topic, but remarkably, searching the help for "trig" turns up nothing except for UGen class documentation.

@telephon
Copy link
Member

If the idea is to set the a specific ugen input to a value that is available before the first sample is calculated, why call it PreTrig? There is nothing triggering it, is it?

Do I misunderstand it if I think that this is a layer of communication between ugens established when theCtors are called?

@jamshark70
Copy link
Contributor Author

How about PreSample?

I'm open to suggestions -- as I said elsewhere, I'm really terrible at naming classes and methods. I'd appreciate suggestions over questions.

Yes, this is when Ctors are called. Each Ctor is supposed to set ZOUT0. Later units can use ZIN0, which may have been populated by a previous unit.

@ssfrr
Copy link

ssfrr commented Mar 26, 2018

Maybe I missed it in this discussion, but has anyone suggested adding an init argument to these UGens that depend on the previous sample value?

E.g. if I have

{
   var trig = Trig1.kr(Impulse.kr(0), 0.1);
   Lag.kr(trig, 0.05)
}.plot(duration: 0.2);

In this case it's ambiguous whether I'd expect Lag to start at 0 and start immediately ramping up to 1, or be initialized at 1 and then ramp down at t=0.1. My mental model is that Lag has some internal state, it seems least magical to just have an argument that allows you to set that state directly when you're creating the Lag, rather than relying on some weird out-of-band UGen interaction.

Another way to think about it is that each of these UGens is computing some sequence y[n] given input x[n], x[n-1], x[n-2], ..., and the very first thing they output is y[0]. If y[n] is only a function of x[n] then we're OK, because that value is available from the input UGen. The issue is that some of them depend on x[n-1] (i.e. y[n] = f(x[n], x[n-1]), which for the first sample requires x[-1], which is not defined by their input UGen.

So you could think of a PreSample UGen as being a UGen that defines an output y[n] = 1 (or some specified value) for all n < 0, but then what does it output for n >= 0?

Another possible solution would be to have ALL UGens support a preout argument that would specify their output y[n] for all n <= 0, but that seems like more work.

@ssfrr
Copy link

ssfrr commented Mar 26, 2018

Another way to define a PreSample UGen would be to take an input signal that determines its output for n >= 0, so you'd use it like: Lag.kr(PreSample.kr(1, trig), 0.05). And the PreSample would be a UGen outputting y[n]=1 for n < 0 and y[n] = trig[n] for n >= 0. That seems like a more complicated API than an init argument, but I don't have much handle on the SC internals so maybe there's a compelling argument against init.

@jamshark70
Copy link
Contributor Author

PreSample might be reasonable -- cleaner than the + approach suggested above.

Adding arguments to existing UGens is murder for non-sclang clients. So I'd say no on that idea.

@telephon
Copy link
Member

Just trying to draw a conclusion:

  1. every UGen supplies an initial output value at ctor time.
  2. if a UGen needs a value before the initial output value of one of its arguments, it relies on its own reasoning. Typical cases are: 0 for trig inputs, or the same as the initial value for continuous signals.

What would PreSample be used for?

@ssfrr
Copy link

ssfrr commented Mar 26, 2018

Adding arguments to existing UGens is murder for non-sclang clients. So I'd say no on that idea.

that's unfortunate, I was thinking one nice thing about the init argument would be that it could be totally backwards-compatible (if nil it could fall back to whatever the current behavior is).

@telephon - there are cases where it's ambiguous what the user wants for the x[-1] input sample, e.g. see my Lag example above

@Sciss
Copy link
Contributor

Sciss commented Mar 26, 2018

@ssfrr No, this will only work as client/server are paired with a particular new version. There is another thread about solving the problem of adding new arguments with default values on the server side, but it's not resolved: http://new-supercollider-mailing-lists-forums-use-these.2681727.n2.nabble.com/safe-addition-of-arguments-to-existing-ugens-td7638483.html

@telephon
Copy link
Member

@ssfrr I agree that Lag (and all UGens that depend on x[-1]) can be thought of in at least two different ways, depending where you think the previous value comes from.

In general, we take the interpretation of a signal as being defined by the receiver. This is why I propose that the receiver should just decide what it is.

If we really think that we want to be able to specify a separate previous value that such a receiver could pick up, we have to add something general to the architecture.

But perhaps there are not so many cases after all? Then we could just add separate UGens.

@ssfrr
Copy link

ssfrr commented Mar 27, 2018

First, thanks all for taking the time to discuss this with me, hopefully my ignorance on SC implementation details isn't too frustrating, and it's been super helpful for me in understanding how these pieces fit together.

I think that @Sciss makes a pretty compelling argument above that x[-1] should just be considered to be undefined. It seems like this means that each UGen should decide for itself what its inputs looked like before n=0, or equivalently, each UGen should initialize its internal state without regard for its inputs.

@jamshark70's post here seems to lay out well the main changes - make sure Ctors actually output y[0] and not y[-1], and make sure that trigger-receiving UGens act as if x[-1] = 0.

For Lag I can see how it makes sense for it act as if x[-1] == x[0], and maybe it's not super critical that it be configurable, especially given that adding additional constructor args isn't currently feasible.

So now I'm onboard with @telephon's question of when PreSample would be used. It seems like it breaks the contract that the y[0] that's output by the Ctor is the actual y[0] - maybe it's useful for a workaround for other UGens that break that contract, too?

@telephon
Copy link
Member

@ssfrr you are welcome - it is good to discuss this.

Thank you for the condensation: we all agree as it seems that we shouldn't be relying on any UGen providing x[-1].

It would be easy and reasonable to add a UGen that just sets x[0] (not x[-1]), and otherwise lets through its input. But again, I'm curious to see an example.

@ssfrr
Copy link

ssfrr commented Mar 27, 2018

I guess one question is whether such a UGen would act "correctly" and set y[0] in the Ctor the same as the actually-output y[0], or whether it would be a sort of workaround for other poorly-behaving UGens. In the former case it seems like FirstSample or something might be a better name (it's outputting y[0] not y[-1]). In the latter case it seems like there are different combinations of what it outputs in the Ctor vs. what it actually outputs, depending on exactly what problem it's looking to solve.

@mossheim mossheim changed the title Lag can magically init to the wrong value UGens init to unexpected values Apr 7, 2018
@mossheim
Copy link
Contributor

mossheim commented Apr 7, 2018

[Changed the title because this issue has become a much more general discussion.]

@sonoro1234
Copy link
Contributor

sonoro1234 commented Apr 7, 2018

#2343 (comment)

I understand that the problem with init values is mainly for using it as triggers.
The simplest solution is that the UGen expecting triggers inits prevTrig to 0 in the Ctor.
As for the initial output value UGen_next(unit, 1); does the job as said in #2333
Only UGens expecting triggers should be reworked.
am I missing something?

@jamshark70
Copy link
Contributor Author

FWIW: I just lost about an hour this afternoon to Impulse.ar(0) -- a Phasor was starting with incorrect values because of #1886. The root of 1886 is actually this bug -- the fact that Impulse's Ctor outputs 0 even if the first true output value will be 1.

This has been logged for about five years.

I would estimate at least 20% of the time, if I try to use Impulse as an initial trigger, I get some kind of weird behavior.

I realize that there may never be a comprehensive review of UGen Ctors, but... can we actually, finally fix Impulse, please? Can we agree on what the right solution should be?

Maybe it is that PreSample unit proposed earlier, which could then be added to an Impulse in a pseudo-UGen.

I apologize if I'm sounding a bit irritated. But I've been bitten by this one bug in this one UGen many, many times over the years... I'm fed up enough that... I had sworn that I wouldn't do any more C++ PRs (or even any PRs), but for this, if we can agree on what is the right thing to do, I might have to make an exception. Because we really can't let this bug sit around anymore. I don't want to suffer another afternoon of "what the hell is my correctly-written SynthDef doing" when the issue and its cause are known.

@dyfer
Copy link
Member

dyfer commented Jul 3, 2021

I think there's a WIP for Impulse: #4150 @mtmccrea has started comprehensive work on initial value fixes, but it seems to have slowed down somewhat.

@jamshark70
Copy link
Contributor Author

I see... kind of a big job, because fixing Impulse also requires fixing other units that initialize trigger inputs incorrectly.

😢

I guess after my show, I'll review those two issues and see what else needs to be done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: server plugins
Projects
None yet
Development

No branches or pull requests

9 participants