Fix for Bug #1023804: Stop AutoDJ shuffle from putting two of the same song together#287
Conversation
…ces of the same track next to each other. It checks the IDs of the tracks before and after the positions of the tracks that will be swapped, and if there's a match, it tries again with a new position. It tries a maximum of 10 times, which should be far more than enough for normal situations. It uses the PlaylistTracks table in the database to get its track ID numbers. I didn't notice any significant performance decreases, even with hundreds of the same 10-15 tracks in the AutoDJ playlist.
Here's a brief description of how the algorithm now works:
Loop through the set of tracks to be shuffled:
1) Set Track A as the current point in the shuffle set
2) Repeat a maximum of 10 times or until a good place (1/4 of the
playlist away from a conflict) is reached:
a) Pick a random track within the shuffle set (Track B)
b) Check 1/4 of the playlist up and down (clamped at the
beginning and end) from Track B's position for Track A
c) Check 1/4 of the playlist up and down (clamped at the
beginning and end) from Track A's position for Track B
d) If there was a conflict, store the position if it was better
than the already stored best position. The position is deemed
"better" if the distance (square of the difference) of
the closest conflict (Track B near Track A's position and vv)
is larger than previous iterations.
3) If no good place was found, use the stored best position
4) Swap Track A and Track B
The "tempTrackDistance" variable in the following might be confusing:
...
for (int count=startPosition; count < endPosition; count++) {
if (trackPositionIds.value(count) == trackBId && count != trackBPosition) {
conflictFound = true;
int tempTrackDistance =
(trackAPosition - count) * (trackAPosition - count);
if (tempTrackDistance < trackDistance || trackDistance == -1)
trackDistance = tempTrackDistance;
}
}
...
The reason it is there is to prevent a longer distance from being saved in
the case that there are multiple conflicts within the range.
The section of code that checks around Track A's and Track B's position is
repetitive; I chose to leave it that way for clarity for now but perhaps
it would be better to extract it into a private function?
I have also renamed a few variables and fixed some comment typos for clarity.
|
Hi Zak, Thank you for the nice feature :-) I have just some nit picks:
|
There was a problem hiding this comment.
Do you mean changing shuffleSet to something like shuffledPositions or newPositions? Or do you mean avoiding creating the copied variables in the first place? The reason I did it that way was so that the position and ID data could be passed in as constants, and the algorithm needs to be able to keep track of the newly shuffled positions as it goes along. I could remove the "const" from those two variables in the function declaration and just use them directly, but that differed from the rest of the functions in the class so it seemed like the wrong thing to do.
|
Ah, sorry I didn't address the 10-tries concern earlier. The reason I did it that way was to stay truer to the original algorithm's randomness (it doesn't actually try 10 times for every track; it's just an arbitrary limit to avoid an infinite loop in the extreme situation that the algorithm doesn't find a non-conflicting place randomly, which usually happens when there are more than 3 copies of a track in the playlist). I think if you just added a constant or "slid" the track through the playlist until there were no conflicts (which could also end up being impossible if there are enough duplicate tracks), it would turn into more of a sorting algorithm than a shuffling algorithm. I haven't experimented with that technique, but I would be happy to try if you think that would be better :) |
|
Thank you for explaining. |
Made a few changes to match the coding guidelines for Mixxx.
|
Hi Zak, I have just tested your branch and it seams not to work reliable for small playlists. A 3 track playlist with two doublets should have always the same result I think searchDistance should be clamped at 1. It does not work reliable for a 5 tracks Playlist es well. If there is one doublet, It should be always on other track between, but that does not happen in any case. Should it be ? (By the way "count" should be renamed to "pos") May be there is an issue, when swapping the doublets? Yo may put a lot of qDebug statements int the code to trace it. An other issue is the "Re-queue tracks after playback" option, often used for radio DJs. At least if this is enabled, you may also test the wrapping case. Kind regards, Daniel |
|
Thanks for taking the time to look through this code, and I'm sorry there have been problems with it! Good call on clamping searchDistance to 1; I was too focused on large playlists and neglected small ones. I have a quick question: in order to keep the shuffling function cleaner, would I be allowed to import math.h into this class? I've found the problem with the algorithm when dealing with short playlists; it can get "stuck" in an unsolvable orientation. For example, with 3 tracks containing a doublet (call them ABA), the algorithm could end up randomly ordering the first two as BA, leaving the entire order of the playlist BAA, and when it gets to the final position, it is unable to create a gap between the A's (no matter which position it swaps the final A with, it leaves either AAB or BAA). I have a couple ideas for dealing with this:
I'm leaning towards option 2 even though it's totally different than the algorithm I've been working on. It's more elegant, and it's not truly random but I think people are more concerned with the appearance of it being random rather than true randomness :) |
Yes of cause. Do not reinvent the wheel ;-) The ABA might be too special for creating a special case for it. What does the user expect when randomize it, especially if we decide to consider the re-queue option.
But IMHO we should first merge this. What is the smallest sensible play-list that should work with the current algorithm? Can we define one, just to sort out bugs? I think the current algorithm works already much better than the original one. So if you can explain the issues (they are no bugs) we can merge it without issue special cases. |
The algorithm now "wraps" around the playlist, so that if the tracks are re-queued they will still be 1/4 of the playlist apart. The most extreme case that this algorithm can now handle is a 4-track playlist with two copies of one of the tracks.
|
Sorry for the delay in finishing this! I've finished implementing your suggestions. I solved the re-queue issue by "wrapping" the search around the playlist (e.g. if part of the searchDistance range is beyond the end of the playlist, it will search the remainder of the distance at the beginning). I also found an off-by-one bug that was causing issues. It appears that the limit to the algorithm (even with two doublets) is 4 tracks. Due to the random nature of the algorithm, it is still possible for two tracks to be placed next to each other in this case, but it's unlikely (it seems to be around a 5% chance), in which case the playlist can just be shuffled again (although realistically, I don't think playlists are going to be this small in practice). However, it doesn't always look like the tracks are shuffling when there are 4 tracks with two doublets (since the duplicates will swap with each other), so I'd suggest a practical limit of no more extreme of a case than a 4-track playlist with one doublet :) |
Hey, we re all spare time workers :-). Thank you for you time. I have just tested you branch and it seams that all issues are fixed. I have just a minor code style issue. @ALL: Any objection to merge this to master? |
There was a problem hiding this comment.
Just want to check to make sure, does placing this utility function here (and not making it a private member of the class) fit within the style?
There was a problem hiding this comment.
I think we have no style guide about this, but since it is only used here, it is more natural to be a private member.
|
@zak-reynolds: Thank you for polishing. IMHO we can merge this now. Are you finished with your work? |
|
Yup! As long as you're happy with the style, I consider it done. I think the spotify-like algorithm is a cool idea for the future, but there are other things that are more important to work on now. |
Fix for Bug #1023804: Stop AutoDJ shuffle from putting two of the same song together
add links to mastodon including verification
2 participants
I've modified the shuffling algorithm to try to keep 1/4 of the length of the playlist between tracks. It works in both normal and extreme situations at an acceptable rate, still taking a fraction of a second shuffling a playlist of 1600+ of the same 80 tracks. Details of the algorithm can be found in the comments and the 2nd commit message. I've tried to adhere to the style guidelines but this is my first time contributing, so I'm all ears if I've missed anything!