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

Jump to bottom

Beats: Make return value of mutation methods optional #4409

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Beats: Make return value of mutation methods optional #4409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 28 additions & 8 deletions src/engine/controls/bpmcontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,11 @@ void BpmControl::adjustBeatsBpm(double deltaBpm) {
const auto centerBpm = mixxx::Bpm(math_max(kBpmAdjustMin, bpm.value() + deltaBpm));
mixxx::Bpm adjustedBpm = BeatUtils::roundBpmWithinRange(
centerBpm - kBpmAdjustStep / 2, centerBpm, centerBpm + kBpmAdjustStep / 2);
pTrack->trySetBeats(pBeats->setBpm(adjustedBpm));
const auto newBeats = pBeats->trySetBpm(adjustedBpm);
if (!newBeats) {
return;
}
pTrack->trySetBeats(*newBeats);
}

void BpmControl::slotAdjustBeatsFaster(double v) {
Expand Down Expand Up @@ -191,7 +195,10 @@ void BpmControl::slotTranslateBeatsEarlier(double v) {
if (pBeats) {
const double sampleOffset = frameInfo().sampleRate * -0.01;
const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
pTrack->trySetBeats(pBeats->translate(frameOffset));
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
}
}

Expand All @@ -208,7 +215,10 @@ void BpmControl::slotTranslateBeatsLater(double v) {
// TODO(rryan): Track::frameInfo is possibly inaccurate!
const double sampleOffset = frameInfo().sampleRate * 0.01;
const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;
pTrack->trySetBeats(pBeats->translate(frameOffset));
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
}
}

Expand Down Expand Up @@ -245,7 +255,11 @@ void BpmControl::slotTapFilter(double averageLength, int numSamples) {
averageBpm = BeatUtils::roundBpmWithinRange(averageBpm - kBpmTabRounding,
averageBpm,
averageBpm + kBpmTabRounding);
pTrack->trySetBeats(pBeats->setBpm(averageBpm));
const auto newBeats = pBeats->trySetBpm(averageBpm);
if (!newBeats) {
return;
}
pTrack->trySetBeats(*newBeats);
}

// static
Expand Down Expand Up @@ -961,8 +975,11 @@ void BpmControl::slotBeatsTranslate(double v) {
if (pBeats) {
const auto currentPosition = frameInfo().currentPosition.toLowerFrameBoundary();
const auto closestBeat = pBeats->findClosestBeat(currentPosition);
const mixxx::audio::FrameDiff_t delta = currentPosition - closestBeat;
pTrack->trySetBeats(pBeats->translate(delta));
const mixxx::audio::FrameDiff_t frameOffset = currentPosition - closestBeat;
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
}
}

Expand All @@ -980,8 +997,11 @@ void BpmControl::slotBeatsTranslateMatchAlignment(double v) {
// otherwise it will always return 0 if sync lock is active.
m_dUserOffset.setValue(0.0);

const mixxx::audio::FrameDiff_t frameOffset = getPhaseOffset(frameInfo().currentPosition);
pTrack->trySetBeats(pBeats->translate(-frameOffset));
const mixxx::audio::FrameDiff_t frameOffset = -getPhaseOffset(frameInfo().currentPosition);
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
}
}

Expand Down
28 changes: 15 additions & 13 deletions src/library/dlgtrackinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,33 +520,35 @@ void DlgTrackInfo::clear() {
}

void DlgTrackInfo::slotBpmDouble() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Double);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::Double);
}

void DlgTrackInfo::slotBpmHalve() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::Halve);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::Halve);
}

void DlgTrackInfo::slotBpmTwoThirds() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::TwoThirds);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::TwoThirds);
}

void DlgTrackInfo::slotBpmThreeFourth() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeFourths);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::ThreeFourths);
}

void DlgTrackInfo::slotBpmFourThirds() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::FourThirds);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::FourThirds);
}

void DlgTrackInfo::slotBpmThreeHalves() {
m_pBeatsClone = m_pBeatsClone->scale(mixxx::Beats::BpmScale::ThreeHalves);
updateSpinBpmFromBeats();
slotBpmScale(mixxx::Beats::BpmScale::ThreeHalves);
}

void DlgTrackInfo::slotBpmScale(mixxx::Beats::BpmScale bpmScale) {
const auto scaledBeats = m_pBeatsClone->tryScale(bpmScale);
if (scaledBeats) {
m_pBeatsClone = *scaledBeats;
updateSpinBpmFromBeats();
}
}

void DlgTrackInfo::slotBpmClear() {
Expand Down Expand Up @@ -610,7 +612,7 @@ void DlgTrackInfo::slotSpinBpmValueChanged(double value) {
if (oldValue == bpm) {
return;
}
m_pBeatsClone = m_pBeatsClone->setBpm(bpm);
m_pBeatsClone = m_pBeatsClone->trySetBpm(bpm).value_or(m_pBeatsClone);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the only occurrence where the optional is not used. Unsure if we want to add an infallible setBpm just for that, especially because it would be inconsistent with the other beats mutation methods. If we add it, maybe we should add it for the other ones for consistency. Or we leave it like this. I don't really care, let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind as well.

}

updateSpinBpmFromBeats();
Expand Down
1 change: 1 addition & 0 deletions src/library/dlgtrackinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class DlgTrackInfo : public QDialog, public Ui::DlgTrackInfo {
void slotBpmThreeFourth();
void slotBpmFourThirds();
void slotBpmThreeHalves();
void slotBpmScale(mixxx::Beats::BpmScale bpmScale);
void slotBpmClear();
void slotBpmConstChanged(int state);
void slotBpmTap(double averageLength, int numSamples);
Expand Down
12 changes: 6 additions & 6 deletions src/test/beatgridtest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,22 @@ TEST(BeatGridTest, Scale) {
mixxx::Bpm(bpm));

EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
pGrid = pGrid->scale(Beats::BpmScale::Double);
pGrid = *pGrid->tryScale(Beats::BpmScale::Double);
EXPECT_DOUBLE_EQ(2 * bpm.value(), pGrid->getBpm().value());

pGrid = pGrid->scale(Beats::BpmScale::Halve);
pGrid = *pGrid->tryScale(Beats::BpmScale::Halve);
EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());

pGrid = pGrid->scale(Beats::BpmScale::TwoThirds);
pGrid = *pGrid->tryScale(Beats::BpmScale::TwoThirds);
EXPECT_DOUBLE_EQ(bpm.value() * 2 / 3, pGrid->getBpm().value());

pGrid = pGrid->scale(Beats::BpmScale::ThreeHalves);
pGrid = *pGrid->tryScale(Beats::BpmScale::ThreeHalves);
EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());

pGrid = pGrid->scale(Beats::BpmScale::ThreeFourths);
pGrid = *pGrid->tryScale(Beats::BpmScale::ThreeFourths);
EXPECT_DOUBLE_EQ(bpm.value() * 3 / 4, pGrid->getBpm().value());

pGrid = pGrid->scale(Beats::BpmScale::FourThirds);
pGrid = *pGrid->tryScale(Beats::BpmScale::FourThirds);
EXPECT_DOUBLE_EQ(bpm.value(), pGrid->getBpm().value());
}

Expand Down
12 changes: 6 additions & 6 deletions src/test/beatmaptest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,22 +63,22 @@ TEST_F(BeatMapTest, Scale) {
auto pMap = Beats::fromBeatPositions(m_pTrack->getSampleRate(), beats);

EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
pMap = pMap->scale(Beats::BpmScale::Double);
pMap = *pMap->tryScale(Beats::BpmScale::Double);
EXPECT_DOUBLE_EQ(2 * bpm.value(), pMap->getBpm().value());

pMap = pMap->scale(Beats::BpmScale::Halve);
pMap = *pMap->tryScale(Beats::BpmScale::Halve);
EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());

pMap = pMap->scale(Beats::BpmScale::TwoThirds);
pMap = *pMap->tryScale(Beats::BpmScale::TwoThirds);
EXPECT_DOUBLE_EQ(bpm.value() * 2 / 3, pMap->getBpm().value());

pMap = pMap->scale(Beats::BpmScale::ThreeHalves);
pMap = *pMap->tryScale(Beats::BpmScale::ThreeHalves);
EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());

pMap = pMap->scale(Beats::BpmScale::ThreeFourths);
pMap = *pMap->tryScale(Beats::BpmScale::ThreeFourths);
EXPECT_DOUBLE_EQ(bpm.value() * 3 / 4, pMap->getBpm().value());

pMap = pMap->scale(Beats::BpmScale::FourThirds);
pMap = *pMap->tryScale(Beats::BpmScale::FourThirds);
EXPECT_DOUBLE_EQ(bpm.value(), pMap->getBpm().value());
}

Expand Down
17 changes: 9 additions & 8 deletions src/track/beatgrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,9 @@ mixxx::Bpm BeatGrid::getBpmAroundPosition(audio::FramePos position, int n) const
return getBpm();
}

BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
std::optional<BeatsPointer> BeatGrid::tryTranslate(audio::FrameDiff_t offset) const {
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clonePointer();
return std::nullopt;
}

mixxx::track::io::BeatGrid grid = m_grid;
Expand All @@ -275,9 +275,9 @@ BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, m_beatLengthFrames);
}

BeatsPointer BeatGrid::scale(BpmScale scale) const {
std::optional<BeatsPointer> BeatGrid::tryScale(BpmScale scale) const {
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clonePointer();
return std::nullopt;
}

mixxx::track::io::BeatGrid grid = m_grid;
Expand All @@ -304,11 +304,12 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
break;
default:
DEBUG_ASSERT(!"scale value invalid");
return clonePointer();
return std::nullopt;
}

if (!bpm.isValid()) {
return clonePointer();
qWarning() << "BeatGrid: Scaling would result in invalid BPM!";
return std::nullopt;
}

bpm = BeatUtils::roundBpmWithinRange(bpm - kBpmScaleRounding, bpm, bpm + kBpmScaleRounding);
Expand All @@ -317,9 +318,9 @@ BeatsPointer BeatGrid::scale(BpmScale scale) const {
return std::make_shared<BeatGrid>(MakeSharedTag{}, *this, grid, beatLengthFrames);
}

BeatsPointer BeatGrid::setBpm(mixxx::Bpm bpm) const {
std::optional<BeatsPointer> BeatGrid::trySetBpm(mixxx::Bpm bpm) const {
VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
return clonePointer();
return std::nullopt;
}

mixxx::track::io::BeatGrid grid = m_grid;
Expand Down
6 changes: 3 additions & 3 deletions src/track/beatgrid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ class BeatGrid final : public Beats {
// Beat mutations
////////////////////////////////////////////////////////////////////////////

BeatsPointer translate(audio::FrameDiff_t offset) const override;
BeatsPointer scale(BpmScale scale) const override;
BeatsPointer setBpm(mixxx::Bpm bpm) const override;
std::optional<BeatsPointer> tryTranslate(audio::FrameDiff_t offset) const override;
std::optional<BeatsPointer> tryScale(BpmScale scale) const override;
std::optional<BeatsPointer> trySetBpm(mixxx::Bpm bpm) const override;

////////////////////////////////////////////////////////////////////////////
// Hidden constructors
Expand Down
14 changes: 7 additions & 7 deletions src/track/beatmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,9 +532,9 @@ mixxx::Bpm BeatMap::getBpmAroundPosition(audio::FramePos position, int n) const
numberOfBeats, m_sampleRate, lowerFrame, upperFrame);
}

BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
std::optional<BeatsPointer> BeatMap::tryTranslate(audio::FrameDiff_t offset) const {
if (!isValid()) {
return clonePointer();
return std::nullopt;
}

BeatList beats = m_beats;
Expand All @@ -555,9 +555,9 @@ BeatsPointer BeatMap::translate(audio::FrameDiff_t offset) const {
return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, m_nominalBpm);
}

BeatsPointer BeatMap::scale(BpmScale scale) const {
std::optional<BeatsPointer> BeatMap::tryScale(BpmScale scale) const {
VERIFY_OR_DEBUG_ASSERT(isValid()) {
return clonePointer();
return std::nullopt;
}

BeatList beats = m_beats;
Expand Down Expand Up @@ -596,16 +596,16 @@ BeatsPointer BeatMap::scale(BpmScale scale) const {
break;
default:
DEBUG_ASSERT(!"scale value invalid");
return clonePointer();
return std::nullopt;
}

mixxx::Bpm bpm = calculateNominalBpm(beats, m_sampleRate);
return std::make_shared<BeatMap>(MakeSharedTag{}, *this, beats, bpm);
}

BeatsPointer BeatMap::setBpm(mixxx::Bpm bpm) const {
std::optional<BeatsPointer> BeatMap::trySetBpm(mixxx::Bpm bpm) const {
VERIFY_OR_DEBUG_ASSERT(bpm.isValid()) {
return clonePointer();
return std::nullopt;
}

const auto firstBeatPosition = mixxx::audio::FramePos(m_beats.first().frame_position());
Expand Down
6 changes: 3 additions & 3 deletions src/track/beatmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ class BeatMap final : public Beats {
// Beat mutations
////////////////////////////////////////////////////////////////////////////

BeatsPointer translate(audio::FrameDiff_t offset) const override;
BeatsPointer scale(BpmScale scale) const override;
BeatsPointer setBpm(mixxx::Bpm bpm) const override;
std::optional<BeatsPointer> tryTranslate(audio::FrameDiff_t offset) const override;
std::optional<BeatsPointer> tryScale(BpmScale scale) const override;
std::optional<BeatsPointer> trySetBpm(mixxx::Bpm bpm) const override;

////////////////////////////////////////////////////////////////////////////
// Hidden constructors
Expand Down
16 changes: 13 additions & 3 deletions src/track/beats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <QString>
#include <QVector>
#include <memory>
#include <optional>

#include "audio/frame.h"
#include "audio/types.h"
Expand Down Expand Up @@ -167,13 +168,22 @@ class Beats : private std::enable_shared_from_this<Beats> {
/// Translate all beats in the song by `offset` frames. Beats that lie
/// before the start of the track or after the end of the track are *not*
/// removed.
virtual BeatsPointer translate(audio::FrameDiff_t offset) const = 0;
//
/// Returns a pointer to the modified beats object, or `nullopt` on
/// failure.
virtual std::optional<BeatsPointer> tryTranslate(audio::FrameDiff_t offset) const = 0;

/// Scale the position of every beat in the song by `scale`.
virtual BeatsPointer scale(BpmScale scale) const = 0;
//
/// Returns a pointer to the modified beats object, or `nullopt` on
/// failure.
virtual std::optional<BeatsPointer> tryScale(BpmScale scale) const = 0;

/// Adjust the beats so the global average BPM matches `bpm`.
virtual BeatsPointer setBpm(mixxx::Bpm bpm) const = 0;
//
/// Returns a pointer to the modified beats object, or `nullopt` on
/// failure.
virtual std::optional<BeatsPointer> trySetBpm(mixxx::Bpm bpm) const = 0;

protected:
/// Type tag for making public constructors of derived classes inaccessible.
Expand Down
10 changes: 7 additions & 3 deletions src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,10 +359,14 @@ bool Track::trySetBpmWhileLocked(mixxx::Bpm bpm) {
return trySetBeatsWhileLocked(std::move(pBeats));
} else if (m_pBeats->getBpm() != bpm) {
// Continue with the regular cases
if (kLogger.debugEnabled()) {
kLogger.debug() << "Updating BPM:" << getLocation();
const auto newBeats = m_pBeats->trySetBpm(bpm);
if (newBeats) {
if (kLogger.debugEnabled()) {
kLogger.debug() << "Updating BPM:" << getLocation();
}

return trySetBeatsWhileLocked(*newBeats);
}
return trySetBeatsWhileLocked(m_pBeats->setBpm(bpm));
}
return false;
}
Expand Down
6 changes: 5 additions & 1 deletion src/widget/wtrackmenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,11 @@ class ScaleBpmTrackPointerOperation : public mixxx::TrackPointerOperation {
if (!pBeats) {
return;
}
pTrack->trySetBeats(pBeats->scale(m_bpmScale));
const auto scaledBeats = pBeats->tryScale(m_bpmScale);
if (!scaledBeats) {
return;
}
pTrack->trySetBeats(*scaledBeats);
}

const mixxx::Beats::BpmScale m_bpmScale;
Expand Down