Add Tap Tempo Feature (branch renamed)

[sakertooth]

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

[LmmsBot]

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

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)

:robot:

{"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"}

[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]

I just tested it. Here are some things I noticed:

• Clicking "Precision" or "Mute" checkboxes puts the focus on those controls, so using the spacebar will select/deselect those boxes instead of working as tap tempo input.
• If the rate that I use tap Spacebar or click changes too much, it will briefly say the tempo is zero, which is a little strange
• I wonder if Tap Tempo could be integrated with the global tempo control somehow, and whether that would be worthwhile?
• Tap Tempo should have a better icon
• Mute Tap Tempo, set the LMMS global tempo to some value, then click play in the Song-Editor to start the metronome. Then click Tap Tempo at a rate significantly lower than the metronome's rate for a while before trying to match the rate of the metronome. If you do this, the BPM that Tap Tempo gives should be a noticeably lower BPM than the global tempo, and slowly increase to approach the value that it should be. Is this something you're able to replicate? Play around with it a little - for me, the BPM often doesn't quite match what it should be - it often settles on an equilibrium which isn't very accurate.

[sakertooth]

Clicking "Precision" or "Mute" checkboxes puts the focus on those controls, so using the spacebar will select/deselect those boxes instead of working as tap tempo input.

Yeah, I've noticed that too. A bit annoying. Will fix.

If the rate that I use tap Spacebar or click changes too much, it will briefly say the tempo is zero, which is a little strange

This is because the counter gets reset when the BPM on the display does not closely reflect the current BPM. This gives a chance for the counter to get more accurate readings in the following taps. When the user taps, there are two intervals: the one from the current time and the time when the user first tapped the button, and the other is from the current time and the last 3 taps. The latter is more accurate, while the former stabilizes better at a certain number but does not play well to rapid changes in BPM. If the difference between these two BPM's is greater than 30 (an arbitrary threshold I set), then the counter gets reset. Instead of resetting however, I could just set the counter to use the BPM from the more accurate interval (currentTime - prevTime), and then do further calculations using the stable interval (currentTime - startTime).

I'm open to suggestion if you have a better algorithm that not only stabilizes but also responds well to rapid changes in BPM.

Tap Tempo should have a better icon

Not the best artist :sweat_smile:. Hopefully someone can come by and improve this somewhat?

Mute Tap Tempo, set the LMMS global tempo to some value, then click play in the Song-Editor to start the metronome. Then click Tap Tempo at a rate significantly lower than the metronome's rate for a while before trying to match the rate of the metronome. If you do this, the BPM that Tap Tempo gives should be a noticeably lower BPM than the global tempo, and slowly increase to approach the value that it should be. Is this something you're able to replicate? Play around with it a little - for me, the BPM often doesn't quite match what it should be - it often settles on an equilibrium which isn't very accurate.

As previously mentioned, when the BPM from the stable interval and the accurate interval diverge and pass a threshold of a difference of 30 BPM, the counter gets reset. So, if you get the counter to reset and then start playing the tempo of the project, it should converge to it. I could change this behavior to use the assign the display BPM to the BPM from the more accurate interval when the threshold is passed instead of resetting, but it wouldn't be a slow ramp up, so I would have to make the more accurate interval a bit more stable.

The whole algorithm is a bit confusing, so once again I am open to any adjustments to it. The algorithm starts of simple, and we just calculate the BPM from the interval currentTime - startTime, but if user where to change their pace, it wouldn't accurately reflect that, which is were it gets a bit more intricate.

[zonkmachine]

I wonder if Tap Tempo could be integrated with the global tempo control somehow, and whether that would be worthwhile?

I was thinking this too. You could have a context menu option to launch the tap tempo app.

[sakertooth]

I think this is good to be merged. There are things that could be added as discussed above but I think it's better to merge now and add stuff later.

I'm just going to add a "Sync" button that syncs the displayed BPM with the project tempo, but after that I think that'll be it. The other features/changes mentioned might be beneficial, but could probably be looked at in another PR if the need arises.

[sakertooth]

I made a couple small fixes.

There's also a memory leak detected after closing LMMS. It only happens when the Tap Tempo metronome sound plays, but I haven't figured out why it happens yet. Here's the error message:

lmms: /home/dalton/Documents/lmms_sakertooth/src/3rdparty/rpmalloc/rpmalloc/rpmalloc/rpmalloc.c:1328: _rpmalloc_span_finalize: Assertion `(span->list_size == span->used_count) && "Memory leak detected"' failed.
Aborted (core dumped)

That's unrealted to this PR. This has been an outstanding issue that was looked at previously with #5733, although I'm not sure why it still happens.

(PS: I'm in favor of ditching rpmalloc altogether.)

[messmerd]

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