electro-smith / DaisySP

A Powerful DSP Library in C++
https://www.electro-smith.com/daisy
Other
903 stars 143 forks source link

Adsr remake #160

Closed sdiedrichsen closed 3 years ago

sdiedrichsen commented 3 years ago

That's a much leaner version of the envelope based on a first order SVF. Improvements:

TheSlowGrowth commented 3 years ago

@sdiedrichsen I may be too late, but it seems like you're trying to manually fix the clang-format errors. I don't know which text editor you're using, but with VS Code and a local installation of clang-format, you can auto-apply the formatting with Ctrl-Alt-F or Cmd-Alt-F. Other editors have the same feature. If that doesn't work, you can also run the clang-format executable manually on a file to make the edits for you.

Doing all this by hand must be very frustrating and annoying for you.

sdiedrichsen commented 3 years ago

@TheSlowGrowth Yes, I installed clang-format already, but's it's V12, which differs about formatting to V10, but it's much better. I think, I learned my lesson. I installed the format plug-in for VS Code to get this going. I hope, I find some time over the weekend to finalize this. Did you see the graph above regarding shapes and mixing?

TheSlowGrowth commented 3 years ago

Yes, I saw it. The mixing indeed looks strange, not very usable imo. Setting the target higher seems like a solid choice. Only problem is that the response is very nonlinear, most of the interesting action already happens between target = 1 ... 1.5. Again, I think it's a sort of hyperbola response, but I haven't had the time to verify this thought.

sdiedrichsen commented 3 years ago

OK guys, the last commit is quite big but contains nearly all wishes you asked for. I didn't go with the enum class, since the enum is part of an existing API and I don't want to break existing projects.

sdiedrichsen commented 3 years ago

These are the shapes: image

stephenhensley commented 3 years ago

With just a quick look, this is looking solid to me. I'll have time to do a little testing and get it merged in the next few days!

Thanks a ton for the contribution, and thanks to everyone else for the feedback, and reviews!

stephenhensley commented 3 years ago

Okay, finally had a chance to give it a test.

Seems great, and the shapes are solid, but there is one minor thing. It seems like the decay/release times take about 2 times longer than the time set by the SetTime function.

For example, when running the seed/adsr example from DaisyExamples, the Attack, and Decay times are both set to 100ms. However, on a scope I'm seeing the decay segment take about 200ms to reach the sustain level. This is corroborated by the Release stage which measures in at about 20ms instead of the 10ms set in the example.

sdiedrichsen commented 3 years ago

Stephen, I have to admit, that I didn't check, if the times match with the old implementation. I'll write a test and check. From the electrical engineering standpoint, the time you dial in is a time constant tau 𝛕. That's the time for an exponential function to decay to 1/e. image

sdiedrichsen commented 3 years ago

Of course we can adjust this differently, if the old implementation differs a lot. But that's how Bob Moog did it.

stephenhensley commented 3 years ago

Yeah, I totally get that. And actually I didn't check the previous version to see if it was the same or not (since it was also based around the filter time constant). It might actually be the same.

I can flash the version from the web programmer a bit later and check. If it's the same as the previous version we can just leave it.

stephenhensley commented 3 years ago

@sdiedrichsen confirmed that they were pretty similar on the previous version (if not a bit further from the target time). So I'm signing off.

Thanks again for the contribution!

jwjmayo commented 3 years ago

not sure if this also happens with field examples (don't own one) but if a note is held until decay portion fully decays to zero , it never recovers. I'm looking into this with my own code, but has anyone tested this with examples? The if(!env_.IsRunning()) method found in the examples seems to fail with the updated version when notes are held until decay reaches zero.

oxiinstruments commented 2 years ago

Damn so that was the issue in my proto. Suddenly the sound stopped, and debugging showed it was just before the ADSR. It's seems I'm a bit late to the party, so thanks for fixing it!

Maybe an simpler linear alternative could work too. There're many examples out there.