tim-janik / anklang

MIDI and Audio Synthesizer and Composer
https://anklang.testbit.eu/
Mozilla Public License 2.0
51 stars 3 forks source link

Sallen-Key Filter for BlepSynth device #10

Closed swesterfeld closed 8 months ago

swesterfeld commented 1 year ago

Here is the Sallen-Key Filter for the BlepSynth. As discussed there are three parameters, one to select the filter type, and one for the Ladder Filter Type and one for the Sallen Key Filter Type. Obviously it would be nice to hide the unnecessary parameters.

This requires a new PandaResampler (due to factor 4 oversampling), right now I hardcoded the include path to the location I checked it out, the repo is available here: https://github.com/swesterfeld/pandaresampler

The branch also has some other improvements:

tim-janik commented 8 months ago

Locally, I have added pandaresampler to my Anklang git repo now:

    EXTERNAL: pandaresampler: add swesterfeld/pandaresampler commit from 2023-02-14 10:18:45

    git submodule add --name pandaresampler https://github.com/swesterfeld/pandaresampler.git external/pandaresampler
    git -C external/pandaresampler checkout 29097dc786b6d75a9deb12e70ec8960a78f7f8f5

Having a tag in pandaresampler that the above commit could refer to would be nicer...

However, it doesn't look like Anklang can make use of that repo.

I.e. in order to use pandaresampler, there needs to be a way to use it without configure.ac (we can ship a panda-config.h in Anklang if that helps) and dependencies need to be MPL-2 compatible.

swesterfeld commented 8 months ago

PandaResampler is a header only library. You don't need to use configure at all.

Just set an include path to the lib directory and include pandaresampler.hh. Also build pandaresampler.cc. If you want to avoid building pandaresampler.cc to an object file, you can

``

define PANDA_RESAMPLER_HEADER_ONLY

``

and only include pandaresampler.hh.

The configure stuff is only for testing pandaresampler (also zita resampler is only used for tests). You can completely omit any dependencies. Effectively only hiir is required which is already bundled in the lib/ directory so you need neither dependencies nor anything outside the pandaresampler repo.

As for licenses:

PandaResampler is released under MPL-2.0, however it also contains code from hiir which is licensed under WTFPL2.

tim-janik commented 8 months ago

Here is the Sallen-Key Filter for the BlepSynth. As discussed there are three parameters, one to select the filter type, and one for the Ladder Filter Type and one for the Sallen Key Filter Type. Obviously it would be nice to hide the unnecessary parameters.

Can you extend on that, i.e. what parameter should be disabled, depending on the what value of what other parameter? Here's what I can decypher from the code:

filter_type_sensitive = true;
ladder_mode_sensitive = FILTER_TYPE_LADDER == get (pid_filter_type_);
skfilter_mode_sensitive = FILTER_TYPE_SKFILTER == get (pid_filter_type_);

Is that everything or missing something?

Note that the above could simply be combined into one setting by merging filter type and modes, pseudo code:

    ChoiceS filter_choices;
    filter_choices += { "—"_uc, "Bypass Filter" };
    filter_choices += { "LP1"_uc, "1 Pole Lowpass Ladder Filter, 6dB/Octave" };
    filter_choices += { "LP2"_uc, "2 Pole Lowpass Ladder Filter, 12dB/Octave" };
    filter_choices += { "LP3"_uc, "3 Pole Lowpass Ladder Filter, 18dB/Octave" };
    filter_choices += { "LP4"_uc, "4 Pole Lowpass Ladder Filter, 24dB/Octave" };
    filter_choices += { "LP1"_uc, "1 Pole Lowpass Sallen-Key Filter, 6dB/Octave" };
    filter_choices += { "LP2"_uc, "2 Pole Lowpass Sallen-Key Filter, 12dB/Octave" };
    filter_choices += { "LP3"_uc, "3 Pole Lowpass Sallen-Key Filter, 18dB/Octave" };
    filter_choices += { "LP4"_uc, "4 Pole Lowpass Sallen-Key Filter, 24dB/Octave" };
    filter_choices += { "LP6"_uc, "6 Pole Lowpass Sallen-Key Filter, 36dB/Octave" };
    filter_choices += { "LP8"_uc, "8 Pole Lowpass Sallen-Key Filter, 48dB/Octave" };
    filter_choices += { "BP2"_uc, "2 Pole Bandpass Sallen-Key Filter, 6dB/Octave" };
    filter_choices += { "BP4"_uc, "4 Pole Bandpass Sallen-Key Filter, 12dB/Octave" };
    filter_choices += { "BP6"_uc, "6 Pole Bandpass Sallen-Key Filter, 18dB/Octave" };
    filter_choices += { "BP8"_uc, "8 Pole Bandpass Sallen-Key Filter, 24dB/Octave" };
    filter_choices += { "HP1"_uc, "1 Pole Highpass Sallen-Key Filter, 6dB/Octave" };
    filter_choices += { "HP2"_uc, "2 Pole Highpass Sallen-Key Filter, 12dB/Octave" };
    filter_choices += { "HP3"_uc, "3 Pole Highpass Sallen-Key Filter, 18dB/Octave" };
    filter_choices += { "HP4"_uc, "4 Pole Highpass Sallen-Key Filter, 24dB/Octave" };
    filter_choices += { "HP6"_uc, "6 Pole Highpass Sallen-Key Filter, 36dB/Octave" };
    filter_choices += { "HP8"_uc, "8 Pole Highpass Sallen-Key Filter, 48dB/Octave" };
    pid_filter_mode_ = add_param ("Filter Mode", "Mode", std::move (filter_choices), 0, "", "Filter Mode to be used");

Other than that, I can add the logic to realize ladder_mode_sensitive=false, skfilter_mode_sensitive=false via insensitive UI elements.

swesterfeld commented 8 months ago

Can you extend on that, i.e. what parameter should be disabled, depending on the what value of what other parameter? Here's what I can decypher from the code:

filter_type_sensitive = true;
ladder_mode_sensitive = FILTER_TYPE_LADDER == get (pid_filter_type_);
skfilter_mode_sensitive = FILTER_TYPE_SKFILTER == get (pid_filter_type_);

Is that everything or missing something?

Yes, this is correct. It could be merged in one choice - as you say - but that would become increasingly cluttered if we choose to add more filter types later on, so although I could live with one big choice, I'd rather see it implemented as two. I also remember that we had a lengthy discussion before I submitted the PR on how to do it, and the outcome of this discussion was this per-filter-type choice implementation I submitted now.

For the sake of completeness, for automation a single choice might be a little better than what we have now.

Other than that, I can add the logic to realize ladder_mode_sensitive=false, skfilter_mode_sensitive=false via insensitive UI elements.

Insensitive is really not the best option here. It would be better if we had something like

[ <filter type> ] [ <filter mode> ]

where filter mode depends on what filter type is. In SpectMorph I have implemented this for pretty much the same filter settings. The code really doesn't make filter mode insensitive but shows/hides the corresponding property.

Changing the filter type doesn't trigger a relayout in SpectMorph because the positions of the comboboxes are in absolute pixel coordinates, which is something I would really suggest we should do as well in order to allow devices to have good control on their look and feel in Anklang.

tim-janik commented 8 months ago

Can you extend on that, i.e. what parameter should be disabled, depending on the what value of what other parameter? Here's what I can decypher from the code:

filter_type_sensitive = true;
ladder_mode_sensitive = FILTER_TYPE_LADDER == get (pid_filter_type_);
skfilter_mode_sensitive = FILTER_TYPE_SKFILTER == get (pid_filter_type_);

Is that everything or missing something?

Yes, this is correct. It could be merged in one choice - as you say - but that would become increasingly cluttered if we choose to add more filter types later on, so although I could live with one big choice, I'd rather see it implemented as two. I also remember that we had a lengthy discussion before I submitted the PR on how to do it, and the outcome of this discussion was this per-filter-type choice implementation I submitted now.

For the sake of completeness, for automation a single choice might be a little better than what we have now.

Is this desirable? Do other DAWs or Plugins do/need automation across filer types and modes in one sweep? I'd guess that automation changes across filter types probably cannot be guaranteed to be click-free.

Other than that, I can add the logic to realize ladder_mode_sensitive=false, skfilter_mode_sensitive=false via insensitive UI elements.

Insensitive is really not the best option here.

It's a start. Currently, the Anklang engine has no efficient support for parameters changing hints (which is needed here, the changes you are suggesting go beyond changing just parameter values) and for parameter changes cascading (i.e. causing other parameters to also change). So my current plan is: 1) Implement a suitable data structure that can transport cascading parameter changes from the engine to the main_thread; 2) Support hint changes throughout the UI parameter implementations; 3) Apply sensitivity to filter mode in response to filter type changes.

It would be better if we had something like

[ <filter type> ] [ <filter mode> ]

where filter mode depends on what filter type is. In SpectMorph I have implemented this for pretty much the same filter settings. The code really doesn't make filter mode insensitive but shows/hides the corresponding property.

That may be doable with the new parameter infrastructure that just landed in trunk (which probably need some UI bugs straightened out still) via dynamic ChoiceS support.

Changing the filter type doesn't trigger a relayout in SpectMorph because the positions of the comboboxes are in absolute pixel coordinates, which is something I would really suggest we should do as well in order to allow devices to have good control on their look and feel in Anklang.

Pixel perfect layout is a completely different can of worms. At the moment, there is no definitive layout area or direction that Anklang could provide as a reference for plugins, let alone layouting guarantees for size or ratio of controls corresponding to properties.

When it comes to the current layouting of the BlepSynth properties (which is by no means set in stone), a significant improvement can be had (also from a usability perspective) by just re-arranging the groups to: Osc1 Mix Osc2 VolEnv Filter FilterEnv ... But apart from that, we probably need hints and/or logic to enforce that groups with similar sets of properties like Osc1 and Osc2 are layed out identically. That is beyond the scope of this issue however, so we will address it at a later point.

swesterfeld commented 8 months ago

For the sake of completeness, for automation a single choice might be a little better than what we have now.

Is this desirable? Do other DAWs or Plugins do/need automation across filer types and modes in one sweep? I'd guess that automation changes across filter types probably cannot be guaranteed to be click-free.

I agree that automating the filter type cannot be click-free. I'd even say it could be acceptable to not support automation for the filter type at all. For the filter modes, it can be made click free at least if the underlying filter structure supports it. For instance the ladder filter we have could cross fade between its four low pass outputs (although we currently don't do that).

Other than that, I can add the logic to realize ladder_mode_sensitive=false, skfilter_mode_sensitive=false via insensitive UI elements.

Insensitive is really not the best option here.

It's a start. [...]

I agree. Its better to merge without the best possible UI than to postpone the merge for a long time. Not merging blocks new DSP enhancements on the blepsynth, so if you say you can do the insensitivity implementation in a reasonable amount of time an then merge the PR, I'm totally fine with that.

Pixel perfect layout is a completely different can of worms.

Yes. Can be solved later on.

But apart from that, we probably need hints and/or logic to enforce that groups with similar sets of properties like Osc1 and Osc2 are layed out identically. That is beyond the scope of this issue however, so we will address it at a later point.

Yes. Can be solved later on.