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

BPM mutliplier should be using the current deck BPM rather than the file BPM #12738

Closed
acolombier opened this issue Feb 5, 2024 · 11 comments · Fixed by #13671
Closed

BPM mutliplier should be using the current deck BPM rather than the file BPM #12738

acolombier opened this issue Feb 5, 2024 · 11 comments · Fixed by #13671

Comments

@acolombier
Copy link
Member

Bug Description

How to reproduce

Here is the usecase:

  • Deck A has a track with a BPM of 80

  • Deck B has a track with a BPM of 128

  • Adjust the BPM of deck A and B to 110 (using tempo fader)

  • Enable sync using sync_enable or beatsync_tempo

Expected

The BPM multiplier should be 1, and both track should keep the exact same BPM value (110 in the example)

Actual

The non-sync master (of the channel calling beatsync_tempo) BPM of the non deck gets set to 55 (if it's A) or 220 (if it's B)

Fix options

  1. In synccontrol.cpp, SyncControl::slotControlBeatSyncTempo and SyncControl::reinitLeaderParams could use getBpm() instead of fileBpm() if valid
  2. Add new CO to ignore the multiplier, e.g beatsync_tempo_absolute and absolute_sync_enable

While 1. feels like a better fix, I guess it may also be a breaking change in some cases so I thought I will mention 2. although it definitely feels sub-optimal

Version

2.4.0

OS

All

@acolombier acolombier added the bug label Feb 5, 2024
@daschuer
Copy link
Member

daschuer commented Feb 5, 2024

This is a feature. We consider the smallest bpm change as the optimum to avoid chipmunk voices.
55/80 = -30 % 110/80 = +38 %

@acolombier
Copy link
Member Author

acolombier commented Feb 5, 2024

Yes, that makes sense in the case you simply want to match track's BPM effortlessly, however if you have already adjusted the BPM manually previously and want them to remain in-sync going forward (if you change the BPM of one), wouldn't it make sense for it to consider the adjusted BPM instead, rather then make a big jump from 110 to 55 as soon as you enable the sync? If not as default, would you consider as a configurable behaviour?

@daschuer
Copy link
Member

daschuer commented Feb 5, 2024

I can imagine to add an extra rule that uses a snap region for this case. A preferences option will be to strict probably.
What are reasonable borders for this new behaviour in corner cases?

@acolombier
Copy link
Member Author

Somewhere around 5% should be more than enough to address my usecase.
I am wondering if it wouldn't be more natural for the user to use the current BPM of the deck in case it has been adjusted manually (e.g via rate, not using any of the (beat)sync_* COs)

@ronso0
Copy link
Member

ronso0 commented Feb 6, 2024

How would that work in the unattended AutoDJ case?
Would it be possible with a certain track set that this lowers (raises) the rate slider until we hit the boundary?

@acolombier
Copy link
Member Author

I'm not sure I understand the usecase, does AutoDJ uses the rate CO directly? If yes, I guess the implementation could have some extra logic to detect rate adjusting orchestrated by AutoDJ and consider it automatic as well?

@daschuer
Copy link
Member

daschuer commented Feb 7, 2024

If we look to the transition of these tracks in Auto-DJ:
A 110 : Rate 0 % = 110
B 80 : Rate -31 % = 55
A 120 : Rate - 8 % = 110
B 180 (played at -31 % = 124): Rate -38 % = 110

For the last track with 180 220 BPM would be better Rate + 22%

You see that applying the current BPM will not work well in case of Auto DJ.

But it seems we have actual an an issue in the initial case:
80 : Rate +38 % = 110
120 : Rate +83 % = 220

+83 % rate slider is definitely too high. I will have a look.

@acolombier
Copy link
Member Author

I feel like you might have misunderstood the bug I'm reporting. The other day, I was playing those two tracks together, and I wanted to increase the global BPM back to 128. As both track were already manually synch'ed (to 110) using the tempo fader, I made one of the deck sync leader, and as soon as I enabled sync on the other deck, the rate divide by two, before I even had time to adjust increase the plating rate of leader as I was hoping to do in my routine, making the mix very weird as that vocal (the following deck was a vocal) suddenly was playing twice as slow.

I think the current multiplier logic makes complete sense as it is currently, but hopefully you will agree that the behavior I am describing is a bug.

Currently, this is how I have fixed in my dev branch, and it still to work still fine with the normal sync behavior you are describing. It is defintely flawed and I would assume it needs more work.

diff --git a/src/engine/sync/synccontrol.cpp b/src/engine/sync/synccontrol.cpp
index 66655a3465..6bb2496255 100644
--- a/src/engine/sync/synccontrol.cpp
+++ b/src/engine/sync/synccontrol.cpp
@@ -279,7 +279,11 @@ void SyncControl::reinitLeaderParams(
         kLogger.trace() << "SyncControl::reinitLeaderParams" << getGroup()
                         << beatDistance << baseBpm << bpm;
     }
-    m_leaderBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm);
+    auto currentBpm = getBpm();
+    if (!currentBpm.isValid()) {
+        bpm = fileBpm();
+    }
+    m_leaderBpmAdjustFactor = determineBpmMultiplier(currentBpm, baseBpm);
     updateLeaderBpm(bpm);
     updateLeaderBeatDistance(beatDistance);
 }
@@ -434,7 +438,11 @@ void SyncControl::slotControlBeatSyncTempo(double value) {
         return;
     }
 
-    double multiplier = determineBpmMultiplier(fileBpm(), target->getBaseBpm());
+    auto bpm = getBpm();
+    if (!bpm.isValid()) {
+        bpm = fileBpm();
+    }
+    double multiplier = determineBpmMultiplier(bpm, target->getBaseBpm());
     m_pRateRatio->set(target->getBpm() * multiplier / localBpm);
 }

@daschuer
Copy link
Member

daschuer commented Feb 7, 2024

+83 % rate slider is definitely too high. I will have a look

It turns out that it is correct. When I sync the 120 bpm track to 80 it is shifted to 160, which is correct. When I now increase the tempo to from 80 110, the 120 bpm track is consequently playing at 220 bpm. This is really an extreme edge case.

@daschuer
Copy link
Member

daschuer commented Feb 7, 2024

It looks like a simple patch works for you.

--- a/src/engine/sync/synccontrol.cpp
+++ b/src/engine/sync/synccontrol.cpp
@@ -279,7 +279,7 @@ void SyncControl::reinitLeaderParams(
         kLogger.trace() << "SyncControl::reinitLeaderParams" << getGroup()
                         << beatDistance << baseBpm << bpm;
     }
-    m_leaderBpmAdjustFactor = determineBpmMultiplier(fileBpm(), baseBpm);
+    m_leaderBpmAdjustFactor = determineBpmMultiplier(fileBpm(), bpm);
     updateLeaderBpm(bpm);
     updateLeaderBeatDistance(beatDistance);
 }

It fixes my surprising rate of +83 % at least.

@acolombier
Copy link
Member Author

Sorry @daschuer I missed the notification.
I remember your fix was also where I started, but I remembered only this was breaking other usecase (can't remember what sadly)
I'll try an work on comprehensive patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants