-
Notifications
You must be signed in to change notification settings - Fork 3
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
Added LPF to delay module #58
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! Will check this out later today. |
Might want to check my routing. Attempted to follow the patterns, but it looks like something is failing in testing.... |
at the top of the delay module you need to import the filter class. Also where you're setting your defaults it shouldn't have ".node"
not also where you're connecting your filter to your delay module, that should end with a semi-colon |
Aha! I knew there was something I wasn't doing right. I hadn't gotten the hang of this pattern yet. Sorry about that. I hope it still has some value! Nathan McElwain
|
@CuttyBang Hey no problem man. Do you want to update the PR and then I can merge it in? |
Absolutely. I'm gonna hop on it this afternoon, as soon as I can get back to my desk! Thank you for keeping the PR alive! Nathan McElwain
|
No problem! I've been busy but I'm back and working on getting this to a 1.0 state over the next few months. If you have any other ideas feel free to submit them. Also, I made an update last night so that you can spin up an example synth locally in your browser if you wanna test any new features (see the Example section in the README file for setting it up). |
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.
Repaired the paths and syntax error. Added a comment to explain the routing a bit.
Wow, what a flake! Sorry man. I guess I just got completely sidetracked and spaced on this in the worst way possible. At any rate, I finally fixed it, and added a comment about it. |
Thanks man! Will check this out when I get home tomorrow. Appreciate the contribution. |
@CuttyBang Hey, just wanted to let you know we are reworking some of the library internals to simplify things. The Delay module shouldn't be changing too much (if at all), and I'll be spending some time porting it over later this week and looking at your code changes. |
@CuttyBang So wavedef has been restructured a bit - the Delay module is now located at src/modules/Delay.js. Otherwise, not much else has changed, save for stricter linting rules and a few additional methods here and there. How do you feel about porting over this change to the new code base and adding unit tests in Delay.test.js? |
Yea man, sure. I'm game. I see all the restructuring going on. Looks great. I'll hop on this some time over the next few days. |
6548b87
to
04b6b49
Compare
0db0b3a
to
82753d2
Compare
62aa691
to
9803931
Compare
Added a LPF to the Delay module and chained it in. Creates dubby tape delay sound. Can be BPF if preferred. Freqs just need to be shaved on each pass. Thanks guys.