Pad wavetables #54

JohannesLorenz · 2020-04-04T17:09:41Z

This is a draft and basic approach to use a wheel in PAD synth to cycle through waves with different basewave parameters in realtime (those waves are what I call "table" here). So, e.g., by turning the wheel, you can smoothly turn a saw into a power wave.

Tester notes:

There is no patch for zyn fusion yet. It can be tested in FLTK.
The computation is slow (128 as usual), so using tiny buffers can be good for testing.

Review hints:

The four commits can be reviewed independently.

There are some issues:

The implementation is far from complete. In the end, I hope we can use modulators to cycle through the wavetable (though in some cases, a wheel might be preferable). But it already works right now.
There are many minor TODOs (e.g. proper interpolation between OscilGen's integer parameters).
The current implementation is just a copy of the "discrete" mode (with very few exceptions). However, the tables can also be generated for bandwidth mode (it just may take longer because you may want to use larger buffers in bandwidth mode). So, instead of adding "WaveTable" as fourth PAD synth mode, I'd rather make it a checkbox that can be applied for all current modes - If that makes sense.

No functional change, just cascading of `Sample` access. This is done because it makes reading the next commit more easy.

fundamental · 2020-05-09T19:50:28Z

There's a lot of changes towards std::vector (or similar style containers). I'd recommend against this as it does complicate looking at buffer memory placement (i.e. alignment for SSE ops), masks any reallocation, and doesn't really buy us much IMO.

I'm guessing it will be easier to read the individual commits as recommended. (I should follow up on the additional readthrough fairly soon).

JohannesLorenz · 2020-05-09T20:28:05Z

There's a lot of changes towards std::vector

Do you mean those in OscilGen or the new Sample::smp in PADnoteParameters?

i.e. alignment for SSE ops

Do you plan to use SSE and vector will make problems because it restricts memory loading? In this case, I guess we could just replace vector by our own custom container sse_vector that has the same interface as vector?

I'm guessing it will be easier to read the individual commits as recommended

Yes, that should be really be easier.

JohannesLorenz · 2020-05-09T20:33:02Z

Btw, the main reason for using std::vector in OscilGen was to not need to wrtie a copy ctor for OscilGen.

fundamental · 2020-05-16T17:39:24Z

src/Params/PADnoteParameters.cpp

- p->sample[n].size = rtosc_argument(m,0).i;
- p->sample[n].basefreq = rtosc_argument(m,1).f;
+ p->setCurSampleSize(n, rtosc_argument(m,0).i);
+ p->setCurBaseFreq(n, rtosc_argument(m,1).f);


The whole setting a sample operation is (in my mind at least) a single atomic action. As such do you think it would make sense to have a single setter, which would keep the update somewhat more atomic (not atomic in the multi-threading sense though).

fundamental · 2020-05-16T17:40:10Z

src/Params/PADnoteParameters.cpp

+ // ...take sample buffer
+ p->sample[n].smp[idx] = ((float**)blob.data)[idx];
+ }
+ //XXX TODO memory management (deallocation of current smp buffer)


I would recommend against large copies on the realtime side. Just take the pointer and send the old one back to be freed.

fundamental · 2020-05-16T17:41:42Z

src/Params/PADnoteParameters.cpp

 {
 float harmonics[synth.oscilsize];
 memset(spectrum, 0, sizeof(float) * size);
 memset(harmonics, 0, sizeof(float) * synth.oscilsize);

 //get the harmonic structure from the oscillator (I am using the frequency amplitudes, only)
- oscilgen->get(harmonics, basefreq, false);
+ oscilgen_ptr->get(harmonics, basefreq, false);


I am unclear why this change is needed.

fundamental · 2020-05-16T17:48:25Z

src/Params/PADnoteParameters.cpp

+ assert(minIdx <= oscilgen_ptr->Pbasefuncpar*Sample::num_smps_per_key + 1.0f);
+ assert(maxIdx >= oscilgen_ptr->Pbasefuncpar*Sample::num_smps_per_key - 1.0f);
+#else
+ std::size_t minIdx, maxIdx;


So, the idea with Pad Synth is to create a 2D matrix where dimension one is time, dimension two is expected sampling frequency. There's some randomization so typically you can't trivially walk between the different sampling freq rows.

I would have expected wavetable synthesis to turn this into a 3D tensor where you have timeXnote-freqXnote-parameter. It looks like instead there's just timeXnote-parameter here.

Not sure if that's a misreading or not though.

Before this commit, we had Sample sample[PAD_MAX_SAMPLES] (freqs), and each Sample had one float* smp (array for time). Now, each Sample contains std::vector<float*> smp instead of float* smp - each vector element for one wavetable-parameter. So it's indeed 3D now.

fundamental · 2020-05-16T17:52:46Z

src/Params/PADnoteParameters.h

+ // copying is deleted at all
+ Sample(const Sample& source) = delete;
+ Sample& operator=(const Sample& source) = delete;
+ Sample& operator=(Sample&& source) = default;


As long as the copy constructors are correctly handled, then I guess this is alright, though in my experience it's still slightly more error prone than a simple struct with raw pointers (and a bit harder to debug unless you have the gdb scripts to understand std::vectors all working and those tend to break at any non-trivial levels of optimization).

fundamental · 2020-05-16T17:55:22Z

src/Synth/OscilGen.h

@@ -170,10 +199,10 @@ class OscilGen:public Presets
 oldmodulationpar3;


- fft_t *basefuncFFTfreqs; //Base Function Frequencies
+ std::vector<fft_t> basefuncFFTfreqs; //Base Function Frequencies


I don't know how to attach a comment to a whole commit in this interface, but IMO this commit seems unnecessary. For the later commits, all that is needed is harmonic content given wave parameter=x, right?

Would it make sense to have a simple get_harmonics_with_param_equal_x(x)? Perhaps I'm oversimplying things.

I thought it's required because the PADnoteParameters::sample vectors can be calculated by different threads for the different frequencies (PAD synth multithreading - "If one thread wants to use wave parameter x, the other one can't use wave parameter y"). Most parts of OscilGen::get look easy to make thread-safe, but a call to prepare seems always needed due to the new basefunc parameter (needPrepare returns true if that one is changed), and I guess making prepare thread safe is as bad as copying the whole class?

fundamental · 2020-05-16T17:58:10Z

src/Synth/OscilGen.h

+ operator const FFTwrapper*() const { return fft; }
+ FFTwrapper* operator->() { return fft; }
+ const FFTwrapper* operator->() const { return fft; }
+ } fft;


I'd be really wary about this functionality. We can't have full smart pointers due to the whole multi-threaded situation and this copy would allow for a "weak pointer", which is a complexity that can bite back in unexpected ways. Also, with regards to FFTW, there are modes which assume that the library knows the address of the pointer passed to it. I don't think we're currently using one, but we may end up doing so in the future and this would complicate things as the pointer address of some of the fft_t* references would change.

fundamental · 2020-05-16T17:59:50Z

src/Params/PADnoteParameters.h

+ void setCurSampleSize(std::size_t idx, int val) {
+ sample[idx].size = val;
+ }
+ float curBaseFreq(std::size_t idx) const


Seems like we might want to force this one to inline.

We could ask the compiler to inline, but FWIR most compilers do their own thing anyways. I think in the end, some runtime tests should be done to see if there's any significant slowdown.

JohannesLorenz · 2020-10-18T13:33:13Z

For general information, this PR is likely superseded by #61.

JohannesLorenz requested a review from fundamental April 4, 2020 17:09

JohannesLorenz force-pushed the pad-wavetables branch 2 times, most recently from 0732fd5 to 5224e81 Compare April 8, 2020 19:46

JohannesLorenz added 4 commits April 13, 2020 12:09

Make OscilGen copyable

9eda36e

Getters/Setters for PADnoteParameters::Sample

b861c99

No functional change, just cascading of `Sample` access. This is done because it makes reading the next commit more easy.

Add wavetable modulation to PAD

ff3af6b

Extend FLTK UI for wavetable modulation

881233a

JohannesLorenz force-pushed the pad-wavetables branch from 5224e81 to 881233a Compare April 23, 2020 17:50

fundamental reviewed May 16, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pad wavetables #54

Pad wavetables #54

JohannesLorenz commented Apr 4, 2020 •

edited

fundamental commented May 9, 2020

JohannesLorenz commented May 9, 2020

JohannesLorenz commented May 9, 2020

fundamental May 16, 2020

fundamental May 16, 2020

fundamental May 16, 2020

fundamental May 16, 2020 •

edited

JohannesLorenz May 16, 2020

fundamental May 16, 2020

fundamental May 16, 2020

JohannesLorenz May 16, 2020

fundamental May 16, 2020

fundamental May 16, 2020

JohannesLorenz May 16, 2020

JohannesLorenz commented Oct 18, 2020

Pad wavetables #54

Are you sure you want to change the base?

Pad wavetables #54

Conversation

JohannesLorenz commented Apr 4, 2020 • edited

fundamental commented May 9, 2020

JohannesLorenz commented May 9, 2020

JohannesLorenz commented May 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fundamental May 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohannesLorenz commented Oct 18, 2020

JohannesLorenz commented Apr 4, 2020 •

edited

fundamental May 16, 2020 •

edited