Keylock pitch compensation

[RJ Skerry-Ryan]

This bug is so we can discuss pitch compensation.

Since features_key is now merged, it includes "pitch compensation". When you enable keylock, the pitch slider is adjusted so that no change in pitch occurs. This "locks" the pitch at the key you selected.

Disabling keylock also adjusts in the other direction so that no pitch change occurs.

This causes problems for skins without pitch sliders. The pitch adjustment is not visible to the user and can't be removed if there is no pitch slider.

I think what we should do is prevent pitch change when enabling keylock but allow it when disabling and ONLY un-do the compensation we did when enabling it when disabling. That will prevent users w/o pitch sliders from getting "stuck" at a different pitch.

[Owen Williams (ywwg)]

this is the behavior I would expect. Can we detect if the skin has a pitch slider or not?

[jus (jus)]

Honestly, i thought it is a bug at first. We should make it at least optional, since it is an essential feature and differs from the behavior found elsewhere.

This goes against my muscle memory. If a preferences option would be available, i`d change it instantly to the pre1.12 setting. Other than in lp:1026590 was no word from users demanding change. Not saying it can`t be useful, but as default?

Another aspect is that keylock gets even weirder when you load a track into a deck, that has the key locked from the previous track. You can easily end up with a track at +/- 0.00% pitch with the track playing in the wrong key. Resetting the pitch and the retained key from previous track on track load could be a help here.

As for the skins:
Is there even a skin without pitch slider? If someone builds one, it is likely because he uses hardware controllers/timecode media and does not need an on-screen slider. As for resetting pitch we could change the `rate` label in a way, that a right-click resets to defaults.

[Sean M. Pappalardo (pegasus-renegadetech)]

I'm in favor of having no pitch change be the default on both enabling and disabling key lock in the interest of minimizing possible disruptions to program audio, thereby allowing flexibility when mixing since a DJ can enable and disable key lock at will with no fear of sudden pitch changes. (I would use this often.)

I'm also in favor of auto-resetting the pitch slider to 0% and the key to whatever 0% is (if keylock is enabled) on track load since I already do this manually anyway so why not automate it? It should be disable-able though.

[Daniel Schürmann (daschuer)]

I like the current behavior, because this way you have full control with only one slider and the key lock button (very controller friendly)
Workflow:
1. Adjust the key via the rate slider, (tempo follows as usual)
2. Lock at desired key
3. Adjust Tempo if needed

For me, (with no experience from other Apps) locking at the original key feels buggy. If we do this, we need a dedicated key slider.

Not auto reset the key/tempo settings on song load looks like a bug form the GUI point of view. In a possible solution we have to deal with the rate slider of the controller which usually is not servo driven. Maybe it is the right solution not to reset everything to 0, but reset to a "pure" rate change at the current controller position with key unlocked, as we where at 1. of the work flow.

[Owen Williams (ywwg)]

If keylock is on and the pitch slider has been adjusted and a new track is loaded, I think the key should be locked to the natural key of the new track. I know DJs who just keep keylock on all the time and expect the songs to be in the original key.

But I do want to be able to disable keylock and go back to the vinyl-style scaler. I'm not sure there's an intuitive way of doing this other than having Yet Another Button. Maybe if keylock is on, longpress goes back to linear scaler? That's just not discoverable is the problem.

[jus (jus)]

@daschuer re:If we do this, we need a dedicated key slider.
I think it is out of question that we'll have a dedicated control ( Knob) for adjusting the key, once the refactoring of the skin system is done.

[Daniel Schürmann (daschuer)]

@jus. Yes of cause, a dedicated control for adjusting the keys is desirable. But the controllers may not have it.

[Daniel Schürmann (daschuer)]

Bug #1026590 is related.

[RJ Skerry-Ryan (rryan)]

So, it's been about a year. I find this feature super frustrating. I'm always doing:

1) beatmatch
2) realize I forgot keylock since the track I adjusted is chipmunking
3) enable keylock
4) realize that did nothing
5) disable keylock
6) set pitch to zero
7) enable keylock
8) re-beatmatch

As Jus comments, it goes against my muscle memory.

My conclusion: If the user doesn't touch the pitch knob we should not be touching it for them.

[Owen Williams (ywwg)]

On my mapping I have a "reset pitch" button, so in your case if the track is chipmunking and you enable keylock, you'd push the keylock reset and and it would go back to natural pitch.

That said, I'm ok with changing the behavior. I've tried the intended method, which is key-matching in vinyl mode, locking, and then bpm-matching in keylock mode, and it sucks. Hearing the key-match is impossible when the bpms don't align.

I'm not sure what the solution is for users without a pitch knob other than adding a control in the skins.

[RJ Skerry-Ryan (rryan)]

Hm, given that pitch-adjust and key detection is a headline feature in 1.12.0 I think the skins need pitch knobs.

[Owen Williams (ywwg)]

What about little arrow buttons? like

[<]A#[>]

[RJ Skerry-Ryan (rryan)]

Would the steps be 1/12th of an octave? If so then you can't use it to tweak a slightly out of tune key match.

[Daniel Schürmann (daschuer)]

@ RJ:

So in your case you need key-lock to original key, at 3) right?

1) beatmatch
2) realize I forgot keylock since the track I adjusted is chipmunking
3) enable keylock

this is the "non harmonic key" mode. We can do a preference option for this and make it default.

Since it turns out that the "beatmatch first" use-case is more common, we may reflect this in Mixxx.

If we still want to allow the one slider approach with this, what about introducing a rate silder mode switch in skin or as preference option. This may replace the keylock button on the controllers.
Modes:
* 1) Rate slider -> what you have on a turntable (The CPU saver)
* 2) Pitch slider (tempo lock)
* 3) Tempo slider (key Lock, this is what we have now)

So we have 1) after track load,
* beat match (the old way)
* switch to pitch mode 2)
* match key (or right click slider or key display to reset key)

We have original disused it here:
https://github.com/mixxxdj/mixxx/pull/47

I had also a look at CD Players manuals:
* Numark CDX "Key Lock – holds the music at the current key"
* American RADIUS 1000 Locks: at original key
* The DENON DN-S1200 has slider modes Modes Pitch on/off and Key adjust on/off

So it looks like we have the whole range out there .. :-/

Conclusion:
* Since we have only one Key button + one LED and one pitch slider on most controllers a preference option would be nice.
What about this
"lock button is:"
* Lock a at original key = Mixxx 1.11
* Lock current Key (Tempo adjust) = what we have right now
* Lock current Tempo (Pitch adjust) = what we probably miss
Default is rate slider (tempo and pitch)

[Owen Williams (ywwg)]

This is why too many preferences is really annoying. We tried to have a one-slider solution and it didn't work, so we are making sure that all the skins have key adjustments for users without a separate knob.

[RJ Skerry-Ryan (rryan)]

Workflow-wise, you don't need a whole slider for the pitch so a 3-mode rate slider is not necessary. All you need is an adjustment knob for fine-tuning and ideally something nice for triggering sync_key if you choose to do harmonic mixing.

The symmetry between locking pitch and locking tempo is something that will only appeal to an engineer. The rate slider is primarily used for adjusting the track's speed and we should keep it that way in Mixxx.

As for keylock locking at the current pitch the engine code that accomplishes this feature is kind of tricky. I would hesitate to make it a preference option because supporting both will be harder.

[Daniel Schürmann (daschuer)]

What about this:
1: add a tempo_lock control
2: add a speed_lock control wich is configurable from prefernces
3: replace key_lock with speed_lock within the skins

Add a mode combobox in preferences:
* Lock locks at original key (classic)
* Lock is tempo lock (beatmatch fist) => this seams to be the best foe harmonic Mixing using Sync feature
* Lock is key lock
* Auto key lock at > 4 % => this seams to be the ideal compromise between save CPU and key lock allways on use case.

If we use a new meta control, it seems that we can integate this independently without smash up the rest.

[Owen Williams (ywwg)]

RJ, the buttons in my skin do pitch_up_small and pitch_down_small, and the center button does a reset. I'm not sure what steps they use but they seem to be less than half-steps (A click doesn't always change the displayed key) . My controller has a clicky wheel for adjusting pitch so we should make sure that pitch_XX_small is good.

One crazy idea is that when master sync is on, the pitch slider adjusts musical key instead of rate. I find that once master sync is on I never adjust the speed with a rate sliders because the hardware sliders aren't up to date with the position that master sync chose (even with soft-takeover, the amount of error if I try to tweak a small amount will be noticable). Of course the question is how to adjust the BPM when master sync is on, and how to indicate this mode in the skins for people who are not expecting it...

[Daniel Schürmann (daschuer)]

I have thought a bit more about my comment https://bugs.launchpad.net/mixxx/+bug/1258617/comments/17 and played with the pitch knob.

What means pitch = 0?
* Keylock off: combined speed slider (pitch and tempo) -> 0 is at the natural speed slider pitch
* Keylock on: independent speed slider = tempo slider -> 0 is at the original track pitch

Switching Key lock switches the pitch slider scale and makes a fixed scale midi knob out of sync which might be one issue of this bug that we should fix.

How common will it be to map the pitch knob to a fixed scale button? Since we reset the pitch anyway it is a candidate to be out of sync often. So what about this:
Normalize the pitch knob always to the original deck pitch.

This way we do not need a key_lock button at all for RJs use case:
1) beatmatch
2) realize I forgot keylock since the track I adjusted is chipmunking
3) right click pitch button

4) (optional) .. than match key

In the meanwhile, I was convinced that the "key match first" use case is unnatural and we need no support for it.
So if we have the outlined fix, It is just a skin designer decision, to offer a pitch knob and a key_lock button or just a tempo_lock button. (no preference option required)

A tempo_lock button is nice if you rely on the new sync feature, where you do not need to change the tempo manual.
This is almost the same as Owen crazy idea.

[Daniel Schürmann (daschuer)]

Stated to work on the fixed scale for the pitch button.

[RJ Skerry-Ryan (rryan)]

Daniel -- I'm not sure what you mean by "fixed scale". Could you explain?

The "pitch" control is -1 for a full octave adjust down and +1 for a full octave adjust up. Its scale never changes and it is a relative adjustment to the track's native pitch. That's a feature, not a bug!

[Owen Williams (ywwg)]

I don't see a problem with non-endless rotary knobs being used as key adjustment controls -- 0.5 should be original track key, and users can use soft-takeover to prevent unexpected jumps. The knob can do +/- 1 octave and that should be enough.

[Daniel Schürmann (daschuer)]

> Its scale never changes and it is a relative adjustment to the track's native pitch.

The issue is that this is not true.

Currently the 0 point jumps depending on the keylock. Thats the issue for me. I am working on a fix, where 0 is always the original track key. This will make th pitch slider more reliable and is a good base to fix this issue entirely.

[Daniel Schürmann (daschuer)]

Any comments or objections to fix it?

[Daniel Schürmann (daschuer)]

https://github.com/mixxxdj/mixxx/pull/415
https://github.com/mixxxdj/mixxx/pull/416

[Daniel Schürmann (daschuer)]

An other aspect is discussed here: https://bugs.launchpad.net/mixxx/+bug/1398204

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7214