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 quantized play from the pre-roll in beatmap tracks #3745

Merged
merged 4 commits into from
Apr 5, 2021

Conversation

daschuer
Copy link
Member

This fixes https://bugs.launchpad.net/mixxx/+bug/1920084

It also fixes a bug that the user offset during sync was applied to the wrong deck.
And that quantize play syncs sometimes to the last beat rather than the next.

Tests are in place.

@Holzhaus
Copy link
Member

Holzhaus commented Mar 24, 2021

Thanks. I've not tested this yet, but my impression was that root issue is that there ain't any beats before the track start. We should interpolate them from the beat distance between beat 1 and 2. This would also fix the issue that you can beatjump backwards from beat 1 into the Preroll. With beatgrids, this is possible.

@daschuer
Copy link
Member Author

Exactly this is done here.

The beatloop issue remains. This need to be fixed in another place.

@uklotzde
Copy link
Contributor

main or 2.3?

@daschuer daschuer changed the base branch from main to 2.3 March 24, 2021 15:10
@daschuer
Copy link
Member Author

Ups ... 2.3

@Holzhaus
Copy link
Member

Exactly this is done here.

Doesn't work for me:

Peek 2021-03-24 17-19
Left Deck: BeatMap (Preroll has no beat markers and does not support beatjumps)
Right Deck: BeatGrid (Preroll has beat markers and supports beatjumps)

I we don't have to fix it here, I'm fine merging this and then getting #2961 in a mergeable state for 2.4 ASAP.

@daschuer
Copy link
Member Author

Ah, yes. I think this should be fixed by actually marking the beats in the preroll.

This here is a solution if you have regions with no beats. I just use the next possible beat in the future for matching them.

@daschuer
Copy link
Member Author

Done.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 2, 2021

I think this doesn't work correctly (or I don't understand what this PR is supposed to fix). Here' s a video of the issue: https://www.youtube.com/watch?v=MKlzFQ_CpKU

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

@ywwg I am not familiar with the sync code.

No more concerns about the code in general.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 5, 2021

I think this doesn't work correctly (or I don't understand what this PR is supposed to fix). Here' s a video of the issue: https://www.youtube.com/watch?v=MKlzFQ_CpKU

@daschuer IIUC this PR fixes a different bug that let's the track jump to the first beat? If so, LGTM.

@daschuer
Copy link
Member Author

daschuer commented Apr 5, 2021

Correct

@Holzhaus Holzhaus merged commit d410800 into mixxxdj:2.3 Apr 5, 2021
@daschuer daschuer deleted the lp1920084 branch August 1, 2021 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants