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 debug assertion caused by BpmControl #3721

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Fix debug assertion caused by BpmControl #3721

merged 1 commit into from
Mar 18, 2021

Conversation

uklotzde
Copy link
Contributor

Occurs when tapping the bpm with "Assume constant tempo" disabled.

Occurs when tapping the bpm with "Assume constant tempo" disabled.
@uklotzde uklotzde added this to the 2.3.0 milestone Mar 17, 2021
@uklotzde uklotzde requested a review from daschuer March 17, 2021 21:54
@Holzhaus
Copy link
Member

Holzhaus commented Mar 17, 2021

Code looks good, but didn't test.

I'm unsure if ignoring the tapping is correct, because it's a bit unexpected from the user's perspective. We could also throw away the beatmap and replace it with a beatgrid that has the tapped Bpm. But that's debatable and I don't want to add even more changes here, when #2961 gets rid of the beatscapabilities stuff anyway.

@daschuer
Copy link
Member

The Tap feature was disabled in the beats track preference for beats maps. The user has to cklick assume const first to enable the button.

But since it is a CO we cannot disable the button on the controllers.

Here we have the trade of protecting the user from his faults versus doing what the user expects.

The debug assertion has made a behavior visible that was always there. We had never complains about a broken tab button. So it is save to merge this as it is.

@daschuer daschuer merged commit 2303824 into mixxxdj:2.3 Mar 18, 2021
@uklotzde uklotzde deleted the bpmcontrol branch March 19, 2021 00:17
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.

3 participants