fix beatsync_phase depending on quantize #1679

daschuer · 2018-05-22T00:41:35Z

This PR fixes
https://bugs.launchpad.net/mixxx/+bug/1772525
and
https://bugs.launchpad.net/mixxx/+bug/1772526

right click on sync seeking in phase.

daschuer · 2018-05-24T20:59:50Z

This should also go to 2.1.1.
Who likes to review?

daschuer · 2018-05-26T19:15:00Z

I really like to have this included in 2.1.1 who can have a look?

Be-ing · 2018-05-27T21:40:07Z

This seems okay from brief testing, but I am wary to introduce such a big UX change in a point release. Are you sure this won't be unwanted in some use cases?

daschuer · 2018-05-27T21:56:40Z

Thank you for review.
I am pretty sure, because currently the right click in Shade and Deere does the same as the left click.
After this change, you sync also the phase with the right click, so it is an additional, recently requested feature, for the else redundant right click.
If you have a different opinion here, I can revert the skin changes and propose them for 2.2.
What do you think?

Be-ing · 2018-05-29T04:57:36Z

This changes the behavior of sync_enabled as well as beatsync. (On my controller I do not have beatsync mapped, only sync_enabled.)

rryan · 2018-05-29T15:38:00Z

No tests?

daschuer · 2018-05-29T23:06:37Z

MM, that's odd, reading this code

mixxx/src/engine/sync/enginesync.cpp

Line 169 in d8fe4dd

// Users also expect phase to be aligned when they press the sync button.

It was original implemented that sync_enable also syncs phase.
But I think that was wrong and we have evidentially fixed the "issue"?

How continue here?

In 2.1 right and left click do not sync phase
Original left click syncs phase, right click not
Ieft click not syncing phase works natural for me. I do not want to have a shift if with quantize off.
beatsync should really sync phase and BPM as documented.

I propose to keep the "buggy" behavior of sync_enable, but keep all other changes of this branch targeting it to 2.1.

Any other ideas?

ywwg · 2018-05-30T02:35:21Z

No tests?

agree, the sync code is very brittle and has benefited tremendously from the exhaustive unit tests to keep it from regressing. It sounds like there should at least be a test that confirms that phase is not synced when sync is pressed.

ywwg · 2018-05-30T02:36:44Z

But I think that was wrong

I believe on other programs, pushing sync would sync both bpm and phase, especially when both decks were playing. I replicated that behavior when writing this code. It's worth checking to see what happens when the decks are not synced, and are both playing, and then you click sync.

daschuer · 2018-05-30T05:57:59Z

The linked text was introduced here: #133
I have touched and probably brake something here: #738
So the current 2.1.0 behaviour was like this since 2.0 without much complains.

From the comments in #133 it looks like coupling the phase sync with quantize is desired.

For me this makes fully sense that this is coupled to quantize. This way the user can enable quantize when there is a good beatgrid and can disable it otherwise. If you enable quantize during sync enabled, the track moves slowly in phase. This matches the initial behaviour as well.

So IMHO the only thing to fix (change) here, is "beatsync" and "beatsync_phase"
It should match the docs on the wiki page.

Do you agree?

And yes, that should be verified by a test.

daschuer · 2018-05-30T21:17:40Z

OK, now the sync_enabled behaviour is unchanged, beatsync_phase works independently from quantize and a test for both is in place. Is this OK now?

ywwg

my only real question is about the use of EXPECT_LT in the tests, otherwise this looks fine. thanks!

ywwg · 2018-05-30T22:59:51Z

src/test/enginesynctest.cpp

@@ -1429,3 +1429,52 @@ TEST_F(EngineSyncTest, ZeroBpmNaturalRate) {
    EXPECT_EQ(0.0,
              ControlObject::getControl(ConfigKey(m_sGroup1, "local_bpm"))->get());
 }
+
+TEST_F(EngineSyncTest, SyncPhaseToIfQuantize) {


how about "QuantizeImpliesSyncPhase" (in your wording, it should be "too")

ywwg · 2018-05-30T23:01:04Z

src/test/enginesynctest.cpp

+    pButtonSyncEnabled1->set(1.0);
+    ProcessBuffer();
+    // 0.02 where set after testing both cases.
+    EXPECT_LT(0.02, ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get());


expect less-than? Should this be expect_float_eq or similar instead?

ywwg · 2018-05-30T23:01:41Z

res/skins/Shade/style.qss

+    selection-color: #0f0f0f;
+    background-color: #0f0f0f;
+    selection-background-color: #888;
+    /* This combination of negative top/bottom padding & margin


I think your branch is behind a bit, causing these library CSS diffs

Be-ing · 2018-05-31T02:18:43Z

So IMHO the only thing to fix (change) here, is "beatsync" and "beatsync_phase"

This would still change the behavior of the SyncButton in Components.

daschuer · 2018-05-31T06:24:12Z

Should bratsync also respect quatize? If we do so, we have the same as sync_enable. So it would be redundant. I see a demand for an bratsync button, that works like in other Apps and unconditionally syncs tempo and phase.

I do not understand why we need a hack in Components at all. You should be able to just map sync_enable, to get the desired behaviour.

ywwg · 2018-05-31T13:09:30Z

My recollection is that "beatsync" was kept around for backwards-compatibility purposes. A short-press of sync_enable is indeed identical.

Be-ing · 2018-05-31T13:18:15Z

A short-press of sync_enable is indeed identical.

Not when set through a controller script.

daschuer · 2018-05-31T14:49:53Z

@Be-ing can you explain why we need a hack in Components? Why can't you use plain sync_enable, to get the desired behaviour?

Do I understand it right, you wish that we keep bratsync quantize dependent?
If yes, we might need a unconditional alternative in 2.2 as requested, to make Mixxx behave like other tools. I am happy without it though. "Beatsync_tempo_phase" ?

Be-ing · 2018-05-31T14:57:14Z

@Be-ing can you explain why we need a hack in Components?

The push-and-hold behavior for sync_enabled was implemented as a hack that only affects XML mappings and skins, not controller scripts.

Be-ing · 2018-05-31T15:01:46Z

The push-and-hold behavior for sync_enabled was implemented as a hack that only affects XML mappings and skins, not controller scripts.

IMO this should not be changed because there should be a way to immediately activate master sync without having to push and hold a button.

ywwg · 2018-05-31T15:30:43Z

what's wrong with push and hold?

Be-ing · 2018-05-31T15:32:18Z

It's easy to forget to hold, which can easily mess up a mix.

ywwg · 2018-05-31T15:38:20Z

has this happened or are you speculating? on my controller sync is a lighted button, so when I push and hold it it stays lit. It's easy to tell when master sync is on or off based on the lighted state of the button, so there's nothing to forget.

Be-ing · 2018-05-31T15:45:57Z

Yes that happened to me when I had my controller mapped with push-and-hold, so I didn't keep it mapped that way very long and switched back to a simple toggle button. Seeing the master sync state on the controller doesn't solve the problem of having to hold down a button and manipulate other controls for a mix within a short span of time.

ywwg · 2018-05-31T16:02:32Z

I'm a little unclear of the use-case you describe. When I use master sync I turn it on at the beginning of my set and it stays on the whole time. Or if I'm turning it on it's during the previewing stage where I'm not in a rush. What were you doing that you needed to turn it on quickly while you were doing other things?

Be-ing · 2018-05-31T16:04:12Z

I only use sync occasionally, so I have it off most of the time.

ywwg · 2018-05-31T16:07:12Z

is push-and-hold an operation on the buttons, or on the control object? Maybe it should be up to the controller configuration to decide how that works.

daschuer · 2018-05-31T19:41:54Z

It is an option of the ControlObject. I see, you cannot give a button release command from Controller scripts.

Ok so i will restore the beatsync behavior.

…1772526

…ustment via quantize is not deired.

daschuer · 2018-06-01T13:02:59Z

Ups, I have messed up my 2.1 branch. Fixed now.

daschuer · 2018-06-03T22:51:25Z

@Be-ing:ready to merge? I would like to release 2.1.1 including this.

Be-ing · 2018-06-04T11:25:09Z

I'm confused what this new sync_phase Control is intended to do. Now right clicking the sync button makes the track seek so the play position jumps to the beat grid of the track that sync_phase was pressed on. Isn't it meant to align the beatgrid of the track sync_phase is pressed on to the other track's beatgrid?

daschuer · 2018-06-04T11:57:01Z

This PR does not introduce any new control.
I think you mean the existing control beatsync_phase.
Before this PR it does nothing in case of quantize of. Now it always independent from quantize and seeks the track on which the button is pressed. No idea why it is the other track in your case.

daschuer · 2018-06-05T23:18:34Z

I have double checked it and only the track where I press sync does the seek.
Do you have a recipe to reproduce it?

daschuer · 2018-06-07T17:50:11Z

@Be-ing: merge?

Be-ing · 2018-06-08T12:12:31Z

I'm confused what this new sync_phase Control is intended to do. Now right clicking the sync button makes the track seek so the play position jumps to the beat grid of the track that sync_phase was pressed on. Isn't it meant to align the beatgrid of the track sync_phase is pressed on to the other track's beatgrid?

I think I got confused before because I tested tracks with very different tempos. sync_phase works as described.

Be-ing

I am a little confused now what this PR changes at this point. IIUC the only behavior change is that beatsync only syncs phase if quantize is enabled now?

Be-ing · 2018-06-08T12:08:55Z

src/engine/sync/synccontrol.cpp

+    if (isPlaying() && m_pQuantize->toBool()) {
+        // only sync phase if the deck is playing and if quantize is enabled.
+        // this way the it is up to the user to decide if a seek is desired or not.
+        // This is helpfull if the beatgrid of the track doe not fit at the current


Be-ing · 2018-06-08T12:09:03Z

src/engine/sync/synccontrol.cpp

+        // only sync phase if the deck is playing and if quantize is enabled.
+        // this way the it is up to the user to decide if a seek is desired or not.
+        // This is helpfull if the beatgrid of the track doe not fit at the current
+        // payposition


playposition

Be-ing · 2018-06-08T12:09:15Z

src/test/enginesynctest.cpp

@@ -1429,3 +1429,52 @@ TEST_F(EngineSyncTest, ZeroBpmNaturalRate) {
    EXPECT_EQ(0.0,
              ControlObject::getControl(ConfigKey(m_sGroup1, "local_bpm"))->get());
 }
+
+TEST_F(EngineSyncTest, SyncPhaseToIfQuantize) {


Be-ing · 2018-06-08T12:14:17Z

src/test/enginesynctest.cpp

+}
+
+
+


extra newlines at end of file

Be-ing · 2018-06-08T12:16:22Z

src/engine/enginebuffer.cpp

@@ -1179,7 +1179,7 @@ void EngineBuffer::processSeek(bool paused) {
            return;
    }

-    if ((seekType & SEEK_PHASE) && !paused && m_pQuantize->toBool()) {
+    if (!paused && ((seekType & SEEK_PHASE) || m_pQuantize->toBool())) {


I am not familiar with this area of the code. Can you clarify what this change does?

This allows to sync phase independent from quantize now. The sync signal of the controls that must not sync phase if quatize is of, is done in their buissnes logic.

Okay thank you for explaining.

daschuer · 2018-06-08T14:00:37Z

I am a little confused now what this PR changes at this point. IIUC the only behavior change is that beatsync only syncs phase if quantize is enabled now?

The changes are that beatsync_phase syncs always phase even if quantize is disabled and we put that to the right klick instead of other redundant controls.
The other controls have not chsnges, and the description in the Mixxx Wikki have been corrected.

daschuer · 2018-06-08T16:05:06Z

Done!

Be-ing · 2018-06-09T00:46:29Z

I'm still confused. I just tested master and beatsync_phase works regardless of whether quantize is enabled and beatsync only syncs phase if quantize is enabled. So what does this change other than the skins?

daschuer · 2018-06-10T12:37:39Z

beatsync_phase did not work independently from quantize before.

Be-ing · 2018-06-10T15:47:34Z

I just tested master and beatsync_phase works regardless of whether quantize is enabled

I checked again and couldn't reproduce that. So this PR LGTM.

daschuer · 2018-06-10T16:43:00Z

Thank you.

daschuer changed the base branch from 1.12 to 2.1 May 22, 2018 00:42

daschuer added this to the 2.1.1 milestone May 24, 2018

daschuer force-pushed the lp1772525 branch from f762d52 to 496477f Compare May 30, 2018 21:13

ywwg reviewed May 30, 2018

View reviewed changes

daschuer added 7 commits June 1, 2018 13:06

fix beatsync control syncing phase, an old regression since 2.0

0783a13

Added beatsync to right click on sync in Shade and Deere, fixes bug #…

c65d450

…1772526

Restore quantize dependency of sync_enable lost in a previous commit

ef28e68

Added the test SyncPhaseToIfQuantize

6ab15f8

Restore quantize dependency of beatsync lost in a previous commit

4853ddc

extend test for beatsync

8d82ec8

Change right click sync to beatsync_phase, for the case permanent adj…

f2437fc

…ustment via quantize is not deired.

daschuer force-pushed the lp1772525 branch from fd1e60b to f2437fc Compare June 1, 2018 13:01

Be-ing suggested changes Jun 8, 2018

View reviewed changes

fix typos

55715f1

Be-ing changed the title ~~beatsync fix~~ fix beatsync_phase depending on quantize Jun 10, 2018

Be-ing merged commit 869158f into mixxxdj:2.1 Jun 10, 2018

Be-ing mentioned this pull request Aug 1, 2018

Allow maximum deck speed of 4x normal #1662

Merged

daschuer deleted the lp1772525 branch September 7, 2021 21:07

This was referenced Aug 23, 2022

beatsync control does not sync phase #9302

Closed

Right click sync in Shade does not sync phase #9303

Closed

Hotcue quantization on, regardless of deck/sync #9345

Closed

fix beatsync_phase depending on quantize #1679

fix beatsync_phase depending on quantize #1679

Conversation

daschuer commented May 22, 2018

daschuer commented May 24, 2018

daschuer commented May 26, 2018

Be-ing commented May 27, 2018

daschuer commented May 27, 2018

Be-ing commented May 29, 2018 • edited Loading

rryan commented May 29, 2018

daschuer commented May 29, 2018 • edited Loading

ywwg commented May 30, 2018

ywwg commented May 30, 2018

daschuer commented May 30, 2018

daschuer commented May 30, 2018

ywwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented May 31, 2018

daschuer commented May 31, 2018 • edited Loading

ywwg commented May 31, 2018

Be-ing commented May 31, 2018

daschuer commented May 31, 2018

Be-ing commented May 31, 2018

Be-ing commented May 31, 2018

ywwg commented May 31, 2018

Be-ing commented May 31, 2018

ywwg commented May 31, 2018

Be-ing commented May 31, 2018

ywwg commented May 31, 2018

Be-ing commented May 31, 2018

ywwg commented May 31, 2018

daschuer commented May 31, 2018 • edited Loading

daschuer commented Jun 1, 2018

daschuer commented Jun 3, 2018

Be-ing commented Jun 4, 2018

daschuer commented Jun 4, 2018

daschuer commented Jun 5, 2018

daschuer commented Jun 7, 2018

Be-ing commented Jun 8, 2018

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 8, 2018

daschuer commented Jun 8, 2018

Be-ing commented Jun 9, 2018

daschuer commented Jun 10, 2018

Be-ing commented Jun 10, 2018

daschuer commented Jun 10, 2018

Be-ing commented May 29, 2018 •

edited

Loading

daschuer commented May 29, 2018 •

edited

Loading

daschuer commented May 31, 2018 •

edited

Loading

daschuer commented May 31, 2018 •

edited

Loading