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

SeratoBeatsImporter: Fix import/export of non-const beatgrids #3885

Merged
merged 10 commits into from May 24, 2021

Conversation

Holzhaus
Copy link
Member

This should fix the import of Serato BeatGrids where no non-terminal markers are present. The previous code was wrong, I don't know what I was thinking.

@Holzhaus Holzhaus added this to the 2.3.0 milestone May 20, 2021
@Holzhaus Holzhaus added this to In progress in 2.3 release via automation May 20, 2021
src/track/serato/beatsimporter.cpp Outdated Show resolved Hide resolved
src/track/serato/beatsimporter.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

I can now export the beatgrid and them import it. However, If I export the imported beatgrid again, the last marker in missing. So there is some other subtle bug in the export code. I'll investigate tomorrow.

@Holzhaus Holzhaus force-pushed the serato-beatgrid-import-fix branch from 98fcb44 to b88882d Compare May 21, 2021 17:45
@Holzhaus Holzhaus force-pushed the serato-beatgrid-import-fix branch from b88882d to e6b2d48 Compare May 21, 2021 21:47
@Holzhaus
Copy link
Member Author

Yeeeees, I think I finally fixed it. Sorry, had to rewrite some parts. Please have a look and tell me when I should add comments. At least we now have proper round trip tests that check for correctness.

@Holzhaus Holzhaus changed the title SeratoBeatsImporter: Fix import of terminal marker beats SeratoBeatsImporter: Fix import/export of non-const beatgrids May 21, 2021
@Holzhaus Holzhaus removed the ui label May 21, 2021
@Holzhaus Holzhaus requested a review from uklotzde May 21, 2021 22:26
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
src/track/serato/beatsimporter.cpp Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

Holzhaus commented May 22, 2021

Please also test this manually. Testing instructions:

  1. Check the Serato Metadata Export field in the library preferences
  2. Create a beatgrid/beatmap that you can distinguish from the default analyzer result (e.g. shift it or let the analyzer create a non-const beatmap and check "Assume constant tempo afterwards)
  3. Export the file tags and restart mixxx
  4. Load track to deck
  5. Reset BPM/Beatgrid and then reimport file tags -> Is the beatgrid correct?
  6. Also: Remove ~/.mixxx and load track to deck -> Does the track have the correct beatgrid even before the analysis finished?

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.

Code looks good. My ideas/suggestions for improving safety and readability even further are optional.

2.3 release automation moved this from In progress to Needs Review May 23, 2021
@uklotzde
Copy link
Contributor

LGTM

@uklotzde uklotzde merged commit 3fd7adc into mixxxdj:2.3 May 24, 2021
2.3 release automation moved this from Needs Review to Done May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants