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

Compressor #179

Merged
merged 30 commits into from Jul 1, 2017

Conversation

Projects
None yet
2 participants
@jvntf
Copy link
Contributor

jvntf commented Jun 13, 2017

new effect node, compressor. not fully tested

@jvntf jvntf changed the title Compressor (WIP) Compressor Jun 20, 2017

@therewasaguy
Copy link
Member

therewasaguy left a comment

looking good, just some small comments / questions, and after the previous merge we have a merge conflict

// var self = this;
// var errorTrace = new Error().stack;

if (attack) {this.attack(attack);}

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

for all of these, if (attack) is a == comparison, and 0 will be "falsey". So if we want to allow for 0, we should do a more explicit check like if (attack) !== 'undefined'

};

p5.Compressor.prototype.attack = function (attack){
if (attack) {this.compressor.attack.value = attack;}

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

I think all of these are audioParams which means they can, in theory, be scheduled over time. Do we want to allow for this?

This comment has been minimized.

@jvntf

jvntf Jun 22, 2017

Author Contributor

i think we do! i'm adding a time option now--
but, this is one of the chunks of code that appears many times in the library, I wonder if it would be a worthwhile future project to make a base class for functions that are implemented specifically for many different object in p5-sound

This comment has been minimized.

@therewasaguy

therewasaguy Jun 22, 2017

Member

That's a great idea!

it('can set params', function(){
var compressor = new p5.Compressor();
compressor.set(0.5, 20, 15, -50, 0.75);
expect(compressor.attack(0.5)).to.equal(0.5);

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

compressor.attack()

expect(compressor.drywet(0.5)).to.equal(0.5);
});

it('can set params', function(){

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

it might be good to test the .set method with incomplete params and ensure it sets the values appropriately

@therewasaguy
Copy link
Member

therewasaguy left a comment

src/compressor is looking really good. Just left a few comments. I will review the example separately

*/

p5.Compressor.prototype.attack = function (attack, time){
var self = this;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

The context isn't changing in these method, so we can just use this without creating the variable self.

var self = this creates a reference to this instance of the compressor.

We would typically create a reference if we were referring to this compressor in a callback function or some other context.



p5.Compressor.prototype.release = function (release, time){
var self = this;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

just use this



p5.Compressor.prototype.threshold = function (threshold, time){
var self = this;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

just use this



p5.Compressor.prototype.knee = function (knee, time){
var self = this;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

just use this

if (knee != 'undefined') {this.knee(knee);}
if (ratio != 'undefined') {this.ratio(ratio);}
if (threshold != 'undefined') {this.threshold(threshold);}
if (release != 'undefined') {this.release(release);}

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

These type checks should be typeof attack !== 'undefined' with typeof and double equals.

Might be good to test that these values can be undefined.

This seems like a good candidate for an options object, but I think the API is good for now.

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

(by the way, linting will help catch this kind of thing and suggest best practices, so maybe we should merge that PR!)



p5.Compressor.prototype.ratio = function (ratio, time){
var self = this;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

just use this


/**
* Built on Web Audio Dynamics Compressor Node
* https://www.w3.org/TR/webaudio/#the-dynamicscompressornode-interface

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

This merits a more details explanation and an example. That could be done in a future PR but let's make sure to provide some information about what a compressor is, and what all these parameters means. It is a difficult concept to explain, but an interactive example should help :)

expect(compressor.knee()).to.equal(20);
expect(compressor.ratio()).to.equal(15);
expect(compressor.threshold()).to.equal(-50);
expect(compressor.release()).to.equal(0.75);

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

it would be good to test

  • what happens when parameters are undefined
  • what happens when parameters are invalid (we should expect an error)

This comment has been minimized.

@jvntf

jvntf Jun 30, 2017

Author Contributor

should we be throwing an Error()? i think compressor.method() tests an undefined parameter (and it just returns the current value). currently passing in an invalid parameter yields the same result as undefined

self.compressor.ratio.value = ratio;
self.compressor.ratio.cancelScheduledValues(self.ac.currentTime + 0.01 + t);
self.compressor.ratio.linearRampToValueAtTime(ratio, self.ac.currentTime + 0.02 + t);
} else if (typeof ratio != 'undefined') {

This comment has been minimized.

@therewasaguy
* @class p5.Compressor
* @return {Object} Compressor Object
*/
p5.Compressor = function() {

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

Should the constructor accept the same arguments as set (and call set)?

This comment has been minimized.

@jvntf

jvntf Jun 30, 2017

Author Contributor

I don't have a problem with this! I didn't include set parameters just to match what seemed to be standard in other effects

@therewasaguy

This comment has been minimized.

Copy link
Member

therewasaguy commented Jun 29, 2017

Can you git rm examples/files/beat.mp3.asd in your next commit?

@therewasaguy therewasaguy merged commit 49f1d81 into processing:master Jul 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment