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

Revert end of track #3867

Merged
merged 3 commits into from
May 23, 2021
Merged

Revert end of track #3867

merged 3 commits into from
May 23, 2021

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented May 15, 2021

Restore auto-disabling of explicit leader at end of track

ywwg added 2 commits May 15, 2021 10:52
This reverts commit 77cd3f7.

It turns out this was good to have.  Reverting.
@ywwg ywwg requested a review from Holzhaus May 15, 2021 15:32
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks, this works for me.

However, there is still some other issue, because the tone of the playing deck changes when I right-click the sync button on the other deck: https://www.youtube.com/watch?v=_bsrPJ6I6Gc I didn't notice that before #3698 was merged.

@@ -14,6 +14,8 @@
#include "util/logger.h"
#include "util/math.h"

const double kTrackPositionMasterHandoff = 0.99;
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this cutoff instead of using the actual end? Just wondering, because 0.01 can be very different of a 2 minute track compared to a premixed 2 hour set.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ywwg Please comment those decisions for others and your future self. Otherwise no one will able to reason why this particular value has been chosen.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's a good point. I think I was worried about a case where we hit 1.0 and the deck stops before the handoff occurs. I think that's not an issue though

@uklotzde uklotzde added this to the 2.4.0 milestone May 17, 2021
@ywwg
Copy link
Member Author

ywwg commented May 22, 2021

However, there is still some other issue, because the tone of the playing deck changes when I right-click the sync button on the other deck: https://www.youtube.com/watch?v=_bsrPJ6I6Gc I didn't notice that before #3698 was merged.

I can't see the other deck in your capture so I'm not quite sure what you're talking about. Can you record again with a less wide window?

@Holzhaus
Copy link
Member

However, there is still some other issue, because the tone of the playing deck changes when I right-click the sync button on the other deck: https://www.youtube.com/watch?v=_bsrPJ6I6Gc I didn't notice that before #3698 was merged.

I can't see the other deck in your capture so I'm not quite sure what you're talking about. Can you record again with a less wide window?

Yeah sorry, I only noticed after upload. The only thing you can't see is that I right-click the "SYNC" button on deck B multiple times to make it explicit leader (and you can hear it, because there's audible key change of the deck A in that case).

@ywwg
Copy link
Member Author

ywwg commented May 22, 2021

I don't think this specific PR causes the pitch issue, can I address that in a separate bug fix?

@ywwg
Copy link
Member Author

ywwg commented May 22, 2021

Ah, the problem is that when you set the stopped track as explicit leader, it resets the beat distance value and the playing track changes speed briefly to resync.

@ywwg
Copy link
Member Author

ywwg commented May 23, 2021

Ooof yes the pitch issues you found are real, but definitely not related to this fix. I am working hard on them elsewhere, but I'd like to merge this to fix the problem of explicit leader getting stuck

@Holzhaus Holzhaus merged commit f5fb8b4 into mixxxdj:main May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants