From 75ef6f6a0e02e1bf4b898186141376d2ccf7b80a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 7 Feb 2025 02:10:08 +0900 Subject: [PATCH 1/5] Use random generation in carousel stress test --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 45 ++++++++++--------- .../SongSelect/TestSceneBeatmapCarouselV2.cs | 2 +- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index f17f312e9f..db433b93d2 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -187,29 +187,32 @@ namespace osu.Game.Tests.Visual.SongSelect protected void AddBeatmaps(int count, int? fixedDifficultiesPerSet = null, bool randomMetadata = false) => AddStep($"add {count} beatmaps{(randomMetadata ? " with random data" : "")}", () => { for (int i = 0; i < count; i++) - { - var beatmapSetInfo = TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4)); - - if (randomMetadata) - { - char randomCharacter = getRandomCharacter(); - - var metadata = new BeatmapMetadata - { - // Create random metadata, then we can check if sorting works based on these - Artist = $"{randomCharacter}ome Artist " + RNG.Next(0, 9), - Title = $"{randomCharacter}ome Song (set id {beatmapSetInfo.OnlineID:000}) {Guid.NewGuid()}", - Author = { Username = $"{randomCharacter}ome Guy " + RNG.Next(0, 9) }, - }; - - foreach (var beatmap in beatmapSetInfo.Beatmaps) - beatmap.Metadata = metadata.DeepClone(); - } - - BeatmapSets.Add(beatmapSetInfo); - } + BeatmapSets.Add(CreateTestBeatmapSetInfo(fixedDifficultiesPerSet, randomMetadata)); }); + protected static BeatmapSetInfo CreateTestBeatmapSetInfo(int? fixedDifficultiesPerSet, bool randomMetadata) + { + var beatmapSetInfo = TestResources.CreateTestBeatmapSetInfo(fixedDifficultiesPerSet ?? RNG.Next(1, 4)); + + if (randomMetadata) + { + char randomCharacter = getRandomCharacter(); + + var metadata = new BeatmapMetadata + { + // Create random metadata, then we can check if sorting works based on these + Artist = $"{randomCharacter}ome Artist " + RNG.Next(0, 9), + Title = $"{randomCharacter}ome Song (set id {beatmapSetInfo.OnlineID:000}) {Guid.NewGuid()}", + Author = { Username = $"{randomCharacter}ome Guy " + RNG.Next(0, 9) }, + }; + + foreach (var beatmap in beatmapSetInfo.Beatmaps) + beatmap.Metadata = metadata.DeepClone(); + } + + return beatmapSetInfo; + } + private static long randomCharPointer; private static char getRandomCharacter() diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs index 3c5cf16e92..30ca26ce68 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselV2.cs @@ -77,7 +77,7 @@ namespace osu.Game.Tests.Visual.SongSelect Task.Run(() => { for (int j = 0; j < count; j++) - generated.Add(TestResources.CreateTestBeatmapSetInfo(RNG.Next(1, 4))); + generated.Add(CreateTestBeatmapSetInfo(3, true)); }).ConfigureAwait(true); }); From 50d880e2ae3e3abfd58a795d26461ff39aa82070 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 7 Feb 2025 02:10:38 +0900 Subject: [PATCH 2/5] Fix unnecessary `BeatmapSet.Metadata` lookups --- osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs index 0298616aa8..3cdbbb4fed 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterSorting.cs @@ -36,7 +36,7 @@ namespace osu.Game.Screens.SelectV2 switch (criteria.Sort) { case SortMode.Artist: - comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.BeatmapSet!.Metadata.Artist, bb.BeatmapSet!.Metadata.Artist); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.Metadata.Artist, bb.Metadata.Artist); if (comparison == 0) goto case SortMode.Title; break; @@ -46,7 +46,7 @@ namespace osu.Game.Screens.SelectV2 break; case SortMode.Title: - comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.BeatmapSet!.Metadata.Title, bb.BeatmapSet!.Metadata.Title); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(ab.Metadata.Title, bb.Metadata.Title); break; default: From 9d979dc3f4adb523269fb14f6d049986dab9d61b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 7 Feb 2025 02:37:16 +0900 Subject: [PATCH 3/5] Refactor grouping to be much more efficient --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 2 +- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 207 ++++++++---------- 2 files changed, 93 insertions(+), 116 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 4126889892..137a8e8eab 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -289,5 +289,5 @@ namespace osu.Game.Screens.SelectV2 #endregion } - public record GroupDefinition(string Title); + public record GroupDefinition(object Data, string Title); } diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index a8caebad7a..8838ce67ad 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Threading; using System.Threading.Tasks; using osu.Game.Beatmaps; @@ -36,137 +35,115 @@ namespace osu.Game.Screens.SelectV2 this.getCriteria = getCriteria; } - public async Task> Run(IEnumerable items, CancellationToken cancellationToken) => await Task.Run(() => + public async Task> Run(IEnumerable items, CancellationToken cancellationToken) { - setItems.Clear(); - groupItems.Clear(); + return await Task.Run(() => + { + setItems.Clear(); + groupItems.Clear(); - var criteria = getCriteria(); - var newItems = new List(items.Count()); + var criteria = getCriteria(); + var newItems = new List(); - // Add criteria groups. + BeatmapInfo? lastBeatmap = null; + GroupDefinition? lastGroup = null; + + HashSet? groupRefItems = null; + HashSet? setRefItems = null; + + switch (criteria.Group) + { + default: + BeatmapSetsGroupedTogether = true; + break; + + case GroupMode.Difficulty: + BeatmapSetsGroupedTogether = false; + break; + } + + foreach (var item in items) + { + cancellationToken.ThrowIfCancellationRequested(); + + var beatmap = (BeatmapInfo)item.Model; + + if (createGroupIfRequired(criteria, beatmap, lastGroup) is GroupDefinition newGroup) + { + // When reaching a new group, ensure we reset any beatmap set tracking. + setRefItems = null; + lastBeatmap = null; + + groupItems[newGroup] = groupRefItems = new HashSet(); + lastGroup = newGroup; + + addItem(new CarouselItem(newGroup) + { + DrawHeight = GroupPanel.HEIGHT, + DepthLayer = -2, + }); + } + + if (BeatmapSetsGroupedTogether) + { + bool newBeatmapSet = lastBeatmap?.BeatmapSet!.ID != beatmap.BeatmapSet!.ID; + + if (newBeatmapSet) + { + setItems[beatmap.BeatmapSet!] = setRefItems = new HashSet(); + + addItem(new CarouselItem(beatmap.BeatmapSet!) + { + DrawHeight = BeatmapSetPanel.HEIGHT, + DepthLayer = -1 + }); + } + } + + addItem(item); + lastBeatmap = beatmap; + + void addItem(CarouselItem i) + { + newItems.Add(i); + + groupRefItems?.Add(i); + setRefItems?.Add(i); + + i.IsVisible = i.Model is GroupDefinition || (lastGroup == null && (i.Model is BeatmapSetInfo || setRefItems == null)); + } + } + + return newItems; + }, cancellationToken).ConfigureAwait(false); + } + + private GroupDefinition? createGroupIfRequired(FilterCriteria criteria, BeatmapInfo beatmap, GroupDefinition? lastGroup) + { switch (criteria.Group) { - default: - BeatmapSetsGroupedTogether = true; - newItems.AddRange(items); - break; - case GroupMode.Artist: - BeatmapSetsGroupedTogether = true; - char groupChar = (char)0; + char groupChar = lastGroup?.Data as char? ?? (char)0; + char beatmapFirstChar = char.ToUpperInvariant(beatmap.Metadata.Artist[0]); - foreach (var item in items) - { - cancellationToken.ThrowIfCancellationRequested(); - - var b = (BeatmapInfo)item.Model; - - char beatmapFirstChar = char.ToUpperInvariant(b.Metadata.Artist[0]); - - if (beatmapFirstChar > groupChar) - { - groupChar = beatmapFirstChar; - var groupDefinition = new GroupDefinition($"{groupChar}"); - var groupItem = new CarouselItem(groupDefinition) { DrawHeight = GroupPanel.HEIGHT }; - - newItems.Add(groupItem); - groupItems[groupDefinition] = new HashSet { groupItem }; - } - - newItems.Add(item); - } + if (beatmapFirstChar > groupChar) + return new GroupDefinition(beatmapFirstChar, $"{beatmapFirstChar}"); break; case GroupMode.Difficulty: - BeatmapSetsGroupedTogether = false; - int starGroup = int.MinValue; + int starGroup = lastGroup?.Data as int? ?? -1; - foreach (var item in items) + if (beatmap.StarRating > starGroup) { - cancellationToken.ThrowIfCancellationRequested(); - - var b = (BeatmapInfo)item.Model; - - if (b.StarRating > starGroup) - { - starGroup = (int)Math.Floor(b.StarRating); - var groupDefinition = new GroupDefinition($"{starGroup} - {++starGroup} *"); - - var groupItem = new CarouselItem(groupDefinition) - { - DrawHeight = GroupPanel.HEIGHT, - DepthLayer = -2 - }; - - newItems.Add(groupItem); - groupItems[groupDefinition] = new HashSet { groupItem }; - } - - newItems.Add(item); + starGroup = (int)Math.Floor(beatmap.StarRating); + return new GroupDefinition(starGroup + 1, $"{starGroup} - {starGroup + 1} *"); } break; } - // Add set headers wherever required. - CarouselItem? lastItem = null; - - if (BeatmapSetsGroupedTogether) - { - for (int i = 0; i < newItems.Count; i++) - { - cancellationToken.ThrowIfCancellationRequested(); - - var item = newItems[i]; - - if (item.Model is BeatmapInfo beatmap) - { - bool newBeatmapSet = lastItem?.Model is not BeatmapInfo lastBeatmap || lastBeatmap.BeatmapSet!.ID != beatmap.BeatmapSet!.ID; - - if (newBeatmapSet) - { - var setItem = new CarouselItem(beatmap.BeatmapSet!) - { - DrawHeight = BeatmapSetPanel.HEIGHT, - DepthLayer = -1 - }; - - setItems[beatmap.BeatmapSet!] = new HashSet { setItem }; - newItems.Insert(i, setItem); - i++; - } - - setItems[beatmap.BeatmapSet!].Add(item); - item.IsVisible = false; - } - - lastItem = item; - } - } - - // Link group items to their headers. - GroupDefinition? lastGroup = null; - - foreach (var item in newItems) - { - cancellationToken.ThrowIfCancellationRequested(); - - if (item.Model is GroupDefinition group) - { - lastGroup = group; - continue; - } - - if (lastGroup != null) - { - groupItems[lastGroup].Add(item); - item.IsVisible = false; - } - } - - return newItems; - }, cancellationToken).ConfigureAwait(false); + return null; + } } } From 3da615481eb59a2aad22501e74121f2b0e06323e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 7 Feb 2025 17:38:24 +0900 Subject: [PATCH 4/5] Change `switch` to simple conditional for now --- .../Screens/SelectV2/BeatmapCarouselFilterGrouping.cs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index 8838ce67ad..db407fd647 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -51,16 +51,7 @@ namespace osu.Game.Screens.SelectV2 HashSet? groupRefItems = null; HashSet? setRefItems = null; - switch (criteria.Group) - { - default: - BeatmapSetsGroupedTogether = true; - break; - - case GroupMode.Difficulty: - BeatmapSetsGroupedTogether = false; - break; - } + BeatmapSetsGroupedTogether = criteria.Group != GroupMode.Difficulty; foreach (var item in items) { From 29b0b62ffa55ebb4ac4b107a691c95a3f72f516d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 7 Feb 2025 17:39:38 +0900 Subject: [PATCH 5/5] Rename variables to something more sane --- .../SelectV2/BeatmapCarouselFilterGrouping.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs index db407fd647..cb5a40918c 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs @@ -48,8 +48,8 @@ namespace osu.Game.Screens.SelectV2 BeatmapInfo? lastBeatmap = null; GroupDefinition? lastGroup = null; - HashSet? groupRefItems = null; - HashSet? setRefItems = null; + HashSet? currentGroupItems = null; + HashSet? currentSetItems = null; BeatmapSetsGroupedTogether = criteria.Group != GroupMode.Difficulty; @@ -62,10 +62,10 @@ namespace osu.Game.Screens.SelectV2 if (createGroupIfRequired(criteria, beatmap, lastGroup) is GroupDefinition newGroup) { // When reaching a new group, ensure we reset any beatmap set tracking. - setRefItems = null; + currentSetItems = null; lastBeatmap = null; - groupItems[newGroup] = groupRefItems = new HashSet(); + groupItems[newGroup] = currentGroupItems = new HashSet(); lastGroup = newGroup; addItem(new CarouselItem(newGroup) @@ -81,7 +81,7 @@ namespace osu.Game.Screens.SelectV2 if (newBeatmapSet) { - setItems[beatmap.BeatmapSet!] = setRefItems = new HashSet(); + setItems[beatmap.BeatmapSet!] = currentSetItems = new HashSet(); addItem(new CarouselItem(beatmap.BeatmapSet!) { @@ -98,10 +98,10 @@ namespace osu.Game.Screens.SelectV2 { newItems.Add(i); - groupRefItems?.Add(i); - setRefItems?.Add(i); + currentGroupItems?.Add(i); + currentSetItems?.Add(i); - i.IsVisible = i.Model is GroupDefinition || (lastGroup == null && (i.Model is BeatmapSetInfo || setRefItems == null)); + i.IsVisible = i.Model is GroupDefinition || (lastGroup == null && (i.Model is BeatmapSetInfo || currentSetItems == null)); } }