Release section of envelope issues

[YurkoFlisk]

This problem may be similar to #3086, but more general I think.
When the release section of envelope is entered (whether it's for volume or not) and AMT knob is somewhere between 0 and 1, there's a click even with nonzero release time, which is well noticeable if AMT is around 0.5 and sustain value is at max. Zip with 2 example presets is attached: one for volume envelope, second for cutoff envelope (with volume envelope set to AMT 1 so that it doesn't cause problems).

The issue seems to be in EnvelopeAndLfoParameters.cpp and is the way m_rEnv is computed in void EnvelopeAndLfoParameters::updateSampleVars. In particular, void EnvelopeAndLfoParameters::fillLevel function suggests that this array is used for calculating envelope levels during release period, and it's values are multiplied by the envelope value from which the release began (m_pahdEnv[_release_begin] or m_sustainLevel, depending on release beginning time). So, for a continuous transition, at the first release frame this value should be multiplied by 1 (or very close to that), so m_rEnv[0] should be 1, whereas, by the way it's filled in updateSampleVars function, m_rEnv[0] is m_amount. This also explains why there's no problem when AMT is 1.
What's interesting is that if m_rEnv is calculated starting from 1, release period should always start smoothly, so even if m_amount is 0 we will have release from actual value. In particular, since for the volume section default release time is already nonzero, there won't be click at the end by default, so this should resolve #3086 as well.

So, it seems like change here from
const float rfI = ( 1.0f / m_rFrames ) * m_amount;
to
const float rfI = ( 1.0f / m_rFrames );
should fix the problems. I'll try to setup compilation process and test whether this will fix the problem and not introduce other ones.

ClickExamples.zip

[YurkoFlisk]

I tried the changes (sorry for long time) (this branch) and, with them applied, in attached project (in zip) three 3OSC presets sound correctly, whereas without them there are clicks due to envelope discontinuity.
test.zip

However, when amount is zero, due to the logic of EnvelopeAndLfoParameters::updateSampleVars either the envelope is not used or release is 1-sample wide, so in this case (including default 3OSC preset) there is still a click and this means that the change doesn't make envelope 'enabled' by default (which could be the problem because this was the temporal fix for #3086 and was then reverted).

So, this doesn't resolve #3086, but seems to resolve the general problem in calculating release section of envelope addressed in this issue (#4672).

[zonkmachine]

Hi! Thanks for looking into this! I may look into your finds over the holidays, just don't hope too much... :)

[zonkmachine]

Holidays are now over and I'm back testing this.

I tried the changes (sorry for long time) (this branch)

I can confirm that this looks like a fix for the issue with transients in the envelope with amount set to ~0.5 . Needs more testing but it looks promising.

@@ -490,7 +490,7 @@ void EnvelopeAndLfoParameters::updateSampleVars()

-	const float rfI = ( 1.0f / m_rFrames ) * m_amount;
+	const float rfI = 1.0f / m_rFrames;

So, this doesn't resolve #3086,

No. This was fixed in #6908 with three separate issues solved and this here is a fourth.

@michaelgregorius You may want to have a look at this.

[michaelgregorius]

@zonkmachine, @YurkoFlisk, for me the change removes the click.

I am not sure though if the ADSRs are implemented correctly in the first place. I'm under the impression that the release stage always takes the same time to reach silence, regardless of whether it starts at full sustain, i.e. 1, or at a lower level, e.g. 0.2. This is not how others plugins (and I guess real ADSRs) behave.

The current implementation is also rather resource intensive as it might create long buffers for something that should be computable. On top of that it also does memory allocation for these buffers (which I assume happen in the audio thread).

It would be really great if the ADSRs would be implemented as self-contained classes which compute the next value from a minimal state. They could also implement linear and exponential decay. However, doing so would likely be the ultimate backwards compatibility headache.

[Rossmaxx]

Close since #6908 got merged?

[zonkmachine]

Close since #6908 got merged?

That's a separate issue. I'm looking into this one.

[FyiurAmron]

@zonkmachine hard to say if this if relevant, but I stumbled upon a variation of this - tl;dr 3osc creates clicks on keyoff due to abrupt termination of the wave - even with envs. edit: yup, I forgotten to turn all the ASDR knobs to the values needed for reasonable behaviour in current version. Seems related to this but a solution of just setting env AMT to 1.0 and release to 0.1 did the trick.

[zonkmachine]

@zonkmachine ... 3osc with lopass filter from the filter tab (+reso in my case, but read on) creates clicks on keyoff due to abrupt termination of the wave - even with envs. Disabling lopass filter eliminates the clicks. Reso makes this more pronounced, but even with 0 reso this is still audible. Seems like a bug. Is this a bug different to any of those two, or is it a new one? Is a repro case needed for this?

It would be really helpful with a project that demonstrates what you're seeing.

[FyiurAmron]

@zonkmachine OK, I went to the bottom of this. Not really a distinct bug, but a result of the current UI/UX decisions mixed with some quirks probably reported here plus me being LMMS newb. In my case, the envelope didn't have the full 1.0 amount, and after noticing that I still missed that the release was 0. With no lopass, it wasn't audible on the sawwave, but with lopass it got exposed, see below:
(no filtering)

(with lopass)

Nothing to fix additionally, but definitely something to keep in mind if anyone still gets the clicks. TBH I'd set 1.0 env and > 0 release (and other reasonable env settings) as the defaults, since it's really easy to miss this, especially with no visual clue which point on the ADSR env is which (i.e. with more than one point in the same place, you might mistake one for the other etc.) - I guess that's the reason why some toolings use colours and/or letters on the env graph,

e.g.

Anyway, many thanks for the work on this, kudos!

[FyiurAmron]

FWIW, my problem basically mirrored stuff from #6121 , #5527 , and some other ones, so it seems it's not opaque only to me :)

[zonkmachine]

Here is the tones/envelopes of a test run with single notes and just decay. I'm sweeping the envelope from 0 to close to 1.0 (max) and the notes are of different length. You can see that the one liner applied in the lower graph is adding the envelope full on after key off.

1 - master
2 - master + fix

Here is a close up of the key release of the two test runs above and a third one, with the envelope off added. You can see how the release frames that is default when there is no envelope engaged is not part of the equation when the envelope is engaged. With just a hint of envelope on you will essentially get less decay than with the envelope off.

1 - master
2 - master + fix
3 - master envelope off

• The fix presented here works by adding proper release to the note. It's probably not the way you want this to work but it definitely sound nicer.
• The default release frames with no envelope is not present with just a hint of decay and this is what makes the noise.
• The 'Amount' knob of the volume envelope pans between all on/off action and envelope control.

I know of no other synth that works this way but I haven't tried FruityLoops. I can see no case where any setting other than full on/off of the volume envelope is really useful.

[Spekular]

I can see no case where any setting other than full on/off of the volume envelope is really useful.

It's pretty great for transient shaping; by adjusting the amount knob on a quickly decaying envelope you can reduce the volume of the tail without cutting it off entirely.

[michaelgregorius]

If I understand correctly then this is quite different to how ADSRs work in others DAWs and synths and so obviously also confusing to users who have experience with ADSRs. The amount should not have any effect on the shape of the curve itself, i.e. it should not introduce any discontinuities between any stages, e.g. the last sample of the sustain stage and the first one of the release stage.

A "normal" ADSR has a continuous shape that's defined by the ADSR settings and the amount settings only attenuate that full shape, i.e. it "squeezes" it in a certain way.

[zonkmachine]

Please note how the Tripple Oscillator applyFadeIn() applies to the note both when the envelope is engaged and when it isn't. This could probably be fixed by similarly applying the default release before mixing in the envelope value..

3osc - no envelope
3osc - envelope amount at 0.01

[michaelgregorius]

The question is what kind of fix is intended. As noted above the same ADSR envelope should be calculated for an amount of 1 and amount of 0.01. In the latter case the signal should just be attenuated by -40 dbFS (0.01 amplification) compared to the one at 1.

[YurkoFlisk]

@michaelgregorius @zonkmachine Regarding release duration, I'm not very familiar with other DAWs, but from what I see looking at open source code is that there are cases of both release duration being parameter of the envelope (which is what is implemented here via m_rFrames and which is fixed by my fix by avoiding double m_amount multiplication) and release rate being parameter (which means that the same value is subtracted from envelope level at each sample while releasing irrespective of point from which the release originated; this results in shorter/longer release if released from smaller/larger level). stk, for example, implements both of these options (see ADSR.h and ADSR.cpp). FL studio documentation says that release is the time it takes to reach zero from sustain level, but doesn't specify what happens if release starts from a different level. I think it would be nice for this to be explicitly documented in LMMS (currently it isn't) and maybe even made customizable.

Btw, while we're at it, neither documentation nor GUI reflects the fact that duration of each (non-sustain) envelope stage quadratically, via expKnobVal (btw why is it called like that if it implements x |-> x|x| mapping which is not exponential?), depends on the knob value. E.g., if you double the attack knob value, the graph will make attack part twice wider but the actual attack stage duration will quadruple (it's very easy to hear with small sustain and decay). Also, decay is explained everywhere I looked (including aforementioned FL studio docs) as the time it takes to reach the sustain value, but here it is multiplied by (1 - sustain) and thus would act as 'decay rate', if not for the fact that it is also expKnobVal'ed afterwards: that is, if you change sustain from 0 to 0.5, the graph may lead you to think that 'decay slope' didn't change, but actually it became steeper (audibly so in this case), because decay time got divided by 4 while distance from hold to sustain only got divided by 2 (while in ADSRs as described elsewhere the slope would become flatter!).

Regarding ADSR shape and m_amount, the shape of envelope for cutoff and resonance is already (with my fix and, as I think, as was originally intended) effectively multiplied by it once afterwards (i.e. squeezed) because m_addAmount == 0 and it's easy to see how m_amount can be 'factored out' of calculations in this case. For volume, however, the amount acts as wet/dry knob (up to release, because release is applied to their sum at the beginning of release), which is AFAIK a unique LMMS decision. If amount changes its meaning to just a multiplicative factor for this case, it will be essentially redundant since per-instrument volume knob already exists. For cutoff and resonance, of course, amount still has important meaning, though I think it's better to make amount knob actually represent relevant value for each case. E.g. for cutoff it would represent actual frequency delta amplitude rather than a factor for CUT_FREQ_MULTIPLIER (which is undocumented).

As an aside, I think some knobs could have their min/max values adjustable through the same dialog that now opens to change the current value upon doubleclick. It would make it possible to support wide range of values for such knobs without the need for arbitrary limits (e.g. 1 * CUT_FREQ_MULTIPLIER = 6000 Hz is currently the largest frequency delta amplitude) that exist to tradeoff with knob's UX. Knobs could then be restricted (possibly by default) to more comfortably 'slide' through narrower range of values if the user wants to while still allowing the use of wider range of values.

[michaelgregorius]

Regarding the ADSRs I am under the impression that one could differentiate between two implementations:

• "Analogish": The current value of a slope is computed using a multiplicative factor f. Example: the current value c is 0 and the ADSR gets a gate signal. The target value t is set to 1 but not immediately set. Instead for each step/sample the next current value c_n is computed using a factor between 0 and 1 as c_n = c + (t - c) * f.
• Digital: This is more or less the implementation that you have described for FL Studio. If for example the sustain stage is left and the release stage is entered then the slope is computed using the number of steps/time that is should take to reach 0 from the current value.

Because analog ADSRs likely use something like capacitors that are loaded and unloaded exponentially they should not behave like the digital implementation. The reason is that they will simply start to load/unload the capacitors using the current voltage. Put differently: the analog ADSRs will not compute anything for their behavior as they have no real "knowledge" of their current state.