-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adsr remake #160
Adsr remake #160
Conversation
It's based on a geometric series
with all time set to 0. and sustain at 0.
~Adsr() {} | ||
/** Initializes the Adsr module. | ||
\param sample_rate - The sample rate of the audio engine being run. | ||
*/ | ||
void Init(float sample_rate); | ||
void Init(float sample_rate, int blockSize = 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment line about blockSize
in the above comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I used clang format v12 on my system, but v10 seems to have a different idea about nested trigraphs.
@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. |
@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. |
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. |
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. |
v12 has a different opinion on that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
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! |
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 For example, when running the |
Of course we can adjust this differently, if the old implementation differs a lot. But that's how Bob Moog did it. |
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. |
@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! |
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. |
Damn so that was the issue in my proto. Suddenly the sound stopped, and debugging showed it was just before the ADSR. Maybe an simpler linear alternative could work too. There're many examples out there. |
That's a much leaner version of the envelope based on a first order SVF.
Improvements: