processing / processing-sound

Audio library for Processing built with JSyn
https://processing.org/reference/libraries/sound/
GNU Lesser General Public License v2.1
149 stars 50 forks source link

Envelope resets the amplitude of an oscillator to one on play #3

Closed defrost256 closed 6 years ago

defrost256 commented 6 years ago

In the Env.play() method the value for the envelope data for the SegmentedEnvelope assumes the value of the attack is 1f instead of the expected input.amp, taking control away from the developer. Easy fix:

SegmentedEnvelope env = new SegmentedEnvelope(new double[] {
                attackTime, input.amp, // attack
                // gradual decay towards sustain level across entire sustain period
                sustainTime, sustainLevel, // sustain
                releaseTime, 0.0 });

instead of

SegmentedEnvelope env = new SegmentedEnvelope(new double[] {
                attackTime, 1.0, // attack
                // gradual decay towards sustain level across entire sustain period
                sustainTime, sustainLevel, // sustain
                releaseTime, 0.0 });

On another note I think sustain level might be more intuitive represented as a fraction of amplitude as well

SegmentedEnvelope env = new SegmentedEnvelope(new double[] {
                attackTime, input.amp, // attack
                // gradual decay towards sustain level across entire sustain period
                sustainTime, sustainLevel * input.amp, // sustain
                releaseTime, 0.0 });

I'm going to make a PR for the first one shortly, since that's an actual problem, but I'll wait with the second one in case there is discussion to be had about it. Keep up the good work!

kevinstadler commented 6 years ago

Oops sorry I didn't see the more contentious second point about the sustain level in the issue! Yes I think representing the sustain level as a fraction of the overall amplitude makes sense, so if you're happy to do a pull request for that go on!

defrost256 commented 6 years ago

Hey, thanks for listening, I am afraid these changes might break the library for "legacy" users. Hope they won't come at me with torches and pitchforks 🔥

kevinstadler commented 6 years ago

I think if you let the envelope's amplitude alone it's fully compatible with the old version of the library, where the absolute amplitude couldn't be controlled in the first place! Thanks for the PR, there's another sound card-selection bug fix in the pipeline so this one should be available from the content manager soon...