Envelope sustain level does not follow correctly in analog mode

[sourcebox]

• Surge Version: 1.6.4.1
• Plugin type: VST2
• Bits: 64

Steps to reproduce the behavior:

1. Open surge with the init sound
2. Press and hold a note
3. Move the Amp EG sustain slider down to 0 and to the max again. The sustain level will follow as expected.
4. Release the note
5. Switch Amp EG to analog mode
6. Press and hold a note again
7. Move the sustain slider down to 0 and to the max again. The sustain level will not rise to the maximum again.

The Filter EG shows the same behaviour.

Expected behavior
The sustain level should follow with the decay time when moving the slider in analog mode in the same way as in digital mode.

Desktop (please complete the following information):

• OS: macOS 10.12
• Host: Ableton Live
• Version: 10.1

[baconpaul]

Thanks. This may be the same as #825 so let me link them. I’m on a couple of week surge break but will look in early 2020 if no one else does. Appreciate the super clear report

[baconpaul]

OK so I've confirmed this is indeed the case. Same thing happens if (say) you modulate the sustain with a unipolar LFO. In digital mode it goes up and down in the sustain phase and in analog mode it does not.

The thing I'm uncertain of is: Is that the desired behavior. The code for the analog envelope is a bit hairy - and to be honest I haven't dug through it myself in detail (all that SSE2 direct stuff! All that state!). But I certainly will.

Just to set expectations though: This one may get fixed with an update to the manual saying "in analog mode Sustain is not modulate able once you are in the delay section" or some such. At least for now.

[mkruselj]

My expectation would be if digital mode works like that, it should also have the same behavior in analog mode...

[sourcebox]

Typical analog envelopes behave like that because the circuitry is essentially an RC network which is fed by the sustain level as long as in the sustain phase. So when you call the mode "analog" I would expect something like that.

[baconpaul]

Yeah I agree. It should do what you say it should do. I will investigate

[baconpaul]

OK so using the new test framework I've been chipping away at this.

An interesting thing is that the analog envelope behaves much differently than the digital one. The digital one meets the constraints you would expect (turn around at attack time and so on). But the analog one seems to attack more quickly than the attack time, and seems to sustain at half the sustain level.

Here's a picture from VCV Rack, for instance, of the same thing configured as analog and digital.

No big deal of course, it just means the test cases for analog are looser. And then I'll get around to fixing the core problem here too I hope through putting in some clear tests of current behavior.

[baconpaul]

Edit It's actually sustaining at s-squared not s/2. But at 0.5 that's the same! :)

[baconpaul]

Well except it isn't doing that consistently either!

[baconpaul]

Tiny note to self for when I dive back on to this: https://fgiesen.wordpress.com/2016/04/03/sse-mind-the-gap/

[baconpaul]

OK I now know why this is happening. I had to re-write a parallel implementation of that SSE goop in plain C++ code to see it, Basically here is the inner loop

  auto gate = !released;
                                  auto v_gate = gate ? v_cc : 0.f;

                                  // discharge = _mm_and_ps(_mm_or_ps(_mm_cmpgt_ss(v_c1_delayed, one), discharge), v_gate);
                                  discharge = ( ( v_c1_delayed > 1 ) || discharge ) && gate;
                                  v_c1_delayed = v_c1;

                                  auto S = s * s;

                                  auto v_attack = discharge ? 0 : v_gate;
                                  auto v_decay = discharge ? S : v_cc;
                                  auto v_release = v_gate;

                                  auto diff_v_a = std::max( 0.f, v_attack  - v_c1 );
                                  auto diff_v_d = std::min( 0.f, v_decay   - v_c1 );
                                  auto diff_v_r = std::min( 0.f, v_release - v_c1 );

                                  const float shortest = 6.f;
                                  const float longest = -2.f;
                                  const float coeff_offset = 2.f - log( samplerate / BLOCK_SIZE ) / log( 2.f );

                                  float coef_A = pow( 2.f, std::min( 0.f, coeff_offset - a ) );
                                  float coef_D = pow( 2.f, std::min( 0.f, coeff_offset - d ) );
                                  float coef_R = pow( 2.f, std::min( 0.f, coeff_offset - r ) );

                                  v_c1 = v_c1 + diff_v_a * coef_A;
                                  v_c1 = v_c1 + diff_v_d * coef_D;
                                  v_c1 = v_c1 + diff_v_r * coef_R;

makes perfect sense, right. Push around based on gate or discharge state. Even pretty easy to read in that non-sse form.

The problem is this line

                                  auto diff_v_d = std::min( 0.f, v_decay   - v_c1 );

or in SSE-land

         __m128 diff_v_d = _mm_min_ss(_mm_setzero_ps(), _mm_sub_ss(v_decay, v_c1));

What that said is that the decay differential is capped out at 0. Decay can never push up your envelope.

So what this means is when we increase S and are holding a note the diff mins out at 0 and so the integrator doesn't increase the voltage.

That line should probably be something like

                                auto diff_v_d = ( discharge && gate ) ? v_decay - v_c1 : std::min( 0.f, v_decay   - v_c1 );

and also in SSE; but making that change will require me to make sure that the min isn't saturated when discharge && gate are set in normal constant sustain operation.

I think I'll push the unit tests to master now and then keep working on this this week with the fix.

[baconpaul]

@sourcebox in the next nightly which should be out in about 20 minutes, and also in 1.6.5 which should be out in early Feb, this is fixed.

[sourcebox]

Looks good so far. Thanks for your effort.