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.

Sign up for GitHub

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: Move identical method implementations from BeatGrid/BeatMap #4234

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Beats: Move identical method implementations from BeatGrid/BeatMap #4234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17b9502
Beats: Move identical method implementations from BeatGrid/BeatMap
Holzhaus Aug 19, 2021
b0fe71d
Beats: Remove comments stating the obvious
Holzhaus Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 0 additions & 35 deletions src/track/beatgrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,41 +143,6 @@ bool BeatGrid::isValid() const {
return m_sampleRate.isValid() && bpm().isValid() && firstBeatPosition().isValid();
}

// This could be implemented in the Beats Class itself.
// If necessary, the child class can redefine it.
Comment on lines -146 to -147
Copy link
Contributor

Choose a reason for hiding this comment

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

Why write this comment instead of just doing it...

Copy link
Contributor

Choose a reason for hiding this comment

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

@Be-ing I guess you got confused by the diff? This code and comments have been deleted because they became obsolete.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that. I am just lamenting that this wasn't already done 10 years ago.

audio::FramePos BeatGrid::findNextBeat(audio::FramePos position) const {
return findNthBeat(position, 1);
}

// This could be implemented in the Beats Class itself.
// If necessary, the child class can redefine it.
audio::FramePos BeatGrid::findPrevBeat(audio::FramePos position) const {
return findNthBeat(position, -1);
}

// This is an internal call. This could be implemented in the Beats Class itself.
audio::FramePos BeatGrid::findClosestBeat(audio::FramePos position) const {
if (!isValid()) {
return audio::kInvalidFramePos;
}
audio::FramePos prevBeatPosition;
audio::FramePos nextBeatPosition;
findPrevNextBeats(position, &prevBeatPosition, &nextBeatPosition, true);
if (!prevBeatPosition.isValid()) {
// If both positions are invalid, we correctly return an invalid position.
return nextBeatPosition;
}

if (!nextBeatPosition.isValid()) {
return prevBeatPosition;
}

// Both position are valid, return the closest position.
return (nextBeatPosition - position > position - prevBeatPosition)
? prevBeatPosition
: nextBeatPosition;
}

audio::FramePos BeatGrid::findNthBeat(audio::FramePos position, int n) const {
if (!isValid() || n == 0) {
return audio::kInvalidFramePos;
Expand Down
5 changes: 1 addition & 4 deletions src/track/beatgrid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,10 @@ class BeatGrid final : public Beats {
// Beat calculations
////////////////////////////////////////////////////////////////////////////

audio::FramePos findNextBeat(audio::FramePos position) const override;
audio::FramePos findPrevBeat(audio::FramePos position) const override;
bool findPrevNextBeats(audio::FramePos position,
audio::FramePos* prevBeatPosition,
audio::FramePos* nextBeatPosition,
bool snapToNearBeats) const override;
audio::FramePos findClosestBeat(audio::FramePos position) const override;
audio::FramePos findNthBeat(audio::FramePos position, int n) const override;
std::unique_ptr<BeatIterator> findBeats(audio::FramePos startPosition,
audio::FramePos endPosition) const override;
Expand Down Expand Up @@ -83,7 +80,7 @@ class BeatGrid final : public Beats {
mixxx::Bpm bpm() const;

// For internal use only.
bool isValid() const;
bool isValid() const override;

// The sub-version of this beatgrid.
const QString m_subVersion;
Expand Down
30 changes: 0 additions & 30 deletions src/track/beatmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,36 +281,6 @@ bool BeatMap::isValid() const {
return m_sampleRate.isValid() && m_beats.size() >= kMinNumberOfBeats;
}

audio::FramePos BeatMap::findNextBeat(audio::FramePos position) const {
return findNthBeat(position, 1);
}

audio::FramePos BeatMap::findPrevBeat(audio::FramePos position) const {
return findNthBeat(position, -1);
}

audio::FramePos BeatMap::findClosestBeat(audio::FramePos position) const {
if (!isValid()) {
return audio::kInvalidFramePos;
}
audio::FramePos prevBeatPosition;
audio::FramePos nextBeatPosition;
findPrevNextBeats(position, &prevBeatPosition, &nextBeatPosition, true);
if (!prevBeatPosition.isValid()) {
// If both positions are invalid, we correctly return an invalid position.
return nextBeatPosition;
}

if (!nextBeatPosition.isValid()) {
return prevBeatPosition;
}

// Both position are valid, return the closest position.
return (nextBeatPosition - position > position - prevBeatPosition)
? prevBeatPosition
: nextBeatPosition;
}

audio::FramePos BeatMap::findNthBeat(audio::FramePos position, int n) const {
if (!isValid() || n == 0) {
return audio::kInvalidFramePos;
Expand Down
5 changes: 1 addition & 4 deletions src/track/beatmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,10 @@ class BeatMap final : public Beats {
// Beat calculations
////////////////////////////////////////////////////////////////////////////

audio::FramePos findNextBeat(audio::FramePos position) const override;
audio::FramePos findPrevBeat(audio::FramePos position) const override;
bool findPrevNextBeats(audio::FramePos position,
audio::FramePos* prevBeatPosition,
audio::FramePos* nextBeatPosition,
bool snapToNearBeats) const override;
audio::FramePos findClosestBeat(audio::FramePos position) const override;
audio::FramePos findNthBeat(audio::FramePos position, int n) const override;
std::unique_ptr<BeatIterator> findBeats(audio::FramePos startPosition,
audio::FramePos endPosition) const override;
Expand Down Expand Up @@ -82,7 +79,7 @@ class BeatMap final : public Beats {
BeatMap(const BeatMap& other);

// For internal use only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, adding such a comment to a private method is confusing. Unrelated, but let's clean this up now.

bool isValid() const;
bool isValid() const override;

const QString m_subVersion;
const audio::SampleRate m_sampleRate;
Expand Down
30 changes: 30 additions & 0 deletions src/track/beats.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,36 @@ int Beats::numBeatsInRange(audio::FramePos startPosition, audio::FramePos endPos
return i - 2;
};

audio::FramePos Beats::findNextBeat(audio::FramePos position) const {
return findNthBeat(position, 1);
}

audio::FramePos Beats::findPrevBeat(audio::FramePos position) const {
return findNthBeat(position, -1);
}

audio::FramePos Beats::findClosestBeat(audio::FramePos position) const {
if (!isValid()) {
return audio::kInvalidFramePos;
}
audio::FramePos prevBeatPosition;
audio::FramePos nextBeatPosition;
findPrevNextBeats(position, &prevBeatPosition, &nextBeatPosition, true);
if (!prevBeatPosition.isValid()) {
// If both positions are invalid, we correctly return an invalid position.
return nextBeatPosition;
}

if (!nextBeatPosition.isValid()) {
return prevBeatPosition;
}

// Both position are valid, return the closest position.
return (nextBeatPosition - position > position - prevBeatPosition)
? prevBeatPosition
: nextBeatPosition;
}

audio::FramePos Beats::findNBeatsFromPosition(audio::FramePos position, double beats) const {
audio::FramePos prevBeatPosition;
audio::FramePos nextBeatPosition;
Expand Down
10 changes: 7 additions & 3 deletions src/track/beats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ class Beats {
/// Starting from frame position `position`, return the frame position of
/// the next beat in the track, or an invalid position if none exists. If
/// `position` refers to the location of a beat, `position` is returned.
virtual audio::FramePos findNextBeat(audio::FramePos position) const = 0;
audio::FramePos findNextBeat(audio::FramePos position) const;

/// Starting from frame position `position`, return the frame position of
/// the previous beat in the track, or an invalid position if none exists.
/// If `position` refers to the location of beat, `position` is returned.
virtual audio::FramePos findPrevBeat(audio::FramePos position) const = 0;
audio::FramePos findPrevBeat(audio::FramePos position) const;

/// Starting from frame position `position`, fill the frame position of the
/// previous beat and next beat. Either can be invalid if none exists. If
Expand All @@ -100,7 +100,7 @@ class Beats {

/// Starting from frame position `position`, return the frame position of
/// the closest beat in the track, or an invalid position if none exists.
virtual audio::FramePos findClosestBeat(audio::FramePos position) const = 0;
audio::FramePos findClosestBeat(audio::FramePos position) const;

/// Find the Nth beat from frame position `position`. Works with both
/// positive and negative values of n. Calling findNthBeat with `n=0` is
Expand Down Expand Up @@ -155,6 +155,10 @@ class Beats {

/// Adjust the beats so the global average BPM matches `bpm`.
virtual BeatsPointer setBpm(mixxx::Bpm bpm) = 0;

protected:
/// For internal use only.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment can be removed. That is clear from the method being protected.

virtual bool isValid() const = 0;
};

} // namespace mixxx