-
Notifications
You must be signed in to change notification settings - Fork 758
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
Comments
It appears that I can work around it like this:
But... come on. That's crazy. (Please, somebody tell me I'm not the only person who thinks this is lunacy.) |
I think the step toward a real solution is to make a sort of "written A lot of the problems seem to come from Impulse.kr/ar(0). best, .h.h. On 17/09/16 10:23, jamshark70 wrote:
|
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... |
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. |
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:
|
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?
I think the fallacy is to believe that there is such thing as y(-1). For example: Let's say the |
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:
// 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
} |
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 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.
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:
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. |
Hi James, sounds reasonable to me. Perhaps post that last message on sc-dev, I'm On 19/09/16 15:30, jamshark70 wrote:
|
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. |
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. |
Out of curiosity, other than breaking existing code, how much havoc would it cause to change the first sample of Impulse to 0? |
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.
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. |
Fair. We of course shouldn't change something so prevalent but I was wondering about how important trigger-at-start behavior was.
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. |
(For people coming to this in the future: there's more discussion at #2333 ) |
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 |
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. |
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. |
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). |
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.
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 Even if we do nothing else, I would like to propose this specific change because:
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.)
|
Perhaps a better alternate version, because there are multiple paths by which
|
I am experimenting with Nathan(?)'s suggestion of a pre-trigger unit. However, this diff doesn't compile.
Replacing
So I'm missing something. |
There's a typo--the added lines around L247 should use |
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. |
OK,
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.
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. |
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 Do I misunderstand it if I think that this is a layer of communication between ugens established when the |
How about 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. |
Maybe I missed it in this discussion, but has anyone suggested adding an E.g. if I have
In this case it's ambiguous whether I'd expect Another way to think about it is that each of these UGens is computing some sequence So you could think of a Another possible solution would be to have ALL UGens support a |
Another way to define a |
PreSample might be reasonable -- cleaner than the Adding arguments to existing UGens is murder for non-sclang clients. So I'd say no on that idea. |
Just trying to draw a conclusion:
What would |
that's unfortunate, I was thinking one nice thing about the @telephon - there are cases where it's ambiguous what the user wants for the |
@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 |
@ssfrr I agree that 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. |
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 @jamshark70's post here seems to lay out well the main changes - make sure Ctors actually output For So now I'm onboard with @telephon's question of when |
@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 It would be easy and reasonable to add a UGen that just sets |
I guess one question is whether such a UGen would act "correctly" and set |
[Changed the title because this issue has become a much more general discussion.] |
I understand that the problem with init values is mainly for using it as triggers. |
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 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. |
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. |
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? |
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.
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.
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 initializeunit->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.)
The text was updated successfully, but these errors were encountered: