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

Effect.js and modified Filter.js #173

Merged
merged 2 commits into from Jun 11, 2017

Conversation

Projects
None yet
3 participants
@jvntf
Copy link
Contributor

jvntf commented Jun 1, 2017

No description provided.

*/
p5.Filter.prototype.disconnect = function() {
this.output.disconnect();
};

p5.Filter.prototype.dispose = function() {
// remove reference from soundArray

This comment has been minimized.

@therewasaguy

therewasaguy Jun 1, 2017

Member

p5.Filter's dispose method can call p5.Effect.prototype.dispose.apply(this) and then just dispose of the pieces that are unique to this class.

For methods that extend superclass methods and have arguments, you can also pass in the arguments object like this:

p5.Effect.prototype.method.apply(this, arguments)

This comment has been minimized.

@jvntf

jvntf Jun 3, 2017

Author Contributor

cool! i wasn't sure if I could add to the Effect.prototype.dispose in objects that extended it.

*/

p5.Filter = function (type) {
p5.Effect.call(this, 'Filter');

This comment has been minimized.

@therewasaguy

therewasaguy Jun 1, 2017

Member

I don't think we do anything with this 'Filter' argument, so it can probably be removed?

Also, a convention from other modules is to explicitly require dependencies. So we'd require p5 Effect at the top of this file, like var Effect = require('effect') and then refer to it as Effect within this module. That way this module isn't making any assumptions about whether p5.Effect already exists. With that approach we can remove `require('effect') from src/app.js as it'll get required by the modules that actually use it. I think the grunt task will result in similar output either way. Do you think that's a good approach?

@therewasaguy
Copy link
Member

therewasaguy left a comment

@jvntf this is looking really good! Just a few small comments.

@@ -2,6 +2,7 @@ define(function (require) {
'use strict';

var p5sound = require('master');
var Effect = require('effect');

This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

You can change all "p5.Effect" in this module to "Effect" (once you add "return p5.Effect" to the Effect module)

this.output = undefined;
};


This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

return p5.Effect; here so that Effect can be explicitly required by other modules

this.input = undefined;

this.output.disconnect();
this.output = undefined;

This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

don't forget to dispose of the wet/dry nodes here (or remove them from the constructor until we're ready to use them in a future PR)

It might also be good to clear the reference to the audio context (this.ac = undefined)

*/

p5.Filter = function (type) {
p5.Effect.call(this);
this.biquad = this.ac.createBiquadFilter();

This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

check the spacing on line 81 (but really I should add linting to do this automatically)



// Map mouseX to a the cutoff frequency for our lowpass filter
filterFreq = map (mouseX, 0, width, 10, 22050);

This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

we could add constrain(0, 22050) to get rid of a warning message in the console when we try to set frequency above 22050


//filter = new p5.LowPass();

//filter = new p5.Filter();

This comment has been minimized.

@therewasaguy

therewasaguy Jun 4, 2017

Member

looks like unnecessary comments?

@jvntf

This comment has been minimized.

Copy link
Contributor Author

jvntf commented Jun 5, 2017

I have implemented the wet/dry feature via ToneJS CrossFade as well as a 'chain' feature in p5.effect. Added comments as well. Note: addition of wet/dry changes the way that effects are written-> effects that inherit from Effect.js must add a new processing node and connect nodes as follows: this.input -> this.some_effect -> this.wet.

this.input.disconnect();
this.input = undefined;
this.output.disconnect();
this.output = undefined;

This comment has been minimized.

@GoToLoop

GoToLoop Jun 5, 2017

Maybe you should consider using operator delete in place of assigning undefined to a property:
https://developer.Mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete

this.input.disconnect();
//this.input = undefined;
delete this.input

This comment has been minimized.

@therewasaguy

therewasaguy Jun 6, 2017

Member

Good thinking, @GoToLoop. I was under the impression that assigning to undefined was the best way to remove references for garbage collection. But this article and this one seem to suggest that assigning to null (rather than undefined) is the way to go?

This comment has been minimized.

@GoToLoop

GoToLoop Jun 6, 2017

  • Assigning undefined or null, and even assigning anything else to the property, is fully enough for GC purposes.
  • Operator delete is for removing the property itself from its object as well.
  • If we execute this.input = undefined; or this.input = null;, and then console.log('input' in this);, it's still gonna print true!
  • Only for when we delete this.input;, console.log('input' in this); is gonna print false.
  • Therefore for 100% purge, use operator delete rather than simply assigning undefined or null to the property. ;-)

This comment has been minimized.

@GoToLoop

GoToLoop Jun 6, 2017

  • Just did a quick reading on the 2 articles you've linked to.
  • They're very old. And they still had fears about the very old IE's non-compliant behaviors.
  • Nowadays most of those glitches are gone, especially when activating "use strict";.
  • The rule is simple: if there's only 1 property referencing an object, if we reassign that property w/ something else, or outright delete the property itself, the now unreferenced object is gonna be collected by the GC eventually; very much akin on how Java's GC does. @:-D

This comment has been minimized.

@therewasaguy

therewasaguy Jun 9, 2017

Member

ah, thanks @GoToLoop ! Sounds like we should use delete for all dispose methods

@jvntf jvntf force-pushed the jvntf:pull_request branch 3 times, most recently from ee62eff to 5df5d3c Jun 6, 2017

@jvntf jvntf force-pushed the jvntf:pull_request branch from 5df5d3c to 21311f5 Jun 9, 2017

@therewasaguy therewasaguy merged commit 9a5f5c3 into processing:master Jun 11, 2017

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