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

Add Tap Tempo Feature #6372

Closed
wants to merge 3 commits into from
Closed

Add Tap Tempo Feature #6372

wants to merge 3 commits into from

Conversation

sakertooth
Copy link
Contributor

This PR adds a Tap Tempo Feature in the Tools menu. Users can use either the mouse or keyboard to tap.
image

All it does is get the periods between each beat in seconds and uses that to incrementally update the average BPM.
The BPM resets after 2 seconds of inactivity.

  • There still needs to be a logo however, so hopefully someone can resolve that. I'm not too good on design, and I left the logo as pitch black. I was considering using the metronome logo, but I ran into difficultes trying to find it.
  • For some reason, I have had discrepancies using the in-built metronome, but had rather okay results with other metronome's I found online from YouTube videos and websites. My PC may have had unusual latency though. If someone could test this that would be great.

@LmmsBot
Copy link

LmmsBot commented Apr 14, 2022

🤖 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://output.circle-artifacts.com/output/job/a072d608-7470-4490-b6f7-4e4938e63cec/artifacts/0/lmms-1.3.0-alpha.1.184+gcac99d633-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16191?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/b5c9cb58-4ba7-418e-ba74-177556bd6940/artifacts/0/lmms-1.3.0-alpha.1.184+gcac99d633-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16192?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/8y72s2pc70b9pdgc/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43240798"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/bwf24nha4saedxxc/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43240798"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/6ceda086-2b23-40ac-9128-763c07f5face/artifacts/0/lmms-1.3.0-alpha.1.184+gcac99d633-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16190?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/2a01f057-0bef-4cc5-8ebc-e91ba74c02c1/artifacts/0/lmms-1.3.0-alpha.1.184+gcac99d633-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16189?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "9b224421e15a791b474fecc03d141039d48aa256"}

@superpaik
Copy link
Contributor

I tested on W10.
Tested against the inside metronome, I wasn't able to sinc with it. It always gives less bpms than the metronome.
Maybe the latency it's because it's build as an effect, and not inside the core ¿?
As for the code, I'm not a developer but I being following some threats about it and I don't think that you should be using QT functions unless it's strictly necessary (for the GUI). You should use c++ functions when possible.

@sakertooth
Copy link
Contributor Author

I tested on W10. Tested against the inside metronome, I wasn't able to sinc with it. It always gives less bpms than the metronome. Maybe the latency it's because it's build as an effect, and not inside the core ¿? As for the code, I'm not a developer but I being following some threats about it and I don't think that you should be using QT functions unless it's strictly necessary (for the GUI). You should use c++ functions when possible.

I will try to see if the latency can be eliminated using the C++ standard library instead of QT. Perhaps will it be more accurate and thus sync with the metronome. Especially given the fact that small differences between periods for each beat mean rather large changes to the resulting BPM.

@allejok96
Copy link
Contributor

This feature is a good addition!

I think the problem comes down to storing m_bpmAverate as int. Even if QTime::currentTime was slightly delayed, the time between events should be correct. It's probably a rounding issue. To make it ridiculously correct you could count time difference from the first click always, instead of adding it up.

Superpaik has a point in using as little Qt as possible to make LMMS easier to port to other toolkits in the future, but in this case it's 90% Qt already and contained in the same file, so if it makes it simpler, I'd say go for QTime.

The button is slightly huge tho.

@allejok96
Copy link
Contributor

You should probably connect to pressed instead of clicked although in the end the average will be the same I guess.

@sakertooth
Copy link
Contributor Author

sakertooth commented Apr 14, 2022

This feature is a good addition!

I think the problem comes down to storing m_bpmAverate as int. Even if QTime::currentTime was slightly delayed, the time between events should be correct. It's probably a rounding issue. To make it ridiculously correct you could count time difference from the first click always, instead of adding it up.

Superpaik has a point in using as little Qt as possible to make LMMS easier to port to other toolkits in the future, but in this case it's 90% Qt already and contained in the same file, so if it makes it simpler, I'd say go for QTime.

The button is slightly huge tho.

Funny you say that, because just as I was switching to use instead of QTime, I also noticed that I was taking the average of the whole BPM, rather than taking the average of the double and just presenting it as an int. That was probably the central issue.

For the reason why I was taking the average instead of the period from the first click, it seemed like the small differences between the periods made substantial changes in the BPM (#5181 (comment)). I was also experimenting with a weighted average as well, in order to give more weight to more recent clicks, but I would have to do more testing to ensure it is more favorable than just the basic mean.

My upcoming commit uses the steady_clock and std::chrono::duration instead of QTime and, to be fair, it did not really introduce much complexity. The QTime implementation would surely be simpler but I see little reason in reverting at the moment.

And connecting to pressed instead of clicked does make sense, as there might be latency after the release rather than just calculating it as soon as the button is pressed. Thank you for noticing that. Will apply the changes soon.

Personal preference, but I am in favor of the large button. I will probably make it just a tad bit smaller though.

Use steady_clock and std::chrono::duration instead of QTime
Connect pressed to m_bpmButton instead of clicked
Remove "Reset after 2 seconds" comment
Change size to 200x200
Inline recentBpmAverage
Adjust formatting
@qnebra
Copy link

qnebra commented Apr 14, 2022

What about integrating it with "Tempo Set Value" window. Also, shouldn't be it able to set tempo value? Instead of being reliable on user to manually enter tempo value into "Tempo Set Value" window?

@allejok96
Copy link
Contributor

See also #6119

@sakertooth sakertooth deleted the branch LMMS:master April 14, 2022 20:45
@sakertooth sakertooth closed this Apr 14, 2022
@sakertooth sakertooth deleted the master branch April 14, 2022 20:45
@sakertooth sakertooth restored the master branch April 14, 2022 20:46
@sakertooth sakertooth mentioned this pull request Apr 14, 2022
@sakertooth sakertooth reopened this Apr 14, 2022
@sakertooth sakertooth closed this Apr 14, 2022
@sakertooth sakertooth reopened this Apr 14, 2022
@sakertooth
Copy link
Contributor Author

The branch has been renamed from sakertooth:master to sakertooth:tap-tempo. See #6375. Sorry for the inconvience.

@sakertooth sakertooth closed this Apr 14, 2022
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.

5 participants