processing
p5.Signal question --> remove it
in the codebase https://github.com/processing/p5.js-sound/blob/e5a58ba1e3efa6f65c60da7a4513178fffa2df32/src/signal.js#L82
shouldn't it be
p5.Signal.prototype.fade = Signal.prototype.linearRampToValueAtTime;
otherwise we are adding the fade method to the tone.js Signal API , seems bit confusing to me :confused:
hmm, this is pretty confusing! I don't see the "fade" method being used anywhere, and in fact the API is not exposed to anyone who is using the library. The documentation is inaccurate, p5.Signal classes don't exist and are probably outside the scope of what p5.sound should focus on. I guess the idea was to allow for chaining like in this example at https://p5js.org/reference/#/p5.Signal
carrier.freq( modulator.mult(-400).add(220) );
...but that could be accomplished with .scale(-1, 1, -400 + 220, 400 + 220) to map the signals.
So I think we can remove the chaining functionality, delete this file, and continue to use Tone.js signals internally for p5.Oscillator and p5.Env's .add, .mult and .scale methods
@therewasaguy 1.Deleting file of p5.Signal & erasing documentation. 2.And moving example of https://p5js.org/reference/#/p5.Signal Into example folder to show chaining
Can i make a pr with these two as my primary goal ?.
@endurance21
1.Deleting file of p5.Signal & erasing documentation.
Yes, it'd be great if you want to help make this happen!
2.And moving example of https://p5js.org/reference/#/p5.Signal Into example folder to show chaining
I think that if we delete p5.Signal, we'll lose the chaining functionality. And that's probably ok, we can just lose this example as well, or re-implement it using .scale() instead of chaining
@therewasaguy I was having second thoughts to re implement p5.signal may be as a wrapper to tone's signal interface? let me know if it is feasible :grinning:
That seems to be the thought I had in 2014, but we haven't had requests for it, so I don't think it's a feature we should add. Adding features also means more maintenance and makes the library more confusing when people look at the documentation. If anything, I think we should be removing features.
There are many places in the codebase where P5.signal is being used !! they are
-
p5.js-sound/src/amplitude.js
-
p5.js-sound/src/envelope.js
-
p5.js-sound/src/pulse.js
simply deleting the p5.signal file will make no sense here , should i replace the p5.signal with tone's signal every where ?
also if we come to
- p5.js-sound/src/pulse.js
there is a instance of p5.signalAdd , and the strange thing is that , i can't find the that class anywhere defined in the whole codebase ☹️
great find @endurance21. I think that p5.SignalAdd, as well as p5.Signal, are undefined properties of p5...yikes! That's a good thing as we don't need to check for instances of p5.Signal. If this means some functionality is broken, we should make a note of that and revive it. If it's beyond the scope of the initial PR (and something that has not been working for a while) we can make that its own issue/PR.
By the way, the screenshots are useful, but I'll find it easier to review if you can paste a link to the lines that you want to reference so that I can view the full context.
https://github.com/processing/p5.js-sound/blob/master/src/pulse.js#L109-L114
^ In this case, seems like we can replace the internal usage with a Tone Signal
we can simply replace this line , as we are removing the p5.Signal so there is no need to check for that !
- https://github.com/processing/p5.js-sound/blob/8c35cce681066e8cdab5096f46193022820a6bae/src/amplitude.js#L164
same here , we can remove this too
- https://github.com/processing/p5.js-sound/blob/8c35cce681066e8cdab5096f46193022820a6bae/src/envelope.js#L814