[WIP] Remove beatmap and beatgrid #2861

hacksdump · 2020-06-13T05:09:23Z

This is a continuation of #2512

Goals:

Address all review comments from 2512
Fix tests
Remove UI features (Since they are being developed in [WIP] Bars and downbeats for common time signature. #2844)
Add backward compatibility for functions that generate BeatGrid by generating an intermediate BeatMap before passing to Beats class.
Migrate user data from old BeatGrid & BeatMap protobufs
Break up Beats class into an inner functional class that doesn't inherit from QObject and an outer QObject wrapper.
Add protobuf definitions for TimeSignature, Phrase and Sections and write test cases to test their operations.

…//github.com/JaviVilarroig/mixxx into bars_phrases_and_variable_BPM_in_beats_class

…variable_BPM_in_beats_class

Added support in BeatGrid and some minor fixes in BeatMap.

- Added Beats.setBar The skin work is just a placeholder. I'm not so good in design.

- Extended BeatIterator for Bar and Phrase(partial) support. - Extended BeatMap for Bar and Phrase(partial) management. - Waveformrendererbeat extended to include the definition of the bar beat color in the XML skin files. - Tango Skin modified to include a button for marking bar beats.

…variable_BPM_in_beats_class

- Filename renamed to match class name - Property names improved - UItility constants renamed to fit variable coding rules

downbeat positions.

protobuff, improvement in some tooltips and name changes in some methods and properties.

- BeatMap functionality moved to Beats class - BeatGrid class removed - BeatsFactory only creates Beats - BeatGridTest removed - Beats class moved to mixxx namespace - Beats class uses frame number instead of sample number The original methods still exists. They transform sample into frames and call a method with the same name with "New" sufix. This is temporary until the rest of the code is adapted to use frames - BeatsTest adapted to new Beats class

hacksdump · 2020-06-13T05:11:39Z

The review comments in #2512 need not be repeated here. Those will be taken care of.

hacksdump · 2020-06-13T05:12:31Z

CodeFactor code check fails due to JS files.

uklotzde · 2020-06-13T07:03:17Z

@daschuer Unfortunately this branch still contains unrelated and unnecessary changes in TrackDAO that will cause merge conflicts with #2524.

I am currently checking the whole diff to master. In this special case I would propose to squash all commits, remove all unrelated changes, and restart the branch with a clean rebase on master. To prevent more of this and more serious issues that could be caused by inappropriate merge commits.

Update: This applies to all commits until and including 588b495. The following commits are safe.

build/depends.py

uklotzde · 2020-06-13T08:32:59Z

https://github.com/uklotzde/mixxx/tree/rebuild-remove-beatmap-beatgrid

Only the essential changes up to 430598d, rebased on current master
One commit (f9b4626) to fix the build
Tests are failing with a SIGSEGV

This is what I consider clean restart, though I don't have the time to preserve all the original commits. The actual number of changes is rather moderate.

Still many open issues, especially regarding the class design. The warnings about the copy constructor already indicate what is fundamentally wrong. Also the cyclic dependency on Track is unhandy and probably unnecessary. I left some TODOs on the class definition.

Be-ing · 2020-06-13T09:26:55Z

I propose "BeatVector" as a less ambiguous name for this new class.

src/proto/beats.proto

src/track/beats.cpp

Be-ing · 2020-07-16T15:14:20Z

src/track/beats.cpp

@@ -746,83 +781,85 @@ Bpm BeatsInternal::getBpmAroundPosition(FramePos curFrame, int n) const {
    stopBeat.set_frame_position(upper_bound.getValue());
    return calculateBpm(startBeat, stopBeat);
 }
-TimeSignature BeatsInternal::getSignature(FramePos frame) const {
+
+TimeSignature BeatsInternal::getSignature(int beatIndex) const {
    if (!isValid()) {


What is the meaning of an invalid time signature? A time signature with 0 as one of the numbers?

src/track/beats.cpp

daschuer · 2020-07-16T16:16:53Z

https://github.com/mixxxdj/mixxx/wiki/Beat-and-Bar-Edit-Workflow
Contains now a truly backward compatible prototype.
This requires to store the integer presentation of the frame position with the double representation.

If we don't want to be backward compatible we are free to switch over to a sparse representation.
Apart form the optimized storage space I think we have some benefits whenever we edit the beats again, because this way we see exactly what moves beats in the spares areas.

The current definition is in between without making use of one of both benefits, that's why is think we should adjust our current model.

hacksdump · 2020-07-16T16:24:23Z

The current definition is in between without making use of one of both benefits, that's why is think we should adjust our current model.

Yes. It is a hybrid between sparse representation and BeatMap.
TimeSignature, Phrase and Section are using the sparse representation model.

But the individual beats are stored like a beatmap.

hacksdump · 2020-07-16T16:26:01Z

This requires to store the integer presentation of the frame position with the double representation.

Why do we require int to be backwards compatible? We do have a migration path. So it doesn't matter what the new storage format is.

If we don't want to be backward compatible we are free to switch over to a sparse representation.

As I mentioned above, migration will take care of new data format.

src/track/beats.cpp

Be-ing · 2020-07-16T18:20:19Z

src/track/beats.cpp

+    TimeSignature timeSignatureBefore;
+    int timeSignatureBeforeMarkerIndex = 0;
+    for (const auto& timeSignatureMarker : m_timeSignatureMarkers) {


Factor this out to a function to get the time signature for any position.

daschuer · 2020-07-16T21:54:17Z

Why do we require int to be backwards compatible? We do have a migration path. So it doesn't matter what the new storage format is.

We have discussed to make the beats set by the new editor also a available for the 2.3 branch. I don't think that this is really necessary if we don't change the beat format of tracks where the beats are not touched.

Is this the case in this branch?

This will allow us to design a new pootop format from the scratch like a sparse presentation.
What do you think?

hacksdump · 2020-07-23T16:10:16Z

Closing in favour of #2961.

JaviVilarroig added 22 commits November 6, 2019 14:41

Improvement in proto for beats blob

148e216

Added Signature class for storing track time signature information

39a90b4

Added Signature class

dc8fbe7

Added Beats::getSignature

9ee4695

Merge branch 'bars_phrases_and_variable_BPM_in_beats_class' of https:…

05a4720

…//github.com/JaviVilarroig/mixxx into bars_phrases_and_variable_BPM_in_beats_class

Fix repo

2d38e75

Merge remote-tracking branch 'upstream/master' into bars_phrases_and_…

c1428f0

…variable_BPM_in_beats_class

Added Signature manipulation to BeatGrid.

590be5d

BeatGrid/BeatMap Signature support

b2c9eca

Added support in BeatGrid and some minor fixes in BeatMap.

Removed forgotten qDebug unneeded trace

0084ddc

- Added button in Tango skin for setting bar beat

c8b8ae4

- Added Beats.setBar The skin work is just a placeholder. I'm not so good in design.

Merge remote-tracking branch 'upstream/master' into bars_phrases_and_…

87cc607

…variable_BPM_in_beats_class

Adds null_signature and default_signature constants.

01583e8

Rename Signature to TimeSignature in protobuff and wrapper class.

b533ac4

Fixes in TimeSignature class.

72e030a

- Filename renamed to match class name - Property names improved - UItility constants renamed to fit variable coding rules

Protect naked pointers in BpmControl.

9d652ca

BeatMap::SetBar to use the real Signature of the track to calculate the

91c9930

downbeat positions.

Fix issue with set bar button nor properly reacting to press an hover.

df0f083

Multiple small fixes on Beats. Added default value to TimeSignature

26b358f

protobuff, improvement in some tooltips and name changes in some methods and properties.

Forgoten update on cmake list for previous commit

48ec71b

hacksdump marked this pull request as draft June 13, 2020 05:09

hacksdump changed the title ~~Remove beatmap and beatgrid~~ [WIP] Remove beatmap and beatgrid Jun 13, 2020

uklotzde reviewed Jun 13, 2020

View reviewed changes

build/depends.py Outdated Show resolved Hide resolved

revert all UI changes

7302922

crisclacerda reviewed Jul 15, 2020

View reviewed changes

src/proto/beats.proto Show resolved Hide resolved

use beat indices as marker basis

77c8d82

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/proto/beats.proto Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/proto/beats.proto Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Show resolved Hide resolved

hacksdump added 4 commits July 16, 2020 22:12

change section message

1e2565a

remove redundant ctor initializer

cc0d8ef

remove legacy name from comment

de26dd4

add debug asserts

e76bfc3

Be-ing reviewed Jul 16, 2020

View reviewed changes

src/track/beats.cpp Show resolved Hide resolved

Be-ing reviewed Jul 16, 2020

View reviewed changes

add placeholder parameters

1967c0f

hacksdump mentioned this pull request Jul 23, 2020

New Beats with UI [GSoC 2020] #2961

Draft

8 tasks

hacksdump closed this Jul 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Remove beatmap and beatgrid #2861

[WIP] Remove beatmap and beatgrid #2861

hacksdump commented Jun 13, 2020 •

edited

Loading

hacksdump commented Jun 13, 2020

hacksdump commented Jun 13, 2020

uklotzde commented Jun 13, 2020 •

edited

Loading

uklotzde commented Jun 13, 2020

Be-ing commented Jun 13, 2020

Be-ing Jul 16, 2020

daschuer commented Jul 16, 2020 •

edited

Loading

hacksdump commented Jul 16, 2020

hacksdump commented Jul 16, 2020 •

edited

Loading

Be-ing Jul 16, 2020 •

edited

Loading

daschuer commented Jul 16, 2020

hacksdump commented Jul 23, 2020

[WIP] Remove beatmap and beatgrid #2861

[WIP] Remove beatmap and beatgrid #2861

Conversation

hacksdump commented Jun 13, 2020 • edited Loading

hacksdump commented Jun 13, 2020

hacksdump commented Jun 13, 2020

uklotzde commented Jun 13, 2020 • edited Loading

uklotzde commented Jun 13, 2020

Be-ing commented Jun 13, 2020

Be-ing Jul 16, 2020

Choose a reason for hiding this comment

daschuer commented Jul 16, 2020 • edited Loading

hacksdump commented Jul 16, 2020

hacksdump commented Jul 16, 2020 • edited Loading

Be-ing Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

daschuer commented Jul 16, 2020

hacksdump commented Jul 23, 2020

hacksdump commented Jun 13, 2020 •

edited

Loading

uklotzde commented Jun 13, 2020 •

edited

Loading

daschuer commented Jul 16, 2020 •

edited

Loading

hacksdump commented Jul 16, 2020 •

edited

Loading

Be-ing Jul 16, 2020 •

edited

Loading