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

Looping/Beatjump: use seconds if track has no beats #12961

Merged
merged 7 commits into from
May 28, 2024
Merged

Looping/Beatjump: use seconds if track has no beats #12961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
514b71e
Looping/Beatjump: use seconds if track has no beats
ronso0 Mar 14, 2024
1cfcc1b
Looping: fix tests after implementing fake beats for tracks with no b…
ronso0 Mar 14, 2024
079cf44
update tooltips and control descriptions for 'no beats, use seconds' …
ronso0 Mar 14, 2024
01ed90b
Looping: act quantized only if we have true track beats ...
ronso0 Mar 15, 2024
9e91ead
Merge remote-tracking branch 'mixxx/2.5' into loop-jump-seconds-with-…
ronso0 May 27, 2024
e7ffb5b
adjust looping/seconds tooltip and control description
ronso0 May 27, 2024
761c7d6
Looping: revert to beats nullptr if no track is loaded
ronso0 May 27, 2024
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
30 changes: 22 additions & 8 deletions src/controllers/controlpickermenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,21 +639,27 @@ ControlPickerMenu::ControlPickerMenu(QWidget* pParent)
QMenu* pLoopMenu = addSubmenu(tr("Looping"));
// add beatloop_activate and beatlooproll_activate to both the
// Loop and Beat-Loop menus to make sure users can find them.
QString noBeatsSeconds = QChar('(') +
tr("if the track has no beats the unit is seconds") + QChar(')');
QString beatloopActivateTitle = tr("Loop Selected Beats");
QString beatloopActivateDescription = tr("Create a beat loop of selected beat size");
QString beatloopActivateDescription =
tr("Create a beat loop of selected beat size") + noBeatsSeconds;
QString beatloopRollActivateTitle = tr("Loop Roll Selected Beats");
QString beatloopRollActivateDescription = tr("Create a rolling beat loop of selected beat size");
QString beatloopRollActivateDescription =
tr("Create a rolling beat loop of selected beat size") + noBeatsSeconds;
QString beatLoopTitle = tr("Loop %1 Beats");
QString reverseBeatLoopTitle = tr("Loop %1 Beats set from its end point");
QString beatLoopRollTitle = tr("Loop Roll %1 Beats");
QString reverseBeatLoopRollTitle = tr("Loop Roll %1 Beats set from its end point");
QString beatLoopDescription = tr("Create %1-beat loop");
QString reverseBeatLoopDescription = tr(
"Create %1-beat loop with the current play position as loop end");
QString beatLoopRollDescription = tr("Create temporary %1-beat loop roll");
QString beatLoopRollDescription =
tr("Create temporary %1-beat loop roll") + noBeatsSeconds;
QString reverseBeatLoopRollDescription =
tr("Create temporary %1-beat loop roll with the current play "
"position as loop end");
"position as loop end") +
noBeatsSeconds;

QList<double> beatSizes = LoopingControl::getBeatSizes();

Expand Down Expand Up @@ -735,8 +741,14 @@ ControlPickerMenu::ControlPickerMenu(QWidget* pParent)
QMenu* pBeatJumpMenu = addSubmenu(tr("Beat Jump / Loop Move"));
QString beatJumpForwardTitle = tr("Jump / Move Loop Forward %1 Beats");
QString beatJumpBackwardTitle = tr("Jump / Move Loop Backward %1 Beats");
QString beatJumpForwardDescription = tr("Jump forward by %1 beats, or if a loop is enabled, move the loop forward %1 beats");
QString beatJumpBackwardDescription = tr("Jump backward by %1 beats, or if a loop is enabled, move the loop backward %1 beats");
QString beatJumpForwardDescription =
tr("Jump forward by %1 beats, or if a loop is enabled, move the "
"loop forward %1 beats") +
noBeatsSeconds;
QString beatJumpBackwardDescription =
tr("Jump backward by %1 beats, or if a loop is enabled, move the "
"loop backward %1 beats") +
noBeatsSeconds;
addDeckControl("beatjump_forward",
tr("Beat Jump / Loop Move Forward Selected Beats"),
tr("Jump forward by the selected number of beats, or if a loop is "
Expand Down Expand Up @@ -777,8 +789,10 @@ ControlPickerMenu::ControlPickerMenu(QWidget* pParent)
// Loop moving
QString loopMoveForwardTitle = tr("Move Loop +%1 Beats");
QString loopMoveBackwardTitle = tr("Move Loop -%1 Beats");
QString loopMoveForwardDescription = tr("Move loop forward by %1 beats");
QString loopMoveBackwardDescription = tr("Move loop backward by %1 beats");
QString loopMoveForwardDescription = tr("Move loop forward by %1 beats") +
noBeatsSeconds;
QString loopMoveBackwardDescription = tr("Move loop backward by %1 beats") +
noBeatsSeconds;

QMenu* pLoopmoveFwdSubmenu = addSubmenu(tr("Loop Move Forward"), pBeatJumpMenu);
foreach (double beats, beatSizes) {
Expand Down
52 changes: 34 additions & 18 deletions src/engine/controls/loopingcontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ LoopingControl::LoopingControl(const QString& group,
m_bAdjustingLoopInOld(false),
m_bAdjustingLoopOutOld(false),
m_bLoopOutPressedWhileLoopDisabled(false),
m_prevLoopSize(-1) {
m_prevLoopSize(-1),
m_trueTrackBeats(false) {
m_currentPosition.setValue(mixxx::audio::kStartFramePos);
m_pActiveBeatLoop = nullptr;
m_pRateControl = nullptr;
Expand Down Expand Up @@ -420,7 +421,7 @@ mixxx::audio::FramePos LoopingControl::nextTrigger(bool reverse,
// When the LoopIn button is released in reverse mode we jump to the end of the loop to not fall out and disable the active loop
// This must not happen in quantized mode. The newly set start is always ahead (in time, but behind spacially) of the current position so we don't jump.
// Jumping to the end is then handled when the loop's start is reached later in this function.
if (reverse && !m_bAdjustingLoopIn && !m_pQuantizeEnabled->toBool()) {
if (reverse && !m_bAdjustingLoopIn && !(quantizeEnabledAndHasTrueTrackBeats())) {
m_oldLoopInfo = loopInfo;
*pTargetPosition = loopInfo.endPosition;
return currentPosition;
Expand All @@ -434,7 +435,8 @@ mixxx::audio::FramePos LoopingControl::nextTrigger(bool reverse,
// When the LoopOut button is released in forward mode we jump to the start of the loop to not fall out and disable the active loop
// This must not happen in quantized mode. The newly set end is always ahead of the current position so we don't jump.
// Jumping to the start is then handled when the loop's end is reached later in this function.
if (!reverse && !m_bAdjustingLoopOut && !m_pQuantizeEnabled->toBool()) {
if (!reverse && !m_bAdjustingLoopOut &&
!(quantizeEnabledAndHasTrueTrackBeats())) {
m_oldLoopInfo = loopInfo;
*pTargetPosition = loopInfo.startPosition;
return currentPosition;
Expand Down Expand Up @@ -716,7 +718,7 @@ void LoopingControl::setLoopInToCurrentPosition() {
// silence of the last buffer. This position might be not reachable in
// a future runs, depending on the buffering.
mixxx::audio::FramePos position = math_min(info.currentPosition, info.trackEndPosition);
if (m_pQuantizeEnabled->toBool() && pBeats) {
if (quantizeEnabledAndHasTrueTrackBeats()) {
mixxx::audio::FramePos prevBeatPosition;
mixxx::audio::FramePos nextBeatPosition;
if (pBeats->findPrevNextBeats(position, &prevBeatPosition, &nextBeatPosition, false)) {
Expand Down Expand Up @@ -781,9 +783,10 @@ void LoopingControl::setLoopInToCurrentPosition() {
loopInfo.seekMode = LoopSeekMode::MovedOut;
}

if (m_pQuantizeEnabled->toBool() && loopInfo.startPosition.isValid() &&
if (quantizeEnabledAndHasTrueTrackBeats() &&
loopInfo.startPosition.isValid() &&
loopInfo.endPosition.isValid() &&
loopInfo.startPosition < loopInfo.endPosition && pBeats) {
loopInfo.startPosition < loopInfo.endPosition) {
m_pCOBeatLoopSize->setAndConfirm(pBeats->numBeatsInRange(
loopInfo.startPosition, loopInfo.endPosition));
updateBeatLoopingControls();
Expand Down Expand Up @@ -875,7 +878,7 @@ void LoopingControl::setLoopOutToCurrentPosition() {
// silence of the last buffer. This position might be not reachable in
// a future runs, depending on the buffering.
mixxx::audio::FramePos position = math_min(info.currentPosition, info.trackEndPosition);
if (m_pQuantizeEnabled->toBool() && pBeats) {
if (quantizeEnabledAndHasTrueTrackBeats()) {
mixxx::audio::FramePos prevBeatPosition;
mixxx::audio::FramePos nextBeatPosition;
if (pBeats->findPrevNextBeats(position, &prevBeatPosition, &nextBeatPosition, false)) {
Expand Down Expand Up @@ -951,7 +954,7 @@ void LoopingControl::setLoopOutToCurrentPosition() {
loopInfo.seekMode = LoopSeekMode::MovedOut;
}

if (m_pQuantizeEnabled->toBool() && pBeats) {
if (quantizeEnabledAndHasTrueTrackBeats()) {
m_pCOBeatLoopSize->setAndConfirm(pBeats->numBeatsInRange(
loopInfo.startPosition, loopInfo.endPosition));
updateBeatLoopingControls();
Expand Down Expand Up @@ -1249,15 +1252,24 @@ void LoopingControl::trackLoaded(TrackPointer pNewTrack) {

void LoopingControl::trackBeatsUpdated(mixxx::BeatsPointer pBeats) {
clearActiveBeatLoop();
m_pBeats = pBeats;
if (m_pBeats) {
LoopInfo loopInfo = m_loopInfo.getValue();
if (loopInfo.startPosition.isValid() && loopInfo.endPosition.isValid()) {
double loaded_loop_size = findBeatloopSizeForLoop(
loopInfo.startPosition, loopInfo.endPosition);
if (loaded_loop_size != -1) {
m_pCOBeatLoopSize->setAndConfirm(loaded_loop_size);
}
if (pBeats) {
m_pBeats = pBeats;
m_trueTrackBeats = true;
} else if (m_pTrack) {
// no beats, use fake beats so we can use seconds as beat unit
m_pBeats = getFake60BpmBeats();
m_trueTrackBeats = false;
} else {
// no track, no beats
m_pBeats = pBeats;
m_trueTrackBeats = false;
}
LoopInfo loopInfo = m_loopInfo.getValue();
if (loopInfo.startPosition.isValid() && loopInfo.endPosition.isValid()) {
double loaded_loop_size = findBeatloopSizeForLoop(
loopInfo.startPosition, loopInfo.endPosition);
if (loaded_loop_size != -1) {
m_pCOBeatLoopSize->setAndConfirm(loaded_loop_size);
}
}
}
Expand Down Expand Up @@ -1391,6 +1403,10 @@ bool LoopingControl::currentLoopMatchesBeatloopSize(const LoopInfo& loopInfo) co
return positionNear(loopInfo.endPosition, loopEndPosition);
}

bool LoopingControl::quantizeEnabledAndHasTrueTrackBeats() const {
return m_pQuantizeEnabled->toBool() && m_trueTrackBeats;
}

double LoopingControl::findBeatloopSizeForLoop(
mixxx::audio::FramePos startPosition,
mixxx::audio::FramePos endPosition) const {
Expand Down Expand Up @@ -1549,7 +1565,7 @@ void LoopingControl::slotBeatLoop(double beats,
currentPosition = pBeats->findNBeatsFromPosition(currentPosition, -beats);
}

bool quantize = m_pQuantizeEnabled->toBool();
bool quantize = quantizeEnabledAndHasTrueTrackBeats();
// loop_in is set to the closest beat if quantize is on and the loop size is >= 1 beat.
// The closest beat might be ahead of play position and will cause a catching loop.
switch (loopAnchor) {
Expand Down
14 changes: 14 additions & 0 deletions src/engine/controls/loopingcontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,17 @@ class LoopingControl : public EngineControl {
void clearLoopInfoAndControls();
void updateBeatLoopingControls();
bool currentLoopMatchesBeatloopSize(const LoopInfo& loopInfo) const;
bool quantizeEnabledAndHasTrueTrackBeats() const;

// Fake beats that allow using looping/beatjump controls with no beats:
// one 'beat' = one second
mixxx::BeatsPointer getFake60BpmBeats() {
auto fakeBeats = mixxx::Beats::fromConstTempo(
frameInfo().sampleRate,
mixxx::audio::kStartFramePos,
mixxx::Bpm(60.0));
return fakeBeats;
}

// Given loop in and out points, determine if this is a beatloop of a particular
// size.
Expand Down Expand Up @@ -252,6 +263,9 @@ class LoopingControl : public EngineControl {
// objects below are written from an engine worker thread
TrackPointer m_pTrack;
mixxx::BeatsPointer m_pBeats;
// Flag that allows to act quantized only if we have true track beats.
// See quantizeEnabledAndHasTrueTrackBeats()
bool m_trueTrackBeats;

friend class LoopingControlTest;
};
Expand Down
31 changes: 23 additions & 8 deletions src/skin/legacy/tooltips.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,9 +786,12 @@ void Tooltips::addStandardTooltips() {
<< tr("Loop Double")
<< tr("Doubles the current loop's length by moving the end marker.");

QString noBeatsSeconds = tr("If the track has no beats the unit is seconds.");

add("beatloop_size")
<< tr("Beatloop Size")
<< tr("Select the size of the loop in beats to set with the Beatloop button.")
<< noBeatsSeconds
<< tr("Changing this resizes the loop if the loop already matches this size.");

add("beatloop_halve")
Expand All @@ -801,6 +804,7 @@ void Tooltips::addStandardTooltips() {
add("beatloop_activate")
<< tr("Beatloop")
<< QString("%1: %2").arg(leftClick, tr("Start a loop over the set number of beats."))
<< noBeatsSeconds
<< quantizeSnap
<< QString("%1: %2").arg(rightClick, tr("Temporarily enable a rolling loop over the set number of beats."))
<< tr("Playback will resume where the track would have been if it had not entered the loop.");
Expand All @@ -812,21 +816,32 @@ void Tooltips::addStandardTooltips() {

add("beatjump_size")
<< tr("Beatjump/Loop Move Size")
<< noBeatsSeconds
<< tr("Select the number of beats to jump or move the loop with the Beatjump Forward/Backward buttons.");

add("beatjump_forward")
<< tr("Beatjump Forward")
<< QString("%1: %2").arg(leftClick + " " + loopInactive, tr("Jump forward by the set number of beats."))
<< QString("%1: %2").arg(leftClick + " " + loopActive, tr("Move the loop forward by the set number of beats."))
<< QString("%1: %2").arg(rightClick + " " + loopInactive, tr("Jump forward by 1 beat."))
<< QString("%1: %2").arg(rightClick + " " + loopActive, tr("Move the loop forward by 1 beat."));
<< QString("%1: %2").arg(leftClick + " " + loopInactive,
tr("Jump forward by the set number of beats."))
<< QString("%1: %2").arg(leftClick + " " + loopActive,
tr("Move the loop forward by the set number of beats."))
<< QString("%1: %2").arg(rightClick + " " + loopInactive,
tr("Jump forward by 1 beat."))
<< QString("%1: %2").arg(rightClick + " " + loopActive,
tr("Move the loop forward by 1 beat."))
<< noBeatsSeconds;

add("beatjump_backward")
<< tr("Beatjump Backward")
<< QString("%1: %2").arg(leftClick + " " + loopInactive, tr("Jump backward by the set number of beats."))
<< QString("%1: %2").arg(leftClick + " " + loopActive, tr("Move the loop backward by the set number of beats."))
<< QString("%1: %2").arg(rightClick + " " + loopInactive, tr("Jump backward by 1 beat."))
<< QString("%1: %2").arg(rightClick + " " + loopActive, tr("Move the loop backward by 1 beat."));
<< QString("%1: %2").arg(leftClick + " " + loopInactive,
tr("Jump backward by the set number of beats."))
<< QString("%1: %2").arg(leftClick + " " + loopActive,
tr("Move the loop backward by the set number of beats."))
<< QString("%1: %2").arg(rightClick + " " + loopInactive,
tr("Jump backward by 1 beat."))
<< QString("%1: %2").arg(rightClick + " " + loopActive,
tr("Move the loop backward by 1 beat."))
<< noBeatsSeconds;

add("loop_exit")
<< tr("Loop Exit")
Expand Down
22 changes: 0 additions & 22 deletions src/test/hotcuecontrol_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,28 +1092,6 @@ TEST_F(HotcueControlTest, CueLoopWithSavedLoopToggles) {
EXPECT_TRUE(m_pLoopEnabled->toBool());
}

TEST_F(HotcueControlTest, CueLoopWithoutLoopOrBeats) {
createAndLoadFakeTrack();

EXPECT_DOUBLE_EQ(static_cast<double>(HotcueControl::Status::Empty), m_pHotcue1Enabled->get());
EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
m_pHotcue1Position->get())
.isValid());
EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
m_pHotcue1EndPosition->get())
.isValid());

m_pHotcue1CueLoop->set(1);
m_pHotcue1CueLoop->set(0);

EXPECT_DOUBLE_EQ(static_cast<double>(HotcueControl::Status::Set), m_pHotcue1Enabled->get());
EXPECT_FRAMEPOS_EQ_CONTROL(mixxx::audio::kStartFramePos, m_pHotcue1Position);
EXPECT_FALSE(mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
m_pHotcue1EndPosition->get())
.isValid());
EXPECT_FALSE(m_pLoopEnabled->toBool());
}

Comment on lines -1095 to -1116
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove these unit tests? Now we have more cases to test and not less.

Copy link
Member Author

Choose a reason for hiding this comment

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

This tests that there is no (cue) loop created if the track has no beats. This situation will not occur anymore (with a track loaded).

We don't need to test "no beats" cases --> less tests.
What new cases are you referring to?

TEST_F(HotcueControlTest, SavedLoopToggleDoesNotSeek) {
// Setup fake track with 120 bpm and calculate loop size
TrackPointer pTrack = loadTestTrackWithBpm(120.0);
Expand Down
Loading
Loading