Add limiter to Playground runtime #11
Conversation
✅ Deploy Preview for prismatic-pudding-943ac5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Hey @mark-mxwl thanks so much for doing this, and I'm sorry for the delay! The code change looks great but it does open up a little version complexity that I describe below. Let me know what you think!
src/components/playground/runtime.js
Outdated
| @@ -44,9 +46,11 @@ export class Runtime { | |||
| throw new Error('Missing render function on default export'); | |||
|
|
|||
| const userOutput = render(); | |||
| // This limiter setting preserves approx. 1 dB of headroom. | |||
| const limit = (input) => el.compress(1, 10, -4, 20, input, input); | |||
There was a problem hiding this comment.
The compress function that we're using right here comes from the el library imported from @elemaudio/core version at the top of the file. But the userOutput signal that we get from invoking the user module is built using the el export of whatever version @elemaudio/core is loaded in the user module.
It's really quite rare that there would be a conflict between these versions, but these versions do sometimes have requirements onto the renderer. For example, the core library might export a utility that uses a native node that only a recent version of the renderer knows about.
I do anticipate the next major version bringing some breaking changes, so we could arrive in a situation here where the user code is running a v4 web-renderer instance and v4 elemaudio/core but the limiter here is written against v3 elemaudio/core, and we could be getting into tricky territory there.
Even if this wouldn't be a problem, I think it'd be safer to make sure that we try to use the el.compress function from the same version that the user is running. Luckily, we know exactly what version that is because we force it using the importMap script above. Then on line 16 we fetch a matching web-renderer version. I think the fix here would be to add another line right below 16 which fetches the appropriate elemaudio/core version and grabs the el library out of that, then uses that compress function here to apply the limiting
There was a problem hiding this comment.
Thanks for getting back, @nick-thompson. I see your concern, and I totally agree that keeping the versions aligned on import is a much better solution. I'll try to knock out the changes this weekend.
| const WebRenderer = pkg.default; | ||
|
|
||
| this.el = coreLib.el; |
There was a problem hiding this comment.
Added coreLib import for version matching, and this.el to the runtime constructor. Great suggestion 👌
| @@ -44,9 +47,11 @@ export class Runtime { | |||
| throw new Error('Missing render function on default export'); | |||
|
|
|||
| const userOutput = render(); | |||
| // This limiter preserves approx. .5 dB of headroom at max input. | |||
| const limit = (input) => this.el.compress(1, 50, -4.5, 50, input, input); | |||
There was a problem hiding this comment.
Pushed the limiter a bit harder to account for any ludicrous experimentation. 🦾
Closes: #4
Added limiter function to
runtime.jsusingel.compresswith limiter-style settings. The current settings preserve approx. 1 dB headroom at max input. Also updatedpackage.jsonto the latest versions of @elemaudio/core and @elemaudio/web-renderer to ensure correct type classification w/isNode.