Improved Oscillators -- fixed

[he29-net]

This PR finishes earlier work by @curlymorphic, see discussion in #4397.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.176%2Bg06bc4f416-mingw-win32.exe (build link)

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.176%2Bg06bc4f4-linux-x86_64.AppImage (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.176%2Bg06bc4f416-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14206-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.176%2Bg06bc4f416-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14206?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14209-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.176%2Bg06bc4f4-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14209?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14207-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.176%2Bg06bc4f416-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14207?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "4ecebd5be6139734e114b1d03f304badc4dc6ef4"}

[he29-net]

So I was thinking about the various modulation modes, how can they cause aliasing even in band-limited wavetable mode, and if there is any easy fix. Sorry if some of the points and examples are (too) obvious, I'm just trying to make sure I don't miss any details and that I understand things correctly.

1. Phase modulation

At a first glance it seems fine: the modulator just says "when playing your waveform, add this offset to the location you plan to be playing". Since the waveform currently playing is appropriate for given frequency it does not cause aliasing when playing at the nominal rate.

However, this never happens in practice, since the offset itself likely changes relatively fast, the modulator is not static. For example, if the modulator contains an increasing slope, the modulated oscillator essentially skips through the waveform at a faster rate. This results in a tone with a higher frequency -- and also in aliasing, because the selected wavetable is not appropriate for the increased frequency. Another way to think about it is that the phase shift causes the playback position to skip ahead -- skipping some samples means lowering sampling rate of the one-cycle waveform, resulting in aliasing.

I don't see any easy and reliable fix. But perhaps it would be possible to measure the slope in the signal of the modulator, estimate its effect on frequency of the modulated signal, and select appropriate band-limited table for the effective frequency. But it's hard to say how reliable and robust that would be in practice. I.e., what happens when the modulator is just white noise?

2. Amplitude modulation

Deceptively simple, but possibly even harder to fix. Consider that an oscillator generates a slow triangle at 100 Hz, and there is a modulator that generates a saw wave at 10 kHz. From the point of view of the modulator, the triangle wave is essentially a DC signal, and the AM modulation simply imprints the 10 kHz saw wave into the slow moving triangle wave, adding a lot of high frequency content that will be "misinterpreted" by the sparse 100 Hz sampling.

One possible solution I can think of would be to actually keep using WT mode for AM modulators. In one of the last commits I disabled WT mode on oscillators that are used as PM/AM/FM modulators, so that the control signal is not "disturbed" by the ringing. I.e. when you do frequency modulation using saw wave, you expect the frequency to gradually rise and then switch to a different frequency immediately. You don't want the frequency to jump around near the transition because of ringing caused by the limited bandwidth.

BUT, perhaps WT mode on AM modulators could be used to prevent aliasing: if the modulator signal itself uses a wavetable with frequency content limited to the same number of bands that the modulated signal uses, whatever AM "imprints" into the waveform of the modulated signal will have limited frequency content. I assume it would not be perfect and the interaction of both signals could still create some high frequency content, but by limiting the frequency content brought by the modulator the result could at least be "less bad".

3. Frequency modulation

Looking at the implementation, it is very similar to PM to a large extent. Instead of saying what offset to add to the playback position, the modulator just says by how much to increase the phase / offset for each sample. So there is no guesswork: it should be relatively easy to use the modulator value to figure out the new effective frequency, and pick a suitable wavetable that won't cause aliasing (as suggested by Johanness earlier).

---

Frankly I expected frequency modulation to be the biggest problem, but now it seems to me that AM is the one with the most potential for unpredictable behavior.

While it is tempting to try figuring it all out, add the fixes to this PR and make (almost) all aliasing problems in TripleOscillator a thing of the past, I'm worried that it turns out more complex than it seems and it will further delay this PR (and therefore block the refactor). I did not even consider various combinations yet: imagine for example WT OSC1 phase modulated by standard OSC2, which is itself AM modulated by WT OSC3; OSC2 modulator signal therefore "inherits" ringing from OSC3, potentially causing problems when modulating OSC1. Feels like this could get very crazy very quickly (or maybe I'm just too deep in the tunnel and overthinking it).

So I would say it is probably better to just wrap this PR up, cross it off the list and continue with further improvements in a new PR.

[JohannesLorenz]

So for FM, I guess we both agree.

For AM, you brought a good example, and I also could imagine that using WT in the modulator just does what we want. We could try it out with your example.

For PM, as you say, it's also modulating the frequency by skipping through the table at a faster rate (the picture below shows a sine modulating a sine). I assume we could pick the right table, since the rate of skipping through the table effectively gives you the frequency.

---

Delaying to another PR? I'd agree:

I imagine that FM/PM could be fixed easily by us. Though I also think they are buggily implemented already (as e.g. the volume knob in that song you showed me), so adding a further bug does not harm. The AM example is (probably) fixable by the user picking "WT" for the modulator (but we could test that now with your example, testing of this PR has not been done anyways).

[he29-net]

I just tested the AM case with a similar setup I proposed: first oscillator is detuned -24 semitones and generates a slow triangle wave, and is AM modulated by the second one, which is detuned +12 semitones and generates a fast saw wave (saw has a lot of frequency content so it should be a good "benchmark"). LMMS runs at 96k sample rate so it is easier to see if there is aliasing (otherwise it could be confused with valid frequency content).

The results pretty much speak for themselves:

There is still frequency content above 20 kHz when WT mode is enabled for the modulator, but compared to the first case (which has massive aliasing even with Nyquist limit at 48 kHz!) it is a night and day difference.

So I re-enabled the use of band-limited signals by oscillators operating as AM modulators since avoiding them in this case was clearly a mistake.

[JohannesLorenz]

While the difference is very clear in the spectrogram, before now I did never hear any difference. Now I found a way: Use a single saw wave in 3osc, and add a GLAME Butterworth Lowpass filter at ~80 Hz. Without WT, you'll hear something like noise in the bass frequencies. Without LP, however, the correct harmonics of the saw are often so loud that you almost don't hear that aliasing noise.

About the question whether we should enable AA by default... Should we just delay the decision? Maybe we get more feedback from users after merging.

[he29-net]

Try some higher tones; when you play notes around C4 or C5, the harmonics that are reflecting down are pretty weak and hard to notice. But when you play higher notes, it can get pretty nasty on its own, no filters needed:
https://user-images.githubusercontent.com/20932751/124183474-91ce8300-dab8-11eb-804b-4679978fb800.mp4
(triangle, MoogSaw +12 detune, saw +24 detune, plus some small fine detune values).

I think enabling it by default would be a good idea. It does not only avoid the issues that are clearly audible, it should also help with "creeping issues" that appear as more instruments are added: all the aliasing noise from each instance gradually adds up, and in the end the signal to noise ratio will be an absolute potato. In mixing terms, it would be probably called "muddy"? Though that's probably a term used when it happens in low-ish frequencies. But it should be undesirable at all ranges..

Musicians coming from technical background may understand what is happening, but people who open LMMS for the first time and start playing with the default synthesizer may not even realize something is wrong. Or perhaps they do, in which case they may think "When I did the same in FL demo yesterday, it sounded nicer. I guess I shouldn't expect free SW to be as good.." and go pirate FL instead.

So while this will not fix everything (e.g., open new project, open TripleOsc, switch both mixing modes to FM, try holding random keys and be amazed) and has some downsides (a bit lower performance), I think we should strive for the best first impression and default settings, and this seems like something that should help with that. (Along with having envelopes turned on by default: the clicking is another embarrassing "default technical glitch". But I think that was already discussed before, somewhere.)

[he29-net]

Also, for the GUI appearance: maybe the setting could be inverted? I.e. "go back to legacy oscillators"? So that the orange WT button does not glow all the time as if it was something extra and worth the user's attention (when it is in fact "just the ordinary default"). Or perhaps make it black text (enabled) and gray text (disabled), to make it less "in your face"?

That could be reworked along with the change to AA or HQ (still not sure which one of these two would be better, considering that we have a plan to fix PM/FM to make it "really HQ").

[JohannesLorenz]

Wow, OK, for such high notes, it partially even sounds pitched without WT (like with Microtuner). I agree using it as default, however, how do we want to treat old savefiles? (Remember how it changed a sound in one of the songs we tried?)

Per the UI, I have really no preferences. I don't know a good abbreviation for "not hq". Well, "LF" for "Low-Fi" is too unusual. Maybe "LO". "SD" for "Standard Definition" sounds wrong, too (aliasing can't be called "standard"). Maybe we'll have more ideas in the next days.

[he29-net]

[...] however, how do we want to treat old savefiles? (Remember how it changed a sound in one of the songs we tried?)

I would say the original settings should be definitely preserved in old projects. Even if it may be an improvement in most cases, it would still be a silent change to people's projects that were made and tweaked and mixed with the aliasing present, possibly breaking them in subtle ways.

So if the tripleoscillator element does not contain any value for useWaveTable or what's it called, WT mode should be kept disabled. But a new TripleOsc instance and the default instance in a new project should have WT mode enabled to promote its use.

Presets should probably also "stay bad", since we cannot say what was the original intention (maybe some of them should sound nasty?). Although I'm willing to be convinced otherwise.. :)) Someone more experienced with music production could perhaps later make a PR that enables WT on suitable presets.

[JohannesLorenz]

I would say the original settings should be definitely preserved in old projects.

I think you've just won a ticket for coding that WT is activated by default for new 3oscs, but not for old 3oscs or old projects. Or you delay it to another PR, as you prefer.

[Spekular]

I would say the original settings should be definitely preserved in old projects.

I think you've just won a ticket for coding that WT is activated by default for new 3oscs, but not for old 3oscs or old projects.

That's why we have DataFile upgrade methods :)

[he29-net]

I was hoping that an upgrade method won't be needed, but from what I tried yesterday, it seems I understood the project loading incorrectly. I expected that a parameter not found in the XML file will get the default value for given type of model (false in this case), but instead it simply keeps whatever the default value was on the instrument whose model is being loaded. So yeah, new upgrade method it is..

[he29-net]

Band-limited waveforms are now used by default. I had a small problem with the default template, since it is reported as version 1.2.0; that means the upgrade routine runs for it as well and overwrites the new default. I thought about just updating the template, but there was a lot of things from other upgrade methods that I did not understand, and I also did not want to base the template on the crazy 1.3.0-commit@$$%$@$^-type identifier. So I just check in the upgrade routine if the attribute already exists and avoid touching it if it does.

I also modified the icon, using HQ instead of WT and removing the color to make it stand out less when enabled (since it is there just for compatibility reasons, it is not supposed to be a prominent control element). I tried to emulate the "5x5 pixel font" used to describe the knobs.

[JohannesLorenz]

The new design looks better to me, it makes clearer that "HQ" is the normal case. Hopefully, users won't think: "Wow, there's a new feature to make TripleOscillator sound more oldschool" 😃

[JohannesLorenz]

I think it's time for testing review.

An incomplete list of things to test:

1. Loading an old savefile
2. Loading a new savefile (both HQ and non-HQ must be restored)
3. Are all the waves correct in e.g. audacity? Are the phases correct, too?
4. Does the user defined table work? E.g. is a square wave shaped correctly?

[JohannesLorenz]

I just checked all your code changes after last review.

[JohannesLorenz]

I'm not sure if it is necessary to make this a class variable. Currently, for 3osc, it is enough, because in 3osc, an oscillator can only be either modulating or be audio. However, e.g. in zyn it can be both.

You could add the parameter directly to getSample, I guess?

[he29-net]

I made it a member variable because it was much simpler and a smaller change. To pass it as an argument to getSample, I would first need to propagate it from update() through each updateXX() and all the calls to different shape specializations. That's like 11 lines to change only for PM and there is 6 updateXX() variants in total, so 66 changes just to pass the parameter. Compared to one line.

Thinking back, I assume that may be also the reason why m_freq exists; if everything that defines a "global state" of the oscillator at the time of update was passed as an argument, the argument list would never end. Although I'm not entirely opposed to passing the bool modulator as a parameter if you think it is better or easier to follow.

[JohannesLorenz]

Hmm. Good point with the m_freq comparison, that might be the reason. In zyn, we just pass m_freq through all the sub functions, but the number of functions is much higher here due to template magic.

What about adding a comment above the m_isModulator delcaration, which states that it's only a class variable in order to have less function parameters, and that it can be easily converted into a function parameter if needed?

[he29-net]

Good idea with the comment; added.

[JohannesLorenz]

Testing review finished, no complaints.

[JohannesLorenz]

@he29-net Great! We have code and test review finished. So it's OK from my side, and others have had enough time to complain 😃

I think we can squash+merge this. Do you mind writing a commit message for the squashed commit? (Or you can do it yourself, not sure if you have those permissions)

[he29-net]

@JohannesLorenz All right, thanks for the review! The PR turned out to be a bit more complex than fixing one or two memory issues. :)

As for the commit message, perhaps something like:
Add a band-limited, alias-free wavetable oscillator option to the Oscillator class. Use it by default for Triple Oscillator. ? I don't have a write access, so you can push the button. :))