Add Tap Tempo

[sakertooth]

This is the #6372 PR, but with the branch renamed for organization.
Resolves #5181.

[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! 🎩

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.193+g4dd8c8a5f-linux-x86_64.AppImage (build link)

Windows

• Windows 32-bit: lmms-1.3.0-alpha.1.193+g4dd8c8a5f-mingw-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha.1.193+g4dd8c8a5f-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.193+g4dd8c8a5f-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/71e86c39-d5b7-4c25-a202-f9b9b7cb49da/artifacts/0/lmms-1.3.0-alpha.1.193+g4dd8c8a5f-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17080?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/868919fd-0d73-40e3-baa4-83322c19462b/artifacts/0/lmms-1.3.0-alpha.1.193+g4dd8c8a5f-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17083?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/7abb7d44-b14a-4650-b029-0f688b240385/artifacts/0/lmms-1.3.0-alpha.1.193+g4dd8c8a5f-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17082?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/g9c0jl726p8cayn1/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43767649"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/h0na9rcrw5rlm9ih/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43767649"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d825c31c-51ae-48c5-a176-3e4c9e5071af/artifacts/0/lmms-1.3.0-alpha.1.193+g4dd8c8a5f-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17084?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "dd2d3e1b4a30841a7891965a296a2e6f8ebb56f7"}

[allejok96]

Before you go ahead and fix any of my comments, I want to say I liked the old code better. It was simpler and more standalone. This line is the biggest pain point:

return tool->model()->displayName() == "Tap Tempo";

Checking the display name isn't such a great idea as it could (potentially, not right now) be translated. But I see there is no other way. So maybe this integration is overkill.

Besides, opening the tap tempo tool from the tempo spinbox makes you think they are integrated is some way (like you could set the tempo etc) but they are actually not. And now we have two ways to open the same tap tempo window which feels odd.

Having it integrated in the "Set tempo" dialog is no better, because that window locks up the LMMS interface when opened. And maybe you just want to check the tempo of something and write that value to an automation track...

In the end of the day, writing a number in the tempo spinbox isn't that hard. Tap tempo is a good, simple and versatile tool, even without all that integration.

[sakertooth]

I figured that adding a Tap Tempo option in the caption menu would be user-friendly but looking back it did feel rather problematic to add such a small change. It is true that finding the Tap Tempo plugin from the MainWindow in such a manner using displayName and providing access to m_tools without considering potential translations is a bit unusual. I also dont like how it attempts to find the plugin each time, which feels mostly wasteful and unncecessary, but it felt like the safest way.

Either way, given the ongoing major rework of the codebase, I dont think that such a small addition like Tap Tempo should make much, if any, change to the existing code, but rather add on to it in a modular way. Reverting.

[allejok96]

I have a little feature request: displaying BPM as ms and Hz somewhere below the button perhaps. Could be good if you want to set a delay or rate knob that isn't a tempo sync knob for some reason.

[sakertooth]

I have a little feature request: displaying BPM as ms and Hz somewhere below the button perhaps. Could be good if you want to set a delay or rate knob that isn't a tempo sync knob for some reason.

I have added this feature, but I must ask: wouldn't knowing the Hz be enough? I dont mind adding BPM in ms however, I just currently think it will be simpler to have just the Hz.

[Spekular]

There are a lot of plugins where e.g. delays or envelopes can be specified in milliseconds, in which case it's valuable to know how long each beat is.

[sakertooth]

Seems like it's rather complete. Hopefully someone can verify if everything is in place for good measure.

[allejok96]

I made some touchups

[sakertooth]

I made some touchups

Thank you for this, although I think it would be nice if the tooltip was more descriptive. Very minor, but to indicate a kind of action, maybe "Display high precision" would work better. Do you agree with this change before I add it in?

[RoxasKH]

So wait, we're making the tempo tap displaying decimals BPMs while LMMS itself doesn't allow decimals BPMs?

I'm asking because imo it should be the opposite like in FL Studio: you have the opportunity to slightly change your BPM with decimals in the BPM normal box, but when using the tap tempo feature, i think it's more useful and reasonable for it to show and set only integer values, considering that obviously human tapping isn't accurate and you usually wanna get as a result the nearest integer BPM more than the actual weird value you get with your tapping.

[Spekular]

Anyone who wants an integer value can easily round to it from a more specific value, whereas the opposite is not true. I would default to displaying two decimals, personally, so it's easier to tell your BPM has actually stabilized to an accurate value.

You can't always assume that the song you're working with has a fixed integer BPM in the first place, and an integer display is misleading for songs with a non-integer or slightly varying bpm. With just whole numbers one might assume that e.g. the following sequence of 8 taps is stable: 84.05, 84.08, 84.14, 84.20, 84.22, 84.25, 84.30, 84.35.

[sakertooth]

Rebased. I think this is complete.

[allejok96]

Everything looks good, now I'll only add that the metronome icon would look nicer.

[messmerd]

@sakertooth I still wonder why that sets off the rpmalloc leak detection but using the metronome doesn't, nor previewing samples.

[messmerd]

Works pretty well when I tested it, and it behaves similarly to other Tap Tempo tools that I tried online.

I think I'm fine with merging it now, provided the rpmalloc issue is not a problem with this PR.