Custom playlist music order was ignored. Queue music by filter and collection. #30343
Conversation
Added the ability to filter music by search or by music collection to play.
|
Bit weird to see this opened given that I explicitly assigned myself to the issue with the intent to work on it but sure I guess? |
|
I saw it only after the problem was solved... |
|
I'm not claiming approval, as there might be a simpler approach to fixing this issue. |
|
Yeah not a problem I guess, just a tad strange. I'll review this by Monday at the latest. |
wezwery
changed the title
osu.Game/Overlays/Music/Playlist.cs
Outdated
| if (currentCriteria == criteria) | ||
| updateMusicControllerPlaylist(); | ||
|
|
||
| items.FilterCompleted += () => Scheduler.AddOnce(updateMusicControllerPlaylist); | ||
|
|
||
| void updateMusicControllerPlaylist() | ||
| { | ||
| musicController.Playlist.Clear(); | ||
| musicController.Playlist.AddRange(AllVisibleSets); | ||
| } |
There was a problem hiding this comment.
So I was skeptical that this did anything, in particular that this will support reordering items correctly, and yet... it does? It does because every reorder of items in the list calls OnItemsChanged(), and every OnItemsChanged() for this container calls Filter(). And thus the list is cleared and re-populated anew on every item reorder.
Stress-testing it on a huge database (~230MB) it is pretty visible in there, although maybe not as much as I'd actually expect it to be after inferring the above:
@ppy/team-client am I the only one who has a problem with this? The code is otherwise (deceptively?) simple and it appears to work correctly...
There was a problem hiding this comment.
@bdach if you think it's okay then i'm fine with the simple approach until someone complains about performance.
…ylist.cs is not loaded.
|
Test failures require addressing |
|
I don't understand why github tests don't pass, because everything works for me. |
osu.Game/Overlays/MusicController.cs
Outdated
| .Select(s => new RealmLive<BeatmapSetInfo>(s, realm)); | ||
| private IEnumerable<Live<BeatmapSetInfo>> getBeatmapSets() | ||
| { | ||
| return Playlist.IsDefault |
There was a problem hiding this comment.
What is the meaning of this conditional? Why is this ever required?
There was a problem hiding this comment.
This is necessary because github tests fail music overlay tests when loading beatmaps. The playlist is always empty.
|
The tests fail because the game does not have time to load all the game components, so it first tries to pass the tests and then load the remaining components. Although on a local Windows machine all the latest code passes tests. I can revert the changes back to bd467a6 to make the tests pass. |
…protected items to keep theme music.
Added the ability to filter music by search or by music collection to play.