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.

Sign up for GitHub

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

Jump to bottom

DemandEnvGen overshooting at high curve #1355

Closed
eleses opened this issue Mar 21, 2015 · 5 comments
Closed

DemandEnvGen overshooting at high curve #1355

eleses opened this issue Mar 21, 2015 · 5 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs.
Milestone
3.8

Comments

@eleses
Copy link
Contributor

eleses commented Mar 21, 2015

This might be related to #1241 but I saw it happening at audio rate, contrary to what that report/discussion says.

The output of following code is expected to converge to a squarish shape (with some flat gaps in between) at high (negative) curvature, but produces some obvious diagonal artifacts.

({  arg ontimefq=100.0, offtimex=3, offtimfq=500, amp = 1, curve = -300; // bug at high curve!
    var ontime = ontimefq.reciprocal, offtime = offtimex*ontime + offtimfq.reciprocal;
    DemandEnvGen.ar(
        Dseq([0, amp, 0, amp.neg, 0], inf),
        Dseq([ontime/4, ontime/4, ontime/4, ontime/4, offtime], inf),
        5, Dseq([curve, curve.neg, curve, curve.neg, 0], inf));
}.plot(0.1, s);)

Plot

The bug is definitely not in plot because I can see it in a Stethoscope too. Also, the equivalent code using EnvGen does not exhibit this bug:

({  arg ontimefq=100.0, offtimex=3, offtimfq=500, amp = 1, curve = -300;
    var ontime = ontimefq.reciprocal, offtime = offtimex*ontime + offtimfq.reciprocal;
    EnvGen.ar(Env.circle( // works ok
        [0, amp, 0, amp.neg, 0],
        [ontime/4, ontime/4, ontime/4, ontime/4, offtime],
        [curve, curve.neg, curve, curve.neg, 0]));
}.plot(0.1, s);)

Plot2
@scztt scztt added the bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. label Apr 15, 2015
@scztt scztt added this to the 3.7 milestone Apr 15, 2015
@telephon
Copy link
Member

Checked briefly.

Just for the record: DemandEnvGen uses (almost) the same code as EnvGen:

case shape_Curve : {
                        if (fabs(curve) < 0.001) {
                            unit->m_shape = shape = 1; // shape_Linear
                            unit->m_grow = (endLevel - level) / count;
                        } else {
                            double a1 = (endLevel - level) / (1.0 - exp(curve));
                            unit->m_a2 = level + a1;
                            unit->m_b1 = a1;
                            unit->m_grow = exp(curve / count);
                        }
                    } break;
<...>
case shape_Curve : {
                    double a2 = unit->m_a2;
                    double b1 = unit->m_b1;
                    double grow = unit->m_grow;
                        b1 *= grow;
                        level = a2 - b1;
                    unit->m_b1 = b1;
                } break;

vs in EnvGen:

case shape_Curve : {
            if (fabs(curve) < 0.001) {
                unit->m_shape = 1; // shape_Linear
                unit->m_grow = (endLevel - level) / counter;
            } else {
                double a1 = (endLevel - level) / (1.0 - exp(curve));
                unit->m_a2 = level + a1;
                unit->m_b1 = a1;
                unit->m_grow = exp(curve / counter);
            }
        } break;

<...>

    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) {
            if (!gateCheck( i ) )
                break;
            ZXP(out) = level;
            b1 *= grow;
            level = a2 - b1;
        }
        unit->m_b1 = b1;
    } break;

@crucialfelix crucialfelix modified the milestones: 3.7.x, 3.8 Apr 7, 2016
@baconpaul
Copy link
Contributor

Hi!

I am a total new developer on this codebase, I picked this issue to learn mechanics.

Along the way, I was able to fix this but am having a bit of a hard time either "taking" this bug or making a fork on my github repo. I'll work on that a little later.

The problem in the DemandUGen is that the curve "grows" by exp( curve / count ) (in the setup region) where count is initialized as phase. This is then applied for each sample until phase hits zero then it's all re-initialized.

But of course if count is non-integral and curve is big, you overshoot a teeny bit. So I fixed it by doing a ceil( count ) in the init, and I get perfect alignment.

The constructor also seems odd; even though the example here uses .ar() the _k generator is called. I tried to fix the constructor to route to the right function, but couldn't. I applied the fix to the _a code also, though, in case.

Any advice on workflow or community would be appreciated. And I'm more than happy for you to not accept the diff or suggest I do things differently or whatever. I am very conscious, like I said, of being new to the community. Thanks for any comments or suggestions.

diff --git a/server/plugins/DemandUGens.cpp b/server/plugins/DemandUGens.cpp
index 2e674e9..ce00382 100644
--- a/server/plugins/DemandUGens.cpp
+++ b/server/plugins/DemandUGens.cpp
@@ -819,7 +819,7 @@ void DemandEnvGen_next_k(DemandEnvGen *unit, int inNumSamples)
                                                        double a1 = (endLevel - level) / (1.0 - exp(curve));
                                                        unit->m_a2 = level + a1;
                                                        unit->m_b1 = a1;
-                                                       unit->m_grow = exp(curve / count);
+                                                       unit->m_grow = exp(curve / ceil( count) );
                                                }
                                        } break;
                                        case shape_Squared : {
@@ -1079,7 +1079,7 @@ void DemandEnvGen_next_a(DemandEnvGen *unit, int inNumSamples)
                                                        double a1 = (endLevel - level) / (1.0 - exp(curve));
                                                        unit->m_a2 = level + a1;
                                                        unit->m_b1 = a1;
-                                                       unit->m_grow = exp(curve / count);
+                                                       unit->m_grow = exp(curve / ciel( count) );
                                                }
                                        } break;
                                        case shape_Squared : {

@baconpaul
Copy link
Contributor

Oh hey, figured it out and made a pull request which again I'm happy if you reject or accept, and again for which I apologize if I haven't followed protocol

#2163

@baconpaul
Copy link
Contributor

And (so embarrassing) the pull request with the code that compiles is #2164

sorry!

@telephon
Copy link
Member

telephon commented Jun 5, 2016 •
edited
Loading

thanks! no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs.
Projects
None yet
Milestone
3.8
Development

No branches or pull requests
5 participants
@scztt @crucialfelix @telephon @eleses @baconpaul