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

lp1935703: Use nearest frame boundary in lookupBeatPositions() #4095

Merged
merged 12 commits into from
Jul 11, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jul 10, 2021

Beat searches using fractional positions should be allowed, only saving
fractional positions should cause a debug assertion. This should fix the
issue reported here: https://bugs.launchpad.net/mixxx/+bug/1935703/comments/6

@Holzhaus Holzhaus added this to In Progress in Semanting Type Refactoring via automation Jul 10, 2021
@Holzhaus
Copy link
Member Author

@JoergAtGithub please test.

@uklotzde
Copy link
Contributor

Please rebase

Beat searches using fractional positions should be allowed, only saving
fractional positions should cause a debug assertion. This should fix the
issue reported here:
https://bugs.launchpad.net/mixxx/+bug/1935703/comments/6
@Holzhaus Holzhaus force-pushed the quantizecontrol-frame-boundary branch from fd056d3 to 015ad88 Compare July 10, 2021 12:57
@Holzhaus Holzhaus changed the title QuantizeControl: Use nearest frame boundary in lookupBeatPositions() Use nearest frame boundary in lookupBeatPositions() Jul 10, 2021
@Holzhaus Holzhaus changed the title Use nearest frame boundary in lookupBeatPositions() lp1935703: Use nearest frame boundary in lookupBeatPositions() Jul 10, 2021
@coveralls
Copy link

coveralls commented Jul 10, 2021

Pull Request Test Coverage Report for Build 1018795091

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • 246 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 28.642%

Files with Coverage Reduction New Missed Lines %
src/track/track.cpp 246 51.18%
Totals Coverage Status
Change from base Build 1017932377: 0.01%
Covered Lines: 20092
Relevant Lines: 70149

💛 - Coveralls

src/track/beatmap.cpp Outdated Show resolved Hide resolved
src/track/beatmap.cpp Outdated Show resolved Hide resolved
@@ -10,6 +10,15 @@ using namespace mixxx;

namespace {

int countBeatsInIterator(std::unique_ptr<BeatIterator> pIterator) {
Copy link
Member

Choose a reason for hiding this comment

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

An iterator is a pointer tow a beat. So there can be now beats in it.
numberOfRemainingBeats() or such might be fit better.
Or numberOfBeatsinRange() ..

Copy link
Member

Choose a reason for hiding this comment

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

The iterator is created by std::lower_bound
"Returns an iterator pointing to the first element in the range [first, last]"

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -481,15 +481,14 @@ std::unique_ptr<BeatIterator> BeatMap::findBeats(
return std::unique_ptr<BeatIterator>();
}

Beat startBeat = beatFromFramePos(startPosition);
Beat endBeat = beatFromFramePos(endPosition);
Beat startBeat = beatFromFramePos(startPosition.toUpperFrameBoundary());
Copy link
Member

Choose a reason for hiding this comment

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

What is the rational to shrink the range here compared to the original version?
I think a comment would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

It this whole function unused? Delete!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, it is used in WaveformRenderBeat::draw() to find the visible beats in a time range. So it is still required.

Copy link
Member Author

@Holzhaus Holzhaus Jul 10, 2021

Choose a reason for hiding this comment

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

What is the rational to shrink the range here compared to the original version?
I think a comment would be helpful.

Beats can only appear a full frame positions.
Let's say a beat is at pos 100. If I say "Give me all beats between 100.5 and 200", it should not give me the beat at 100.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Holzhaus Holzhaus requested a review from daschuer July 10, 2021 22:06
@daschuer
Copy link
Member

Thank you.

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.

LGTM

@daschuer daschuer merged commit 32dad22 into mixxxdj:main Jul 11, 2021
Semanting Type Refactoring automation moved this from In Progress to Done Jul 11, 2021
@Holzhaus Holzhaus added this to Done in Beats Refactoring Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants