Sync Lock: minor refactoring. #3695
Conversation
|
There are also a couple of debug trace additions that were useful during testing |
|
However I'm not doing the huge master -> Leader rename just yet. I want to wait until the big patch goes in, and then I'll do a mass rename. |
|
The moment the Big Patch (#3698) is approved that will happen |
There was a problem hiding this comment.
What is the case this PR shall fix?
In my test, I have set the reverse control of one or both track. My expectation is that the beats are in sync in the same way as if the track where played forward.
The tempo sync works flawlessly. However the phase sync fails.
I have tested the quantize play and seeks using the waveform overview.
In forward mode it stays in sync, in backward mode it fails.
|
I'll check that. This fix was for an obsolete situation with one-track-scratching-another, so it only really applies when both tracks are moving forward or backward together. Maybe I should just remove that bit? |
src/engine/controls/bpmcontrol.cpp
Outdated
| @@ -470,6 +470,11 @@ double BpmControl::calcSyncAdjustment(bool userTweakingSync) { | |||
| double syncTargetBeatDistance = m_dSyncTargetBeatDistance.getValue(); | |||
There was a problem hiding this comment.
This can make use of a comment.
// This is the distance from the last beat in forward direction.
I am not sure if it is a good idea to use the forward direction in a "beat_distance" control.
We need here the distance from the last beat in play direction. I think it can be expected that the "beat_distance" control does this natively. Otherwise we need here the play direction of the sync target.
There was a problem hiding this comment.
No, I think it should always use the forward direction, like this:
|---| -> 0.0/1.0
^
|---| -> 0.75
^
|---| -> 0.5
^
|---| -> 0.25
^
|---| -> 0.0/1.0
^
There was a problem hiding this comment.
What else do we use "beat_distance" for?
If we need to know in every single place the direction as well to make use of this value, it is a good indicator that the current forward only implementation is too unhandy and error prone.
There was a problem hiding this comment.
My mapping uses it for the glowing ring around the jog wheels: 876cd47
There was a problem hiding this comment.
It looks like your code is also broken in reverse direction an will benefit from the change.
When I got it right it re-implements the "beat_active" control which will be fixed with #3608.
There was a problem hiding this comment.
What it does is it calculates the position in the bar (or rather: the position in a series of 4 beats) and transforms it into a Midi value between 0x00 and 0x7F. That illuminates to a led indicator around the jog wheels where 0x00 is the 12 o'clock position, 0x20 is 3 o'clock, 0x40 is 6 o'clock, and so on.
You can see it in this video on left deck: https://youtu.be/2QqVZkZuA0g
It doesn't work correctly when playing backwards, but that's unrelated to beat_distance. The issue is that I just count up beat indices to get the beat in the measure because we don't have that yet. If we had something like current_beat_index_in_measure CO, it would work perfectly.
If we suddenly change the beat distance value when the playback direction changes, it wouldn't work and it would be impossible to fix it at all, even if we had - measure detection COs.
There was a problem hiding this comment.
This should be fixable by changing the getBeatDistance code in syncable.cpp. Much like the half/double calculation, if we do all the conversion in the syncable code, the rest of the program can pretend that all the decks are moving forwards and it should work fine
src/engine/controls/bpmcontrol.cpp
Outdated
| @@ -470,6 +470,11 @@ double BpmControl::calcSyncAdjustment(bool userTweakingSync) { | |||
| double syncTargetBeatDistance = m_dSyncTargetBeatDistance.getValue(); | |||
| // We want the untweaked beat distance, so we have to add the offset here. | |||
| double thisBeatDistance = m_pThisBeatDistance->get() + m_dUserOffset.getValue(); | |||
| // If we are moving backwards, we have to invert the calculations. | |||
| if (reversed) { | |||
| syncTargetBeatDistance = 1.0 - syncTargetBeatDistance; | |||
There was a problem hiding this comment.
This needs to be applied if the sync target plays reverse, not the follower.
src/engine/controls/bpmcontrol.cpp
Outdated
| @@ -470,6 +470,11 @@ double BpmControl::calcSyncAdjustment(bool userTweakingSync) { | |||
| double syncTargetBeatDistance = m_dSyncTargetBeatDistance.getValue(); | |||
| // We want the untweaked beat distance, so we have to add the offset here. | |||
| double thisBeatDistance = m_pThisBeatDistance->get() + m_dUserOffset.getValue(); | |||
There was a problem hiding this comment.
Below (line 496) we aim to undo the added offset. This will move in the opposite direction if we are in reverse mode.
For my understanding you have discovered a bug, that should be fixed. Even though it is a rare situation, the sync should be rock solid. |
|
I agree. But this is not the PR to do it. I'm going to remove the backwards fix, because it's incomplete. So this PR will only be a slight rename / refactor |
|
I'll file a bug |
|
Thank you for adopting the bug. Will you add a solution here or in a separate PR. In the later case, I suggest to rebase this PR without the first commit. |
4e0006f to
c8a2300
Compare
|
I'll fix it in a later PR. I had to squash all the commits to do it right |
There was a problem hiding this comment.
Does this PR contain any functional changes?
- Replace "Bpm" with "BaseBpm" in some method/variable names
- Add
isFollowerhelper method - Rename snake_case variables with camelCase
- Add some trace logging
- Add a few comments
If I didn't miss anything, I think this is good to merge, despite the misleading PR title.
This refactoring sets up a major major patching coming in for sync lock! Get hyped.
This is mostly a noop PR