Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIDI range MKII (extracted from microtonal PR) #5868

Merged
merged 26 commits into from Apr 21, 2021

Conversation

he29-net
Copy link
Contributor

This PR largely covers what was originally in #5349, plus some other bells and whistles added later in #5522.

It should have all the code to fix #1857 and on the first glance it seems to be working correctly on its own. Parts that depend on the Microtuner code are commented out and / or replaced by constants, they should be easy to resolve as conflicts after this is merged.

For now marking as a draft, since there was a mention (I think on Discord) that some of the presets have a bad base note value, likely caused by a wrong conflict resolution in the recent months.

@LmmsBot
Copy link

LmmsBot commented Dec 31, 2020

🤖 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

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13590-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg62f49a8b4-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13590?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13586-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg62f49a8b4-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13586?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/i7p3gyw1be759m2t/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38776880"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/9bye4wfjk0xa5fn7/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38776880"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13589-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg62f49a8-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13589?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13587-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.125%2Bg62f49a8b4-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13587?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "b303d9f5e01dacb4c26b0d5d115b1590cdcf0bf4"}

@PhysSong PhysSong self-requested a review January 1, 2021 02:26
Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial review for this PR.

src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved
src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved
src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved
src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved
@he29-net he29-net marked this pull request as ready for review January 1, 2021 13:04
@he29-net
Copy link
Contributor Author

he29-net commented Jan 1, 2021

I went through the presets and found only two that did not have the correct 12 semitone base note offset (AFP/orion.xpf and TripleOsc/PowerStrings.xpf).

I also fixed some other issues after going through the code once again; I believe this PR should be ready for review now.

@PhysSong
Copy link
Member

PhysSong commented Jan 1, 2021

If you're going to keep the key range feature, why don't you add white_key_disabled and black_key_disabled pixmaps instead?

@he29-net
Copy link
Contributor Author

he29-net commented Jan 1, 2021

If you're going to keep the key range feature, why don't you add white_key_disabled and black_key_disabled pixmaps instead?

The drawing code depends on the microtuner (check if mapped, the range check is included in that), so they won't be drawed anyway. If you want I can include them, it does not really make much difference. Or I could even slap together some condition that check the range to allow drawing them. It just seemed like a wasted effort on something that is going to be (hopefully) replaced in a few days by the proper original code.

@he29-net
Copy link
Contributor Author

he29-net commented Jan 1, 2021

Okay, both PianoRoll and PianoView should now draw the out-of-range keys.

@PhysSong PhysSong self-requested a review January 3, 2021 05:39
data/projects/templates/default.mpt Outdated Show resolved Hide resolved
data/projects/templates/default.mpt Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

PhysSong commented Jan 6, 2021

I think it will be nice if user presets can be upgraded automatically. Have you tried it, or not?

@he29-net
Copy link
Contributor Author

he29-net commented Jan 6, 2021

I tried (#5522 (comment)), but found that the upgrade routine does not run when loading a preset, so there is currently no mechanism that I know of that could be used to perform such upgrade.

@PhysSong
Copy link
Member

PhysSong commented Jan 8, 2021

the upgrade routine does not run when loading a preset

No, it does. The real difference is: in preset files, instrumenttrack nodes are in instrumenttracksettings while it's track in project files.

@he29-net
Copy link
Contributor Author

he29-net commented Jan 9, 2021

Oh? I'll have to look at it again then. Thanks for the info.

src/gui/PianoView.cpp Outdated Show resolved Hide resolved
src/gui/PianoView.cpp Outdated Show resolved Hide resolved
@he29-net
Copy link
Contributor Author

I did not look at the presets yet, but meanwhile @IanCaio reported a potential problem on Discord and it turned to be a pretty big can of worms. ^^;

The reported problem was that when a note is placed on Piano Roll and it is out of the range defined by firstKey and lastKey markers, it would still play. This was meant to be solved by the microtuner keyToFreq() function, that would report a frequency of 0 Hz in case the requested key was invalid, so the result should be silence (0 changes per second). I forgot to add the range check to the legacy frequency computation code in NotePlayHandle, so the notes still played when microtuner was disabled. That was an easy fix.

BUT, it lead me to test another thing I missed before: how do actually instruments behave when they get a 0 Hz note. And the results pretty much range from "as expected" to "disastrous". TripleOsc and one other native synth were pretty much the only ones handling it correctly and producing no sound. Some almost worked but produced clicks when trying to play the "non-note". Most other native synths produced some sort of infra-sound + higher harmonics. Zyn and LV2 instruments ignored the range limit, because the MIDI messages also do not have any key range filter (and MIDI based instruments are not handled by the microtuner either). The worst two offenders were Vibed which caused LMMS to crash, and sfxr which on the first try went completely berserk (could not replicate it again though).

So overall, it seems clear that:
a) playing 0 Hz tones is pretty dangerous, because even if we fix the native plugins, it could still cause some badly coded external plugins to explode, and
b) it would be best to avoid playing the out-of-range notes at all, thus fixing LV2 and other MIDI-based synths and also preventing other problems like some playing infrasound or the upper harmonics as if it was a non-zero frequency.

The second point should pretty much take care of everything, but I'll also change the code in #5522 so that if for any reason frequency is requested for a key that is not mapped, 1 Hz is returned instead of 0 to prevent possible crashes and speaker explosions.

@PhysSong
Copy link
Member

Can we make NotePlayHandle somehow notice the range limit without microtuner?

@he29-net
Copy link
Contributor Author

Can we make NotePlayHandle somehow notice the range limit without microtuner?

For NotePlayHandle it would be just a small change in updateFrequency() (simply look at the firstKeyModel and lastKeyModel values in instrument track), but since playing 0 Hz notes turned out to be dangerous I'll have to instead figure out how to prevent the note from playing at all. I did not look into that yet, but I'll try to make some progress on my PRs tonight and tomorrow.

he29-net and others added 2 commits January 23, 2021 23:29
Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
@he29-net
Copy link
Contributor Author

@he29-net Could you resolve the conflicts?

Done. Thanks for the ping, I did not notice there was a conflict.

@PhysSong
Copy link
Member

@gnudles Sorry for pinging, but are there any chances that some Xpressive formulas are affected by this PR? I think key and bnote parameters may be affected, but I'm not sure how should we handle that cases.

@gnudles
Copy link
Contributor

gnudles commented Mar 13, 2021

It might be affected. let me check.

@gnudles
Copy link
Contributor

gnudles commented Mar 13, 2021

@PhysSong, my original intention when I exported / introduced these values (bnote and key) in formula, was indeed to allow customized key to frequency mapping and other scales. From what I understand, this pull request now allows to make all linear key to freq mapping. So Xpressive has now only the advantage of non-linear mappings. since non linear mappings are very rare, and since the plugin released only under alpha version, I think we should consider to remove these variables from the formula?

@gnudles
Copy link
Contributor

gnudles commented Mar 14, 2021

Btw, looks like somebody really liked my plugin XD
https://m.youtube.com/watch?v=G6TfCTm-wmo

@IanCaio
Copy link
Contributor

IanCaio commented Apr 6, 2021

Hey man! Apparently there are 2 new conflicts, simple ones to solve (just on the header includes), could you fix those?

@he29-net
Copy link
Contributor Author

he29-net commented Apr 6, 2021

@IanCaio Thanks, done.


I'm still not sure what to do about the key variables in Xpressive formulas (if anything). I checked the factory presets and the only one using them seems to be "Faded Colors - notes test.xpf", so breaking one "test preset" by removing the variables should not be a major problem. It's hard to say how many users used them, but as gnudles said, it's a plugin only available in an alpha version, so that should not cause much pain either.

@gnudles As for the "feature-completeness": if I understand what you meant by non-linear mapping, it should also be possible to do with the Microtuner. You would simply make a keymap covering all 127 keys, and a scale with 127 scale degrees, creating a "lookup table" for an arbitrary function. Making it would not be as comfortable as writing a single formula, but definitely possible.

Btw. I also saw that video; the "get out of my left ear!" was very close to my first experience with Xpressive, the stereo imbalance in some presets is a bit hard for my brain to accept as a valid input.. :D

@gnudles
Copy link
Contributor

gnudles commented Apr 7, 2021

Ok. I will remove the "note test" from the presets and the stereo balance, and will remove the "key" and "bnote" variables.

@he29-net
Copy link
Contributor Author

@PhysSong With the Xpressive issue resolved, is there anything else you wanted to look into, or is this ready to be merged?

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

  • The Xpressive documentation changes in 8077547 probably belong in this PR.
  • How important is it to keep the commented-out microtuner code here? It's fine as it is, but the history would be cleaner if it weren't there.

src/core/NotePlayHandle.cpp Outdated Show resolved Hide resolved
@he29-net
Copy link
Contributor Author

* The Xpressive documentation changes in [8077547](https://github.com/LMMS/lmms/commit/80775479d9dbd9102a4eee89d09ab105054a53a4) probably belong in this PR.

* How important is it to keep the commented-out microtuner code here? It's fine as it is, but the history would be cleaner if it weren't there.

Thanks for taking a look. I see that gnudles probably plans to only have the key and bnote disabled and not completely removed in #5979, so keeping the documentation update and including it here makes sense. When I made this PR, I assumed it will be just a quick and temporary PR to lighten to load of the Microtuner PR, so I may have missed a few things like this.

The commented-out code is there for the same reason; with the pace things were going before Christmas, I assumed I will be rebasing #5522 a weak or two later, so I left a lot of the code in place so that it generates more obvious merge conflicts. I don't trust git to always magically do all changes correctly, so this would guide me to places that need to be reviewed manually. But if we want this to be a "proper" commit, I can do a cleanup pass, it's not that important (I can just do a bit more testing after the rebase to make sure nothing got broken).

@DomClark
Copy link
Member

The commented-out code is there for the same reason; with the pace things were going before Christmas, I assumed I will be rebasing #5522 a weak or two later, so I left a lot of the code in place so that it generates more obvious merge conflicts. I don't trust git to always magically do all changes correctly, so this would guide me to places that need to be reviewed manually. But if we want this to be a "proper" commit, I can do a cleanup pass, it's not that important (I can just do a bit more testing after the rebase to make sure nothing got broken).

Sorry about the change in pace. This particular change is only aesthetic, so I'll let you weigh up whether it's worth the risks with the merge conflicts.

@he29-net
Copy link
Contributor Author

I pushed the other small changes for now, I'll look at the comments tomorrow-ish. Some of them may be helpful to explain some of the byproducts of the rough extraction from the main PR (i.e. stuff like if (true) placeholders for tests performed by the microtuner, not sure if any of them remained), but if there are others without such purpose I'll try to get rid of them, to make a better impression on the future archeologists digging through our commit history. ^^

@he29-net
Copy link
Contributor Author

The comment purge is complete; I left only two of them (explaining some if (true) and if (false) placeholders) and noted the line numbers for the others to check manually after the Microtuner rebase. Maybe I'm just too paranoid and it won't be needed, I'm curious to see how git handles it. :)

@Veratil Veratil merged commit f288137 into LMMS:master Apr 21, 2021
he29-net added a commit to he29-net/lmms that referenced this pull request Apr 23, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Update MIDI range to match MIDI specification

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
Co-authored-by: Kevin Zander <veratil@gmail.com>
Co-authored-by: Hyunjin Song <tteu.ingog@gmail.com>
Co-authored-by: Spekular <Spekular@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIDI-based instruments play an octave too low by default
8 participants