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.

Sign up for GitHub

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

Jump to bottom

Custom playlist music order was ignored. Queue music by filter and collection. #30343

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
+33 −4
Open

Custom playlist music order was ignored. Queue music by filter and collection. #30343

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5c23a2c
Fixed a bug where custom playlist music order was ignored.
wezwery Oct 18, 2024
814d2ed
Return BeatmapSets visibility to private.
wezwery Oct 18, 2024
9ee8e4f
Removed nullable from IEnumerable<Live<BeatmapSetInfo>>
wezwery Oct 18, 2024
854a412
Formatting code and change equals methods.
wezwery Oct 19, 2024
aa57ca3
Add Playlist to MusicController. It seems to work.
wezwery Oct 21, 2024
7b989b5
Code formatting. Calling a method once per frame.
wezwery Oct 21, 2024
1916990
Register a realm notification for beatmaps changes. Tests seem to work.
wezwery Oct 23, 2024
a75ad5c
Added empty playlist checks to other tests in TestSceneMusicActionHan…
wezwery Oct 23, 2024
42d712b
Remove bad code. Add beatmapsets to MusicController.Playlist when Pla…
wezwery Oct 24, 2024
0fcd5e7
Adding spaces
wezwery Oct 24, 2024
d4aaa86
Get all beatmap sets when playlist is empty
wezwery Nov 1, 2024
bd467a6
Code Quality fix
wezwery Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions osu.Game/Overlays/Music/Playlist.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Collections.Generic;
using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
Expand All @@ -21,6 +23,9 @@ public partial class Playlist : OsuRearrangeableListContainer<Live<BeatmapSetInf

private FilterCriteria currentCriteria = new FilterCriteria();

[Resolved]
private MusicController musicController { get; set; } = null!;

public new MarginPadding Padding
{
get => base.Padding;
Expand All @@ -46,9 +51,21 @@ public void Filter(FilterCriteria criteria)

items.SearchTerm = criteria.SearchText;
currentCriteria = criteria;

if (currentCriteria == criteria)
updateMusicControllerPlaylist();

items.FilterCompleted += () => Scheduler.AddOnce(updateMusicControllerPlaylist);

void updateMusicControllerPlaylist()
{
musicController.Playlist.Clear();
musicController.Playlist.AddRange(AllVisibleSets);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

image

@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...

Copy link
Member

Choose a reason for hiding this comment

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

@bdach if you think it's okay then i'm fine with the simple approach until someone complains about performance.

}

public Live<BeatmapSetInfo>? FirstVisibleSet => Items.FirstOrDefault(i => ((PlaylistItem)ItemMap[i]).MatchingFilter);
public IEnumerable<Live<BeatmapSetInfo>> AllVisibleSets => Items.Where(i => ((PlaylistItem)ItemMap[i]).MatchingFilter);

protected override OsuRearrangeableListItem<Live<BeatmapSetInfo>> CreateOsuDrawable(Live<BeatmapSetInfo> item) =>
new PlaylistItem(item)
Expand Down
19 changes: 11 additions & 8 deletions osu.Game/Overlays/MusicController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public partial class MusicController : CompositeDrawable
[Resolved]
private IBindable<IReadOnlyList<Mod>> mods { get; set; } = null!;

public readonly BindableList<Live<BeatmapSetInfo>> Playlist = new BindableList<Live<BeatmapSetInfo>>();

public DrawableTrack CurrentTrack { get; private set; } = new DrawableTrack(new TrackVirtual(1000));

[Resolved]
Expand Down Expand Up @@ -90,6 +92,8 @@ protected override void LoadComplete()
{
base.LoadComplete();

Playlist.AddRange(getBeatmapSets());

beatmap.BindValueChanged(b =>
{
if (b.NewValue != null)
Expand Down Expand Up @@ -256,8 +260,8 @@ private PreviousTrackResult prev(bool allowProtectedTracks)
playableSet = getNextRandom(-1, allowProtectedTracks);
else
{
playableSet = getBeatmapSets().TakeWhile(i => !i.Value.Equals(current?.BeatmapSetInfo)).LastOrDefault(s => !s.Value.Protected || allowProtectedTracks)
?? getBeatmapSets().LastOrDefault(s => !s.Value.Protected || allowProtectedTracks);
playableSet = Playlist.TakeWhile(i => !i.Value.Equals(current?.BeatmapSetInfo)).LastOrDefault(s => !s.Value.Protected || allowProtectedTracks)
?? Playlist.LastOrDefault(s => !s.Value.Protected || allowProtectedTracks);
}

if (playableSet != null)
Expand All @@ -275,7 +279,6 @@ private PreviousTrackResult prev(bool allowProtectedTracks)
/// </summary>
/// <param name="onSuccess">Invoked when the operation has been performed successfully.</param>
/// <param name="allowProtectedTracks">Whether to include <see cref="BeatmapSetInfo.Protected"/> beatmap sets when navigating.</param>
/// <returns>A <see cref="ScheduledDelegate"/> of the operation.</returns>
public void NextTrack(Action? onSuccess = null, bool allowProtectedTracks = false) => Schedule(() =>
{
bool res = next(allowProtectedTracks);
Expand Down Expand Up @@ -352,10 +355,10 @@ private bool next(bool allowProtectedTracks)
playableSet = getNextRandom(1, allowProtectedTracks);
else
{
playableSet = getBeatmapSets().SkipWhile(i => !i.Value.Equals(current?.BeatmapSetInfo))
.Where(i => !i.Value.Protected || allowProtectedTracks)
.ElementAtOrDefault(1)
?? getBeatmapSets().FirstOrDefault(i => !i.Value.Protected || allowProtectedTracks);
playableSet = Playlist.SkipWhile(i => !i.Value.Equals(current?.BeatmapSetInfo))
.Where(i => !i.Value.Protected || allowProtectedTracks)
.ElementAtOrDefault(1)
?? Playlist.FirstOrDefault(i => !i.Value.Protected || allowProtectedTracks);
}

var playableBeatmap = playableSet?.Value.Beatmaps.FirstOrDefault();
Expand All @@ -376,7 +379,7 @@ private bool next(bool allowProtectedTracks)
{
Live<BeatmapSetInfo> result;

var possibleSets = getBeatmapSets().Where(s => !s.Value.Protected || allowProtectedTracks).ToList();
var possibleSets = Playlist.Where(s => !s.Value.Protected || allowProtectedTracks).ToList();

if (possibleSets.Count == 0)
return null;
Expand Down
Loading