-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 6 commits
a320ada
92bdfa1
636434b
5ee1c71
edc5d01
4b6e4be
783c041
51af7c4
72d5d84
bd77c26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,12 +9,25 @@ | |||||
class InternalClock; | ||||||
class EngineChannel; | ||||||
|
||||||
#define BPM_CONFIG_GROUP "[BPM]" | ||||||
#define SYNC_LOCK_ALGORITHM_CONFIG_KEY "sync_lock_algorithm" | ||||||
Swiftb0y marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
/// 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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
// 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; | ||||||
|
||||||
|
@@ -140,17 +153,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); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -267,9 +268,7 @@ DlgPrefDeck::DlgPrefDeck(QWidget* parent, | |
} | ||
setRateRangeForAllDecks(m_iRateRangePercent); | ||
|
||
// | ||
// Key lock mode | ||
// | ||
connect(buttonGroupKeyLockMode, | ||
QOverload<QAbstractButton*>::of(&QButtonGroup::buttonClicked), | ||
this, | ||
|
@@ -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, | ||
|
@@ -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>( | ||
|
@@ -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(BPM_CONFIG_GROUP, SYNC_LOCK_ALGORITHM_CONFIG_KEY), | ||
EngineSync::SyncLockAlgorithm::PREFER_SOFT_LEADER)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing this snippet while starting the review a couple weeks ago was the entire reason for why I implemented #11883 😅 |
||
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) { | ||
|
@@ -550,6 +559,8 @@ void DlgPrefDeck::slotResetToDefaults() { | |
checkBoxResetSpeed->setChecked(false); | ||
checkBoxResetPitch->setChecked(true); | ||
|
||
radioButtonSoftLeader->setChecked(true); | ||
|
||
radioButtonOriginalKey->setChecked(true); | ||
radioButtonResetUnlockedKey->setChecked(true); | ||
} | ||
|
@@ -729,6 +740,14 @@ void DlgPrefDeck::slotApply() { | |
m_pConfig->set(ConfigKey("[Controls]", "SpeedAutoReset"), | ||
ConfigValue(configSPAutoReset)); | ||
|
||
if (radioButtonSoftLeader->isChecked()) { | ||
m_pConfig->setValue(ConfigKey(BPM_CONFIG_GROUP, SYNC_LOCK_ALGORITHM_CONFIG_KEY), | ||
static_cast<int>(EngineSync::SyncLockAlgorithm::PREFER_SOFT_LEADER)); | ||
} else { | ||
m_pConfig->setValue(ConfigKey(BPM_CONFIG_GROUP, SYNC_LOCK_ALGORITHM_CONFIG_KEY), | ||
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 | ||
|
There was a problem hiding this comment.
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)