processing / processing-sound

Audio library for Processing built with JSyn
https://processing.org/reference/libraries/sound/
GNU Lesser General Public License v2.1
149 stars 50 forks source link

Implementing an official All Pass #78

Closed pixmusix closed 1 year ago

pixmusix commented 1 year ago

Hello.

According to the documentation there is not an official AllPass filter for processing-sound available to the user. There does appear to be some code for one in the source from 2018 used to implement other effects.

If we are interesting in creating one, I think it should be based on this object from jsyn. http://www.softsynth.com/jsyn/docs/javadocs/com/jsyn/unitgen/FilterOnePoleOneZero.html

Using the already implemented lowpass filter as a guide, I propose something like the below.

package processing.sound;

import com.jsyn.unitgen.FilterOnePoleOneZero;

import processing.core.PApplet;

public class AllPass extends Effect<FilterOnePoleOneZero> {

    public AllPass(PApplet parent) {
        super(parent);
        //init some reasonable values for a0, a1, b1.
    }

    @Override
    protected FilterOnePoleOneZero newInstance() {
        return new FilterOnePoleOneZero();
    }

    public void coeffs(float inputCoeff, float delayedInputCoeff, float delayedOutputCoeff) {
        //Some safety checks to ensure inputs are in a reasonable range.
        this.left.a0.set(inputCoeff);
        this.left.a1.set(delayedInputCoeff);
        this.left.b1.set(delayedOutputCoeff);
    }

    public void process(SoundObject input, float a0, float a1, float b1) {
        this.coeffs(a0, a1, b1);
        this.process(input);
    }
}

Would the processing-sound team be interested in an allpass filter for processing-sound?

Thank you for your time and I'm looking forward to hearing from you. Warm Regards, PixMusix.

kevinstadler commented 1 year ago

Hello,

thanks for your message, an all-pass filter would certainly be a welcome addition to the library! If you wanted to make a pull-request based on your code I'd be happy to merge it. If you're too busy to do it yourself, we will probably commit a good chunk of time to overhauling the library and adding new features over the next few months, in which case I'd be able to add your code (with attribution of course) myself.

Best, Kevin

pixmusix commented 1 year ago

Hi Kevin.

Thanks for getting back to me.

I'd be happy to write the code. I'll also make a simple example for the documentation if you like.

With Processing-Sound being a popular library I'd appreciate it if you, or someone experienced with the package, ran a second pair of eyes over my the code before it is merged. Would that be OK?

kevinstadler commented 1 year ago

That would be great if you could also add an example! No worries we'd certainly go over all the code before it's packaged up and distributed.

As you might have noticed, the library documentation on the Processing website is actually automatically generated from the Javadoc comments in the source files, so if you were feeling inspired you could already add appropriate Javadoc comments to your code, broadly following other effects classes e.g. https://github.com/processing/processing-sound/blob/master/src/processing/sound/BandPass.java

Absolutely no worries if not, as I said we have a substantial codebase overhaul coming up, where documentation would be addressed anyway.

pixmusix commented 1 year ago

Fantastic.

Docs

Ah, I was wondering what @webref was all about. That's cool! I will happily put some of those comments in for the repo. I won't be able to test it before submitting the pull request. I also noticed These htmls files here. Will my comments @webref and @webBrief automatically generate an AllPass.html or would you like me to make one?

Using a different jsyn object

Choosing musical coeffs for OnePoleOneZero was quite challenging even when I knew where to start. I found a jsyn second order allpass filter which is in my opinion a more user-friendly choice for the processing-sound library. Still very functional; less technical knowledge required. Are you happy with this change?

Advice for naming a method.

Jsyn's FilterAllPass calls its unitport "gain" which makes sense from a digital signal processing perspective. I think gain may be confused with amplitude, especially since many processing users are not expected to have background in audio. I'd like to use "drive" instead. Is it OK that I break with the jsyn documentation?

package processing.sound;

import com.jsyn.unitgen.FilterAllPass;
import processing.core.PApplet;

public class AllPass extends Effect<FilterAllPass> {

    public AllPass(PApplet parent) {
        super(parent);
    }

    @Override
    protected FilterAllPass newInstance() {
        return new FilterAllPass();
    }

    public void drive(float g) {
        this.left.gain.set(g);
        this.right.gain.set(g);
    }

    public void process(SoundObject input, float g) {
        this.drive(g);
        this.process(input);
    }
}
pixmusix commented 1 year ago

Received note that this Allpass has been merged into main. Stoked to see what people will make with it.

I'm keen to have a look at how the documentation turns out based on the files from my branch. I'm still not quite sure how that's meant to update to the website but I'm sure it will turn up soon.

Thanks again @kevinstadler for all your hard work.