My one comment for now is that we need to be clear what EnvGen forced-release really means.

Today, I found a commit from 6-7 years ago that fixed a bug, but also accidentally changed the meaning of forced-release.

Let's be careful that we are not about to change its meaning again.

I'm sorry to be persnickety about this, but we've gone in a matter of hours from "oh, I didn't expect that behavior" to a proposal to change the behavior, with almost no discussion about what the behavior should be. Slow down. Not so fast. Let's figure out what it should do before casually dropping something into the product that we might regret later.

All right, thanks for your meaningful input, I'm sorry to have rushed. Of course I'm open for discussion, which I welcome, and never expected my code to be integrated right away. Still figuring out how the contribution process works, I thought the discussion could have been easier with a concrete code proposal.

So, shall we continue discussing it on the issue, or shall I open a RFC? I initially thought an RFC would have been too much for this: I admit I thought this was a bug or a feature that was never fully developed, instead of understanding its intended use as an emergency cut for envs with longer release phases.

I also admit that I have been using .release(x) for quite some time without noticing it was producing a linear ramp instead of the curve I expected.

I admit I thought this was a bug or a feature that was never fully developed, instead of understanding its intended use as an emergency cut for envs with longer release phases.

I think it would be good to understand the use cases before making further changes. It's slippery: just this morning I found a bugfix that accidentally changed the behavior, non-trivially, outside the scope of the reported bug. This kind of thing happens when the feature is not well-understood (a Rorschach feature, if you will -- everybody looks at it and sees what they want to see in it).

I understand the wish to override release duration, perhaps without overriding other release properties. I have a feeling that passing a single float to the server isn't an articulate enough expression for this. If you want to override some properties but not others, I tend to think this should be built into the SynthDef, so that you are in control of the override, rather than relying on magic behavior of special float values. Overloading that one float to do perhaps too much is potentially confusing.

For me, early-release should fade out the envelope immediately, no matter what phase the envelope is in (this latter point was broken in the commit I mentioned) -- for me, non-negotiable. The curve, I'm not really concerned about, as long as it's clear and justified. Final segment curve is probably the least objectionable option, though I'd like to think a little more about cases where that might be wrong.

For me, early-release should fade out the envelope immediately, no matter what phase the envelope is in (this latter point was broken in the commit I mentioned) -- for me, non-negotiable

After I understood this use case (to immediately cut an envelope even if it has a complex release phase), I changed the code to jump to the last node instead of to release node. So, that functionality is preserved, the change is that shape is read from the env's last node, instead of forcing a linear ramp.

The curve, I'm not really concerned about, as long as it's clear and justified

Envelopes that end curved will still end curved, while linear ending envelopes will still end linearly. While preserving the 'old' use-case, this would improve it for others where a curved envelope is more appropriate. That said, I'm open to wait for use cases of a forced linear ramp to show up.

My one comment for now is that we need to be clear what EnvGen forced-release really means.

My input to this important question is that for now there is a source of confusion in having forced and normal release packed into the same method. These two functions have in fact different meanings:
to run the envelope's release phase (when releaseTime is zero), and to immediately shortcut the envelope to release in a set time (when releaseTime is positive). One is sourcing the envelope, the other is almost ignoring it (it takes into account its final value, but not its shape).
I see your point into discouraging the use of this function all together, but with such an inviting name as 'release' this doesn't seem right to me. If we go for such a basic name as release(releaseTime), I think it is normal to expect activation of the envelope's release phase, while assuring that the envelope is released in releaseTime seconds. Such a basic name should IMO better cover basic use cases, like:

• For envelopes with a one-stage release phase (like Env.asr), the decision is unambiguous. The envelope will end in the correct time just by referring to its release stage and stretching it to the set time. It would just be an improvement not to force its shape to be linear. Normal and forced release are equivalent here.
• For envelopes with complex release phases, arbitrarily cut the envelope to its end, in a set time. More complex handling of the different release stages can justifiably require more work, such as defining SynthDef controls and setting their values together with a zero gate. In case of a forced release, using the envelope's final stage's shape instead of forcing a linear one is an equally arbitrary act.

So, maybe I see the point now. Making it work for basic envelopes will blur its meaning as an emergency measure even more, shifting it towards a stretched release. I can't help thinking that this is much more what .release(releaseTime) suggests, than a shortcut release.
Thinking about a negative EnvGen gate argument, it makes enough sense for me that gate: -10 means to end the envelope in 9 seconds (enough sense, not perfect, but I see how 0 is not available for this purpose). It still makes sense for me, that a single float being passed to a UGen would shortcut to the end of an eventual multi-stage release, requiring more complex work to handle more complex cases, which would need more complex decisions.

Again, I'm open to wait for use cases of a forced linear ramp to show up.

Today, I found a commit from 6-7 years ago that fixed a bug, but also accidentally changed the meaning of forced-release.

just this morning I found a bugfix that accidentally changed the behavior, non-trivially, outside the scope of the reported bug

Would you mind posting some links, I think it would be relevant to the current discussion to understand how the meaning of release vs forced release is shifting :)

Would you mind posting some links, I think it would be relevant to the current discussion to understand how the meaning of release vs forced release is shifting :)

4a0e919

--> #753

If I'm reading it right, the bug was: if an EnvGen was oscillating between releasing and non-releasing states in some specific way, it would infloop.

Tim fixed it by preventing a forced-release during any release, but he missed the fact that a forced-release is legal (and desirable) during a normal release. That is IMO not shifting semantics, but a mistaken understanding of the feature's purpose, which introduced the bug that you now can't interrupt a normal release.

... else if (gate <= -1.f && prevGate > -1.f && !unit->m_released) {

In hindsight, the bugfix should have had two flags: m_released meaning "in the middle of a normal release," and m_forced_release for the other. So the above line would read:

else if (gate <= -1.f && prevGate > -1.f && !unit->m_forced_release) {

And a bit below would check else if (unit->m_prevGate > 0.f && gate <= 0.f && unit->m_releaseNode >= 0 && !unit->m_released && !unit->m_forced_release).

(I think... I haven't tried it. Or maybe there's another, better way to implement it.)

My input to this important question is that for now there is a source of confusion in having forced and normal release packed into the same method.

That's a fair point. It's a long standing problem in SC that it isn't clear what is a convenience method and what is a robust "the right way" solution. release straddles that line: the name suggests "right way" but at the end of the day, it's a shortcut for setting a \gate. (For that matter, if your SynthDef uses, say, gt then release won't work at all.)

I won't object further to the change here, but I still think, if you want something specific to happen when overriding release properties, the best "can't-fail" way is to write exactly what you want into the SynthDef.

... else if (gate <= -1.f && prevGate > -1.f && !unit->m_released) {

Thanks, very interesting. Out of curiosity I tried the following condition for forced release:

else if (gate <= -1.f && prevGate != gate)

and I still can't reproduce the crashes from #753. This looks nice to me because it would allow a forced release of a normal release, and even of a previous forced release.
What do you think?

I'll definitely study the case more. The check I removed has nothing to do with this, it was still a leftover from a previous commit of mine

@elgiano thank you for taking it on and looking at it carefully, very welcome indeed.

I dug into it. It looks like the bug was simpler (but hideous) and didn't require a check for m_released.
Before Tim's commit, CHECK_GATE had this check for forced release:
else if (gate <= -1.f && unit->m_prevGate > -1.f)

Tim's fix was this:
else if (gate <= -1.f && prevGate > -1.f && !unit->m_released)

It turns out the bug was not fixed by && !unit->m_released, but by checking prevGate instead of unit->m_prevGate: prevGate is the last gate value evaluated within the current block, while m_prevGate stores the gate's last value from previous block and gets updated only once per block (at the end).
So, without getting into too much detail, EnvGen_next_aa can get stuck in the moment when the gate gets == -1 while the value from past block was still >= -1, without being able to update m_prevGate and thus never getting past forced release detection, always "thinking it has just detected it".

I don't know how acceptable these empiric proofs are, but I checked the following:

1. I copy/pasted LFUGens.cpp from Tim's commit into the current develop
2. I removed && !unit->m_released from the forced release check. No crashes
3. I copy/pasted LFUGens.cpp from before Tim's commit into the current develop
4. I added && !unit->m_released to the forced release check. Crashes
5. I copy/pasted LFUGens.cpp from before Tim's commit into the current develop
6. I changed unit->m_prevGate with prevGate in the forced release check. No crashes

This made me think && !unit->m_released was unnecessary, and prevGate was sufficient.

Now, moving on:
A check like this (gate <= -1.f && gate !=prevGate) would allow forced release to trigger even during a previous forced release, resetting its duration. The problem is that for oscillating audio-rate gates, this would make the server busy recalculating forced release all the time. It's not much, maybe I'm just being conceptual here.
On the other hand, (gate <= -1.f && prevGate > -1) would enable forced release to stop a normal release, but not to change an already started forced release. Then the server would never calculate a forced release more than once.

Let's say someone calls synth.release(100), and then wants to cut the envelope off immediately. With the first check that would be just a .release(0), while with the second one would have to first set \gate > -1 and then do .release(0).
Is anyone interested in this feature? I feel like forced release should be always available, even during a forced release, but I'm not sure about how to proceed.

Would it be best to keep (gate <= -1.f && prevGate > -1) in this PR as a kind of bugfix and propose (gate <= -1.f && gate !=prevGate) as a feature enhancement in a RFC?

@elgiano I'm convinced by your analysis, that's great sleuthing!

A check like this (gate <= -1.f && gate !=prevGate) would allow forced release to trigger even during a previous forced release, resetting its duration. The problem is that for oscillating audio-rate gates, this would make the server busy recalculating forced release all the time. It's not much, maybe I'm just being conceptual here.

My gut feeling is not to do it that way. A gate opening or closing is analogous to a trigger (note that a gate opening isn't "not equal to previous," but rather "previous was <= 0, current is > 0"). The analogous trigger-like expression for below -1 would be exactly "previous >= -1, current < -1."

It definitely doesn't make sense to reset a forced release on every sample.

On the other hand, (gate <= -1.f && prevGate > -1) would enable forced release to stop a normal release, but not to change an already started forced release. Then the server would never calculate a forced release more than once.

It could, if the gate signal dipped below -1, then rose above -1, and fell below it again. That strikes me as sensible behavior.

So I'd go with gate <= -1.f && prevGate > -1.

Thanks @jamshark70, I agree.
I went with gate <= -1.f && prevGate > -1 and updated the Help files as well. So, before I mark it as ready for review, does anyone think I need to write specific tests for this? I've already run ctest and language-side UnitTests, and it passed all of them.