Skip to content
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

Effects w inheritance #178

Merged
merged 4 commits into from
Jun 20, 2017
Merged

Conversation

jvntf
Copy link
Contributor

@jvntf jvntf commented Jun 13, 2017

done: delay, reverb, distortion

@therewasaguy
Copy link
Member

this is looking good! Is it still in progress?

@jvntf jvntf changed the title Effects w inheritance (WIP) Effects w inheritance Jun 20, 2017
@jvntf
Copy link
Contributor Author

jvntf commented Jun 20, 2017

not in progress! But I have used continued to use disconnect() and = 'undefined' in .dispose() methods. I can update the dispose methods for the whole library to use delete in one PR

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, I'm going to merge this but I left a comment about documentation—let's figure out a plan to preserve documentation for children of p5.Effect so that users know they have amp connect disconnect drywet dispose and chain methods

@@ -1,4 +1,4 @@
/*! p5.sound.js v0.3.3 2017-06-06 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the next PR, let's hold off on adding the lib to commits—it'll make it easier to review because we can just focus on the src/ and test/ files.


this.input = this.ac.createGain();
this.output = this.ac.createGain();
p5.Effect.call(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5.Effect works because of the order of app.js, but it's safer not to depend on that and to instead require dependencies explicitly. Try to use Effect, since that's what we required on line 6. Same goes for Filter, if you feel like cleaning up the lines that refer to p5.Filter (if you don't get to it in this PR, I will be happy to)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I will merge and make this change in a separate PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, p5.Filter module needed to return/export the p5.Filter...

this.input.disconnect();
this.output.disconnect();
Effect.prototype.dispose.apply(this);

this._split.disconnect();
this._leftFilter.disconnect();
this._rightFilter.disconnect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are p5.Filters, so we should call dispose instead of disconnect to dispose of all its resources/references

this._leftFilter.dispose();
this._rightFilter.dispose();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make this change

// * @method connect
// * @param {Object} unit
// */
// p5.Distortion.prototype.connect = function(unit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we should probably keep documentation like this within each class/module, even though the methods are covered in p5.Effect module. These documentation blocks determine what shows up on https://p5js.org/reference/#/p5.Distortion methods section.

Another way to handle it might be to create documentation for p5.Effect, and then each class that inherits from p5.Effect includes a link in the documentation saying "inherits methods from p5.Effect".

};

/**
* Send output to a p5.sound or web audio object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more documentation that we want to keep accessible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants