Add convolution node

[piedoom]

(Draft) Implements #15. Managed to get something working and wanted to start a draft with some notes to get any early thoughts. Feel free to reject entirely if this isn't up to snuff, I'm not the best at this, and is good practice in any case! Appreciate any feedback or comments on performance.

Still to-do

• Behaving correctly when the IR sample is changed, or IR channel counts are changed.
  • The amount of buffers allocated is dependent on the IR channel count, too, which may change. May need a "maximum" channel support setting so IR channels can change at runtime without allocations.
• Cover cases for when IR sample channels and buffer output channels are not the same count
  • Mono IR + Stereo audio should yield the IR applying to both output channels instead of just the left
• Ensure correctness (or warn/error for users, if preferable) when using unusual input, output channel number combinations
  • Unreal engine seems to have a few options for downmixing/upmixing that may be useful to investigate
• More robust loading of IR sample in visual node graph example

Changes with explanation

Dependencies

This adds the fft-convolver crate as an optional dependency to firewheel-nodes.

"True Stereo"

The "true stereo" setting for convolution requires the allocation of +2x buffers compared to independent stereo. Because this option is not likely to need to change at runtime, it is a config parameter.

Mono-to-stereo convolution

One potential application of convolution is to add space to a mono audio signal using a stereo IR. I explored adding const generics for both inputs and outputs, but found this overcomplicates things, and instead that users should simply use the mono to stereo node to achieve the same result.

Other

• I added a CC0 IR asset for testing purposes
• I added .DS_Store to .gitignore for Mac developers since it was bugging me
• I added convolution nodes of various settings into the visual node graph (and grouped other mono/stereo nodes for a first pass at consistency there, just to reduce noise a bit and improve scan-ability)

Q: Would make sense to add a supertrait for the num_channels and len_frames definition since both SampleResource and SampleResourceF32 implement it? It could make blanket impls (e.g. on Vecs) a bit more concise.

[BillyDM]

Ah yeah, I suppose adding a supertrait for num_channels and len_frames makes sense.

[BillyDM]

As for wet/dry mix, I realize I should add wet/dry mix DSP in firewheel-core to make that easier. I'll get that done here soon.

(Also don't forget that you will need to delay the dry signal by the same number of frames as the latency of the convolution effect.)

[BillyDM]

Oh and actually, we might not need an enabled parameter since the user can just set the mix to 0.0 to achieve that.

[piedoom]

Thanks for the comments - it would be nice to have wet/dry in DSP core. I imagine a lot of it is similar to the crossfade node.
Totally forgot about the convolution dry mix delay, will fix that and tidy up some of the other mentioned areas!

[BillyDM]

Ok, I added a MixDSP struct which should make it easier to create a wet/dry mix!

[piedoom]

Ok, got some more progress on this.

1. IR samples can now be changed and will work properly. I added a setting in the config for maximum IR sample channels, defaulting to stereo (But users can select mono to save memory if needed).
2. Mono IRs will fill both channels in a stereo Convolution node. I figure that this is a better assumption for default behavior than just convolving the left output.
3. Switched to using the new MixDSP API
4. Removed "enabled" entirely
5. Added input audio buffer to fix dry timing issue
6. Added supertrait SampleResourceInfo and moved num_channels, len_frames methods into it. LMK if I should put this in a different PR (and also if you have a better name, idk about SampleResourceInfo to be honest)
7. Improved asset loading in the visual graph example and allowed for IR selection in the convolution nodes, including mono versions to test.

• Q: Should this also implement a declick when changing IR samples like the sample player?
• Q: I added an explicit panic if CHANNELS > 2 in the node info. Is this an acceptable way to handle behavior?
  • Otherwise we will need some sort of mixdown and mixup, assuming that most IRs are stereo, but IMO that should just be another node
  • Or we could not panic but just only ever use the first 2 channels (and maybe warn?)

[BillyDM]

Hmm, I'm not sure if declicking is necessary or not when changing IR samples. Do you notice any clicking when you change samples?

[BillyDM]

I think panicking if CHANNELS > 2 is fine.

[piedoom]

Alright, I think this is more or less ready. I added a wet amplitude to account for amplitude of convolved samples, and removed true stereo stuff.

one thing I still want to investigate is if the IR buffer can be replaced without re-initializing the convolution buffer (I believe that is why I am hearing clicking when changing samples). I think i might need to open a PR/fork the fft-convolver crate. But yeah, I don't think there is currently a way to mix between two IRs, only two convolution nodes, which isn't ideal.

[piedoom]

ack, i forgot about the pause/play function mentioned on discord for reverb - i think it is applicable here as well. will push a change with a fix.

Edit: pushed

[piedoom]

I think this should be good to go

a couple points:

1. i didnt include the IR in the conv node like you mentioned after chatting with Corvus as it would be a headache -
   makes sense I think to make some better ergonomics in seedling after this, but let me know if that is a dealbreaker.

2 - I know you mentioned something about ensuring the sample rate for the IR is correct, but not sure if that is
possible on the node since it only operates on the SampleResourceF32 trait 🤔