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

Beats frame refactor #4049

Merged
merged 6 commits into from
Jul 7, 2021
Merged

Beats frame refactor #4049

merged 6 commits into from
Jul 7, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jul 2, 2021

Based on #4048.

This changes the Beats object to return the FramePos type from all methods that return positions.
I tried to keep the changes in the calling code as small as possible, which admittedly makes this code quite ugly with all this back/forth conversion. But otherwise it would be much more work and the diff would be huge.

This PR makes it possible to migrate the other features to the framepos type independently later on, which is taken care of in #4061 for example.

@Holzhaus Holzhaus marked this pull request as draft July 2, 2021 17:58
@Holzhaus Holzhaus force-pushed the beats-frame-refactor branch 2 times, most recently from 4a294dc to 926f970 Compare July 6, 2021 14:55
@Holzhaus Holzhaus mentioned this pull request Jul 6, 2021
10 tasks
Holzhaus added 3 commits July 6, 2021 19:16
The method returns a mixxx::audio::FramePos value, so mentioning the
unit in the method name is unnecessary.
@Holzhaus Holzhaus force-pushed the beats-frame-refactor branch from 926f970 to 75b2752 Compare July 6, 2021 17:19
@Holzhaus Holzhaus marked this pull request as ready for review July 6, 2021 17:19
@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 6, 2021

Ready to review. Sorry the diff is so large :-/

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.

Some thoughts and suggestions.

src/engine/controls/cuecontrol.cpp Show resolved Hide resolved
src/engine/controls/loopingcontrol.cpp Show resolved Hide resolved
src/track/beatgrid.cpp Show resolved Hide resolved
src/track/beats.cpp Outdated Show resolved Hide resolved
@uklotzde
Copy link
Contributor

uklotzde commented Jul 6, 2021

If someone wants to double check then please do. Otherwise let's merge it soon.

@uklotzde
Copy link
Contributor

uklotzde commented Jul 7, 2021

If anyone finds an issue afterwards it should be trivial to fix. LGTM

@uklotzde uklotzde merged commit d6236d4 into mixxxdj:main Jul 7, 2021
Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Jul 20, 2021
The beatgrid now uses frames, so this multiplication results in a
beatgrid of half the intended BPM. This was an oversight from mixxxdj#4049.
Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Jul 20, 2021
The beatgrid now uses frames, so this multiplication results in a
beatgrid of half the intended BPM. This was an oversight from mixxxdj#4049.
Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Jul 20, 2021
The beatgrid now uses frames, so this multiplication results in a
beatgrid of half the intended BPM. This was an oversight from mixxxdj#4049.
Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Jul 24, 2021
The beatgrid now uses frames, so this multiplication results in a
beatgrid of half the intended BPM. This was an oversight from mixxxdj#4049.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants