Sync to the loaded deck with the lowest index if no track is playing #1818
Conversation
|
I do not think it is a good idea to put major UX changes in bugfix releases. IMO this change should go to master where it can get lots of testing before it is released. |
|
This is a real bug, try it out. It is somehow a regression from keeping persistent tracks in samplers. |
|
Who wants to test this? Only a few lines of code. |
|
@ywwg can you have a brief look here? |
|
This is also reported here https://bugs.launchpad.net/mixxx/+bug/1808698 |
|
@daschuer what do we need to look into? Verify that it works? Because the code changes are trivial ^^ |
|
Yes, does it work and is it sensible. |
src/engine/sync/enginesync.cpp
Outdated
| // is not, do not sync. | ||
| if (pSyncable->isPlaying() && !other_deck->isPlaying()) { | ||
| // If the requesting deck is playing, or we have already a | ||
| // non plaing deck found, only watch out for playing decks. |
src/test/enginesynctest.cpp
Outdated
| @@ -1340,9 +1340,9 @@ TEST_F(EngineSyncTest, SyncPhaseToPlayingNonSyncDeck) { | |||
| ControlObject::getControl(ConfigKey(m_sGroup1, "play"))->set(1.0); | |||
| ProcessBuffer(); | |||
|
|
|||
| EXPECT_FLOAT_EQ(140.0, | |||
| EXPECT_FLOAT_EQ(100.0, | |||
There was a problem hiding this comment.
What this is testing is that if there is a playing deck with master sync enabled, we should choose that as the sync target. That seems like a different change from what the bug was saying. If a deck is playing with sync, it should definitely be the thing that other decks sync to. So this test change seems wrong to me
| if (pSyncable->isPlaying() && !other_deck->isPlaying()) { | ||
| // If the requesting deck is playing, or we have already a | ||
| // non plaing deck found, only watch out for playing decks. | ||
| if ((foundTargetBpm || pSyncable->isPlaying()) |
There was a problem hiding this comment.
I think the original logic was right. The bug is that if we go through all the decks and we don't find a syncable playing deck, we end up falling back to the last-seen deck.
I think the correct fix could be:
if (pSyncable->isPlaying() && !other_deck->isPlaying()) {
continue;
}
// Only update the target bpm if we haven't already got one, or we found a playing syncable deck
if (!foundTargetBpm || pOtherSyncable->isPlaying()) {
foundTargetBpm = true;
targetBpm = otherDeckBpm;
targetBaseBpm = pOtherSyncable->getBaseBpm();
targetBeatDistance = pOtherSyncable->getBeatDistance();
}
if (pOtherSyncable->isPlaying()) {...
There was a problem hiding this comment.
Edit: (missread your code before)
Both versions behave the same, right?
In this case I would prefere mine, because it has a single continue to reject a syncable. On the other hand I don't really mind ... Or is there an issue with my code?
There was a problem hiding this comment.
ok I see how it's changed around. I think this is fine, thanks!
src/test/enginesynctest.cpp
Outdated
| ControlObject::getControl(ConfigKey(m_sInternalClockGroup, "bpm"))->get()); | ||
| EXPECT_FLOAT_EQ(140.0, ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get()); | ||
| EXPECT_FLOAT_EQ(100.0, ControlObject::getControl(ConfigKey(m_sGroup1, "bpm"))->get()); | ||
| // The exact beat distance will be one buffer past .6, but this is good | ||
| // enough to confirm that it worked. | ||
| EXPECT_GT(0.7, ControlObject::getControl(ConfigKey(m_sGroup1, "beat_distance"))->get()); |
There was a problem hiding this comment.
I think we need a new test explicitly for this bug. Set up three decks, all without sync. Deck 1 has bpm 100, deck 2 has bpm 140, deck 3 has bpm 160. When sync is enabled, the resulting sync bpm should be 140, whereas with the old code it'd be 160
…iates step to explicite test for lp1784185
|
Ok, The test is fixed. Ready to merge! |
|
@ywwg: What do you think now? |
|
Hi sorry for the slow response, checking now! (my github notifications are not set very well :() |
| if (pSyncable->isPlaying() && !other_deck->isPlaying()) { | ||
| // If the requesting deck is playing, or we have already a | ||
| // non plaing deck found, only watch out for playing decks. | ||
| if ((foundTargetBpm || pSyncable->isPlaying()) |
There was a problem hiding this comment.
ok I see how it's changed around. I think this is fine, thanks!
|
Thank you for comming back. |
This fixes https://bugs.launchpad.net/mixxx/+bug/1784185
I have targeted it to 2.1 because it has a limited scope and it has a test.
The current behavior can be really annoying, if the last syncable it a sampler, that was reloaded automatically from a last session