Support for alternative tunings and keyboard mappings

[he29-net]

Hi! This PR adds support for microtonal / xenharmonic scales and alternative keyboard mappings to the LMMS native instruments, as suggested in #1387.

This PR is now pretty much finished and waiting for testing and review. Some of the changes were already reviewed by Veratil in #5349, so it can be used as a reference if needed (mainly concerns the MIDI code, presets and upgrade code).

I might still tweak some things here or there if I find notice some rough edges, but I don't plan to make any big changes or implement more functionality (like MIDI retuning) -- this PR is large enough as it is.

Summary of changes / new features:

• key range is extended to cover MIDI specification (as discussed in Extend LMMS note range to match MIDI specification #5349; fixes MIDI-based instruments play an octave too low by default #1857);
• added a Microtuner Configuration widget used to edit and manage scales / tunings and key mappings;
• available scales and keymaps are stored in the Song class;
• added a Microtuner class to the InstrumentTrack class (used to translate key numbers to frequency based on scale and keymap assigned to the instrument; in the future will be also likely used to generate MIDI retune messages);
• added a microtuner group box to the misc. instrument tab (used to enable / disable microtonal functionality and assign a scale and a keymap);
• Piano View allows a quick selection of the first and last note that the instrument accepts when microtuner (or key range import) is disabled; this can be used to easily setup control of several instruments using one keyboard by splitting it into separate ranges (or even individual keys);
• base note selector no longer moves when user accidentally drags the mouse over it after pressing a piano key.

Known issues:

• the few included default scales and mappings may not reflect what would be actually useful and interesting to users (i.e. I'm open for suggestions here);
• MIDI-based instruments (OpulenZ, VeSTige, Carla) and ZynAddSubFX will not supported by this initial version (microtuner interface is hidden for these instruments to avoid confusion).

[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.199%2Bg2e7b40d8d-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.199%2Bg2e7b40d8d-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

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

Linux

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

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14194-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.199%2Bg2e7b40d8d-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14194?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14193-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.199%2Bg2e7b40d8d-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14193?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1nx3oj21phm9gs0b/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39836750"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/25gg3x8o5fm8mmt9/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39836750"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14191-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.199%2Bg2e7b40d8d-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14191?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14192-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.199%2Bg2e7b40d-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14192?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "fe44d1509daf47e00aea731d183f9130c84c361d"}

[he29-net]

The update to current master is complete. The only issue was with presets, since removing their previously intended changes in the range PR did not propagate to this PR (naturally, since the last commit of a branch being merged does not hold any record of "absence of an action").

Thanks PhysSong for the idea to merge the LCD widget and MIDI range separately. It reduced size of this PR to something like +2000 -100, making it hopefully more digestible. The final number of changed files is 42, so that must be a good sign. :)

[JohannesLorenz]

I think one of the reasons why no one has fully reviewed this is because this is a giant PR (42 files...). PRs of this size can often be decomposed into multiple atomic PRs. Looking at that this PR introduces a list of features, I wonder if this is true? Of course, sometimes you need to add 2 features and both do not work/are useless without the other.

@he29-net Is it possible for you to divide this PR into smaller, independent PRs? Such PR would probably be reviewed much faster.

Aside from reviewing time, it will also look ugly in the history to have one commit changing 42 files with independent things. Such large commits will make bisecting/debugging much more difficult.

[he29-net]

I read what you wrote on Discord earlier today, so I already went through the list of changed files to see if there is anything else that could be split out (like the MIDI range and float LCD spinbox), but the remaining changes are more interdependent.

I could perhaps split out the internal stuff (microtuner code, scale and keymap storage / saving / loading etc.), but without the configuration GUI it would just make it harder to test. Obviously, splitting out the GUI itself makes no sense, because it relies on the "backend" code, it would not work on its own.

The GUI side should be easier to review, though, so someone could perhaps do that here instead of Dom, so that he could focus just on the core stuff.

[JohannesLorenz]

I'd still go with letting the user configure it by hand (XML editing? I can't judge how difficult that gets, I'm not familiar with the PR) and split all the parts. Sounds like slightly more work to test, but hopefully much less work to review the PR. Though I also understand your point.

[he29-net]

I made a vaccum pass and picked up some small independent pieces (most of them new images or comments). It won't help much with the main load, but at least it will make the list visually a bit smaller.. 😅

[DomClark]

A small architectural comment: the interaction between InstrumentTrack and Microtuner could be simplified. Currently, if the microtuner is disabled, then accessing the first/last/base keys of an InstrumentTrack requires calling into the InstrumentTrack, which calls into the Microtuner, which calls back into the InstrumentTrack. This has a number of issues:

• It could be considered unintuitive - if the microtuner is disabled, why should the code access it at all?
• It introduces two-way coupling between InstrumentTrack and Microtuner. This can make them difficult to maintain independently.
• The Microtuner needs to hold a pointer to its parent InstrumentTrack; this makes it difficult to reason about the relative lifetimes of these objects, and complicates moving and copying them. (As QObject subclasses, neither is currently moveable or copyable, but this could change in the future, and such reference cycles are often considered bad practice outside of low-level data structures.)

I would suggest that testing whether the microtuner is enabled and returning the default value if not are done in InstrumentTrack. This allows Microtuner to focus only on its own properties, and leave those of InstrumentTrack to be dealt with by InstrumentTrack. If this is successful, Microtuner should be able to lose its m_instrumentTrack member.

The shared_ptrs still bug me a bit, but I can't think of a simpler way to solve the problem at this stage. It would be nice to have some system in the core for passing values to the audio thread(s), but that's a project for the Future™.

[DomClark]

Why do Keymap and Scale inherit from QObject?

[he29-net]

I'm not quite sure. When I started developing stuff for LMMS a few years ago, I often ran into problems because I did not inherit from the QObject or did not add the Q_OBJECT macro. So I started to adding it by default. :)

...and, trying to build it now without QObject, I can see it fails on undefined tr(). So apparently translations for the default empty scales and keymaps would not work. It's just a line in the constructor, though, so if it is a problem, I could replace it with something language-neutral, like ---.

[DomClark]

I think this is fine to leave for now then. We're going to have to solve this problem more generally as we work to reduce the use of Qt in the core.

[JohannesLorenz]

@DomClark Thanks for your review on this PR! Does your review include the microtuner core (#6036) and the independent changes (#6035), which both seem to be a subset of this PR? (@he29-net please correct if that was wrong)

[DomClark]

My review includes everything in this PR, so if this PR includes #6035 and #6036, then my review does too.

[he29-net]

Thanks again for the review. I'll start going through the comments so that it can move somewhat in parallel with the wavetable oscillator PR.
Splitting this into two smaller PRs was suggested to me to help attract reviewers (by making the individual PRs less oversized). Now it seems they are no longer needed, so I'll close them.

[he29-net]

A small architectural comment: the interaction between InstrumentTrack and Microtuner could be simplified.
...
* It could be considered unintuitive - if the microtuner is disabled, why should the code access it at all?

@DomClark When I was designing it, I thought of the microtuner as an overall "frequency translation manager". Ask it what frequency a key should play and it will respond, either with the default "western tuning" if it is "disabled", or with something else if a custom scale or keymap is used.

* It introduces two-way coupling between `InstrumentTrack` and `Microtuner`. This can make them difficult to maintain independently.

Yeah, that does seem like a bad design, I don't think I even realized that. The solution you propose seems good to me, I will make some wrapper methods in InstrumentTrack, so that it can serve as the "frequency manager" instead.

The shared_ptrs still bug me a bit, but I can't think of a simpler way to solve the problem at this stage. It would be nice to have some system in the core for passing values to the audio thread(s), but that's a project for the Future™.

It would be nice indeed. I was thinking about a simple source / sink system for audio streams (so that the spectrum analyzer could insert probes in various places and display multiple frequency graphs in one window), but keeping track of what is connected to where and what sources and sinks are available would also require some sort of project-wide storage like this.

But it could be more dynamic than a (mostly) static list of scales and keymaps, so I wasn't sure if it would be a good idea for anything even remotely touching realtime stuff. (For now I dropped the project, since it turned out more complex than it first looked and the PR list is clogged by the refactor anyway.)

[he29-net]

So, I getting rid of the InstrumentTrack in Microtuner class went overall relatively well, with just one exception: Microtuner::keyToFreq() needs to know the first key, last key and base note. This either requires keeping access to InstrumentTrack, or passing the values as parameters.

Both solutions would make calling keyToFreq() more "unpleasant" and verbose, but since it is only called from NotePlayHandle at this time, that may not be much of a problem. Actually, with that in mind, I'm thinking that the required "isKeyMapped()" check could be as well made by the caller, leaving only the base note to be passed as a parameter.

(EDIT: I missed some calls in plugins apparently, so the commit that follows is broken; will fix it tomorrow.)

[he29-net]

I would say that the remaining open issues were resolved (the last one is a translation-related "non-issue"), so if I'm not mistaken, this is now only waiting for review or the review changes?

And perhaps more testing, if needed (I think 3 or 4 devs saw at least parts of this PR through its lifetime, but I'm not sure how much focus went into trying to break things during code review :) ).

[DomClark]

I'm happy with the code here now. A final thought in that area:

You said 'When I was designing it, I thought of the microtuner as an overall "frequency translation manager"'. This was realised as a Microtuner class that was responsible for deciding whether to use standard or custom tunings, and responsible for the custom tunings themselves, but delegated responsibility for the standard tuning back to the instrument track. Presumably this was because the instrument track was already responsible for the standard tuning in the original design without the microtuner. After my review, the responsibility for deciding the tuning was moved to the instrument track. This untangled one layer of pointers, but still left an odd situation where one part of the tuning was split off into a different class.

I do wonder, if microtuning had been in LMMS from the beginning, whether we would just have a single Tuner class, encompassing standard- and micro-tunings, and the decision of which to use. Then InstrumentTrack would delegate all tuning choices to an instance of this class. I feel this would be cleaner than having various bits of tuning functionality in classes that aren't really responsible for them. Microtuner doesn't sound like a "tuning decider", more like a "tuning provider", and InstrumentTrack doesn't sound like a "tuning provider". This would avoid both Microtuner holding a pointer to InstrumentTrack, and InstrumentTrack passing the base note to Microtuner.

I'm not suggesting you change this now (although go ahead if you want), but perhaps it would be a nice refactoring effort to do in the future. It would require some changes to the project file structure, moving stored model values from the instrument track to a new child node.

I have some questions on integration with other features, namely the master pitch and base notes:

• From playing around with this, it seems the master pitch always operates in standard tuning, changing the pitch by 1/12 of an octave per step. Would it instead make sense for it to transpose by one scale step instead, for whichever scale happens to be selected?
• The base note has two conceptually different functions, although both are realised as a pitch shift. One is part of specifiying the tuning, to decide which note is associated with the base frequency - e.g. standard tuning has a base note of A4, associated with a base frequency of 440Hz. This is what the base note option within the microtuner is for. The other function is to correct the pitch of an instrument - e.g. when using a sample recorded at C4, not A4, the base note should be set to C4 for the instrument to be in tune with others (try it out with instruments/piano01.ogg). This function is similar to the first, in that you specify a note to associate with a frequency (the frequency of the sample), but in this case you're specifying the tuning that you have, not the tuning that you want. Currently you cannot use both functions at once - if you need to correct the tuning of an instrument, then you cannot use the base note from the scale, and so have to retune all these instruments if you change the scale's base note. I think it would be useful if you could specify the desired and given tunings separately, rather than computing the difference between these and representing it with a single base note.

[he29-net]

I do wonder, if microtuning had been in LMMS from the beginning, whether we would just have a single Tuner class, encompassing standard- and micro-tunings, and the decision of which to use.

That seems like a sensible design. The way I originally saw it, the Microtuner was supposed to be this kind of centralized tuning manager, but when I started implementing it, I saw it more like adding a new feature than a refactor, and that changed the end result quite a bit. Instead of reworking old code, I intentionally made a distinction between "default tuning" and "new microtonal stuff" when possible.

This was my first PR that significantly touched the core, after all, so keeping the old tuning system in place and unchanged meant that I can only break stuff nobody uses yet. But in theory, the old tuning system could be completely removed: the default western tuning (12-EDO) and 1:1 keyboard mapping are already available in the Microtuner (and selected by default). There is not much reason to have two ways to compute the same thing other than 100 % backward compatibility (since the Microtuner can have rounding errors in different places than the original formula, it could occasionally produce slightly different frequency).

One problem with removing it right now is that MIDI-based instruments do not use the LMMS tuning system. Currently, all supported instruments can have Microtuner in the "on" or "off" state. MIDI-based instruments don't show the interface at all, implying it is "off" and won't do anything "unexpected". But when the off switch disappears, the absence of the scale and keymap selectors in MIDI-based instruments does not give any such clear message. So by making the Microtuner "always on", it could become less clear what tuning and mapping are MIDI-based instruments using (or rather that their tuning can't be controlled from LMMS). But perhaps that is just a GUI issue, one sentence explaining why are the controls gone should suffice.

Another problem is kind of related to the "integration with other features". Switching to a custom tuning could have some unexpected effects, so keeping the on / off distinction could be helpful for that reason as well. For example, with the default tuning, detuning a single note is very intuitive: a line in the piano roll tells you which key the pitch starts on and where it ends. If it ends between two keys, you could guess the pitch is going to be somewhere between their pitches. With a custom tuning / mapping, all bets are off. Three subsequent keys may have completely different frequencies, so the detune indicator may not have much sense in relation to them.

If such weird stuff starts happening after a user turns microtuner "on", it will be probably expected. But if someone just changes a scale, not really knowing what it means, note detune not making sense suddenly looks like a bug rather than expected behavior.

Maybe I'm just too worried to make potentially breaking changes, but I would keep the "old default" and "new microtuner" distinction at least for now. We could drop the old tuning formulas after we get some feedback from users that actually use the microtonal features, if they find no issues, or after we implement MIDI retuning messages and have a more complete custom tuning support.

---

As for the feature integration questions:

• Yes, all pitch bends / detuning is in cents or 12-EDO half-steps. From what I tried, it seemed like the only reliable option that would work on all, even completely arbitrary scales and key mappings. For the master pitch, it depends what the users expect to happen when they move the slider: do they expect to get an overall increase or decrease in pitch, or do they expect to change the offset of physical keys that are being played? In the default tuning, both of those are equivalent, so there was no problem with it being ambiguous. I took it literally by the name and treated it as "an overall increase or decrease in pitch", because changing the physical key or scale degree offset in a justly intonated scale would likely only result in a big mess, making master pitch pretty much only useful for equal temperaments.
• Good point about the base note, I did not realize the two functions could be thought of separately. I always considered the "sample correcting" function to be a sort of hack. Most of the time it would only be used on AFP, so the functionality could be better included there (it could even be more user-friendly, rather than "look for the white square and try moving it around" :) ). But I guess it can't be ruled out that other instruments need a correction as well, so we need to keep "the white square" anyway. In that case, having two separate values seems like a good idea. I can't immediately think of a nice GUI solution though, other than perhaps keeping the white square always there and decoupling it from the base note defined in a given keymap. But again, the white square being above some key means almost nothing with a custom scale and mapping; correcting the tuning of a sample in this scenario by "moving a square" would be pretty non-intuitive and much better addressed in AFP. For example, what are you going to do when your scale does not have the note which matches the frequency played in the sample? The "white square" won't let you dial a precise frequency..

Anyway, I should go to bed and maybe think about it again when my brain starts working again. :) Thanks for finding the time to not only do the code review, but also thinking about the wider context. I'm not sure which parts can / should be addressed in this PR, but these are all interesting issues to think about.

[he29-net]

I went through the final points again, but I don't think I can contribute anything else to this PR that would lead to a significant improvement, at least not without risking further delays.

To summarize:

• a refactor to eliminate the old tuning system (keeping only a Tuner class that handles everything) would be nice, but it seems safer to keep the old tried and tested formula in place for now;
• the exact meaning of master pitch was ambiguous, so I picked the version that is more reliable (works better with "unreasonable scales") and closer to the literal meaning of "pitch" (so acting on frequency, not piano key numbers);
• base note number is often used to correct frequency of a sample loaded in Audio File Processor, but to make it both more flexible (tuning samples with arbitrary frequency) and more discoverable (so new users do not have to accidentally be discovering a white square), adding an appropriate setting to the AFP seems better to me, compared to implementing a LMMS-wide "bad frequency correction". Other instruments generally do not work with custom samples so I imagine it would only end up wasting space in the Misc tab.

So, TL;DR, if there are no objections to these last discussion points, merge time?

[PhysSong]

Found some Qt deprecation warnings.

[PhysSong]

QString::SkipEmptyParts is deprecated in Qt 5.15 in favor of Qt::SkipEmptyParts which was introduced in Qt 5.14. There are three more places where QString::SkipEmptyParts is used.

[he29-net]

Looking at the latest "nightly", we are building the appimage with Qt 5.6.3, so I suppose this is just a warning for the future?
As for the other one (QString::sprintf), I will fix it soon in a small UI related update. Thanks for the info.