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

EnvGen releases incorrectly for very very short envelope segments #1023

Closed
jamshark70 opened this issue Jan 22, 2014 · 8 comments
Closed

EnvGen releases incorrectly for very very short envelope segments #1023

jamshark70 opened this issue Jan 22, 2014 · 8 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: server plugins

Comments

@jamshark70
Copy link
Contributor

I seem to have found a disturbing bug in EnvGen. It's worse when the envelope segments are very short, but it also occurs (mildly) for typical env-segment durations. The release segment of an Env.adsr actually goes UP before going down!!! This seems to happen always, which is really awful.

In a simple Env.adsr, we would expect the signal to rise during the attack phase, but thereafter, the slope should be <= 0 (always descending or flat). Instead, I get a very obvious spike when the gate closes and the envelope goes into the release phase.

b.free;
b = Buffer.alloc(s, 4410, 1);

a = {
    var trig = Impulse.ar(30);
    trig = Trig1.ar(trig, TRand.ar(1/120, 1/40, trig));
    RecordBuf.ar(EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.2), trig, timeScale: 1/30), b, loop: 0, doneAction: 2);
    Silent.ar(1)
}.play;

b.getToFloatArray(wait: -1, action: { |data| d = data });

x = List.new; d.doAdjacentPairs { |a, b, i| if(a < b) { x.add(i) } }; x

--> List[ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1023, 1470, 1471, 1472, 1473, 1474, 1475, 1476, 1477, 1478, 1479, 1480, 1481, 1482, 1483, 2495, 2940, 2941, 2942, 2943, 2944, 2945, 2946, 2947, 2948, 2949, 2950, 2951, 2952, 2953, 3967 ]

If, in fact, slope were > 0 only during the attack segments, then we would only see 0..13, 1470..1483 etc. Instead, there's also 1470***, 2495 and 3967.

  • *** Because of TRand, these will be different when you run it.

Look at the floats surrounding one of those indices -- d[1015 .. 1030]:

FloatArray[ 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.5, 0.88936585187912, 0.87712335586548, 0.86504632234573, 0.85313242673874, 0.84137958288193, 0.8297855257988, 0.81834816932678 ]

Just before that point, it's holding flat at the sustain level. Then, instead of decreasing, it jumps up to almost twice its current level.

An alternate version, without using timeScale, also reproduces the issue.

a = {
    var trig = Impulse.ar(30);
    trig = Trig1.ar(trig, TRand.ar(1/120, 1/40, trig));
    RecordBuf.ar(EnvGen.ar(Env.adsr(0.01/30, 0.1/30, 0.5, 0.2/30), trig, timeScale: 1/30), b, loop: 0, doneAction: 2);
    Silent.ar(1)
}.play;

A longer envelope still reproduces the issue.

{ var trig = Impulse.ar(1);
    trig = Trig1.ar(trig, TRand.ar(3/12, 3/4, trig));
    [trig, EnvGen.ar(Env.adsr(0.01, 0.1, 0.5, 0.2), trig)]
}.plot(2);
@jamshark70
Copy link
Contributor Author

A clean rebuild did not resolve the issue.

@jamshark70
Copy link
Contributor Author

Converting the last example to all kr units does not reproduce the problem. So it's only EnvGen.ar, it seems.

@danstowell
Copy link
Member

James,

I'm unable to reproduce this. The list I get when running your first
example is:

 List[ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1470, 1471, 1472,

1473, 1474, 1475, 1476, 1477, 1478, 1479, 1480, 1481, 1482, 1483, 2940,
2941, 2942, 2943, 2944, 2945, 2946, 2947, 2948, 2949, 2950, 2951, 2952,
2953 ]

which corresponds to the three attack segments, all as should be.

(BTW please note that your message has a typo in it which confused me at
first: "1470" is not a glitch but "1023" from your list is.)

I'm on SC 3.6.6 (401b992) on Ubuntu 13.04. Please, please when reporting
things, say what version of SuperCollider you're running, at the very least.

Dan

2014/1/22 jamshark70 notifications@github.com

Converting the last example to all kr units does not reproduce the
problem. So it's only EnvGen.ar, it seems.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1023#issuecomment-32991674
.

http://www.mcld.co.uk

@jamshark70
Copy link
Contributor Author

Currently I'm at or around:

commit 1c422a3
Author: Julian Rohrhuber jrhb@users.sourceforge.net
Date: Sun Jan 5 00:24:10 2014 +0100

help: add loop node example to EnvGen

For this problem, I would guess one of these is the issue, but EnvGen got pretty heavily reorganized and it's hard to see quickly which one of the many changes is responsible.

I can try checking out these individual commits one by one (only five) and, if they build successfully, find out which is the first one where it broke.

$ git log server/plugins/LFUGens.cpp
commit 800c443
Author: Tim Blechmann tim@klingt.org
Date: Sun Oct 20 14:53:38 2013 +0200

EnvGen: fix .kr method

Signed-off-by: Tim Blechmann <tim@klingt.org>

commit 42967f6
Author: Tim Blechmann tim@klingt.org
Date: Sun Oct 20 14:17:19 2013 +0200

Env: rename step2 to hold

'hold' shares the analogy of sample&hold and is therefore more expressive
than step2

Signed-off-by: Tim Blechmann <tim@klingt.org>

commit c51d383
Author: Tim Blechmann tim@klingt.org
Date: Sat Oct 12 16:21:22 2013 +0200

Env: provide new step2 shape, which steps to a value at the end of a shape

the semantics of Env.xyc for \step is very awkward when using, because \step
jumps to the end value of a segment. this means that Env.xzc([[0, 0, \step],
will have the value 1 between time 0 and 1.
using \step2 instead of \step, the value between 0 and 1 will be 0, and it w
at time 1

Signed-off-by: Tim Blechmann <tim@klingt.org>

commit af7c725
Author: Tim Blechmann tim@klingt.org
Date: Fri Oct 11 19:51:26 2013 +0200

plugins: EnvGen - unify envgen perform implemementation

commit dc0646d
Author: Tim Blechmann tim@klingt.org
Date: Fri Oct 11 19:27:05 2013 +0200

plugins: EnvGen - remove duplicated code

@jamshark70
Copy link
Contributor Author

I get:

  • BAD commit af7c725: plugins: EnvGen - unify envgen perform implemementation
  • GOOD commit dc0646d: plugins: EnvGen - remove duplicated code

I can see that a bunch of stuff has been moved into:

template <bool CheckGateOnSustain, typename GateCheck>
static inline void EnvGen_perform

... but I don't see that the logic for shape_Curve has changed very much... except for the gateCheck() which is new in the block-calculation loop.

As a test, I started with af7c725 and commented out the two lines 2964 and 2965:

    case shape_Curve : {
        double a2 = unit->m_a2;
        double b1 = unit->m_b1;
        double grow = unit->m_grow;
        for (int i=0; i<nsmps; ++i) {

// COMMENT THESE TWO

            if (!gateCheck( i ) )
                break;

            ZXP(out) = level;
            b1 *= grow;
            level = a2 - b1;
        }
        unit->m_b1 = b1;
    } break;

And the problem went away -- the last test case that I posted no longer has the spikes, but... without the gate check, (IIUC) the gate may close in the middle of a calculation block, but this won't take effect until the next block boundary. So, just removing it is not the solution.

So, for EnvGen.ar, it's possible to exit from the block calculation early, and I'm guessing that this doesn't update the unit's state variables, (likely) causing the next block to be wrong. It's beyond my expertise to fix it, though.

@danstowell
Copy link
Member

Beyond my expertise too I'm afraid. But from reading af7c725, here's a thought: the way that EnvGen_perform() has been unified is such that for audio-rate it does the same gate-check as before, while for control-rate the gate-check has been replaced by a simple "return false" (line 3010). This seems odd since it would always break out of the curve-generating code blocks for k-rate stuff. It would seem to me that if it were instead a "return true" then that might reinstate the old behaviour.

I emphasise "might" - this is just from reading the patch. I could easily be misreading. @timblechmann may be able to clarify.

@timblechmann
Copy link
Contributor

can have a look, but it might take a few days. the audio-rate ugen has to distinguish between audio-rate and control-rate gate signals ... but having a reproducer, it should be easy to find

@telephon
Copy link
Member

Here is a related issue: #1063 (To avoid confusion, I opened a new one instead of reopening this one)

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

4 participants