Add new beats implementation #4255

Holzhaus · 2021-08-31T22:29:52Z

This adds a new beats implementation that is supposed to replace the other two implementations (obligatory xkcd).

I'm still experimenting with the design and there's still a bunch of stuff missing or not properly tested, but in case someone already wants to leave some comments, feel free to do so.

To Do:

Fix Serato BeatGrid import/export
~~Fix test BeatMapTest.TestBpmAround~~ Replaced with new tests that are verifyable by hand (e000ebf)
Fix test EngineSyncTest.HalfDoubleConsistency
Fix test EngineSyncTest.BeatMapQuantizePlay
Fix test SeratoBeatGridTest.SerializeBeatMap
Based on Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361, which should be merged first.
Calculate predominant BPM instead of arithmetic average in getBpmInRange()

Be-ing · 2021-09-01T19:19:13Z

ping @hacksdump

Holzhaus · 2021-09-01T19:25:51Z

ping @hacksdump

FWIW the new implementation is not really related to the old one. @hacksdump used a list of (index, bpm) tuples to define a beatgrid. This implementation uses (position, beatsTillNextMarker) tuples instead.

daschuer · 2021-10-07T18:09:22Z

How about splitting out the first commits? For me it looks like they are already mature.

JoergAtGithub · 2021-10-07T18:14:49Z

How about splitting out the first commits? For me it looks like they are already mature.

Isn't this done in #4361 ?

daschuer · 2021-10-07T18:22:59Z

Oh yes. Thank you for the hint.

daschuer · 2021-10-07T18:36:49Z

Can you brief describe how the transition from BeatGrid and BeatMap into NewBeats and into a measure based format will work?
Regarding loading and storing beats.

I like the idea of frame based anchor points, because it is native in then engine. However, we had some voices pro a seconds based format.

I am missing the info how measures will be coded.

I think we need to to consolidate a plan, for the final format to have the chance to discuss implications before the code is written.
Do we have one?

Holzhaus · 2021-10-13T11:29:24Z

The last remaining test that fails is EngineSyncTest.HalfDoubleConsistency which suffers from a slight rounding issue:

src/test/enginesynctest.cpp:2018: Failure
Expected equality of these values:
  90.0
    Which is: 90
  ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get()
    Which is: 89.999998686441671
src/test/enginesynctest.cpp:2020: Failure
Expected equality of these values:
  180.0
    Which is: 180
  ControlObject::getControl(ConfigKey(m_sGroup2, "bpm"))->get()
    Which is: 179.99999737288334
[  FAILED  ] EngineSyncTest.HalfDoubleConsistency (62 ms)

This is supposed to combine the beatmap/beatgrid classes into a single, versatile implementation.

…ctor

The tests fails although the results are roughly in the same ballpark (+/- 4 BPM): src/test/beatmaptest.cpp:249: Failure Expected equality of these values: 63.937645572318047 pMap->getBpmAroundPosition( mixxx::audio::kStartFramePos + 4 * approx_beat_length, 4) .value() Which is: 64.419153961777965 src/test/beatmaptest.cpp:254: Failure Expected equality of these values: 118.96668932698844 pMap->getBpmAroundPosition( mixxx::audio::kStartFramePos + 60 * approx_beat_length, 4) .value() Which is: 122.00000000000068 src/test/beatmaptest.cpp:260: Failure Expected equality of these values: 62.937377309576974 pMap->getBpmAroundPosition(mixxx::audio::kStartFramePos, 4).value() Which is: 60.731050014550149 src/test/beatmaptest.cpp:262: Failure Expected equality of these values: 118.96668932698844 pMap->getBpmAroundPosition( mixxx::audio::kStartFramePos + 65 * approx_beat_length, 4) .value() Which is: 122.00000000000068 Due to the test design it's not possible to verify these floating point results manually (on paper), and it's likely that these issues are caused by rounding the gradually increasing beat lengths. The BPM calculation methods are already tested in `BeatsTest`, so we just remove this.

Without this patch, every beats instance created from a beats vector would be stored a `BeatMap` in the database, even if a `BeatGrid` would suffice. This also "fixes" the broken `EngineSyncTest.HalfDoubleConsistency` test which suffered from a minor rounding issue in `Beats::getBpmAroundPosition`, which is causes by the initial play position of an `EngineBuffer` being set to `std::numeric_limits<double>::lowest()`, which leads to slightly inaccurate floating point results in `getBpmAroundPosition()`. Because this commit makes the beats instance used in the test have a constant tempo, this "fixes" the issue because the `getBpmAroundPosition()` can exit early and just return the (accurate) end marker BPM.

This method should return the predominant BPM value instead.

ywwg · 2021-10-21T21:38:00Z

The last remaining test that fails is EngineSyncTest.HalfDoubleConsistency which suffers from a slight rounding issue:

let me know if you need any help with this enginesync test, I know they can be tricky.

Holzhaus · 2021-10-21T21:50:48Z

The last remaining test that fails is EngineSyncTest.HalfDoubleConsistency which suffers from a slight rounding issue:

let me know if you need any help with this enginesync test, I know they can be tricky.

Thanks. The issue is kind of resolved for now, as explained here: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/beat.20grid.20revamp.20re.3A.20pr.20.232961/near/258174413

The root cause we miniscule rounding issues because engine buffer initializes the play position with -DBL_MIN. The actual fix would be to not use such extreme values.

Holzhaus · 2021-10-22T21:08:15Z

Can you brief describe how the transition from BeatGrid and BeatMap into NewBeats and into a measure based format will work? Regarding loading and storing beats.

Right now, the conversion is performed on load. The storage format is not touched, it stays the same. We only convert it on load. This is why the chance of breaking something is small, in the worst case we could just revert the merge and not breaking our user's beatgrids.

I like the idea of frame based anchor points, because it is native in then engine. However, we had some voices pro a seconds based format.

We can consider that, but as this PR does not change the storage format and still loads/writes legacy beatgrids/beatmaps, this is out of scope here and can be discussed later on.

I am missing the info how measures will be coded.

Measures/downbeats are not supported yet, but could be added by adding support for it using the iterator API. Instead of *it returning a FramePos, we could instead return a Beat object with members like beatIndexInBar, timeSignature, isDownbeat, etc.

This will be considerably easier if we enforce that BeatMarkers (consisting of position and beatsTillNextMarker members) only occur at downbeats (which also means that beatsTillNextMarker needs to be a multiple of the number of beats per bar).
Currently this is not the case yet.

I think we need to to consolidate a plan, for the final format to have the chance to discuss implications before the code is written. Do we have one?

As discussed previously on Zulip, we should merge the two implementations into a single one first, without changing the stored data format.

When that is done, we can add some additional attributes such as downbeats/timeSignature as dummy values in memory, still all without changing the stored data format.

Then we can already start working on an editing UI, and detect issues with the in-memory representation early on, without migrating user-data.

After (or maybe even in parallel to) the editing UI development, someone could start working on moving away from the legacy storage formats (beatgrid/beatmap) and implement a native data format. Of course, we should only merge that if we are sure that the format suffices, because this is will make people lose their beatgrids if they downgrade to an older mixxx version.

So the actual data format change is actually the very last step. Of course this needs to be discussed properly.

ywwg · 2021-10-23T14:26:32Z

I played around with this branch and it seems to work well! And propagating beats before the beginning for beatmaps is super great :). I don't really have any comments on the technical implementation other than to say thank you for the new tests.

Be-ing · 2021-10-23T14:27:08Z

There are some conflicts now.

Holzhaus · 2021-10-25T21:37:45Z

There are some conflicts now.

Resolved.

daschuer

LGTM and works also good.
Thank you very much.

Holzhaus added the engine label Aug 31, 2021

github-actions bot added build code quality labels Aug 31, 2021

Holzhaus force-pushed the newbeats branch 5 times, most recently from c289f6e to 603d100 Compare September 1, 2021 19:17

Holzhaus force-pushed the newbeats branch 2 times, most recently from 791e15c to d35d388 Compare September 2, 2021 12:02

Holzhaus force-pushed the newbeats branch from d35d388 to 01f510a Compare September 9, 2021 13:30

github-actions bot added the library label Sep 9, 2021

Holzhaus force-pushed the newbeats branch 3 times, most recently from 753afde to 7e6bbd5 Compare September 9, 2021 23:33

Holzhaus force-pushed the newbeats branch 3 times, most recently from c295621 to d762f97 Compare October 6, 2021 22:35

Holzhaus force-pushed the newbeats branch 3 times, most recently from da8e90a to e000ebf Compare October 13, 2021 11:05

Holzhaus mentioned this pull request Oct 13, 2021

Beats: Make return value of mutation methods optional #4409

Merged

Holzhaus mentioned this pull request Oct 14, 2021

Beats: Refactor getBpm() into getBpmInRange(startPos, endPos) #4361

Merged

Holzhaus added 13 commits October 21, 2021 00:13

Beats: Add new unified beats implementation

4ffe76c

This is supposed to combine the beatmap/beatgrid classes into a single, versatile implementation.

Beats: Implement methods to create object from const tempo or beat ve…

b813753

…ctor

Beats: Implement BPM scaling

a2c8c1b

Beats: Use new beats implementation for beatgrids

00e80d5

Beats: Remove old BeatMap/BeatGrid classes

466b425

BeatMapTest: Fix broken TestNthBeat test

4e9ffdc

Beats: Merge new beats implementation into abstract Beats class

9dbe081

Beats: Add methods to access underlying data structures as tests

1a66fee

BeatMapTest: Fix number of beats in scale test

766f4f5

Beats: Fix Serato BeatGrid import

a1b9b43

BeatsTest: Add more tests for getBpm methods

e4f4460

Holzhaus force-pushed the newbeats branch from f35ca0f to e575f27 Compare October 20, 2021 22:45

Beats: Don't return arithmetic average from getBpmInRange()

7089250

This method should return the predominant BPM value instead.

Holzhaus marked this pull request as ready for review October 21, 2021 13:31

Holzhaus changed the title ~~[WIP] Add new beats implementation~~ Add new beats implementation Oct 21, 2021

Holzhaus mentioned this pull request Oct 22, 2021

Fuzzy beats fix. #4366

Merged

Merge branch 'main' of github.com:mixxxdj/mixxx into newbeats

bf92f0d

daschuer approved these changes Oct 25, 2021

View reviewed changes

daschuer merged commit cb2a2c8 into mixxxdj:main Oct 25, 2021

Holzhaus mentioned this pull request Oct 18, 2024

Beats: Add editing controls for bar annotations #13330

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new beats implementation #4255

Add new beats implementation #4255

Holzhaus commented Aug 31, 2021 •

edited

Loading

Be-ing commented Sep 1, 2021

Holzhaus commented Sep 1, 2021 •

edited

Loading

daschuer commented Oct 7, 2021

JoergAtGithub commented Oct 7, 2021

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Oct 13, 2021

ywwg commented Oct 21, 2021

Holzhaus commented Oct 21, 2021

Holzhaus commented Oct 22, 2021 •

edited

Loading

ywwg commented Oct 23, 2021

Be-ing commented Oct 23, 2021

Holzhaus commented Oct 25, 2021 •

edited

Loading

daschuer left a comment

Add new beats implementation #4255

Add new beats implementation #4255

Conversation

Holzhaus commented Aug 31, 2021 • edited Loading

Be-ing commented Sep 1, 2021

Holzhaus commented Sep 1, 2021 • edited Loading

daschuer commented Oct 7, 2021

JoergAtGithub commented Oct 7, 2021

daschuer commented Oct 7, 2021

daschuer commented Oct 7, 2021

Holzhaus commented Oct 13, 2021

ywwg commented Oct 21, 2021

Holzhaus commented Oct 21, 2021

Holzhaus commented Oct 22, 2021 • edited Loading

ywwg commented Oct 23, 2021

Be-ing commented Oct 23, 2021

Holzhaus commented Oct 25, 2021 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Aug 31, 2021 •

edited

Loading

Holzhaus commented Sep 1, 2021 •

edited

Loading

Holzhaus commented Oct 22, 2021 •

edited

Loading

Holzhaus commented Oct 25, 2021 •

edited

Loading