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: Add editing controls for bar annotations #13330

Closed
wants to merge 1 commit into from
Closed

Beats: Add editing controls for bar annotations #13330

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
6 changes: 6 additions & 0 deletions src/audio/types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ QDebug operator<<(QDebug dbg, Bitrate arg) {
<< Bitrate::unit();
}

QDebug operator<<(QDebug dbg, BeatsPerBar arg) {
return dbg
<< static_cast<BeatsPerBar::value_t>(arg)
<< BeatsPerBar::unit();
}

} // namespace audio

} // namespace mixxx
47 changes: 47 additions & 0 deletions src/audio/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,50 @@ class Bitrate {

QDebug operator<<(QDebug dbg, Bitrate arg);

// The BeatsPerBar is measured in beats and provides information
// about the number expected within a single bar or phrase.
class BeatsPerBar {
public:
using value_t = std::uint32_t;

private:
// The default value is invalid and indicates a missing or unknown value.
acolombier marked this conversation as resolved.
Show resolved Hide resolved
static constexpr value_t kValueDefault = 4;

public:
static constexpr value_t kValueMin = 2; // lower bound (inclusive)
static constexpr value_t kValueMax = 32; // upper bound (inclusive)
static constexpr const char* unit() {
acolombier marked this conversation as resolved.
Show resolved Hide resolved
return "beats";
}

explicit constexpr BeatsPerBar(
value_t value = kValueDefault)
: m_value(value) {
}

constexpr bool isValid() const {
return m_value >= kValueMin && m_value <= kValueMax;
}

constexpr value_t value() const {
return m_value;
}
/*implicit*/ constexpr operator value_t() const {
return value();
}

BeatsPerBar operator+(std::int32_t increment) const {
DEBUG_ASSERT(isValid());
return BeatsPerBar(m_value + increment);
}

private:
value_t m_value;
};

QDebug operator<<(QDebug dbg, BeatsPerBar arg);

} // namespace audio

} // namespace mixxx
Expand All @@ -262,3 +306,6 @@ Q_DECLARE_METATYPE(mixxx::audio::SampleRate)

Q_DECLARE_TYPEINFO(mixxx::audio::Bitrate, Q_PRIMITIVE_TYPE);
Q_DECLARE_METATYPE(mixxx::audio::Bitrate)

Q_DECLARE_TYPEINFO(mixxx::audio::BeatsPerBar, Q_PRIMITIVE_TYPE);
Q_DECLARE_METATYPE(mixxx::audio::BeatsPerBar)
220 changes: 199 additions & 21 deletions src/engine/controls/bpmcontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "engine/controls/bpmcontrol.h"

#include <chrono>

#include "control/controlencoder.h"
#include "control/controllinpotmeter.h"
#include "control/controlproxy.h"
Expand All @@ -13,6 +15,8 @@
#include "util/logger.h"
#include "util/math.h"

using namespace std::literals;

namespace {
const mixxx::Logger kLogger("BpmControl");

Expand All @@ -22,8 +26,8 @@ constexpr double kBpmRangeMax = 200.0;
constexpr double kBpmRangeStep = 1.0;
constexpr double kBpmRangeSmallStep = 0.1;

constexpr double kBpmAdjustMin = kBpmRangeMin;
constexpr double kBpmAdjustStep = 0.01;
constexpr std::chrono::milliseconds kBpmAdjustRepeatInterval = 50ms;
constexpr std::chrono::milliseconds kBpmAdjustTimeBeforeRepeat = 500ms;
constexpr double kBpmTapRounding = 1 / 12.0;

// Maximum allowed interval between beats (calculated from kBpmTapMin).
Expand Down Expand Up @@ -86,6 +90,17 @@ BpmControl::BpmControl(const QString& group,
this,
&BpmControl::slotAdjustBeatsFaster,
Qt::DirectConnection);
m_pAdjustBeatsMuchFaster = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_adjust_much_faster"), false);
m_pAdjustBeatsMuchFaster->setKbdRepeatable(true);
connect(
m_pAdjustBeatsMuchFaster.get(),
&ControlObject::valueChanged,
this,
[this](double v) {
slotAdjustBeatsFaster(v * 10);
},
Qt::DirectConnection);
m_pAdjustBeatsSlower = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_adjust_slower"), false);
m_pAdjustBeatsSlower->setKbdRepeatable(true);
Expand All @@ -94,6 +109,17 @@ BpmControl::BpmControl(const QString& group,
this,
&BpmControl::slotAdjustBeatsSlower,
Qt::DirectConnection);
m_pAdjustBeatsMuchSlower = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_adjust_much_slower"), false);
m_pAdjustBeatsMuchSlower->setKbdRepeatable(true);
connect(
m_pAdjustBeatsMuchSlower.get(),
&ControlObject::valueChanged,
this,
[this](double v) {
slotAdjustBeatsSlower(v * 10);
},
Qt::DirectConnection);
m_pTranslateBeatsEarlier = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_translate_earlier"), false);
m_pTranslateBeatsEarlier->setKbdRepeatable(true);
Expand All @@ -117,6 +143,34 @@ BpmControl::BpmControl(const QString& group,
this,
&BpmControl::slotTranslateBeatsMove,
Qt::DirectConnection);
m_pBeatsSetMarker = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_set_change_marker"), false);
connect(m_pBeatsSetMarker.get(),
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsSetMarker,
Qt::DirectConnection);
m_pBeatsRemoveMarker = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_remove_marker"), false);
connect(m_pBeatsRemoveMarker.get(),
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsRemoveMarker,
Qt::DirectConnection);
m_pBeatsBarCountUp = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_increase_bar_length"), false);
connect(m_pBeatsBarCountUp.get(),
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsBarCountUp,
Qt::DirectConnection);
m_pBeatsBarCountDown = std::make_unique<ControlPushButton>(
ConfigKey(group, "beats_decrease_bar_length"), false);
connect(m_pBeatsBarCountDown.get(),
&ControlObject::valueChanged,
this,
&BpmControl::slotBeatsBarCountDown,
Qt::DirectConnection);

m_pBeatsHalve = std::make_unique<ControlPushButton>(ConfigKey(group, "beats_set_halve"), false);
connect(m_pBeatsHalve.get(),
Expand Down Expand Up @@ -264,7 +318,33 @@ mixxx::Bpm BpmControl::getBpm() const {
return mixxx::Bpm(m_pEngineBpm->get());
}

void BpmControl::adjustBeatsBpm(double deltaBpm) {
void BpmControl::clearActionRepeater() {
m_repeatOperation.stop();
m_repeatOperation.disconnect();
return;
}

void BpmControl::activateActionRepeater(const std::function<void()>& callback) {
VERIFY_OR_DEBUG_ASSERT(callback) {
clearActionRepeater();
return;
}

if (m_repeatOperation.isActive()) {
m_repeatOperation.setInterval(kBpmAdjustRepeatInterval);
} else {
m_repeatOperation.disconnect();
connect(&m_repeatOperation, &QTimer::timeout, this, callback);
m_repeatOperation.start(kBpmAdjustTimeBeforeRepeat);
}
}

void BpmControl::slotAdjustBeatsFaster(double v) {
if (v <= 0) {
clearActionRepeater();
return;
}

const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
Expand All @@ -274,44 +354,68 @@ void BpmControl::adjustBeatsBpm(double deltaBpm) {
return;
}

const mixxx::Bpm bpm = pBeats->getBpmInRange(
mixxx::audio::kStartFramePos, frameInfo().trackEndPosition);
// FIXME: calling bpm.value() without checking bpm.isValid()
const auto centerBpm = mixxx::Bpm(math_max(kBpmAdjustMin, bpm.value() + deltaBpm));
mixxx::Bpm adjustedBpm = BeatUtils::roundBpmWithinRange(
centerBpm - kBpmAdjustStep / 2, centerBpm, centerBpm + kBpmAdjustStep / 2);
const auto newBeats = pBeats->trySetBpm(adjustedBpm);
if (!newBeats) {
return;
activateActionRepeater([this, v]() {
slotAdjustBeatsFaster(v);
});

const auto adjustedBeats =
pBeats->tryAdjustTempo(frameInfo().currentPosition,
v > 1 ? mixxx::Beats::TempoAdjustment::MuchFaster
: mixxx::Beats::TempoAdjustment::Faster);
if (adjustedBeats) {
pTrack->trySetBeats(*adjustedBeats);
}
Comment on lines +361 to 367
Copy link
Member

Choose a reason for hiding this comment

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

This is not understandable without looking up the implementation which is out of sight here.

auto is here std::optional<BeatsPointer> = std::optional<std::shared_ptr<const Beats>>
std::shared_ptr is already optional. So wrapping it into a std::optional is redundant.
Currently the pointer nature is hidden, which may lead to missing null checks.

With this construct we have to deal with nullptr and nullopt.
In case trySetBeats() returns nullptr, the beats are deleted from the track. I don't think this is the desired behaviour here. So let's just remove the optional wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

What you are suggesting make sense.
I looked into it in the span of that change is wide, since this std::optional is already used in the Beat API. I will add an issue to capture your thought and allow us to fix that going forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

#13806

Copy link
Member

Choose a reason for hiding this comment

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

It is OK for me to only alter the code in this PR. Changing the other is also welcome of cause.
I consider this a merge blocker.

pTrack->trySetBeats(*newBeats);
}

void BpmControl::slotAdjustBeatsFaster(double v) {
void BpmControl::slotAdjustBeatsSlower(double v) {
if (v <= 0) {
clearActionRepeater();
return;
}
adjustBeatsBpm(kBpmAdjustStep);
}

void BpmControl::slotAdjustBeatsSlower(double v) {
if (v <= 0) {
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
adjustBeatsBpm(-kBpmAdjustStep);
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

activateActionRepeater([this, v]() {
slotAdjustBeatsSlower(v);
});

const auto adjustedBeats =
pBeats->tryAdjustTempo(frameInfo().currentPosition,
v > 1 ? mixxx::Beats::TempoAdjustment::MuchSlower
: mixxx::Beats::TempoAdjustment::Slower);
if (adjustedBeats) {
pTrack->trySetBeats(*adjustedBeats);
}
}

void BpmControl::slotTranslateBeatsEarlier(double v) {
if (v <= 0) {
clearActionRepeater();
return;
}

activateActionRepeater([this]() {
slotTranslateBeatsEarlier(1);
});
slotTranslateBeatsMove(-1);
Copy link
Member

Choose a reason for hiding this comment

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

This behaves cumbersome now. What is the use case once you have set a change maker?
Probably moving the change marker with all the following beats. However, I am in doubt that the previous bar region is kept unchanged and a padding bar is created. So we may either change the tempo of the previous region, or move them as well.

In general I think these padding beats are a pain. There is no good approach to get rid of them once created.
So I think Mixxx should not allow to create them in the first place.
If a use rally wants them, they can add two change markers

Copy link
Member Author

Choose a reason for hiding this comment

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

So we may either change the tempo of the previous region, or move them as well.

I strongly disagree; Beatgrids are meant to be contained. If you create a new one, it makes sense to inherit the previous one's BPM, but if you adjust the BPM of it, it should not affect any other adjacent grids. When it comes to BPM.

When it comes to global BPM adjustment, this is an existing limitation of non-const tempo AFAIK, so we may consider a new feature, but clearly out of scope for this PR.

In general I think these padding beats are a pain.

I don't think padding bar are necessary any more with the beatgrid reword introduced by this PR

Copy link
Member

Choose a reason for hiding this comment

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

Clarification on this is a relase blocker.

Beatgrids are meant to be contained.

Confirmed. This means to me that the region before the beat change marker shall not change.
The issue is here that it does change the last bar of the previous region, introduceing a highly undesired padding beat by default.

To find a suitable solution, we need to consider the use case.

  • This can be used if the beat marker is on the "boom" and not on the "stchack"

It is easy possible in case we have only one bea like before.

In case the user has adjusted the tempo in some regions, it is likely that the whole track is still affected. So it still should be applied to the whole track.

What other use cases we have?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since you define beatgrid by their start and not their end, it made sense to me to keep the start as immutable. This means that if you shift a beatgrid, the start will move (and thus change the end of the previous grid), but the end will be inferred from the next grid.

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 think we have a use case for this. This will only mess up the beat grid.

These padding beats don't have a counterpart in the music theory, are undesirable in the sync case and Hart to fix once created. Let's avoid them unless the user explicit wants them.

Or do I miss a case?

Copy link
Member Author

Choose a reason for hiding this comment

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

These padding beats don't have a counterpart in the music theory

  1. That's not true. This is only true for tracks with constant BPM - here is an example waiting for this PR. Of course in the perfect world, we could define the a function to drive the BPM value, so we could translate the BPM ramping up from 100 to 140 in the example above. Obviously, it means we need a way to customize or detect that since they may not be linear
  2. music theory is a thing, music practice is another. Here is another example in my library waiting for that PR. Here the BPM is meant to be static, but actually due to how the track has been recorded, it's actually not perfectly stable, and sometime need some offset to adjust the grid on the beat while keeping the 116 BPM. So the natural workaround is to set grid correct on the point of interest (e.g hotcue, or loop) and this padding is a must have here

}

void BpmControl::slotTranslateBeatsLater(double v) {
if (v <= 0) {
clearActionRepeater();
return;
}

activateActionRepeater([this]() {
slotTranslateBeatsLater(1);
});
slotTranslateBeatsMove(1);
}

Expand All @@ -330,7 +434,7 @@ void BpmControl::slotTranslateBeatsMove(double v) {
const double sampleOffset = frameInfo().sampleRate * v * 0.01;
const mixxx::audio::FrameDiff_t frameOffset =
sampleOffset / mixxx::kEngineChannelOutputCount;
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
const auto translatedBeats = pBeats->tryTranslate(frameOffset, frameInfo().currentPosition);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
Expand All @@ -349,6 +453,80 @@ void BpmControl::slotBeatsUndoAdjustment(double v) {
m_pBeatsUndoPossible->forceSet(pTrack->canUndoBeatsChange());
}

void BpmControl::slotBeatsSetMarker(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->trySetMarker(frameInfo().currentPosition);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}

void BpmControl::slotBeatsBarCountUp(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->tryAdjustBeatsPerBar(frameInfo().currentPosition, 1);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}
void BpmControl::slotBeatsBarCountDown(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->tryAdjustBeatsPerBar(frameInfo().currentPosition, -1);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}
void BpmControl::slotBeatsRemoveMarker(double v) {
if (v <= 0) {
return;
}
const TrackPointer pTrack = getEngineBuffer()->getLoadedTrack();
if (!pTrack) {
return;
}
const mixxx::BeatsPointer pBeats = pTrack->getBeats();
if (!pBeats) {
return;
}

const auto modifiedBeats = pBeats->tryRemoveMarker(frameInfo().currentPosition);
if (modifiedBeats) {
pTrack->trySetBeats(*modifiedBeats);
}
}

void BpmControl::slotBpmTap(double v) {
if (v > 0) {
m_bpmTapFilter.tap();
Expand Down Expand Up @@ -1169,7 +1347,7 @@ void BpmControl::slotBeatsTranslate(double v) {
const auto currentPosition = frameInfo().currentPosition.toLowerFrameBoundary();
const auto closestBeat = pBeats->findClosestBeat(currentPosition);
const mixxx::audio::FrameDiff_t frameOffset = currentPosition - closestBeat;
const auto translatedBeats = pBeats->tryTranslate(frameOffset);
const auto translatedBeats = pBeats->tryTranslate(frameOffset, currentPosition);
if (translatedBeats) {
pTrack->trySetBeats(*translatedBeats);
}
Expand Down
Loading
Loading