Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1/12 Bpm rounding #3790

Merged
merged 7 commits into from May 12, 2021
Merged

1/12 Bpm rounding #3790

merged 7 commits into from May 12, 2021

Conversation

daschuer
Copy link
Member

This uses the BeatUtils::roundBpmWithinRange() function for bpm adjusting and tabbing.

Bpm adjustment is now done in 1/48 steps, reaching full integer BPM;
Tabing is rounded to 1/12, which allows to tab full integer BPM with a bit practice.

@daschuer daschuer changed the base branch from main to 2.3 April 14, 2021 21:01
@daschuer daschuer mentioned this pull request Apr 14, 2021
@uklotzde uklotzde added this to the 2.3.0 milestone Apr 16, 2021
@daschuer
Copy link
Member Author

@poelzi: Did this work for you or is there anything to improve?

@Holzhaus
Copy link
Member

Holzhaus commented May 5, 2021

ping @poelzi

@Holzhaus
Copy link
Member

Holzhaus commented May 8, 2021

@daschuer Do you expect any fallout from this? What was the previous tapping resolution?
I can test this if you want.

@daschuer
Copy link
Member Author

daschuer commented May 8, 2021

Before, the was no rounding during tapping.

@daschuer
Copy link
Member Author

daschuer commented May 8, 2021

The other thing that can be tested is shrinking and widening the beat grid using the controls next to the waveform.
Is it too coarse / fine?

@ronso0
Copy link
Member

ronso0 commented May 9, 2021

With the previous tapping algorhythm you could never tap rounded integers -- even if it matched once you would mess it up again with just one more tap
https://bugs.launchpad.net/mixxx/+bug/1882776

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

Is it just me or do the bpm adjustment buttons don't work anymore? Shifting the beatgrid works, but shrinking/growing doesn't make any difference even after pressing the button 20+ times (not even when I scroll to the track end, where changes should be more obvious).

Re: Tapping. Do we have a GUI button for that? If so, I can't find it.

@JoergAtGithub
Copy link
Member

Re: Tapping. Do we have a GUI button for that? If so, I can't find it.

Click on the BPM value

@Holzhaus
Copy link
Member

Holzhaus commented May 9, 2021

Ok, in that case BPM tapping is broken too and clicking the bpm value repeatedly has no effect.

@ronso0
Copy link
Member

ronso0 commented May 9, 2021

Tapping works fine for me (LateNight) and I can edit the beatgrid as before.

But indeed beats_adjust_faster/slower is to coarse now when you need to correct a track that's just slightly off integer BPM

@daschuer
Copy link
Member Author

daschuer commented May 9, 2021

I have restored the 0.01 step width.

But playing with it, I think the feature does not really match the use case. It has never the right step width and the effect depends strong to the bpm and the length of the track.

So I think we can do better. (Not in this PR).

My use case for const tracks is the following:

  • Verify a the first beat
  • adjust the offset
  • Seek through the track towards the end
  • Verify if the markers are still on the beats.
  • Adjust the BPM

Issues:

  • In case it is fully off and the track is short the steps are too fine
  • In case the it is only slightly off and the rack is long, the steps are too big.

What I really want:

  • Set the a beat marker and adjust the BPM to match my first and my last marker.
  • I think we can ditch the BPM adjustment buttons for this.

What do you think?

@Holzhaus
Copy link
Member

Holzhaus commented May 10, 2021

I have no strong opinion on this as the BPM Analyzer mostly does its job correctly for my tracks and I usually only see cases where boom/tshack is misdetected (so I usually match phase manually and then apply the phase correction from the other deck).

But I agree that this "keyframe-like" workflow would be much more straighforward. IMHO we shouldn't invest too much time in this now. We have the #2961, which we should focus on, and we already have a lot of merge conflicts. Let's get that PR into a mergeable state and then follow up with the improved workflow (setting beat markers and interpolation beats in between) for adjusting beatgrids.

@daschuer
Copy link
Member Author

Yes, right at least this it not a 2.3 topic, like this PR.

@ronso0
Copy link
Member

ronso0 commented May 10, 2021

@Holzhaus so you can tap and adjust with this branch?
I'll test again tonight if necessary.

@ronso0
Copy link
Member

ronso0 commented May 11, 2021

so this also fixes https://bugs.launchpad.net/mixxx/+bug/1882776

@ronso0
Copy link
Member

ronso0 commented May 11, 2021

ux still LGTM

@Holzhaus
Copy link
Member

Is it just me or do the bpm adjustment buttons don't work anymore? Shifting the beatgrid works, but shrinking/growing doesn't make any difference even after pressing the button 20+ times (not even when I scroll to the track end, where changes should be more obvious).

BPM tapping is broken too and clicking the bpm value repeatedly has no effect.

I was wrong. Both work fine if (and only if) the track has a beatgrid with constant tempo. I accidently tested on a track where "Assume constant tempo" was not checked, so it didn't work and nothing happens.

Is it is feasible to disable (grey out) the beatgrid adjustment buttons? It's pretty non-obvious that this doens't work because the track has a non-const beatgrid.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.


struct BeatGridData {
double bpm;
double firstBeat;
};

constexpr int kFrameSize = 2;
constexpr double kBpmScaleRounding = 0.001;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this was 1/48 in this PR, which is a multiple of 1/12. Is there a reason for this? If so, have you considered 1/960 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, this can be done into a follow-up PR if needed. I'm going to merge this now.

@Holzhaus Holzhaus merged commit 54652f2 into mixxxdj:2.3 May 12, 2021
@daschuer daschuer deleted the bpmRounding branch September 26, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants