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

Fix const beat grid first beat placement #3965

Merged
merged 4 commits into from
Jun 10, 2021
Merged

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Jun 8, 2021

Make sure the first beat in a const beat grid is as close to the start of the track as possible. Workaround fix for lp#1930930

@ywwg ywwg requested a review from ronso0 June 8, 2021 18:55
@ywwg ywwg requested a review from Holzhaus June 8, 2021 19:00
@daschuer
Copy link
Member

daschuer commented Jun 8, 2021

Thank you for jumping in here. I have not found time at a whole to dive into the issue myselfe.

@ywwg
Copy link
Member Author

ywwg commented Jun 8, 2021

This is kind of a workaround, since I think the first beat calculation should be more robust, but I this fixes the specific issue in a safe way. No worries!

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. This looks already good to me.
I have left only some comments for improvement.
Please give a hint when this can be merged.

src/track/beatutils.cpp Outdated Show resolved Hide resolved
src/track/beatutils.cpp Show resolved Hide resolved
@uklotzde uklotzde added this to the 2.3.0 milestone Jun 9, 2021
@daschuer
Copy link
Member

daschuer commented Jun 9, 2021

This was happening in the majority of new tracks I downloaded.

You mean: ...that the fist beat is in the middle of the track. Yes that's quite obvious eithbe the new algorithm.

My intended question was: What are the track where this is relevant.

It is only relevant whenevet the best detector detects the wrong tempo, and it is not fixable by a multiplayer along with a simple shift for phase correction.

If you have such tracks I am curious, why the detector fails.

@ywwg
Copy link
Member Author

ywwg commented Jun 9, 2021

Let's talk about example tracks on Zulip, it is off topic for this fix. Just because the user "shouldn't" see the problem unless they try to adjust the track doesn't mean there is no bug. We should still apply this fix regardless of whether the detector has a problem with a specific track. If for some reason you think this fix is wrong, please explain why rather than bringing up tangential issues.

@daschuer
Copy link
Member

daschuer commented Jun 9, 2021

Just because the user "shouldn't" see the problem unless they try to adjust the track doesn't mean there is no bug.

I can acknowledge that there is a bug and this PR fixes the bug. I am just curious to know the track where the bug arise.

@daschuer
Copy link
Member

daschuer commented Jun 9, 2021

There is one source code comment open. Do you want to add the suggestion?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

@daschuer daschuer merged commit f535bcb into mixxxdj:2.3 Jun 10, 2021
@daschuer
Copy link
Member

daschuer commented Jun 10, 2021

Do we want to bump the version number because of that? This will reanalyze all the tracks according to the prefence settings. The issue is only relevant when the detection fails anyway.

I am heading to yes, bump the version.

daschuer added a commit to daschuer/mixxx that referenced this pull request Jun 10, 2021
... because of the moved first beat in const beat grits introduced in mixxxdj#3965
@daschuer
Copy link
Member

see: #3973

@ywwg
Copy link
Member Author

ywwg commented Jun 10, 2021

I posted a link to the track in Zulip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants