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

Generate playlists from multiple songs #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SimonTeixidor
Copy link

This PR uses my branch of bliss-rs to generate playlists based on multiple songs, instead of just the currently playing song. To me it makes the most sense to generate a playlist based on the entirety of the current playlist, so I made this the default behaviour and added a flag (--from-current-song) for the old behaviour.

@Polochon-street
Copy link
Owner

Hi,

Time to tackle this one! First, thanks a lot for submitting 😁

Second, I think (at least that's my use case and the use case of some other people I know) that it would be nice to keep the euclidean / playlist from one song as the default.

The use case is to listen to all a library's song at random and then generate a playlist from the song that's currently playing - and I think it's nicer for people if their workflow doesn't becomes disrupted upon update, trying to make changes as little breaking as possible :)

I'll give it a more thorough review in the upcoming days, but that's my two cents so far 😄

@Polochon-street
Copy link
Owner

hi @SimonTeixidor! I just released bliss-audio with your changes as 0.7.0.

I also made some extensive changes on the decoder's backend, so I just merged this b63e087 to make sure everything was working as it should, but it also incorporates the changes you made to the various distance functions :)

I think it would be nice to add a flag to do a queue_from_current_playlist (but maybe not make it the defaut) - what do you think?
Also feel free to change anything I did to accommodate your changes, maybe what I did was not super idiomatic 😅

Cheers!

@SimonTeixidor
Copy link
Author

Hi! Sorry for letting this PR sit for so long, I've been busy with other things lately. It looks like you have pretty much incorporated these changes in #59, right? If so, I think we can just close this PR.

@Polochon-street
Copy link
Owner

Hi! They are indeed incorporated in #59, but would you mind giving it a review before I merge it? You know much better how it works than I do, and I'm not sure about e.g. the parameters for the forest here https://github.com/Polochon-street/blissify-rs/pull/59/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR826-R831 :)

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.

2 participants