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

Expose [BPM],sync_lock_algorithm option in deck preferences. #11828

Merged
merged 10 commits into from Nov 16, 2023

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Aug 14, 2023

Unfortunately this is still required to have a reliable transition with non const beat-grids like in 2.3.

This has been a hidden preferences option introduced here: #3698 as a workaround until unstable / dynamic outro tempos have been fixed, which never happens.
Details have been discussed here: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/sync.20lock.20with.20variable.20tempo.20tracks

With the later improvements we have already introduced and #11831 applied this works now very good for me. You can pick "Follow implicit leader" if you have only a few tracks with non const beat grids, or "Use steady Tempo" if all are non const. This way are safe from messed up transitions. Both modes allow to change that behaviour on the fly by making the on of the tracks or the internal clock the explicit leader.

grafik

The tooltips are:

Follow implicit leader's tempo:
Apply tempo changes from a soft leading track (normally the previous track in a transition) to the follower tracks. After the transition, the follower track will continue with the previous leader's very last tempo. Changes from explicit selected leaders are always applied.

Use steady Tempo:
The tempo of a previous soft leader track at the beginning of the transition is kept steady. After the transition, the follower track will maintain this original tempo. This technique serves as a workaround to avoid dynamic tempo changes, as seen in rubato-style tracks, during the outro. For instance it prevents the follower track from continuing with a slowed-down tempo of the soft leader. This corresponds to the behavior before Mixxx 2.4. Changes from explicit selected leaders are always applied.

Unfortunately this is still required to have a reliable transition with non const beat-grids like in 2.3.
@github-actions github-actions bot added the ui label Aug 14, 2023
@daschuer daschuer changed the base branch from main to 2.3 August 14, 2023 01:01
@daschuer daschuer changed the base branch from 2.3 to 2.4 August 14, 2023 01:01
@daschuer daschuer added this to the 2.4.0 milestone Aug 14, 2023
@ronso0 ronso0 requested a review from ywwg August 14, 2023 12:58
// 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,
Copy link
Member

Choose a reason for hiding this comment

The 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
PREFER_SOFT_LEADER,
PREFER_LEADER,

Copy link
Member Author

@daschuer daschuer Aug 15, 2023

Choose a reason for hiding this comment

The 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.
"Shall the follower follow a dynamic tempo or should both follow a steady tempo?"

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Does it make sense to decouple it?

@daschuer
Copy link
Member Author

Initial description and screenshot updated.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some twekas. Still not a native speaker, but it feels a bit better.

src/preferences/dialog/dlgprefdeckdlg.ui Outdated Show resolved Hide resolved
src/preferences/dialog/dlgprefdeckdlg.ui Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Aug 21, 2023

Sure it's okay to have explanatory tooltips for now, but all that info deserves its own chapter in the manual. We could link to that like we do for the Cue modes.
image

A few more bits should be clarified IMHO, for example how does Mixxx know when a transition starts? Crossfader, channel fader, effective ratio in the main mix, or some combination?

Co-authored-by: ronso0 <ronso0@mixxx.org>
@daschuer daschuer removed the request for review from ywwg September 9, 2023 21:10
src/engine/sync/enginesync.h Outdated Show resolved Hide resolved
Comment on lines 467 to 470
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));
Copy link
Member

Choose a reason for hiding this comment

The 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 😅
Please update this and the snippet below when merging to main.

@daschuer
Copy link
Member Author

The manual can be found here: mixxxdj/manual#589

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a proposal for the tooltip.

src/skin/legacy/tooltips.cpp Outdated Show resolved Hide resolved
Co-authored-by: ronso0 <ronso0@mixxx.org>
@ronso0
Copy link
Member

ronso0 commented Oct 16, 2023

@Holzhaus @Swiftb0y do you have any remarks?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous comments still apply as well, lgtm otherwise.

src/skin/legacy/tooltips.cpp Show resolved Hide resolved
@daschuer
Copy link
Member Author

This is ready for merge.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

src/defs_urls.h Outdated
@@ -53,6 +53,8 @@
MIXXX_MANUAL_URL "/chapters/preferences.html#library"
#define MIXXX_MANUAL_CUE_MODES_URL \
MIXXX_MANUAL_URL "/chapters/user_interface.html#using-cue-modes"
#define MIXXX_MANUAL_SYNC_MODES_URL \
MIXXX_MANUAL_URL "/chapters/user_interface.html#sync-and-rate-controls"
Copy link
Member

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)

@ronso0
Copy link
Member

ronso0 commented Nov 16, 2023

Thank you!

@ronso0 ronso0 merged commit a89f676 into mixxxdj:2.4 Nov 16, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants