Skip to content

Commit 8ff2100

Browse files
peppypeppy
authored
Merge pull request #34087 from peppy/song-select-v2-deletion-handling
Improve song select's automatic selection behaviour when current selection is no longer valid
2 parents 03d2295 + acdd246 commit 8ff2100

File tree

6 files changed

+385
-18
lines changed

6 files changed

+385
-18
lines changed

‎osu.Game.Tests/Visual/SongSelectV2/SongSelectTestScene.cs‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Linq;
66
using osu.Framework.Allocation;
77
using osu.Framework.Audio;
8+
using osu.Framework.Extensions;
89
using osu.Framework.Extensions.ObjectExtensions;
910
using osu.Framework.Graphics;
1011
using osu.Framework.Graphics.Containers;
@@ -145,14 +146,27 @@ protected virtual void LoadSongSelect()
145146
AddUntilStep("wait for filtering", () => !Carousel.IsFiltering);
146147
}
147148

148-
protected void ImportBeatmapForRuleset(int rulesetId)
149+
protected void SortBy(SortMode mode) => AddStep($"sort by {mode.GetDescription().ToLowerInvariant()}", () => Config.SetValue(OsuSetting.SongSelectSortingMode, mode));
150+
151+
protected void GroupBy(GroupMode mode) => AddStep($"group by {mode.GetDescription().ToLowerInvariant()}", () => Config.SetValue(OsuSetting.SongSelectGroupMode, mode));
152+
153+
protected void SortAndGroupBy(SortMode sort, GroupMode group)
154+
{
155+
AddStep($"sort by {sort.GetDescription().ToLowerInvariant()} & group by {group.GetDescription().ToLowerInvariant()}", () =>
156+
{
157+
Config.SetValue(OsuSetting.SongSelectSortingMode, sort);
158+
Config.SetValue(OsuSetting.SongSelectGroupMode, group);
159+
});
160+
}
161+
162+
protected void ImportBeatmapForRuleset(params int[] rulesetIds)
149163
{
150164
int beatmapsCount = 0;
151165

152-
AddStep($"import test map for ruleset {rulesetId}", () =>
166+
AddStep($"import test map for ruleset {rulesetIds}", () =>
153167
{
154168
beatmapsCount = SongSelect.IsNull() ? 0 : Carousel.Filters.OfType<BeatmapCarouselFilterGrouping>().Single().SetItems.Count;
155-
Beatmaps.Import(TestResources.CreateTestBeatmapSetInfo(3, Rulesets.AvailableRulesets.Where(r => r.OnlineID == rulesetId).ToArray()));
169+
Beatmaps.Import(TestResources.CreateTestBeatmapSetInfo(3, Rulesets.AvailableRulesets.Where(r => rulesetIds.Contains(r.OnlineID)).ToArray()));
156170
});
157171

158172
// This is specifically for cases where the add is happening post song select load.

‎osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectCurrentSelectionInvalidated.cs‎

Lines changed: 253 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,253 @@
1+
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
2+
// See the LICENCE file in the repository root for full licence text.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Linq;
7+
using NUnit.Framework;
8+
using osu.Framework.Testing;
9+
using osu.Game.Beatmaps;
10+
using osu.Game.Configuration;
11+
using osu.Game.Screens.Select.Filter;
12+
using osu.Game.Screens.SelectV2;
13+
14+
namespace osu.Game.Tests.Visual.SongSelectV2
15+
{
16+
/// <summary>
17+
/// The fallback behaviour guaranteed by SongSelect is that a random selection will happen in worst case scenario.
18+
/// Every case we're testing here is expected to have a *custom behaviour* – engaging and overriding this random selection fallback.
19+
///
20+
/// The scenarios we care abouts are:
21+
/// - Ruleset change (select another difficulty from same set for the new ruleset, if possible).
22+
/// - Beatmap difficulty hidden (select closest valid difficulty from same set)
23+
/// - Beatmap set deleted (select closest valid beatmap post-deletion)
24+
///
25+
/// We are working with 5 sets, each with 3 difficulties (all osu! ruleset).
26+
/// </summary>
27+
public partial class TestSceneSongSelectCurrentSelectionInvalidated : SongSelectTestScene
28+
{
29+
private BeatmapInfo? selectedBeatmap => (BeatmapInfo?)Carousel.CurrentSelection;
30+
private BeatmapSetInfo? selectedBeatmapSet => selectedBeatmap?.BeatmapSet;
31+
32+
[SetUpSteps]
33+
public override void SetUpSteps()
34+
{
35+
base.SetUpSteps();
36+
37+
for (int i = 0; i < 5; i++)
38+
ImportBeatmapForRuleset(0);
39+
40+
LoadSongSelect();
41+
}
42+
43+
[Test]
44+
public void TestRulesetChange()
45+
{
46+
AddStep("disable converts", () => Config.SetValue(OsuSetting.ShowConvertedBeatmaps, false));
47+
48+
ImportBeatmapForRuleset(0, 1);
49+
ImportBeatmapForRuleset(0, 1);
50+
ImportBeatmapForRuleset(0, 2);
51+
waitForFiltering(5);
52+
53+
ChangeRuleset(1);
54+
waitForFiltering(6);
55+
56+
BeatmapInfo? initiallySelected = null;
57+
AddAssert("selected is taiko", () => (initiallySelected = selectedBeatmap)?.Ruleset.OnlineID, () => Is.EqualTo(1));
58+
59+
ChangeRuleset(0);
60+
waitForFiltering(7);
61+
AddAssert("selected is osu", () => selectedBeatmap?.Ruleset.OnlineID, () => Is.EqualTo(0));
62+
AddAssert("selected is same set as original", () => selectedBeatmap?.BeatmapSet, () => Is.EqualTo(initiallySelected!.BeatmapSet));
63+
64+
ChangeRuleset(1);
65+
waitForFiltering(8);
66+
AddAssert("selected is taiko", () => selectedBeatmap?.Ruleset.OnlineID, () => Is.EqualTo(1));
67+
AddAssert("selected is same set as original", () => selectedBeatmap?.BeatmapSet, () => Is.EqualTo(initiallySelected!.BeatmapSet));
68+
69+
ChangeRuleset(2);
70+
waitForFiltering(9);
71+
AddAssert("selected is catch", () => selectedBeatmap?.Ruleset.OnlineID, () => Is.EqualTo(2));
72+
AddAssert("selected is different set", () => selectedBeatmap?.BeatmapSet, () => Is.Not.EqualTo(initiallySelected!.BeatmapSet));
73+
}
74+
75+
/// <summary>
76+
/// Make sure that deleting all sets doesn't hit some weird edge case / crash.
77+
/// </summary>
78+
[TestCase(SortMode.Title)]
79+
[TestCase(SortMode.Artist)]
80+
[TestCase(SortMode.Difficulty)]
81+
public void TestDeleteAllSets(SortMode sortMode)
82+
{
83+
int filterCount = sortMode != SortMode.Title ? 2 : 1;
84+
85+
SortBy(sortMode);
86+
waitForFiltering(filterCount);
87+
88+
BeatmapSetInfo deletedSet = null!;
89+
90+
for (int i = 0; i < 4; i++)
91+
{
92+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
93+
waitForFiltering(filterCount + 1 + i);
94+
selectionChangedFrom(() => deletedSet);
95+
}
96+
97+
// The carousel still holds an invalid selection after the final deletion. Probably fine?
98+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
99+
AddUntilStep("wait for no global selection", () => Beatmap.IsDefault, () => Is.True);
100+
}
101+
102+
[Test]
103+
public void DifficultiesGrouped_DeleteSet_SelectsAdjacent()
104+
{
105+
SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty);
106+
waitForFiltering(2);
107+
108+
makePanelSelected<PanelGroupStarDifficulty>(2);
109+
makePanelSelected<PanelBeatmapStandalone>(3);
110+
111+
// Deleting second-last, should select last
112+
BeatmapSetInfo deletedSet = null!;
113+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
114+
waitForFiltering(3);
115+
116+
selectionChangedFrom(() => deletedSet);
117+
assertPanelSelected<PanelBeatmapStandalone>(3);
118+
119+
// Deleting last, should select previous
120+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
121+
waitForFiltering(4);
122+
123+
selectionChangedFrom(() => deletedSet);
124+
assertPanelSelected<PanelBeatmapStandalone>(2);
125+
}
126+
127+
[TestCase(SortMode.Title)]
128+
[TestCase(SortMode.Artist)]
129+
public void SetsGrouped_DeleteSet_SelectsAdjacent(SortMode sortMode)
130+
{
131+
int filterCount = sortMode != SortMode.Title ? 2 : 1;
132+
133+
SortBy(sortMode);
134+
waitForFiltering(filterCount);
135+
136+
makePanelSelected<PanelBeatmapSet>(3);
137+
138+
// Deleting second-last, should select last
139+
BeatmapSetInfo deletedSet = null!;
140+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
141+
waitForFiltering(filterCount + 1);
142+
143+
selectionChangedFrom(() => deletedSet);
144+
assertPanelSelected<PanelBeatmapSet>(3);
145+
assertPanelSelected<PanelBeatmap>(0);
146+
147+
// Deleting last, should select previous
148+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
149+
waitForFiltering(filterCount + 2);
150+
151+
selectionChangedFrom(() => deletedSet);
152+
assertPanelSelected<PanelBeatmapSet>(2);
153+
assertPanelSelected<PanelBeatmap>(0);
154+
}
155+
156+
// Same scenario as the test case above, but where selected difficulty before deletion is not first index in the expanded set.
157+
// Basically ensures that the reselection is running `RequestRecommendedSelection` and not just relying on indices.
158+
[TestCase(SortMode.Title)]
159+
[TestCase(SortMode.Artist)]
160+
public void SetsGrouped_DeleteSet_SelectsNextSetRecommendedDifficulty(SortMode sortMode)
161+
{
162+
int filterCount = sortMode != SortMode.Title ? 2 : 1;
163+
164+
SortBy(sortMode);
165+
waitForFiltering(filterCount);
166+
167+
makePanelSelected<PanelBeatmapSet>(2);
168+
makePanelSelected<PanelBeatmap>(2);
169+
170+
AddUntilStep("wait for beatmap to be selected", () => selectedBeatmapSet != null);
171+
172+
BeatmapSetInfo deletedSet = null!;
173+
AddStep("delete selected", () => Beatmaps.Delete(deletedSet = selectedBeatmapSet!));
174+
waitForFiltering(++filterCount);
175+
176+
selectionChangedFrom(() => deletedSet);
177+
assertPanelSelected<PanelBeatmapSet>(2);
178+
assertPanelSelected<PanelBeatmap>(0);
179+
}
180+
181+
[Test]
182+
public void TestHideBeatmap()
183+
{
184+
makePanelSelected<PanelBeatmapSet>(2);
185+
makePanelSelected<PanelBeatmap>(1);
186+
187+
BeatmapInfo hiddenBeatmap = null!;
188+
189+
AddStep("hide selected", () => Beatmaps.Hide(hiddenBeatmap = selectedBeatmap!));
190+
waitForFiltering(2);
191+
192+
AddAssert("selected beatmap below", () => selectedBeatmap!.BeatmapSet, () => Is.EqualTo(hiddenBeatmap.BeatmapSet));
193+
194+
AddStep("hide selected", () => Beatmaps.Hide(hiddenBeatmap = selectedBeatmap!));
195+
waitForFiltering(3);
196+
197+
AddAssert("selected beatmap below", () => selectedBeatmap!.BeatmapSet, () => Is.EqualTo(hiddenBeatmap.BeatmapSet));
198+
assertPanelSelected<PanelBeatmap>(0);
199+
}
200+
201+
private void waitForFiltering(int filterCount = 1)
202+
{
203+
AddUntilStep("wait for filter count", () => Carousel.FilterCount, () => Is.EqualTo(filterCount));
204+
AddUntilStep("filtering finished", () => Carousel.IsFiltering, () => Is.False);
205+
}
206+
207+
private void makePanelSelected<T>(int index)
208+
where T : Panel
209+
{
210+
AddStep($"click panel at index {index} if not selected", () =>
211+
{
212+
var panel = allPanels<T>().ElementAt(index).ChildrenOfType<Panel>().Single();
213+
214+
// May have already been selected randomly. Don't click a second time or gameplay will start.
215+
if (!panel.Selected.Value)
216+
panel.TriggerClick();
217+
});
218+
219+
assertPanelSelected<T>(index);
220+
}
221+
222+
private void selectionChangedFrom(Func<BeatmapSetInfo> deletedSet) =>
223+
AddUntilStep("selection changed", () => selectedBeatmapSet, () => Is.Not.EqualTo(deletedSet()));
224+
225+
private void assertPanelSelected<T>(int index)
226+
where T : Panel
227+
=> AddUntilStep($"selected panel at index {index}", getActivePanelIndex<T>, () => Is.EqualTo(index));
228+
229+
private int getActivePanelIndex<T>()
230+
where T : Panel
231+
=> allPanels<T>().ToList().FindIndex(p =>
232+
{
233+
switch (p)
234+
{
235+
case PanelBeatmapStandalone pb:
236+
return pb.Selected.Value;
237+
238+
case PanelBeatmap pb:
239+
return pb.Selected.Value;
240+
241+
case Panel pbs:
242+
return pbs.Expanded.Value;
243+
244+
default:
245+
throw new InvalidOperationException();
246+
}
247+
});
248+
249+
private IEnumerable<T> allPanels<T>()
250+
where T : Panel
251+
=> Carousel.ChildrenOfType<T>().Where(p => p.Item != null).OrderBy(p => p.Y);
252+
}
253+
}

‎osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectFiltering.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,14 @@ public void TestSorting()
117117
// TODO: old test has this step, but there doesn't seem to be any purpose for it.
118118
// AddUntilStep("random map selected", () => Beatmap.Value != defaultBeatmap);
119119

120-
AddStep(@"Sort by Artist", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Artist));
121-
AddStep(@"Sort by Title", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Title));
122-
AddStep(@"Sort by Author", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Author));
123-
AddStep(@"Sort by DateAdded", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.DateAdded));
124-
AddStep(@"Sort by BPM", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.BPM));
125-
AddStep(@"Sort by Length", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Length));
126-
AddStep(@"Sort by Difficulty", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Difficulty));
127-
AddStep(@"Sort by Source", () => Config.SetValue(OsuSetting.SongSelectSortingMode, SortMode.Source));
120+
SortBy(SortMode.Artist);
121+
SortBy(SortMode.Title);
122+
SortBy(SortMode.Author);
123+
SortBy(SortMode.DateAdded);
124+
SortBy(SortMode.BPM);
125+
SortBy(SortMode.Length);
126+
SortBy(SortMode.Difficulty);
127+
SortBy(SortMode.Source);
128128
}
129129

130130
[Test]

‎osu.Game/Graphics/Carousel/Carousel.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ private void load(AudioManager audio)
307307
/// <summary>
308308
/// Retrieve a list of all <see cref="CarouselItem"/>s currently displayed.
309309
/// </summary>
310-
public IReadOnlyCollection<CarouselItem>? GetCarouselItems() => carouselItems;
310+
public IList<CarouselItem>? GetCarouselItems() => carouselItems;
311311

312312
private List<CarouselItem>? carouselItems;
313313

@@ -691,6 +691,11 @@ private void playTraversalSound()
691691
/// </summary>
692692
protected CarouselItem? CurrentSelectionItem => currentSelection.CarouselItem;
693693

694+
/// <summary>
695+
/// The index in <see cref="GetCarouselItems"/> of the current selection, if available.
696+
/// </summary>
697+
protected int? CurrentSelectionIndex => currentSelection.Index;
698+
694699
/// <summary>
695700
/// Becomes invalid when the current selection has changed and needs to be updated visually.
696701
/// </summary>

0 commit comments

Comments
 (0)