Better convolution reverb by generating impulse responses

[Bubobubobubobubo]

This PR is proposing the replacement of the old reverb model by a parametric reverb generator.

• The base mechanisms stays the same, with .room(x) and .size(x) being used to tweak the wet/dry and reverb size.
• New parameters are introduced to craft bespoke impulse responses:
  • .fade(x): reverb fade time in seconds.
  • .revdim(x): target lowpass filtering goal for reverb tail (in hertz).
  • .revlp(x): lowpass filter applied to incoming wet signal (in hertz).

Documentation and dynamic examples have been added to the Effects page. This is an adaptation of an Apache 2.0 work released here: https://aldel.com/reverbgen/. This work should be usable given that credit is given to the initial dev, Alan de Lespinasse.

Now opening a discussion concerning some details of the implementation. This reverb is generating new impulse responses everytime a parameter is getting tweaked, thus cutting the reverb to start with the new impulse response. This makes it unsuitable for fast algorithmic manipulation. However, this behavior is entirely customizable and we could limit reverb model update to the tweak of one or two parameters. @Froos, what is your opinion about this?

@vasilymilovidov: Now is the time to include your work on impulse responses as well 😄

[vasilymilovidov]

can you check if you also get errors like [cyclist] error: undefined is not an object (evaluating 'reverbs[orbit].duration = duration') and [cyclist] error: Cannot set properties of undefined (setting 'duration') when changing size of the reverb?

[Bubobubobubobubo]

It is likely that I forgot to declare a parameter in the reverb.js file. I already fixed most of these errors two commits ago. Take a look to see how to do so :) sorry for this!

It can also be from that chunky condition in superdough.js, where the code checks if a reverb value has changed.

[vasilymilovidov]

it looks like it was confused by reassignments or something.
like this it works without errors:

function getReverb(orbit,
                   duration = 2,
                   fade,
                   revlp,
                   revdim) {
  // If no reverb has been created for a given orbit, create one
  if (!reverbs[orbit]) {
    const ac = getAudioContext();
    const reverb = ac.createReverb(getAudioContext(),
        duration,
        fade,
        revlp,
        revdim);
    reverb.connect(getDestination());
    reverbs[orbit] = reverb;
  }

  const reverbOrbit = reverbs[orbit];

  if (
      reverbOrbit.duration !== duration ||
      reverbOrbit.fade !== fade ||
      reverbOrbit.revlp !== revlp ||
      reverbOrbit.revdim !== revdim
  ) {
    reverbOrbit.setDuration(duration, fade, revlp, revdim);
    reverbOrbit.duration = duration;
    reverbOrbit.fade = fade;
    reverbOrbit.revlp = revlp;
    reverbOrbit.revdim = revdim;
  }

  return reverbOrbit;
}

[vasilymilovidov]

I have incorporated my ir() implementation into your code. I can make a pr to yours, or wait for this to merge with main and just update my own

[felixroos]

looks & sounds good! fixed the bug + tested a bit.

This reverb is generating new impulse responses everytime a parameter is getting tweaked, thus cutting the reverb to start with the new impulse response. This makes it unsuitable for fast algorithmic manipulation. However, this behavior is entirely customizable and we could limit reverb model update to the tweak of one or two parameters. @Froos, what is your opinion about this?

before this PR, the reverb also was regenerated when size changed, so performance wise, it's not much worse, although the new algorithm might take longer, as it is more complex. generally, patterning size + any of the new params fade / revdim / revlp is not a good idea with this setup. It might help to cache impulse responses, but it might also blow up memory when many different values are used

I think the new control names should be changed to more clearly belong to the room keyword:

• fade -> roomfade|rfade
• revlp -> roomlp(f?) | rlp(f?)
• revdim -> roomdim | rdim not sure about that one, maybe change it to behave as factor of the above?

Additionally, it seems consistent to add r for reverb?

[felixroos]

maybe a simple debounce timeout could be added for the params that regenerate the ir..

[Bubobubobubobubo]

Can't really push this further atm. I can take another look to it at the end of the week if necessary.

[felixroos]

I've changed the naming as mentioned, now there is:

• room
• roomlp / rlp
• roomdim / rdim
• roomfade / rfade
• roomsize / rsize

the first alias will be the alias used by superdirt (relevant for topos @Bubobubobubobubo)
It might make sense to also add the alias r for room

The optimization of recalculations is something we can do in a separate PR. To all properties that trigger a recalc, I've added this line to the docs: When this property is changed, the reverb will be recaculated, so only change this sparsely..

This PR is technically a breaking change, as all the reverbs of existing patterns sound different now (better).
I'll do some more testing of before after of the examples to see if some of the reverb sounds off now.

What do you think?

[Bubobubobubobubo]

Sounds good! It’s also a breaking change compared to the classic SuperDirt implementation but it makes sense to update it now that the model is different.

The reverb sounds good, I guess that this will not raise any complaint given that it is a dramatic improvement over the previous one.