(fix) avoid automatic key offset after track load with keylock mode 'Keep current key'

src/engine/controls/keycontrol.cpp

@@ -7,16 +7,14 @@
 #include "control/controlpotmeter.h"
 #include "control/controlproxy.h"
 #include "control/controlpushbutton.h"
+#include "engine/defs_keylock.h"
 #include "engine/enginebuffer.h"
 #include "mixer/playermanager.h"
 #include "moc_keycontrol.cpp"
 #include "track/keyutils.h"
 
 constexpr bool kEnableDebugOutput = false;
 
-static const double kLockCurrentKey = 1;
-static const double kKeepUnlockedKey = 1;
-
 KeyControl::KeyControl(const QString& group,
         UserSettingsPointer pConfig)
         : EngineControl(group, pConfig),
@@ -147,9 +145,9 @@ void KeyControl::slotRateChanged() {
     updateRate();
 }
 
-// This is called when rate_ratio, vinylcontrol_rate, vinylcontrol_enabled or
-// keylock are changed, but also when EngineBuffer::processTrackLocked requests
-// m_pitchRateInfo struct while rate, pitch or pitch_adjust were just updated.
+/// This is called when rate_ratio, vinylcontrol_rate, vinylcontrol_enabled or
+/// keylock are changed, but also when EngineBuffer::processTrackLocked requests
+/// m_pitchRateInfo struct while rate, pitch or pitch_adjust were just updated.
 void KeyControl::updateRate() {
     if (m_pVCEnabled && m_pVCRate && m_pVCEnabled->toBool()) {
         m_pitchRateInfo.tempoRatio = m_pVCRate->get();
@@ -191,7 +189,9 @@ void KeyControl::updateRate() {
 
     if (m_pKeylock->toBool()) {
         if (!m_pitchRateInfo.keylock) {                    // Enabling keylock
-            if (m_keylockMode->get() == kLockCurrentKey) { // Lock at current pitch
+            if (m_keylockMode->get() ==
+                    static_cast<double>(KeylockMode::LockCurrentKey)) {
+                // Lock at current pitch
                 speedSliderPitchRatio = m_pitchRateInfo.tempoRatio;
                 if constexpr (kEnableDebugOutput) {
                     qDebug() << "   LOCKING current key";
@@ -223,7 +223,7 @@ void KeyControl::updateRate() {
         }
     } else {                           // !m_pKeylock
         if (m_pitchRateInfo.keylock) { // Disabling Keylock
-            if (m_keyunlockMode->get() == kKeepUnlockedKey) {
+            if (m_keyunlockMode->get() == static_cast<double>(KeyunlockMode::KeepLockedKey)) {
                 // adopt speedSliderPitchRatio change as pitchTweakRatio
                 m_pitchRateInfo.pitchTweakRatio *=
                         (speedSliderPitchRatio / m_pitchRateInfo.tempoRatio);
@@ -299,6 +299,8 @@ void KeyControl::updateRate() {
     updateKeyCOs(dFileKey, pitchOctaves);
 }
 
+/// This is called when the file key is changed (by analysis or user input),
+/// or when a track with a different key is loaded
 void KeyControl::slotFileKeyChanged(double value) {
     updateKeyCOs(value,  m_pPitch->get() / 12);
 }
@@ -352,6 +354,9 @@ void KeyControl::setEngineKey(double key, double key_distance) {
     return;
 }
 
+/// This is called when pitch is changed either by user interaction,
+/// or when BaseTrackPlayerImpl resets the pitch on track load per configuration
+/// AND key is locked with KeylockMode::LockCurrentKey
 void KeyControl::slotPitchChanged(double pitch) {
     Q_UNUSED(pitch)
     m_updatePitchRequest = 1;
@@ -391,6 +396,9 @@ void KeyControl::updatePitch() {
     }
 }
 
+/// This is called when pitch_adjust is changed either by user interaction,
+/// or when BaseTrackPlayerImpl resets the pitch on track load per configuration
+/// AND key is unlocked or locked with LockOriginalKey
 void KeyControl::slotPitchAdjustChanged(double pitchAdjust) {
     Q_UNUSED(pitchAdjust);
     m_updatePitchAdjustRequest = 1;

src/engine/defs_keylock.h

@@ -0,0 +1,11 @@
+#pragma once
+
+enum class KeylockMode {
+    LockOriginalKey,
+    LockCurrentKey
+};
+
+enum class KeyunlockMode {
+    ResetLockedKey,
+    KeepLockedKey
+};

src/mixer/basetrackplayer.cpp

@@ -8,6 +8,7 @@
 #include "control/controlobject.h"
 #include "engine/channels/enginedeck.h"
 #include "engine/controls/enginecontrol.h"
+#include "engine/defs_keylock.h"
 #include "engine/engine.h"
 #include "engine/enginebuffer.h"
 #include "engine/enginemixer.h"
@@ -277,7 +278,10 @@ BaseTrackPlayerImpl::BaseTrackPlayerImpl(
     m_pPlay->connectValueChanged(this, &BaseTrackPlayerImpl::slotPlayToggled);
 
     m_pRateRatio = make_parented<ControlProxy>(getGroup(), "rate_ratio", this);
+    m_pPitch = make_parented<ControlProxy>(getGroup(), "pitch", this);
     m_pPitchAdjust = make_parented<ControlProxy>(getGroup(), "pitch_adjust", this);
+    m_pKeylock = make_parented<ControlProxy>(getGroup(), "keylock", this);
+    m_pKeylockMode = make_parented<ControlProxy>(getGroup(), "keylockMode", this);
 
     m_pUpdateReplayGainFromPregain = std::make_unique<ControlPushButton>(
             ConfigKey(getGroup(), "update_replaygain_from_pregain"));
@@ -705,7 +709,16 @@ void BaseTrackPlayerImpl::slotTrackLoaded(TrackPointer pNewTrack,
                 }
             }
             if (reset == RESET_PITCH || reset == RESET_PITCH_AND_SPEED) {
-                m_pPitchAdjust->set(0.0);
+                // With KeylockMode::LockCurrentKey we need to reset `pitch`
+                // instead of `pitch_adjust` to avoid a roundtrip in KeyControl
+                // which would lead `pitch` != 0
+                if (m_pKeylock->toBool() &&
+                        m_pKeylockMode->get() ==
+                                static_cast<double>(KeylockMode::LockCurrentKey)) {
+                    m_pPitch->set(0.0);
+                } else {
+                    m_pPitchAdjust->set(0.0);
+                }
             }
         } else {
             // perform a clone of the given channel

src/mixer/basetrackplayer.h

@@ -238,5 +238,8 @@ class BaseTrackPlayerImpl : public BaseTrackPlayer {
     parented_ptr<ControlProxy> m_pHighFilterKill;
     parented_ptr<ControlProxy> m_pPreGain;
     parented_ptr<ControlProxy> m_pRateRatio;
+    parented_ptr<ControlProxy> m_pPitch;
     parented_ptr<ControlProxy> m_pPitchAdjust;
+    parented_ptr<ControlProxy> m_pKeylock;
+    parented_ptr<ControlProxy> m_pKeylockMode;
 };

src/preferences/dialog/dlgprefdeck.h

@@ -4,6 +4,7 @@
 
 #include "engine/controls/cuecontrol.h"
 #include "engine/controls/ratecontrol.h"
+#include "engine/defs_keylock.h"
 #include "preferences/dialog/dlgpreferencepage.h"
 #include "preferences/dialog/ui_dlgprefdeckdlg.h"
 #include "preferences/usersettings.h"
@@ -34,16 +35,6 @@ namespace TrackTime {
     };
 }
 
-enum class KeylockMode {
-    LockOriginalKey,
-    LockCurrentKey
-};
-
-enum class KeyunlockMode {
-    ResetLockedKey,
-    KeepLockedKey
-};
-
 enum class LoadWhenDeckPlaying {
     Reject,
     Allow,

src/test/enginebuffertest.cpp

@@ -9,6 +9,7 @@
 
 #include "control/controlobject.h"
 #include "engine/controls/ratecontrol.h"
+#include "engine/defs_keylock.h"
 #include "mixer/basetrackplayer.h"
 #include "preferences/usersettings.h"
 #include "test/mixxxtest.h"
@@ -31,9 +32,9 @@ TEST_F(EngineBufferTest, DisableKeylockResetsPitch) {
     // To prevent one-slider users from getting stuck on a key,
     // KeyunlockMode::ResetLockedKey resets the musical pitch.
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-            1.0); // KeylockMode::LockCurrentKey
+            static_cast<double>(KeylockMode::LockCurrentKey));
     ControlObject::set(ConfigKey(m_sGroup1, "keyunlockMode"),
-            0.0); // KeyunlockMode::ResetLockedKey
+            static_cast<double>(KeyunlockMode::ResetLockedKey));
     ControlObject::set(ConfigKey(m_sGroup1, "file_bpm"), 128.0);
     ControlObject::set(ConfigKey(m_sGroup1, "keylock"), 1.0);
     ControlObject::set(ConfigKey(m_sGroup1, "pitch"), 0.5);
@@ -48,9 +49,9 @@ TEST_F(EngineBufferTest, DisableKeylockResetsPitch) {
 TEST_F(EngineBufferTest, DisableKeylockKeepsPitch) {
     // Pitch must not change when unlocking with KeyunlockMode::KeepLockedKey.
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-            1.0); // KeylockMode::LockCurrentKey
+            static_cast<double>(KeylockMode::LockCurrentKey));
     ControlObject::set(ConfigKey(m_sGroup1, "keyunlockMode"),
-            1.0); // KeyunlockMode::KeepLockedKey
+            static_cast<double>(KeyunlockMode::KeepLockedKey));
     ControlObject::set(ConfigKey(m_sGroup1, "file_bpm"), 128.0);
     ControlObject::set(ConfigKey(m_sGroup1, "keylock"), 1.0);
     ControlObject::set(ConfigKey(m_sGroup1, "pitch"), 0.5);
@@ -75,12 +76,42 @@ TEST_F(EngineBufferTest, TrackLoadResetsPitch) {
     ASSERT_NEAR(0.0, ControlObject::get(ConfigKey(m_sGroup1, "pitch_adjust")), 1e-10);
 }
 
+TEST_F(EngineBufferTest, TrackLoadResetsPitch_LockCurrentKey) {
+    // The pitch should be reset to 0 when a new track was loaded when
+    // * rate is not 0
+    // * keylock is ON
+    // * keylock mode is LockCurrentKey,
+    // * Reset Pitch on track load option is enabled
+    //
+    // First test case:
+    // * change tempo with key unlocked -> pitch changes
+    // * lock key
+    // // * reset pitch -> is now 0 OPTIONAL
+    // * load another track -> pitch should (still) be 0
+    config()->set(ConfigKey("[Controls]", "SpeedAutoReset"),
+            ConfigValue(BaseTrackPlayer::RESET_PITCH));
+    ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
+            static_cast<double>(KeylockMode::LockCurrentKey));
+    ControlObject::set(ConfigKey(m_sGroup1, "rate"), 0.5);
+    ControlObject::set(ConfigKey(m_sGroup1, "keylock"), 1.0);
+    ControlObject::set(ConfigKey(m_sGroup1, "reset_key"), 1.0);
+    ProcessBuffer();
+    // Note that pitch_adjust is NOT reset to 0 with KeylockMode::LockCurrentKey
+    ASSERT_DOUBLE_EQ(0.0, ControlObject::get(ConfigKey(m_sGroup1, "pitch")));
+    ProcessBuffer();
+
+    m_pMixerDeck1->loadFakeTrack(false, 0.0);
+    ProcessBuffer();
+
+    ASSERT_DOUBLE_EQ(0.0, ControlObject::get(ConfigKey(m_sGroup1, "pitch")));
+}
+
 TEST_F(EngineBufferTest, PitchRoundtrip) {
     ControlObject::set(ConfigKey(m_sGroup1, "keylock"), 0.0);
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-            0.0); // KeylockMode::LockOriginalKey
+            static_cast<double>(KeylockMode::LockOriginalKey));
     ControlObject::set(ConfigKey(m_sGroup1, "keyunlockMode"),
-            0.0); // KeyunlockMode::ResetLockedKey
+            static_cast<double>(KeyunlockMode::ResetLockedKey));
     ProcessBuffer();
     // we are in kPakmOffsetScaleReseting mode
     ControlObject::set(ConfigKey(m_sGroup1, "rate"), 0.5);
@@ -103,15 +134,15 @@ TEST_F(EngineBufferTest, PitchRoundtrip) {
     ASSERT_DOUBLE_EQ(0.0, ControlObject::get(ConfigKey(m_sGroup1, "pitch_adjust")));
 
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-            1.0); // KeylockMode::LockCurrentKey
+            static_cast<double>(KeylockMode::LockCurrentKey));
     ProcessBuffer();
     // rate must not change
     ASSERT_DOUBLE_EQ(0.5, ControlObject::get(ConfigKey(m_sGroup1, "rate")));
     // pitch must reflect the absolute pitch
     ASSERT_DOUBLE_EQ(0.0, ControlObject::get(ConfigKey(m_sGroup1, "pitch")));
 
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-            0.0); // KeylockMode::LockOriginalKey
+            static_cast<double>(KeylockMode::LockOriginalKey));
     ProcessBuffer();
     // rate must not change
     ASSERT_DOUBLE_EQ(0.5, ControlObject::get(ConfigKey(m_sGroup1, "rate")));
@@ -370,7 +401,7 @@ TEST_F(EngineBufferE2ETest, DISABLED_KeylockReverseTest) {
     ControlObject::set(ConfigKey(kAppGroup, QStringLiteral("keylock_engine")),
             static_cast<double>(EngineBuffer::KeylockEngine::SoundTouch));
     ControlObject::set(ConfigKey(m_sGroup1, "keylockMode"),
-                       0.0);
+            static_cast<double>(KeylockMode::LockOriginalKey));
     ControlObject::set(ConfigKey(m_sGroup1, "rate"), 0.5);
     ControlObject::set(ConfigKey(m_sGroup1, "play"), 1.0);
     ControlObject::set(ConfigKey(m_sGroup1, "keylock"), 1.0);