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

Expose [BPM],sync_lock_algorithm option in deck preferences. #11828

Merged
merged 10 commits into from
Nov 16, 2023
2 changes: 2 additions & 0 deletions src/defs_urls.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
MIXXX_MANUAL_URL "/chapters/preferences.html#library"
#define MIXXX_MANUAL_CUE_MODES_URL \
MIXXX_MANUAL_URL "/chapters/user_interface.html#using-cue-modes"
#define MIXXX_MANUAL_SYNC_MODES_URL \
MIXXX_MANUAL_URL "/chapters/user_interface.html#sync-and-rate-controls"
Copy link
Member

Choose a reason for hiding this comment

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

Now that ___ has been merged, we should link to https://manual.mixxx.org/2.4/chapters/djing_with_mixxx#sync-lock-with-dynamic-tempo

Btw, when I click the (?) link I end up at https://manual.mixxx.org/2.4/chapters/user_interface.html#sync-and-rate-controls + X * PageDown (at the effects section)

#define MIXXX_MANUAL_BEATS_URL \
MIXXX_MANUAL_URL "/chapters/preferences.html#beat-detection"
#define MIXXX_MANUAL_KEY_URL \
Expand Down
6 changes: 3 additions & 3 deletions src/engine/sync/enginesync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,10 @@ Syncable* EngineSync::pickLeader(Syncable* enabling_syncable) {
}

const SyncLockAlgorithm picker = static_cast<SyncLockAlgorithm>(
m_pConfig->getValue<int>(ConfigKey("[BPM]", "sync_lock_algorithm"),
PREFER_IMPLICIT_LEADER));
m_pConfig->getValue<int>(ConfigKey(kBpmConfigGroup, kSyncLockAlgorithmConfigKey),
PREFER_SOFT_LEADER));
switch (picker) {
case PREFER_IMPLICIT_LEADER:
case PREFER_SOFT_LEADER:
// Always pick a deck for a new leader.
if (playing_deck_count == 1) {
return first_playing_deck;
Expand Down
26 changes: 15 additions & 11 deletions src/engine/sync/enginesync.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,34 @@

#include <gtest/gtest_prod.h>

#include <QString>

#include "audio/types.h"
#include "engine/sync/syncable.h"
#include "preferences/usersettings.h"

class InternalClock;
class EngineChannel;

const QString kBpmConfigGroup = QStringLiteral("[BPM]");
const QString kSyncLockAlgorithmConfigKey = QStringLiteral("sync_lock_algorithm");

/// EngineSync is the heart of the Mixxx Sync Lock engine. It knows which objects
/// (Decks, Internal Clock, etc) are participating in Sync and what their statuses
/// are. It also orchestrates sync handoffs between different decks as they play,
/// stop, or request their status to change.
class EngineSync : public SyncableListener {
public:
// Remove pick SyncLockAlgorithm when new beatgrid detection
// and editing code is committed and we no longer need the lock bpm fallback option.
enum SyncLockAlgorithm {
// New behavior, which should work if beatgrids are reliable.
PREFER_SOFT_LEADER,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I'm missing something, but isn't it irrelevant if the leader is implicit/soft or explicit?

Suggested change
PREFER_SOFT_LEADER,
PREFER_LEADER,

Copy link
Member Author

@daschuer daschuer Aug 15, 2023

Choose a reason for hiding this comment

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

The explicit leader (full crown symbol) is always preferred. This feature is about the automatically selected soft leader.
"Shall the follower follow a dynamic tempo or should both follow a steady tempo?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently this preferences option is designed to be extensible for more features that will come on the table once we have rhythm detection implemented.
Does it make sense to decouple it?

// Old 2.3 behavior, which works around some issues with bad beatgrid detection, mostly
// for auto DJ mode.
PREFER_LOCK_BPM
};

explicit EngineSync(UserSettingsPointer pConfig);
~EngineSync() override;

Expand Down Expand Up @@ -140,17 +155,6 @@ class EngineSync : public SyncableListener {
return false;
}

// TODO: Remove pick algorithms during 2.4 development phase when new beatgrid detection
// and editing code is committed and we no longer need the lock bpm fallback option.
// If this code makes it to release we will all be very sad.
enum SyncLockAlgorithm {
// New behavior, which should work if beatgrids are reliable.
PREFER_IMPLICIT_LEADER,
// Old 2.3 behavior, which works around some issues with bad beatgrid detection, mostly
// for auto DJ mode.
PREFER_LOCK_BPM
};

FRIEND_TEST(EngineSyncTest, EnableOneDeckInitsLeader);
FRIEND_TEST(EngineSyncTest, EnableOneDeckInitializesLeader);
FRIEND_TEST(EngineSyncTest, SyncToNonSyncDeck);
Expand Down
41 changes: 30 additions & 11 deletions src/preferences/dialog/dlgprefdeck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "defs_urls.h"
#include "engine/controls/ratecontrol.h"
#include "engine/enginebuffer.h"
#include "engine/sync/enginesync.h"
#include "mixer/basetrackplayer.h"
#include "mixer/playerinfo.h"
#include "mixer/playermanager.h"
Expand Down Expand Up @@ -267,9 +268,7 @@ DlgPrefDeck::DlgPrefDeck(QWidget* parent,
}
setRateRangeForAllDecks(m_iRateRangePercent);

//
// Key lock mode
//
connect(buttonGroupKeyLockMode,
QOverload<QAbstractButton*>::of(&QButtonGroup::buttonClicked),
this,
Expand All @@ -282,9 +281,7 @@ DlgPrefDeck::DlgPrefDeck(QWidget* parent,
pControl->set(static_cast<double>(m_keylockMode));
}

//
// Key unlock mode
//
connect(buttonGroupKeyUnlockMode,
QOverload<QAbstractButton*>::of(&QButtonGroup::buttonClicked),
this,
Expand All @@ -297,21 +294,23 @@ DlgPrefDeck::DlgPrefDeck(QWidget* parent,
pControl->set(static_cast<int>(m_keyunlockMode));
}

//
// Cue Mode
//

// Add "(?)" with a manual link to the label
labelCueMode->setText(labelCueMode->text() + QStringLiteral(" ") +
labelCueMode->setText(labelCueMode->text() + QChar(' ') +
coloredLinkString(
m_pLinkColor,
QStringLiteral("(?)"),
MIXXX_MANUAL_CUE_MODES_URL));

//
// Speed / Pitch reset configuration
//
// Sync Mode
// Add "(?)" with a manual link to the label
labelSyncMode->setText(labelSyncMode->text() + QChar(' ') +
coloredLinkString(
m_pLinkColor,
QStringLiteral("(?)"),
MIXXX_MANUAL_SYNC_MODES_URL));

// Speed / Pitch reset configuration
// Update "reset speed" and "reset pitch" check boxes
// TODO: All defaults should only be set in slotResetToDefaults.
int configSPAutoReset = m_pConfig->getValue<int>(
Expand Down Expand Up @@ -465,6 +464,16 @@ void DlgPrefDeck::slotUpdate() {
index = ComboBoxCueMode->findData(static_cast<int>(cueMode));
ComboBoxCueMode->setCurrentIndex(index);

const EngineSync::SyncLockAlgorithm syncLockAlgorithm =
static_cast<EngineSync::SyncLockAlgorithm>(m_pConfig->getValue<int>(
ConfigKey(kBpmConfigGroup, kSyncLockAlgorithmConfigKey),
EngineSync::SyncLockAlgorithm::PREFER_SOFT_LEADER));
if (syncLockAlgorithm == EngineSync::SyncLockAlgorithm::PREFER_SOFT_LEADER) {
radioButtonSoftLeader->setChecked(true);
} else {
radioButtonLockBpm->setChecked(true);
}

KeylockMode keylockMode =
static_cast<KeylockMode>(static_cast<int>(m_keylockModeControls[0]->get()));
if (keylockMode == KeylockMode::LockCurrentKey) {
Expand Down Expand Up @@ -550,6 +559,8 @@ void DlgPrefDeck::slotResetToDefaults() {
checkBoxResetSpeed->setChecked(false);
checkBoxResetPitch->setChecked(true);

radioButtonSoftLeader->setChecked(true);

radioButtonOriginalKey->setChecked(true);
radioButtonResetUnlockedKey->setChecked(true);
}
Expand Down Expand Up @@ -729,6 +740,14 @@ void DlgPrefDeck::slotApply() {
m_pConfig->set(ConfigKey("[Controls]", "SpeedAutoReset"),
ConfigValue(configSPAutoReset));

if (radioButtonSoftLeader->isChecked()) {
m_pConfig->setValue(ConfigKey(kBpmConfigGroup, kSyncLockAlgorithmConfigKey),
static_cast<int>(EngineSync::SyncLockAlgorithm::PREFER_SOFT_LEADER));
} else {
m_pConfig->setValue(ConfigKey(kBpmConfigGroup, kSyncLockAlgorithmConfigKey),
static_cast<int>(EngineSync::SyncLockAlgorithm::PREFER_LOCK_BPM));
}

m_pConfig->setValue(ConfigKey("[Controls]", "keylockMode"),
static_cast<int>(m_keylockMode));
// Set key lock behavior for every group
Expand Down
Loading