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

EQ.js #183

Merged
merged 61 commits into from Aug 3, 2017

Conversation

Projects
None yet
2 participants
@jvntf
Copy link
Contributor

jvntf commented Jun 17, 2017

No description provided.

@therewasaguy
Copy link
Member

therewasaguy left a comment

Looks like this is off to a nice start, and I love where the example is going! Left some comments, lots to discuss.

if (this.ind == 0 || this.ind == 7){
mouseOnHandle(this.x,this.y) ? (this.y=mouseY, makeAdjustment(this.ind,this.y,this.x)) : true;
} else{
mouseOnHandle(this.x,this.y) ? (this.x=mouseX, this.y=mouseY, makeAdjustment(this.ind,this.y,this.x)) : true;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

@jvntf as we were talking about earlier, to address the issue where the mouse moves too fast, it might help to store a variable currentIndex which represents the control point that is currently being dragged. You can set that on mousePressed(), and then set it to null on mouseReleased().

Then, in draw(), you could move() only the current control point, outside of the for loop

if (currentIndex) {
  ctrlPts[currentIndex].move()
}

This way, even if the mouse moves faster than the circle, you'll still move the circle.

}

this.move = function () {
if (this.ind == 0 || this.ind == 7){

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

I wonder if ControlPoints should have a type as a property. That might make the example easier to understand, rather than inferring the type based on the index

src/eq.js Outdated
p5.EQ = function(_eqsize) {

//default size is 8 band EQ
_eqsize = _eqsize || 8;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

I think 3-bands might be an easier starting point. I've found that many of our users know terms like "bass" and "treble" but don't know how these translate to hz in the frequency spectrum. Whereas power users do, and could change the default to make the most of their EQ bands.

Do you know if a 3-band eq would be able to work the same way as an 8-band, 10-band or 31-band, given the way we're dividing up the frequency spectrum?

This comment has been minimized.

@jvntf

jvntf Jun 21, 2017

Author Contributor

this is a great point! I think the second part also addresses your next comment re: the approach to calculating frequencies and and Q values-- The values I used were relatively arbitrary just to get visual results for testing. Will do some research about standards / default values.

'distortion': 'src/distortion',
'compressor': 'src/compressor',

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

I'll ignore the compressor bits for now and address them in the Compressor PR. In the future, try to keep these commits separate and it'll make it easier to review.

This comment has been minimized.

@jvntf

jvntf Jun 21, 2017

Author Contributor

sorry about this! must have merged a branch by accident-- will commit this compresser stuff out of here

src/eq.js Outdated
this.bands.push(this.ac.createBiquadFilter());
this.bands[i].type = 'peaking';
this.bands[i].frequency.value = i*3150;
this.bands[i].Q.value = 5;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

How did you determine this approach to calculating eq frequencies?

I'm not an expert, but my understanding is that EQs typically divide the frequency spectrum into octave bands (which double in frequency), or some division of octaves (1/3 octave bands are popular). For example, p5.FFT has the getOctaveBands method that defaults to 1/3 octaves https://github.com/processing/p5.js-sound/blob/master/src/fft.js#L590 and this might be helpful in the visualization example. Dividing frequency spectrum into 1/n octave bands gets a result that fits the way we perceive frequency better than linear divisions.

Also curious about the type and Q value.

src/eq.js Outdated
* @{param} param2 {number} Set the Q value (resonance) of a band w/usage: "mod"
* @{param} param1 {string} Set the type of the band filter w/ usage: "type"
*/
p5.EQ.prototype.setBand = function (band, option, param1, param2) {

This comment has been minimized.

@therewasaguy

therewasaguy Jun 20, 2017

Member

Let's discuss the API for p5.EQ.

What are the things we need to be able to do for each band?

  • get/set band frequency
  • get/set band gain
  • get/set band type
  • get/set band Q / res
  • toggle band bypass

All of these are covered by p5.Filter. It would be nice if we could use that API rather than inventing (and asking users to learn) a different set of methods.

There could be a method called band(index) that returns the filter at that index. Then we could follow the p5.Filter API

eq.band(2).freq(300)
eq.band(2).freq() --> returns 300

This comment has been minimized.

@jvntf

jvntf Jun 21, 2017

Author Contributor

I do agree that EQ should do as little re-implementation as possible-- The problem that I have run into with using p5.Filter to control the bands is that the are the BiquadFilters are wrapped in Gain nodes (due to p5.Effect and the input / output standard format of p5.js-sound.

All these issues extra Gain nodes seem to affect the accuracy of the EQ (boosting the overall volume, making it impossible to filter out highs...etc). I think this problem could be addressed either with different methods or by using the design you have indicated above, but with methods such as .freq() implemented in p5.EQ(because they have to work on the raw biquad filter). what do you think?

also, I think these inaccuracies are also present if .amp() was used for individual bands of the EQ, because this method modifies the output.gain.value of any given effect.

This comment has been minimized.

@therewasaguy

therewasaguy Jun 22, 2017

Member

Hmm...

I wonder if these inaccuracies are something that could (should?) be fixed within p5.Filter.

Another approach might be to modify/extend p5.Filter after importing it into the p5.EQ module. I assume you already you try disconnect() to disconnect it from master? Maybe there is some additional patching that could be done, like setting filter.input and filter.output to the biquadFilternNode and deleting the gain nodes.

And/or, could it be worth abstracting out a p5.SimpleFilter class that gives an API that both p5.EQ and p5.Filter could inherit?

Just brainstorming...

This comment has been minimized.

@jvntf

jvntf Jun 28, 2017

Author Contributor

we could remove the gain nodes from p5.Filters-- but ultimately the difference in the API is that users would modify bands of the EQ either by

  • eq.bands[i].set() or .freq() or .res() <- the established filter API
    or
  • eq.setBand(i,"mod",-40) <- to change the gain of the ith band to -40
    to me, it seems more straightforward to use a new .setBand() method from the highest level, rather than requiring users to access elements of the array of band filters. It might still be good to use p5.Filters internally, just to make things clearer for anyone looking at the library.

one more thing to discuss, is whether bands should be ordered from 1 - eqSize (more intuitive) or 0 - [eqSize-1] (to be consistent with array storage)

This comment has been minimized.

@therewasaguy

therewasaguy Jun 29, 2017

Member

If filters are accessed directly via the array, it would make sense to be consistent with array storage. I'm still leaning towards something like this, but we can chat more tomorrow.

@jvntf jvntf force-pushed the jvntf:eq branch from 741c33d to a8c3e21 Jun 26, 2017

@therewasaguy
Copy link
Member

therewasaguy left a comment

this is so close...!

src/eq.js Outdated
Effect.prototype.dispose.apply(this);

while (this.bands.length > 0) {
delete this.bands.pop();

This comment has been minimized.

@therewasaguy

therewasaguy Jul 18, 2017

Member

Be sure to call disconnect on each of the bands' biquadFilterNodes, in addition to removing all variable references, to free up the resources

src/eq.js Outdated
freq = 160;
res = .1;
} else {
freq = this.bands[i-1].freq * factor;

This comment has been minimized.

@therewasaguy

therewasaguy Jul 18, 2017

Member

this line was returning NaN, but calling the freq() function does the trick—nice!

freq = this.bands[i-1].freq() * factor;


for (var i = 0; i < eqSize; i++) {
cntrlPts[i] = new CntrlPt(i, (width/(eqSize-1)) * i, height/2);
eq.bands[i].freq(map(cntrlPts[i].x, 0, width, 160, 20480));

This comment has been minimized.

@therewasaguy

therewasaguy Jul 18, 2017

Member

I think this would be a stronger example if you used the default EQ positions—highlight the exponential scaling that you worked on.

Some nice to haves

  • Labels for the nodes showing their frequency and gain
  • It'd be really cool to scale the frequency spectrum logarithmically. There's so much good stuff at the lower end of the frequency spectrum, every octave gets equal number of pixels on the screen that gives the user more room to play with meaningful frequencies. Here's an attempt I made at log/lin toggle for FFT visualization, in case it's helpful
@jvntf

This comment has been minimized.

Copy link
Contributor Author

jvntf commented Jul 24, 2017

@therewasaguy finished the example for EQ, and made some changes to the way the default spectrum is laid out

@therewasaguy
Copy link
Member

therewasaguy left a comment

I think this is pretty much good to go, though I left a few small comments. Mostly I think we should do a round of documentation before releasing




expect(eq.bands[0].freq()).to.equal(160);

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

this is throwing an error, should it be 100 or 160?


it('can be created and disposed', function() {
var eq = new p5.EQ();
console.log(eq);

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

good practice to remove these console.log's

src/eq.js Outdated
// for (var i = 1; i < _eqsize-1; i++) {
// this.bands[i] = this._newBand(344 * Math.pow(2, i-1), .1);
// this.bands[i-1].biquad.connect(this.bands[i].biquad);
// }

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

can we delete this comment?

src/eq.js Outdated
* @example
* <div><code>
*
* </code></div>

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

this results in an empty example in the documentation

screenshot 2017-07-24 23 47 13

src/eq.js Outdated
*
*/

p5.EQ.prototype.addBand = function(freq, res) {

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

Do we need this method? Or do we need a removeBand method, too?

This is the only documented method for this class, so providing an example and documentation for the bands property will go a long way.

src/eq.js Outdated
_eqsize == 3 ? factor = Math.pow(2,3) : factor = 2;

//bands are stored in an array, index 0 - 3 or 0 - 7
this.bands = [];

This comment has been minimized.

@therewasaguy

therewasaguy Jul 25, 2017

Member

This needs documentation. "An array of modified p5.Filter objects," highlight their freq and gain methods, and provide a couple examples?

src/eq.js Outdated
* @param {Number} [freq6] Frequency value for band with index 6
* @param {Number} [gain6] Gain value for band with index 6
* @param {Number} [freq7] Frequency value for band with index 7
* @param {Number} [gain7] Gain value for band with index 7

This comment has been minimized.

@therewasaguy

therewasaguy Jul 28, 2017

Member

16 parameters is a bit unwieldy, it seems like it would be hard to keep track of which value goes where

src/eq.js Outdated
/**
* The p5.EQ is built with abstracted p5.Filter objects.
* To modify any bands, use methods of the p5.Filter API.
* To add or remove bands, use p5.EQ.addBand() and p5.EQ.removeBand().

This comment has been minimized.

@therewasaguy

therewasaguy Jul 28, 2017

Member

addBand and removeBand aren't implemented

src/eq.js Outdated
delete newFilter.output;
delete newFilter._drywet;
delete newFilter.wet;
return newFilter;

This comment has been minimized.

@therewasaguy

therewasaguy Jul 28, 2017

Member

I think we should overwrite the amp and dispose methods here if the filter should really have the same API. amp (and any other methods that we don't want/need) could console.warn a message to the developer that this method doesn't apply to EQ filters

src/eq.js Outdated
Effect.prototype.dispose.apply(this);

while (this.bands.length > 0) {
delete this.bands.pop().biquad.disconnect();

This comment has been minimized.

@therewasaguy

therewasaguy Jul 28, 2017

Member

nice workaround for the dispose method. I wonder if dispose method could be overwritten (above) to make it useable here in keeping with the Filter API. Maybe that's not necessary as the user wouldn't need to dispose of individual EQ filters...

@therewasaguy

This comment has been minimized.

Copy link
Member

therewasaguy commented Jul 28, 2017

@jvntf I'm working on a PR to your branch with a couple of the small tweaks I just mentioned in review

update eq.js
- create eqFilter class with a proper dispose method and overrides of invalid methods
- comment out the set method
- update comments

@therewasaguy therewasaguy force-pushed the processing:master branch from cd47e8f to ba48038 Jul 28, 2017

@jvntf

This comment has been minimized.

Copy link
Contributor Author

jvntf commented Jul 31, 2017

@therewasaguy cool great idea! i added a few more override methods to eqFilter

@therewasaguy therewasaguy merged commit 06022c9 into processing:master Aug 3, 2017

@therewasaguy

This comment has been minimized.

Copy link
Member

therewasaguy commented Aug 3, 2017

great work, @jvntf!

therewasaguy added a commit to therewasaguy/p5.js-sound that referenced this pull request Aug 15, 2017

add p5.EQ (processing#183)
* started writing compressor.js

* rough structure of compressor

* draft of compressor

* updated gruntfile, added restrictions and error logging to compressor

* removed range error checking

* compressor test

* started draft of EQ

* assigned bands to EQ

* added band mod functions to eq

* added to gruntfile

* eq adjusting band levels based on filter.output.amp()

* eq works based on p5.Filter.biquad.gain.value

* added control points to eq example

* spline curve in example, next: toggle band

* using p5.Filter for bands makes EQ innacurate; handled by deleting output nodes of each filter

* much better results using a raw biquad filter

* added comments to eq.js

* improved example

* toggle works

* added example to eq.js

* fixed the example

* reverted to pre-merge state

* eq changed to have 2 sizes, 3 and 8 band

* finished EQ with options of 3 or 8 bands

* started secondary eq example

* dj mixer working but EQ is connected wrong

* dj mixer example complete with EQs

* ToneJs is not getting imported correctly here

* added tests for eq

* finished both examples and cleaned up

* added doc

* refactored EQ to use a lightweight p5.Filter

* added .gain() to p5.Filter, allows control of Biquad gain attribute

* cleaned up

* added return p5.EQ

* fixed preoload() problem in eq_dj_mixer example

* added a toggle function to p5.Filter()

* reset this._untoggledType when calling .setType()

* corrected some variable name inconsistencies

* updated eq tests to use correct API, corrected some errors

* added .addBand() and filter disconnection on dispose

* changed .freq to .freq()

* draw FFT logarithmically

* added freq and gain labels to eq cntrl points

* adjusted labels

* added to index.html

* added EQ.set() and fixed test

* eslint --fix src/eq.js

* update eq.js

- create eqFilter class with a proper dispose method and overrides of invalid methods
- comment out the set method
- update comments

* added more override methods tp EQFilter.js

* added a few methods to eqFilter

* added inline example to EQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment