-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixing compressor #34
Conversation
I just added softknee function to the compressor as kneeWidth arg in dB, 0 for hard knee I've tested it and it seems to work well el.compress(1, 100, -10, 100, 12, in, in);el.compress(1, 100, -10, 4, 6, in, in);el.compress(1, 100, -30, 10, 24, in, in);Sorry for the cropped out axis, but everything seem in place from the values I can see |
@Mozoloa fantastic PR thank you! A couple of quick changes if you don't mind:
That sound ok? That feels to me like the right to navigate non-breaking changes and bug-fixes |
All good |
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.
Great @Mozoloa thank you again! Three more tiny things if you don't mind 🙏
js/packages/core/lib/dynamics.ts
Outdated
@@ -44,22 +44,70 @@ export function compress( | |||
xn: ElemNode, | |||
): NodeRepr_t; | |||
|
|||
export function compress( | |||
export function compress(a_, b_, c_, d_, e_, f_, g_?) { |
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.
It looks like this was probably a bug leftover from me, but if you don't mind fixing it while we're here I'd appreciate it–
The type signature above doesn't present an optional props
argument up front, so we can write this function signature here as
export function compress(atkMs, relMs, threshold, ratio, sidechain, xn)
and then skip the whole let children = (typeof
stuff below it
* @param {Node} sidechain – sidechain signal to drive the compressor | ||
* @param {Node} xn – input signal to filter | ||
*/ | ||
export function skcompress( |
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.
Let's do the same thing here, remove that props
argument here in the type signature, and then below let's do
export function skcompress(atkMs, relMs, threshold, ratio, kneeWidth, sidechain, xn)
and skip the whole optional arg stuff
README.md
Outdated
[![License](https://img.shields.io/badge/license-MIT-blue.svg)](https://github.com/elemaudio/elementary/blob/main/LICENSE.md) | ||
[![Discord Community](https://img.shields.io/discord/826071713426178078?label=Discord)](https://discord.gg/xSu9JjHwYc) | ||
[![npm installs](https://img.shields.io/npm/dt/%40elemaudio/core?label=npm%20installs&color=%23f472b6)](https://www.npmjs.com/package/@elemaudio/core) |
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.
This is my bad I think– would you mind dropping the changes to README here so that the PR only affects the compressor code? I'll fix up the README changes in a follow-up commit
Ok got it, this will roll out with elem v3.1.0. Thank you @Mozoloa ! |
I've discovered that the compressor's ratio was behaving incorrectly. It was pretty much inversed in it's effect, a ratio of close to 50 was close to actually 1:1 in effect, same with close to 1 being actually close to infinity.
So I've redesigned the compressor to act in a more conventional way
I've tested it and so far it looks like it works alright, ratios are perfectly respected. More info in the code comments.
This might be app breaking for people that had implemented a hotfix on their end by reverse engineering the defective ratio tho.