lp1336866: Fixed bpm.tapButton in common-controller-script.js #1811

Swiftb0y · 2018-09-18T17:11:12Z

When the behavior of the bpm mixxxcontrol got changed in 1.10, it broke the bpm.tapButton function. I changed the key to file_bpm (which works like bpm did before 1.10) and added a "filter" that rejects accidental double presses and missed beats when tapping.

Pegasus-RPG

Thank you for fixing this. It otherwise looks good to me.

Pegasus-RPG · 2018-10-03T04:23:19Z

res/controllers/common-controller-scripts.js

+      return;
+    }
+    bpm.previousTapDelta=tapDelta;
+    // the if-case is meant to be a filter to reject accidental double presses


How does this work in the case of a detected BPM that's double or half of the track's real one?

I don't quite get what you mean. Which BPM is the "detected BPM" (the file_bpm or the tapped one)?
If you understand you correctly, you are asking how the tapped BPM behaves in case its double/half of file_bpm (which might not be correct either)? When I only tap at half the speed of the original track, the track will play at half the speed (as you'd expect).

Swiftb0y · 2018-10-20T21:59:49Z

Ping @Pegasus-RPG are you able to review/merge this in time so it can still go into the 2.2 release candidate?

rryan · 2018-11-04T01:47:53Z

res/controllers/common-controller-scripts.js

    if (tapDelta>2.0) { // reset if longer than two seconds between taps
        bpm.tap=[];
        return;
    }
+    if ((tapDelta > bpm.previousTapDelta*1.8)||(tapDelta<bpm.previousTapDelta*0.4)) {


If this condition happens and we return / ignore the tap, the next tap will only be even farther away from previousTapDelta, so I think what this is saying is that if your tapping ever varies by more than 1.8x or less than 0.4x, you have to wait 2 seconds to hit the tapDelta > 2.0 reset ?

Not quite, the tapDelta is out of the accepted range, the tap interval will be rejected but the previousTapDelta and the bpm[] doesnt change so the tapDelta of that particular tap just gets ignored.

Oh, I see -- bpm.tapTime is updated regardless. So math-wise, this appears to be a way to make the mean less sensitive to outliers by dropping them, but the criteria for ignoring an outlier is similarity to the immediately previous tapdelta?

Exactly. Maybe the name previousTapdelta is a bit missleading as it refers to the tapdelta of the previous successful tapdelta. When you accidentally double pressed the button, the next tapdelta will be the time between the tap and the accidental press (which is not an issue in my opinion as the double press will be very close to the actual tap time wise). it works likewise when you miss a tap.
The only fundamental flaw that this method has is that the function assumes that the first tapDelta is correct. So It won't work accurately (and reject subsequent correct taps) when either one of the first two taps is incorrect.

rryan · 2018-11-04T01:50:05Z

res/controllers/common-controller-scripts.js

@@ -392,10 +393,20 @@ bpm.tapButton = function(deck) {
    var now = new Date()/1000;   // Current time in seconds


Each deck has its own bpm_tap control, which has a low pass filter implemented in C++, and the skins use it for tap buttons. Instead of improving this logic, what about deleting it and setting the bpm_tap control here?

This bpm tap works different from the mixxxcontrol: This one is far more useful in my opinion as it allows one to roughly match the playback speed of the Deck to an external source. The mixxxcontrol on the other hand just manipulates the beatgrid which is only useful when you have to manually set the bpm of a track (which happens rarely). IMO this is far more useful in most scenarios than the mixxxcontrol.

Gotcha -- we could change the logic of the engine implementation instead? I think it makes most sense if whatever the logic is, a tap button in a skin and a tap button on a controller does the same thing.

mixxx/src/engine/bpmcontrol.cpp

Lines 212 to 226 in 1c77858

void BpmControl::slotTapFilter(double averageLength, int numSamples) {

// averageLength is the average interval in milliseconds tapped over

// numSamples samples. Have to convert to BPM now:

if (averageLength <= 0)

return;

if (numSamples < 4)

return;

// (60 seconds per minute) * (1000 milliseconds per second) / (X millis per

// beat) = Y beats/minute

double averageBpm = 60.0 * 1000.0 / averageLength / calcRateRatio();

m_pBeats->setBpm(averageBpm);

}

The primary reason I wanted fix this was because the feature is broken without the first commit and while I was testing, I figured that it might be a good Idea to add some sort of filter to prevent accidental double presses and cases where you just missed the button. The reason why I didn't think about fixing this in the code, is because it had already worked that way in 1.9.0 but was deprecated shortly after in 1.9.2, which is why I figured that there must be a reason why it works this way. IMO, both implementations have use cases but the proposed bpm.tapButton is the far more useful and intuitive version. If I would port the proposed behavior to c++, I would like to keep the old behavior as well (maybe accessible via a separate CO) as it does have its use case when trying to manually determine the bpm of a track.

I'm not really understanding the use case for using a BPM tap button to change the playback tempo of a deck. Why doesn't moving the rate slider work? Doesn't the rate slider give you more precise control than attempting to tap the exact tempo of an external source?

Yes, but a tap button allows the user to get a rough number for the speed hes working with and it usually also syncs the phase to the taps which is difficult when you are only working with the bpm slider (atleast for beginners and intermediates).

Swiftb0y · 2019-08-04T19:58:58Z

Hey, can somebody take a look at this again?

Holzhaus · 2019-10-08T08:15:27Z

@Be-ing ping

uklotzde · 2019-11-29T12:23:09Z

@Be-ing Ready to merge? Might then go into 2.2.4.

Be-ing · 2019-11-29T12:26:50Z

I'm still not really convinced there is a use case for this behavior...

Holzhaus · 2019-11-29T13:12:05Z

I'm not using tapping either, but I guess there are usecases for it, e.g. when you're playing together with a live drummer or something like that. In any case, this feature already exists so we do need to make sure that it's not broken.

Holzhaus

Thanks for contributing. I added some comments (I didn't test this manually yet).
Also, please add a trailing semicolon for each changed line. I'll auto-fix the other ones using eslint later on.

Holzhaus · 2020-01-20T20:27:40Z

res/controllers/common-controller-scripts.js

@@ -421,10 +421,18 @@ bpm.tapButton = function(deck) {
    var now = new Date() / 1000   // Current time in seconds
    var tapDelta = now - bpm.tapTime
    bpm.tapTime = now
+    if (bpm.tap.length < 1.0) {
+      bpm.previousTapDelta=tapDelta


Suggested change

bpm.previousTapDelta=tapDelta

bpm.previousTapDelta = tapDelta;

Holzhaus · 2020-01-20T20:28:40Z

res/controllers/common-controller-scripts.js

@@ -421,10 +421,18 @@ bpm.tapButton = function(deck) {
    var now = new Date() / 1000   // Current time in seconds
    var tapDelta = now - bpm.tapTime
    bpm.tapTime = now
+    if (bpm.tap.length < 1.0) {


Isn't bpm.tap.length an integer?

Suggested change

if (bpm.tap.length < 1.0) {

if (bpm.tap.length < 1) {

Holzhaus · 2020-01-20T20:29:40Z

res/controllers/common-controller-scripts.js

    if (tapDelta > 2.0) { // reset if longer than two seconds between taps
        bpm.tap = []
        return
    }
+    if ((tapDelta > bpm.previousTapDelta*1.8) || (tapDelta < bpm.previousTapDelta*0.4)) {


Please add a comment that explains where these constants come from.

Swiftb0y · 2020-01-20T20:40:04Z

I'll fix and test everything, but I'm afraid I won't be able to make time for it within the next two weeks.

Swiftb0y · 2020-03-22T16:08:52Z

Had to create a new PR because I had git issues. #2587

Holzhaus · 2020-03-22T16:23:41Z

Had to create a new PR because I had git issues. #2587

You can disable the pre-commit hooks by using:

git cherry-pick --no-commit <commit>
git commit --no-verify

Also you can backup the old branch and reuse this PR so that we keep the discussion in a single place:

# Checkout original branch and create backup
git checkout script.bpm.tapbutton_fix
git branch -c script.bpm.tapbutton_fix_backup

# Overwrite current branch state with updated branch
git reset --hard bpm.tapbutton_fix

# Push your changes to this PR
git push -f origin script.bpm.tapbutton_fix

No need to do this now, just in case something like this happens again ;-)

Swiftb0y · 2020-03-23T19:02:07Z

Yeah, I tried it but git doesn't let me reopen branches that were force-pushed while they were closed.

Swiftb0y added 3 commits September 18, 2018 17:56

first fix idea. Yet to test!

7e5dcf3

added filter for accidental bpm.tapButton presses/misses

dcd1f4c

fixed typo in common-controller-scripts.js tapbutton filter

cb2f422

Swiftb0y changed the title ~~Fixed bpm.tapButton in common-controller-script.js~~ lp1336866: Fixed bpm.tapButton in common-controller-script.js Sep 20, 2018

Pegasus-RPG reviewed Oct 3, 2018

View reviewed changes

Be-ing added the controller mappings label Oct 24, 2018

rryan reviewed Nov 4, 2018

View reviewed changes

Merge branch '2.2' into script.bpm.tapbutton_fix

5617e45

uklotzde added this to the 2.2.x milestone Nov 29, 2019

Holzhaus modified the milestones: 2.2.x, 2.2.4 Dec 17, 2019

Holzhaus added the minor bug label Dec 17, 2019

Holzhaus requested changes Jan 20, 2020

View reviewed changes

Swiftb0y mentioned this pull request Mar 22, 2020

lp1336866: Fixed bpm.tapButton in common-controller-script.js #2587

Closed

Swiftb0y closed this Mar 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lp1336866: Fixed bpm.tapButton in common-controller-script.js #1811

lp1336866: Fixed bpm.tapButton in common-controller-script.js #1811

Swiftb0y commented Sep 18, 2018

Pegasus-RPG left a comment

Pegasus-RPG Oct 3, 2018

Swiftb0y Oct 3, 2018

Swiftb0y commented Oct 20, 2018 •

edited

rryan Nov 4, 2018

Swiftb0y Nov 4, 2018

rryan Nov 4, 2018

Swiftb0y Nov 5, 2018

rryan Nov 4, 2018

Swiftb0y Nov 4, 2018

rryan Nov 4, 2018

rryan Nov 4, 2018

Swiftb0y Nov 5, 2018

Be-ing Nov 23, 2018

Swiftb0y Nov 23, 2018

Swiftb0y commented Aug 4, 2019

Holzhaus commented Oct 8, 2019

uklotzde commented Nov 29, 2019

Be-ing commented Nov 29, 2019

Holzhaus commented Nov 29, 2019

Holzhaus left a comment

Holzhaus Jan 20, 2020

Holzhaus Jan 20, 2020 •

edited

Holzhaus Jan 20, 2020

Swiftb0y commented Jan 20, 2020

Swiftb0y commented Mar 22, 2020

Holzhaus commented Mar 22, 2020

Swiftb0y commented Mar 23, 2020

		@@ -392,10 +393,20 @@ bpm.tapButton = function(deck) {
		var now = new Date()/1000; // Current time in seconds

	void BpmControl::slotTapFilter(double averageLength, int numSamples) {
	// averageLength is the average interval in milliseconds tapped over
	// numSamples samples. Have to convert to BPM now:

	if (averageLength <= 0)
	return;

	if (numSamples < 4)
	return;

	// (60 seconds per minute) * (1000 milliseconds per second) / (X millis per
	// beat) = Y beats/minute
	double averageBpm = 60.0 * 1000.0 / averageLength / calcRateRatio();
	m_pBeats->setBpm(averageBpm);
	}

	bpm.previousTapDelta=tapDelta
	bpm.previousTapDelta = tapDelta;

lp1336866: Fixed bpm.tapButton in common-controller-script.js #1811

lp1336866: Fixed bpm.tapButton in common-controller-script.js #1811

Conversation

Swiftb0y commented Sep 18, 2018

Pegasus-RPG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Oct 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 4, 2019

Holzhaus commented Oct 8, 2019

uklotzde commented Nov 29, 2019

Be-ing commented Nov 29, 2019

Holzhaus commented Nov 29, 2019

Holzhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Jan 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Jan 20, 2020

Swiftb0y commented Mar 22, 2020

Holzhaus commented Mar 22, 2020

Swiftb0y commented Mar 23, 2020

Swiftb0y commented Oct 20, 2018 •

edited

Holzhaus Jan 20, 2020 •

edited