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

Sync Lock Explicit Leader #3698

Closed
wants to merge 43 commits into from
Closed

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Mar 12, 2021

Enable sync lock explicit leader selection. This change builds on #3695 and will be updated with changes that happen there.

This is a major change to the Sync Lock code that includes a bunch of cleanup and rationalization. Hopefully the logic is a little easier to follow now. This change adds a "Leader" button to the UI. This button is lit to indicate which deck is the Leader.

testing note! You must use Late Nite Classic to see the new Leader button. This is very temp UI for testing. Orange means that Mixxx picked that deck automatically. That setting will bounce around as you load new tracks. Clicking the button turns it red if you want an explicit master.

Things that need testing by hand:

  • situations with beatgrid changes. Loading new tracks, reanalyzing tracks, etc. https://bugs.launchpad.net/mixxx/+bug/1808698 esp
  • test out explicit leader! This means that the designated deck will have its bpm preferred above others. This is really only noticeable when there are beatmap tracks.
  • using one deck to scratch another is gone. It was too hard to make work with all the edge cases.
  • Try letting an explicit leader hit the end of a track. Is that the expected behavior for you?
  • Just really beat it up.

@daschuer
Copy link
Member

using one deck to scratch another is gone. It was too hard to make work with all the edge cases.

For my understanding this was the main use case for the explicit leader mode.
An other useful thing is to have a fixed internal/external clock. Will this be also part of this PR?

What is the main use case for making a Deck explicit leader? I cannot really think of one right now.

@ywwg
Copy link
Member Author

ywwg commented Mar 13, 2021

If I'm playing two beatmapped tracks, right now Mixxx doesn't know which one should drive the bpm, so currently it just locks the rate to the most recent value. So the main use-case for this change is allowing the user to determine which track is "in charge" of the bpm when there are beatmapped tracks involved. Mixxx can either pick one or the other to drive the bpm, or the user can. (If we add UI for the Internal Clock, we could also lock the bpm to a constant value.)

I can work on restoring make-decks-scratch functionality in a future PR, but I need people to help me think through the edge cases (what happens when the master deck stops? Everything stops?) etc. This change does not make that work impossible.

@daschuer
Copy link
Member

That sounds reasonable, however you have described the solution, not the use case.

I am a bit concerned that we get a good balance between feature and clutter in the GUI. We also need to have use cases in place where we can adjust the edge cases to.

To be honest I can't think of a real use case for playing two tracks in parallel at center Crossfader all the time.

What I can think of to have a short section or loop in a track, that I want to add to the main track, without hearing jouling or something due to temporarily using a fixed clock. This is currently an issue that we should fix.

What else do we want?

@ywwg
Copy link
Member Author

ywwg commented Mar 13, 2021

Playing multiple tracks at the same time is necessary for mixing one track into another, usually over the course of 32 or 64 (or more) bars. With the current code, the sync lock picks a single bpm and locks both tracks to that. This means the beatmap track is pushed and pulled around to remain at a constant bpm, which is not always desireable. This change allows the user to either pick the variable bpm, or a constant tempo.

This feature exists in Traktor and pioneer, and many controllers have a "master" button. I thought we had already decided that explicit leader was a feature we wanted. (#2376)

New behavior aside, the code is much cleaner now. There was a lot of hacky logic, and a lot of error-prone signal circles. The new code is easier to follow now: enginesync.cpp contains all the logic for deciding which decks should be master, and synccontrol sends requests and notifications of state changes, and receives synclock bpm updates.

@ywwg
Copy link
Member Author

ywwg commented Mar 13, 2021

Even if we didn't include the new ui elements that I thought we had approved, this code is also still better because Mixxx automatically chooses a sync leader. Therefore if I'm playing a beatmapped song and bring something else in, Mixxx will keep respecting the variable bpm instead of suddenly locking to the current rate. The user doesn't have to use the explicit leader button if they don't want to. But if they do want to be specific about which bpm they want, they can push the button.

@Be-ing
Copy link
Contributor

Be-ing commented Mar 13, 2021

if I'm playing a beatmapped song and bring something else in, Mixxx will keep respecting the variable bpm instead of suddenly locking to the current rate.

💯
The current behavior doesn't make a lot of sense and is quite awkward and unexpected.

@ronso0
Copy link
Member

ronso0 commented Mar 14, 2021

I am a bit concerned that we get a good balance between feature and clutter in the GUI

As with all advanced controls we can have an optional Sync controls section, like the beatgrid controls or vinyl control, especially for Zulip: halve/double sync when the base/leader BPM changes significantly

@daschuer
Copy link
Member

I thought we had already decided that explicit leader was a feature we wanted. (#2376)

Sorry I gave the impression I am in doubt with this. This feature is very welcome. And Thank you for picking it up.

We need "just" to consider all use cases, to consider the edge cases correctly. This is also very helpful when considering the new analyzer features.

This means the beatmap track is pushed and pulled around to remain at a constant bpm, which is not always desirable.

Does this describe the jitter issue we have with the non const beat map? #3626 aims to fix this.

In addition we have two types of tracks. Electronic track with intentional change and hand made tracks from the 80s or such. How does you used case works with them?

@Holzhaus
Copy link
Member

Does this describe the jitter issue we have with the non const beat map? #3626 aims to fix this.

No. There are non-const tracks that are intentionally non-const (either accelerando/ritardando or sudden tempo changes on a drop). Right now, it's not possible to use sync with such tracks properly because the tempo changes are ignored after enabling sync.

@daschuer
Copy link
Member

There are non-const tracks that are intentionally non-const (either accelerando/ritardando or sudden tempo changes on a drop).

Yes, that are the interesting tracks here. How does your use case work with them?

Right now, it's not possible to use sync with such tracks properly because the tempo changes are ignored after enabling sync.

It is ignored "only" if a second track is playing. This works OK because we can assume that a const tempo is the most common case and this feature avoids that the const track is yowling during a transition from one to another track.
The result is a proper synced transition.

When does this fail?

@Holzhaus
Copy link
Member

For example: two electronic tracks, track 1 with a sudden tempo change.

If I want to play both in sync and replace the high hats from track 1 with the ones from track 2, I currently can't do that.

@daschuer
Copy link
Member

Is that really a use case to play both side by side? I don't think that's a case.
I would normally loop the the track with the high heads. In this case the crowd dances to a track and I can decorate it with loops from other tracks.

Can you confirm it?

@Holzhaus
Copy link
Member

I don't think it matters if you loop the hihat track or not. In any case the tempo change from the other track won't be applied while synced.

@Holzhaus
Copy link
Member

By the way, I think we need this feature anyway for setting a midi clock input as explicit leader, so that track follow tempo changes from an external drum machine.

@daschuer
Copy link
Member

daschuer commented Mar 14, 2021

I don't think it matters if you loop the hihat track or not.

I am not sure if it matters for the code. At least it matters to understand the use case.

I think we can sort out the case to play two non const tracks in sync.

@ywwg
Copy link
Member Author

ywwg commented Mar 14, 2021

Is that really a use case to play both side by side? I don't think that's a case.

absolutely. If I'm mixing two disco tracks together, they will both have extremely variable bpms. Furthermore, being able to mix two tracks together during a tempo change sounds really amazing. Also note, the tracks don't both have to be playing out to master mix -- even if one track is gained down to zero and only audible in pfl, currently the sync will lock at an arbitrary bpm. So this fixes that case as well.

I spun a whole set with this last night and it was great!

@daschuer
Copy link
Member

If I'm mixing two disco tracks together,

For my understanding this is the case where the const clock is the best solution. I am quite happy with this, because the youling of both tracks is limited during a transition from on to the other.

...even if one track is gained down to zero and only audible in pfl, currently the sync will lock at an arbitrary bpm. So this fixes that case as well.

I think this can be considered as bug. I can't think of a case where a playing disco track should be locked to const while cuing a second track in.

Furthermore, being able to mix two tracks together during a tempo change sounds really amazing.

Interesting. Can you share the tracks you have used for this?

@ywwg
Copy link
Member Author

ywwg commented Mar 14, 2021

I think this can be considered as bug.

that's part of my point, this change also fixes that bug.

const clock is the best solution

but this is weird because a track that had been variable is suddenly locked and it might cause audible artifacts as it changes speed to lock into place. If you want a constant clock you can set the Internal Clock as the Explicit Leader. This is exactly how Traktor does it btw :).

@Be-ing
Copy link
Contributor

Be-ing commented Mar 14, 2021

IMO locking a variable track to a constant tempo is only appropriate if it is being synced to a constant tempo track which is the sync leader.

@daschuer
Copy link
Member

const clock is the best solution

but this is weird because a track that had been variable is suddenly locked and it might cause audible artifacts as it changes speed to lock into place.

I use non const beat maps since long and i did not experience artifacts. In almost all cases one of the tracks is const, so it irons only the beats of the non const track, which is a desired behavior.

If you want a constant clock you can set the Internal Clock as the Explicit Leader. This is exactly how Traktor does it btw :).

Do you propose to remove this implicit feature? I am still convinced that this is the best working solution for 99% of the cases, and a no brainier for the user.

@daschuer
Copy link
Member

IMO locking a variable track to a constant tempo is only appropriate if it is being synced to a constant tempo track which is the sync leader.

@Be-ing: Since you use const beat grids luckily this is always true for you.

If you have a transition of two disco tracks you cant really predict if there are slight tempo changes in the outro or intro. In addition that is even worse due to the jitter issue that will be fixed after merging #3626. Our detector also has sometimes issues to detect a stable BPM during intro or outros. I don't know my tracks that well that I know whether a track is affected. I don't know if I should select the in-going our out-going as leader. For this case the current locking strategy works perfectly.

@daschuer
Copy link
Member

@ywwg I am really curious to to reproduce your changing tempo transition. Can you share the used track titles?

@daschuer
Copy link
Member

@JoergAtGithub I am also interested to learn about your use cases. Do you use const beat grids or Beat maps? When do you miss the explicit master feature hard?

@Holzhaus
Copy link
Member

I don't know if I should select the in-going our out-going as leader. For this case the current locking strategy works perfectly.

Then don't set an explicit leader. In that case the internal clock will be leader and sync works as before.

Are you saying we shouldn't add explicit leader support? Because I otherwise I don't understand what we are discussing here.

@JoergAtGithub
Copy link
Member

I like the Traktor behavior, where it's always clear which deck is the leader and I wish to have DVS control to this leader.
Especially for three deck transitions, it's good to know who leads and who follows.
I also like to play the same track on two decks in different flavors. These decks must be always in sync to sound like one track.
A single reference clock is also needed for main effects like echo, look at all the Pioneer gear, which has only one effects unit with beat related controls.

@daschuer
Copy link
Member

daschuer commented Mar 14, 2021

Then don't set an explicit leader. In that case the internal clock will be leader and sync works as before.

Right. I am not arguing against the explicit leader feature.

I have only the impression that we have a mixture of bugs and missing features, that we need to sort out to be finally better than Traktor and well prepared for the new beat grid.

Are you saying we shouldn't add explicit leader support? Because I otherwise I don't understand what we are discussing here.

I think I need to start every post with "I am happy that we are about to reintroduce the explicit sync feature" which is the case, .. really.

I am currently quite happy with the current behavior for the general case. During the recent discussion I don't understand why it is considered as bad in general.

For me, these use cases are most important:

  1. Transition from one to another track using a const BPM even if both tracks have a non const beat map.
  2. Decorating a track with loops or samples form on other track. Here the decorated track shall be the master
  3. Scratching 2.
  4. Fixed tempo by a dedicated clock (intern or extern)

Are these the common use case for most other users as well?

@ywwg ywwg force-pushed the synclock-explicit-rebased branch from d324b1f to 985888e Compare May 14, 2021 15:28
@ywwg
Copy link
Member Author

ywwg commented May 14, 2021

rebased

@ywwg
Copy link
Member Author

ywwg commented May 14, 2021

fixed merge conflict I think

daschuer added a commit that referenced this pull request May 14, 2021
@daschuer
Copy link
Member

Now Jan's commits slipped in.

I have merged this without these commits in 6ed6a09

@daschuer
Copy link
Member

Thank you for bringing this controversial PR to a mergable state. This will make a big difference once the beat map editor is in place.

@daschuer daschuer closed this May 14, 2021
@Holzhaus
Copy link
Member

One thing I noticed: When the track ends, it's still explicit leader. Is that intended? I don't think that makes much sense, unless we want to restore the "scratch two decks at the same time".

@Holzhaus
Copy link
Member

I can even set explicit leader on a stopped deck, which changes the key on the playing deck.

@daschuer
Copy link
Member

All these questions are still floating IMHO, bound to the question how the user expect to use the single sync button and single sync LED on his controller and how the internal clock is expected to be controlled.

For a good decision, I think we need the whole picture how other tools get around these problems.

The idea was to set up a wiki page and describe how the sync button works in other tools with all available preferences option. This shows us the expectations user migrating to Mixxx might have.

Than we need to step back and understand the use cases covered and look if and how Mixxx schould cover them.

My hope is that we can allow a per transition decision in the GUI without any prefernces option.

For now, I like to allow these modes:

  • Smart mode where Mixxx decides
  • Allow to "help" Mixxx if it is not smart enough
  • Hard override Smart mode, including scratching.

I have no idea yet how to control it effective. That's why a market research might be helpful. At least that may help that new users don't need to learn completely new.

If you agree I can start such a wiki page.

@Holzhaus
Copy link
Member

All these questions are still floating IMHO, bound to the question how the user expect to use the single sync button and single sync LED on his controller and how the internal clock is expected to be controlled

I think the explicit leader behavior I reported above is a bug. I'm pretty certain that last time I tested this branch did reset the explicit leader state when the end of track is reached.

Once a track stopped it is not the explicit leader anymore (there is no beatgrid, so I assume the internal clock takes over?)
The only use case for setting/maintaining explicit leader on a stopped I can think of is that you can use it to scratch both decks at once, which is something that is deliberately not covered by this branch because it's an extreme edge case and I think @ywwg left that out because we should streamline the usual workflow first and iterate based on that.

@daschuer
Copy link
Member

Is this only a visual issue, or is a use case affected?

@Holzhaus
Copy link
Member

Both. It's a visual issue, but the explicit leader decks stays leader forever. The next track I load on that deck will also be leader.

And also this:

I can even set explicit leader on a stopped deck, which changes the key on the playing deck.

There is an audible change on the playing deck. No idea why, because the tempo does not change.

@ywwg
Copy link
Member Author

ywwg commented May 15, 2021

Tracks should lose explicit leader at track end. I'll fix

@ywwg
Copy link
Member Author

ywwg commented May 15, 2021

it's ok for an explicit leader to be stopped! That is definitely intended. It's still driving the bpm, but it doesn't need to be playing

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.

7 participants