[WIP] Bars phrases and variable bpm in beats class #2512

JaviVilarroig · 2020-02-22T17:03:37Z

This is still a WIP.
On BeatMaps, the infrastructure for adding variable Signature and BPM data and to mark beat as a bar beat or a phrase beat is mostly done.
On BeatGrids, the infrastructure for adding fixed Signature data and to indicate where to count bar and phrase beats is mostly done.
I have reached the point where it's possible to mark a beat as bar beat and to show them in the waveformrenderer. I have updated Tango skin (disclaimer: I'm not a designer, so it's ugly) to allow to mark bar beats.
By pressing the added button in the beat management interface, the nearest beat becomes a bar beat and all the following beats are recalculated accordingly up to the end of the track or the next phrase bar.
Bar beats are painted in a color defined in the skin, in my example in dim yellow.
Now I will move to add a button for phrase beats. But in the meantime I will love to have your review of the changes done until now.

…//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

daschuer · 2020-02-22T17:55:11Z

Don't worry, just ignore them.

src/proto/beats.proto

res/skins/Tango/buttons/btn_beats_set_bar_beat.svg

src/proto/beats.proto

Be-ing · 2020-02-25T01:25:20Z

The new button to set the bar start beat shows as permanently pressed and pressing it does nothing:

This is with a new database. Tested with both BeatGrid and BeatMap.

src/proto/beats.proto

src/track/beatmap.cpp

daschuer

Some more comments.

daschuer · 2020-02-24T07:11:56Z

res/skins/Tango/buttons/btn_beats_set_bar_beat.svg

+     d="M 12.52,33.264 H 11.4809 L 9.0746,11.116 C 8.96522,10.14989 8.91053,9.4298 8.91054,8.9558 c -2.6e-6,-1.1484 0.30077,-2.069 0.90234,-2.7617 0.61978,-0.69267 1.3581,-1.039 2.2148,-1.0391 0.85676,3.8e-5 1.5768,0.34639 2.1602,1.0391 0.60155,0.69274 0.90233,1.7227 0.90234,3.0898 -5.2e-5,0.43753 -0.03646,1.0482 -0.10938,1.832 l -2.4609,22.148 m -0.54688,4.5391 c 0.83853,5e-6 1.5495,0.30079 2.1328,0.90234 0.60155,0.58334 0.90233,1.2852 0.90234,2.1055 -1.3e-5,0.83854 -0.30079,1.5586 -0.90234,2.1602 -0.58334,0.58333 -1.2943,0.875 -2.1328,0.875 -0.83855,-1e-6 -1.5586,-0.29167 -2.1602,-0.875 -0.58334,-0.60156 -0.87501,-1.3216 -0.875,-2.1602 -2e-6,-0.82031 0.29166,-1.5221 0.875,-2.1055 0.60156,-0.60156 1.3216,-0.90234 2.1602,-0.90234"
+     style="fill:#ff3232;fill-opacity:1"
+     inkscape:connector-curvature="0" />
+</svg>


Did you consider a bar symbol cut from staff lines with the lines in background? Is there anything else what we can use? IMHO the explanation mark is ambiguous.

Or maybe we can use a whole measure for a beat, and four measures for a phrase.
... Or one measure with a clef.

Ideas?

Bufff. As I said, I consider myself a competent developer, just a little bit rusty.
But I'm a terrible designer. I will be very happy if someone with good designer skill can help on tat. I'm open to any kind of proposal.

Did you consider a bar symbol cut from staff lines with the lines in background?

I like that idea. Maybe include a treble clef on the staff to clarify what it is. I am struggling to come up with an idea for a phrase icon though. @ronso0 do you have any ideas or want to contribute icons?

src/engine/controls/bpmcontrol.cpp

JaviVilarroig · 2020-02-25T15:44:40Z

The new button to set the bar start beat shows as permanently pressed and pressing it does nothing:

This is with a new database. Tested with both BeatGrid and BeatMap.

Curious, it works without problem for me.

I had some problems to create BeatMaps. To make sure you are not in a BeatGrid, press the button and look at the log. When in a BeatGrid clicking in the button generates a one line message "STUB BeatGrid::setBar".

Can you please check?

Be-ing · 2020-02-26T02:44:42Z

I thought we were planning to replace BeatMap and BeatGrid with a new class that handles all cases. I think it is a waste of time to work on both of them. BeatMap may be closer to what we need in the end.

JaviVilarroig · 2020-02-26T10:21:06Z

I thought we were planning to replace BeatMap and BeatGrid with a new class that handles all cases. I think it is a waste of time to work on both of them. BeatMap may be closer to what we need in the end.

I agree on the point that BeatGrid is not really useful. It has a lot of limitations.

Must I read that as a proposal for a full revamp of the Beat class?

Be-ing · 2020-02-26T10:42:36Z

Must I read that as a proposal for a full revamp of the Beat class?

Yes. You could remove BeatGrid and make Beats into what BeatMap is now instead of keeping Beats as an abstract base class.

Be-ing · 2020-06-11T07:09:48Z

src/track/beats.h

+    /// Return a string that represent the preferences used to generate
+    /// the beats object.
+    virtual QString getSubVersion() const;
+    virtual void setSubVersion(QString subVersion);


I don't understand the purpose of this subversion concept. The version specifies the protobuf version so we can upgrade old data from the database to new versions in the future. What does a subversion add to that? I think we can remove it.

I am not sure how database migrations will work if we remove the subversion concept (along with versions as well).
Can this lead to problems?

Be-ing · 2020-06-11T07:10:00Z

src/track/beats.h

+    /// Returns a string representing the version of the beat-processing code that
+    /// produced this Beats instance. Used by BeatsFactory for associating a
+    /// given serialization with the version that produced it.
+    virtual QString getVersion() const;


QString? Shouldn't this be an int?

static const QString BEAT_MAP_VERSION; static const QString BEAT_GRID_1_VERSION; static const QString BEAT_GRID_2_VERSION;

These will eventually be removed so I won't change anything about them.
We will rely on the preference settings just to check whether the user wants constant or dynamic BPM. BeatsFactory will read the preference instead of this version.

Do we agree on entirely removing the Beats versioning?

No, we cannot upgrade old data without versioning. The tricky situation we face now is that the legacy formats used a string to encode 2 different pieces of information, the version and the type (BeatMap or BeatGrid). Future upgrades will be more straightforward if we simply use an int to specify the version.

Be-ing · 2020-06-11T07:37:36Z

src/track/beats.cpp

+    double previous_beatpos = -1;
+    track::io::Beat beat;
+
+    foreach (double beatpos, beats) {


foreach is a legacy Qt macro from before C++11. Use standard for (double beatpos : beats)

daschuer

58be257
Is a really problematic commit.
It touches a lot of unrelated lines.

It looks like as if you have applied accidentally clang-format to many files.

There is a big risk that this has broken something. At least it is very hard to verify this in a review.

I am unsure how to proceed here. Mabe we can rebase this branch and redo the merge. But that involved another risk. I am unsure.

src/analyzer/analyzerbeats.cpp

src/engine/controls/bpmcontrol.cpp

hacksdump · 2020-06-11T23:10:37Z

I would much prefer to get this into master ASAP. I hope that we can do that within the next week or sooner.

I'll be working to fix the outstanding issues and review comments on this PR this week and will try to get this merged by the weekend or the beginning of next week.

daschuer · 2020-06-11T23:18:54Z

1851baf
is the real faulty commit. I will try to remove it.

uklotzde · 2020-06-12T00:19:31Z

We need to rebuild this branch starting before the wrong merge from master in 1851baf! @daschuer Thanks for noticing and investigating.

uklotzde · 2020-06-12T00:26:20Z

Manually picking and re-committing changes from another branch is also not safe imho: d0e4b12

daschuer · 2020-06-12T13:37:59Z

This is a branch that removes the faulty merge commit and also some issues from a previous master merge:
https://github.com/daschuer/mixxx/tree/bars_phrases_and_variable_BPM_in_beats_class2

Please reset this branch to mine and push to this PR.

If you have checked out this branch you can do it as follows:

git checkout -b bars_phrases_and_variable_BPM_in_beats_class_backup
git checkout bars_phrases_and_variable_BPM_in_beats_class
git pull https://github.com/daschuer/mixxx.git bars_phrases_and_variable_BPM_in_beats_class2
git reset 588b4957e87b896eda8401ad28b8a5dde9ec675b --hard
git push -f

to check the changes

git diff bars_phrases_and_variable_BPM_in_beats_class_backup

I am unsure about resolving conflicts in
res/skins/Tango/waveform.xml
I think that needs to be adjusted.

Be-ing · 2020-06-12T18:37:06Z

I am unsure about resolving conflicts in
res/skins/Tango/waveform.xml
I think that needs to be adjusted.

All GUI changes in this branch should be reverted. The GUI is implemented in #2844.

daschuer · 2020-06-13T10:50:43Z

OK, So please wait with the reset.
I will rebase my branch again, removing an earlyer merge commit as well.

By the way: for merging I use SmartGit. It works perfectly for resolving conflicts and is free for FOSS projects.

hacksdump · 2020-06-13T16:08:41Z

OK, So please wait with the reset.

Cool. I'll wait for the tree cleanup. Please let me know if I can help with the process.

By the way: for merging I use SmartGit. It works perfectly for resolving conflicts and is free for FOSS projects.

Thanks. This is really awesome.

JaviVilarroig · 2020-06-14T12:06:34Z

Hi everybody.
This PR has become urgent due to the dependencies from the GSoC projects. And I have limited time to dedicate to it due to work and family issues. So I see that I can't offer the responsiveness that is required.
In different conversations with @hacksdump and @crisclacerda they have shown their willingness to use this PR as a base for their works so I propose to transfer the responsibility for this PR to them. I'm assuming that they are OK with this transfer (Can you confirm?)
This will facilitate the work in the GSoC projects, that are heavily time bounded
I'm not sure how to do it technically in GitHub. Any suggestion?
On my hand I can move to some other topics that I have in my personal todo list for Mixxx.
Feedback please.

uklotzde · 2020-06-14T12:50:41Z

@JaviVilarroig Thanks for passing the baton to our GSoC students. I am confident they are able to finish the work you have started.

Unfortunately this branch is unmergeable due some errors that happened while resolving merge conflicts along the way. As mentioned in #2816 I have aggregated all necessary changes into a single commit and applied a final fix commit on top of that, see the branch uklotzde/mixxx/tree/rebuild-remove-beatmap-beatgrid. But I would like to keep you mentioned as the author, because it was not me that did all this work!

How about this pragmatic solution for a clean restart:

You create create a new branch based on master
Cherry pick the 1st commit from uklotzde/rebuild-remove-beatmap-beatgrid, i.e. 7b1c605
Reset the branch to master while keeping the changes, i.e. mixed
Create a new commit from these changes, so that you become the author of all these changes
Finally cherry-pick the 2nd commit from uklotzde/rebuild-remove-beatmap-beatgrid, i.e. b73d015

Afterwards @hacksdump would be able to rebase his changes from #2861 on your branch and we are ready to go.

Also asking @daschuer @hacksdump @crisclacerda for comments.

Holzhaus · 2020-06-14T13:56:36Z

@uklotzde if it's just about authorship, you can just use:

git commit --amend --author="Author Name <email@address.com>" --no-edit

uklotzde · 2020-06-14T14:46:36Z

@Holzhaus Done.

I have force pushed uklotzde/mixxx/tree/rebuild-remove-beatmap-beatgrid, now with @JaviVilarroig as the author of the aggregate commit and a commit message that matches the title and number of this PR.

Be-ing · 2020-06-14T14:50:27Z

Thank you @uklotzde. @hacksdump please rebase your commits in #2861 onto @uklotzde's branch then force push #2861 and we can continue from there.

JaviVilarroig · 2020-06-14T15:41:44Z

Does that mean that I must take no action?

Be-ing · 2020-06-14T15:43:21Z

Correct, there's nothing you need to do @JaviVilarroig.

JaviVilarroig · 2020-06-14T16:16:06Z

Good Copy.
Do I am supossed to close that PR? Just for cleanup.

Be-ing · 2020-06-14T16:34:10Z

Sure, we can close this now in favor of #2861.

daschuer · 2020-06-14T21:42:10Z

I have work in progress for a clean rebase of this branch.
Unfortunately tests are failing.

daschuer · 2020-06-14T23:06:30Z

The clean rebase is here:
#2869

JaviVilarroig added 13 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

This comment has been minimized.

Sign in to view

Holzhaus requested changes Feb 22, 2020

View reviewed changes

src/proto/beats.proto Show resolved Hide resolved

Holzhaus requested changes Feb 23, 2020

View reviewed changes

res/skins/Tango/buttons/btn_beats_set_bar_beat.svg Outdated Show resolved Hide resolved

Holzhaus added the engine label Feb 23, 2020

Be-ing reviewed Feb 25, 2020

View reviewed changes

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

Be-ing reviewed Feb 25, 2020

View reviewed changes

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

Be-ing reviewed Feb 25, 2020

View reviewed changes

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

Be-ing reviewed Feb 25, 2020

View reviewed changes

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

Adds null_signature and default_signature constants.

01583e8

daschuer requested changes Feb 25, 2020

View reviewed changes

Rename Signature to TimeSignature in protobuff and wrapper class.

b533ac4

Be-ing reviewed Jun 11, 2020

View reviewed changes

daschuer requested changes Jun 11, 2020

View reviewed changes

src/analyzer/analyzerbeats.cpp Show resolved Hide resolved

src/engine/controls/bpmcontrol.cpp Show resolved Hide resolved

hacksdump mentioned this pull request Jun 13, 2020

[WIP] Remove beatmap and beatgrid #2861

Closed

7 tasks

Be-ing closed this Jun 14, 2020

daschuer mentioned this pull request Jun 14, 2020

Rebase of: Bars phrases and variable bpm in beats class2 #2869

Closed

hacksdump mentioned this pull request Jul 23, 2020

New Beats with UI [GSoC 2020] #2961

Draft

8 tasks

JaviVilarroig deleted the bars_phrases_and_variable_BPM_in_beats_class branch November 7, 2021 16:14

This was referenced Aug 22, 2022

Emphasize the downbeats of a track by a separate marker on the waveform #6919

Closed

Feature Request: Time aligned mouse transition #8749

Closed

[WIP] Bars phrases and variable bpm in beats class #2512

[WIP] Bars phrases and variable bpm in beats class #2512

Conversation

JaviVilarroig commented Feb 22, 2020

This comment has been minimized.

daschuer commented Feb 22, 2020

Be-ing commented Feb 25, 2020 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaviVilarroig Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaviVilarroig commented Feb 25, 2020

Be-ing commented Feb 26, 2020

JaviVilarroig commented Feb 26, 2020

Be-ing commented Feb 26, 2020

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

daschuer left a comment

Choose a reason for hiding this comment

hacksdump commented Jun 11, 2020

daschuer commented Jun 11, 2020

uklotzde commented Jun 12, 2020

uklotzde commented Jun 12, 2020 • edited Loading

daschuer commented Jun 12, 2020

Be-ing commented Jun 12, 2020

daschuer commented Jun 13, 2020

hacksdump commented Jun 13, 2020 • edited Loading

JaviVilarroig commented Jun 14, 2020

uklotzde commented Jun 14, 2020

Holzhaus commented Jun 14, 2020

uklotzde commented Jun 14, 2020

Be-ing commented Jun 14, 2020

JaviVilarroig commented Jun 14, 2020

Be-ing commented Jun 14, 2020

JaviVilarroig commented Jun 14, 2020

Be-ing commented Jun 14, 2020

daschuer commented Jun 14, 2020

daschuer commented Jun 14, 2020

Be-ing commented Feb 25, 2020 •

edited

Loading

JaviVilarroig Feb 25, 2020 •

edited

Loading

uklotzde commented Jun 12, 2020 •

edited

Loading

hacksdump commented Jun 13, 2020 •

edited

Loading