Add BPM conversions 4/3 and 3/2 by ronso0 · Pull Request #1106 · mixxxdj/mixxx

ronso0 · 2017-01-03T12:52:29Z

Fixing my wishlist bug https://bugs.launchpad.net/mixxx/+bug/1462350
Adding two conversions that one might guess when detected BPM is far from the user estimated BPM. Basically these are the two I came across when beat analysis was wrong on some Drumfunk/Breakbeat tracks. There could be numerous more, but as @daschuer points out: it's hard to know that one needs a 7/8 conversion. So these are sufficient, I suppose.

ninomp · 2017-01-03T16:21:39Z

        break;
+    case FOURTHIRDS:
+        // introduce two beats into every gap
+        scaleDouble();


Maybe you can implement scaleQuadruple() and use it here instead of two calls to scaleDouble().

of course, didn't look it up

Personally I think two scaleDoubles are fine here (less code is usually better and this isn't performance critical so two passes are ok).

Ah -- then two scaleDoubles it is. Sorry for misguidance.

rryan · 2017-01-03T18:34:22Z

Looks like the BeatMap tests aren't passing:

[ RUN ] BeatMapTest.Scale
src/test/beatmaptest.cpp:67: Failure
Value of: pMap->getBpm()
  Actual: 40
Expected: bpm * 4 / 3
Which is: 80
src/test/beatmaptest.cpp:70: Failure
Value of: pMap->getBpm()
  Actual: 60
Expected: bpm * 3 / 2
Which is: 90

ronso0 · 2017-01-04T15:53:27Z

Thank you for testing. I didn't because in mixxx both numerical and visual results are as expected.
Must admit I don't understand all of the test code:

pMap->scale(Beats::DOUBLE);
EXPECT_DOUBLE_EQ(2 * bpm, pMap->getBpm());

pMap->scale(Beats::HALVE);
EXPECT_DOUBLE_EQ(bpm, pMap->getBpm());

pMap->scale(Beats::TWOTHIRDS);
EXPECT_DOUBLE_EQ(bpm * 2 / 3, pMap->getBpm());

pMap->scale(Beats::FOURTHIRDS);
EXPECT_DOUBLE_EQ(bpm * 4 / 3, pMap->getBpm());

DOUBLE and TWOTHIRDS make sense, but I don't see how HALVE can succeed. And why FOURTHIRDS expects 80 from original 40...
Please give me a hint.

ninomp · 2017-01-04T18:29:06Z

It seems that you forgot to write (implement) method scaleQuadruple(). It can be placed here: https://github.com/ronso0/mixxx/blob/a9284a80a4dfbde8ebaf723da5ce68aa365e247b/src/track/beatmap.cpp#L634

ninomp · 2017-01-04T18:35:43Z

Furthermore, concerning unit tests and limiting of BPM, I'm really not sure why is it like that. Sorry, you will have to wait for a more experienced Mixxx developer/contributor.

rryan · 2017-01-04T19:22:13Z

DOUBLE and TWOTHIRDS make sense, but I don't see how HALVE can succeed. And why FOURTHIRDS expects 80 from original 40...
Please give me a hint.

Wow, what the heck? I'll fix this.

rryan · 2017-01-04T19:23:16Z

DOUBLE and TWOTHIRDS make sense, but I don't see how HALVE can succeed. And why FOURTHIRDS expects 80 from original 40...
Please give me a hint.
Wow, what the heck? I'll fix this.

Oh, nevermind -- these are all continuous changes to the beatgrid. So the halve succeeds since the double scaled the original BPM x2 then halving takes it back to the original bpm.

rryan · 2017-01-04T20:00:10Z

    pMap->scale(Beats::THREEFOURTHS);
    EXPECT_DOUBLE_EQ(bpm / 2, pMap->getBpm());
+
+    pMap->scale(Beats::FOURTHIRDS);


So these two should be bpm * 2 / 3 and bpm. Please also update src/test/beatgridtest.cpp.

Oh you could also re-order three-halves to come after two-thirds so that each pair cancels out.

Applied these changes, but I'm not sure why it'll work.
Did the same in beatgridtest.cpp, but I didn't do a testbuild to check it

Oookay, think I got it! Just to bes sure:
TEST(BeatGridTest, Scale) is (like?) and function and called, therein all pGrid->scale(Beats::XXXX) are applied one after another, return values are checked in between.

Yea it's kind of confusing. Do you know how to run the tests locally?

Run scons with the test=1 flag. Then you should end up with a mixxx-test binary, so you can run ./mixxx-test to see if the tests pass. If you don't want to, then the pull request checks by AppVeyor and Travis will let us know if the tests pass anyway.

rryan · 2017-01-04T20:03:22Z

        scaleFourth();
        break;
+    case FOURTHIRDS:
+        if (getBpm() * 4 / 3 > getMaxBpm()) {


I think you can drop these checks (and the check on double)... I'm not sure why we have a max BPM setting for anything other than determining when the BPM detector has gone awry.

rryan · 2017-01-05T00:21:23Z

    EXPECT_DOUBLE_EQ(bpm * 2 / 3, pGrid->getBpm());

+    pGrid->scale(Beats::THREEHALVES);
+    EXPECT_DOUBLE_EQ(bpm * 3 / 2, pGrid->getBpm());


This should be
EXPECT_DOUBLE_EQ(bpm, pGrid->getBpm());
since scaling 3/2 after a 2/3 scale should cancel out

rryan · 2017-01-05T00:22:21Z

+    EXPECT_DOUBLE_EQ(bpm * 3 / 2, pGrid->getBpm());
+
    pGrid->scale(Beats::THREEFOURTHS);
    EXPECT_DOUBLE_EQ(bpm / 2, pGrid->getBpm());


This should be
EXPECT_DOUBLE_EQ(bpm * 3 / 4, pGrid->getBpm());

rryan · 2017-01-05T00:22:44Z

    EXPECT_DOUBLE_EQ(bpm * 2 / 3, pMap->getBpm());

+    pMap->scale(Beats::THREEHALVES);
+    EXPECT_DOUBLE_EQ(bpm * 3 / 2, pMap->getBpm());


this should be just "bpm"

rryan · 2017-01-05T00:22:52Z

+    EXPECT_DOUBLE_EQ(bpm * 3 / 2, pMap->getBpm());
+
    pMap->scale(Beats::THREEFOURTHS);
    EXPECT_DOUBLE_EQ(bpm / 2, pMap->getBpm());


this should be bpm * 3 / 4

rryan · 2017-01-05T00:22:58Z

    EXPECT_DOUBLE_EQ(bpm / 2, pMap->getBpm());
+
+    pMap->scale(Beats::FOURTHIRDS);
+    EXPECT_DOUBLE_EQ(bpm * 4 / 3, pMap->getBpm());


this should be "bpm" now

rryan

Nice, looks good. Now we can wait on Travis to make sure the tests pass there.

rryan · 2017-01-05T01:26:36Z

Hm, doh - looks like you need to define scaleQuadruple in src/track/beatmap.h around line 103.

[CXX] src\track\beatmap.cpp
beatmap.cpp
src\track\beatmap.cpp(576) : error C3861: 'scaleQuadruple': identifier not found
src\track\beatmap.cpp(626) : error C2039: 'scaleQuadruple' : is not a member of 'BeatMap'
        C:\projects\mixxx\src\track/beatmap.h(22) : see declaration of 'BeatMap'
src\track\beatmap.cpp(627) : error C2065: 'm_beats' : undeclared identifier
src\track\beatmap.cpp(627) : error C2228: left of '.first' must have class/struct/union
        type is 'unknown-type'
src\track\beatmap.cpp(629) : error C2065: 'm_beats' : undeclared identifier
src\track\beatmap.cpp(629) : error C2228: left of '.begin' must have class/struct/union
        type is 'unknown-type'
src\track\beatmap.cpp(630) : error C2065: 'm_beats' : undeclared identifier
src\track\beatmap.cpp(630) : error C2228: left of '.end' must have class/struct/union
        type is 'unknown-type'
src\track\beatmap.cpp(634) : error C2065: 'i' : undeclared identifier
src\track\beatmap.cpp(634) : error C2143: syntax error : missing ';' before ')'
src\track\beatmap.cpp(635) : error C2065: 'i' : undeclared identifier
src\track\beatmap.cpp(636) : error C2065: 'm_beats' : undeclared identifier
src\track\beatmap.cpp(636) : error C2228: left of '.insert' must have class/struct/union
        type is 'unknown-type'
scons: *** [win32_build\track\beatmap.obj] Error 2
scons: building terminated because of errors.

timrae · 2017-01-05T13:02:51Z

I didn't even know this feature existed... Why not expose some or all of the buttons in the main GUI?

ronso0 · 2017-01-05T13:17:53Z

@rryan Oh it was late, should have noticed myself. Fixed in a minute.
Did anyone ever mention this conversation & comment interface on github is ...well...not intuitive? Some comments I only came across by mail notification..
@timrae All BPM conversions are available in Track Properties and as mouse context menu in Library, what do you mean? ~~Should they apppear with the beatgrid buttons close to the waveforms?~~

timrae · 2017-01-05T13:21:12Z

what do you mean?

Oh right, I didn't realize they were available from the mouse context menu as well. I meant in the currently playing deck... For example in Deere, when you click the current bpm number, it expands to give you a bunch of bpm related options. It makes sense to put them there... I'm pretty sure Traktor had this when I used it years ago.

ronso0 · 2017-01-05T13:33:14Z

Would be useful. But I don't know how to create a CO from the above code.
Either they appear there or deck has a button that would focus deck's track in library. (like discussed over there: https://bugs.launchpad.net/mixxx/+bug/1403715)

rryan · 2017-01-05T20:08:19Z

        int distance = it->frame_position() - prevBeat.frame_position();
        Beat beat;
-        for (i=1; i<=3, i++;) {
+        for (int i=1; i<=3, i++;) {


oops, should be:

for (int i = 1; i <= 3; i++) {

yes, already fixed it after travis failed for the 3rd time ;)
~~The semicolon should be there at the end, shouldn't it?~~ Ok, not in C++
fixing once more

Previous conversions appear in the manual (p43). Necessary to update this after merge?

Good point -- it would be great if you submit a PR to update the manual too!
https://github.com/mixxxdj/manual

ronso0 · 2017-01-05T20:35:59Z

Hope it's ready now

rryan · 2017-01-05T20:43:46Z

+        // Need to not accrue fractional frames.
+        int distance = it->frame_position() - prevBeat.frame_position();
+        Beat beat;
+        for (int i=1; i<=3, i++) {


Not quite :). If you replace this line with exactly what I write it will work:

for (int i = 1; i <= 3; i++) {

Damn..spaces!
Okay, once more. Appreciate your patience!

The spaces are optional, but the initialization, the comparison, and the increment need to be separated with semicolons (not commas). Looks good now!

rryan · 2017-01-05T23:27:52Z

Looks like the tests are hanging -- I'll go ahead and merge then fix in master. Thanks @ronso0 !

ronso0 · 2017-01-06T13:00:27Z

Thank you.

…-men mixxxdj/mixxx#1106

Add BPM conversions 4/3 and 3/2

6ede5d1

ninomp reviewed Jan 3, 2017

View reviewed changes

use scaleQuadruple() instead of two scaleDouble()

a9284a8

rryan reviewed Jan 4, 2017

View reviewed changes

add scaleQuarduple(), fixed tests

219e345

rryan reviewed Jan 5, 2017

View reviewed changes

fixed beatmaptest & beatgridtest

f750ddd

rryan reviewed Jan 5, 2017

View reviewed changes

ronso0 added 3 commits January 5, 2017 14:34

add scaleQuadruple() to betamap.h

a0ddc51

fix scaleQuadruple() in beatmap.h

d16be5e

fix scaleQuadruple() in beatmap.h again

ca4b16c

rryan reviewed Jan 5, 2017

View reviewed changes

re-fix scaleQuadruple()

9fb3c0d

rryan reviewed Jan 5, 2017

View reviewed changes

re-re-fix scaleQuadruple()

70cb9f0

rryan merged commit 65b82cf into mixxxdj:master Jan 5, 2017

ronso0 deleted the additional-BPM-conversions branch January 5, 2017 23:31

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

esbrandt added a commit to esbrandt/manual that referenced this pull request Jun 24, 2017

Add note on BPM conversions 4/3 and 3/2, added to the BPM Options sub…

7d9cbf7

…-men mixxxdj/mixxx#1106

mixxxbot mentioned this pull request Aug 22, 2022

Wishlist: additional BPM conversions, i.e. 4/3 of detected BPM #8085

Closed

Uh oh!

Conversation

ronso0 commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rryan Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rryan commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ninomp commented Jan 4, 2017

Uh oh!

ninomp commented Jan 4, 2017

Uh oh!

rryan commented Jan 4, 2017

Uh oh!

rryan commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rryan left a comment

Choose a reason for hiding this comment

Uh oh!

rryan commented Jan 5, 2017

Uh oh!

timrae commented Jan 5, 2017

Uh oh!

ronso0 commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timrae commented Jan 5, 2017

Uh oh!

ronso0 commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

ronso0 commented Jan 3, 2017 •

edited

Loading

rryan Jan 4, 2017 •

edited

Loading

rryan commented Jan 3, 2017 •

edited

Loading

ronso0 commented Jan 4, 2017 •

edited

Loading

ronso0 commented Jan 5, 2017 •

edited

Loading

ronso0 Jan 5, 2017 •

edited

Loading