From 2a01e3d148ea69f86e955e5aac771d1cf23d5aec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 30 Oct 2025 14:54:07 +0100 Subject: [PATCH 1/2] Add failing test case --- .../TestSceneBeatmapCarouselArtistGrouping.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselArtistGrouping.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselArtistGrouping.cs index 2c3013af12..2390261cdb 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselArtistGrouping.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselArtistGrouping.cs @@ -4,9 +4,12 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Testing; +using osu.Game.Beatmaps; +using osu.Game.Database; using osu.Game.Graphics.Carousel; using osu.Game.Screens.Select.Filter; using osu.Game.Screens.SelectV2; +using osu.Game.Tests.Resources; using osuTK; namespace osu.Game.Tests.Visual.SongSelectV2 @@ -322,5 +325,38 @@ namespace osu.Game.Tests.Visual.SongSelectV2 SelectNextSet(); AddUntilStep("no beatmap panels visible", () => GetVisiblePanels().Count(), () => Is.Zero); } + + [Test] + public void TestGroupChangedAfterEngagingArtistGrouping() + { + RemoveAllBeatmaps(); + AddStep("add test beatmaps", () => + { + for (int i = 0; i < 5; ++i) + { + var baseTestBeatmap = TestResources.CreateTestBeatmapSetInfo(3); + + var metadata = new BeatmapMetadata + { + Artist = $"{(char)('A' + i)} artist", + Title = $"{(char)('A' + 4 - i)} title", + }; + + foreach (var b in baseTestBeatmap.Beatmaps) + b.Metadata = metadata; + + Realm.Write(r => r.Add(baseTestBeatmap, update: true)); + BeatmapSets.Add(baseTestBeatmap.Detach()); + } + + SortAndGroupBy(SortMode.Title, GroupMode.Title); + SelectNextSet(); + SelectNextSet(); + WaitForExpandedGroup(1); + + SortAndGroupBy(SortMode.Artist, GroupMode.Artist); + WaitForExpandedGroup(3); + }); + } } } From 73e05e3fae13e2b1e2f94825753125a547d7626b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 30 Oct 2025 14:29:40 +0100 Subject: [PATCH 2/2] Switch active carousel group if current selection no longer exists in the previous group This was primarily written to fix https://github.com/ppy/osu/issues/35538, but also incidentally targets some other scenarios, such as: - When switching from artist filtering to title filtering, selection sometimes would stay at the group under which the selection's artist was filed, rather than moving to the group under which the selection's title is filed (in other words, the group that *the selection is currently under*). - When simply assigning a beatmap to a collection such that it would be moved out of the current group, the selection will now follow to the new collection's group rather than staying at its previous position. Whether this is desired is highly likely to be extremely situational, but I don't want to introduce complications unless it's absolutely necessary. This has a significant performance overhead because `CheckModelEquality()` isn't free, but it doesn't seem horrible in profiling. --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 5e84ba0722..5991771d00 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -497,25 +497,35 @@ namespace osu.Game.Screens.SelectV2 // The filter might have changed the set of available groups, which means that the current selection may point to a stale group. // Check whether that is the case. bool groupingRemainsOff = currentGroupedBeatmap?.Group == null && grouping.GroupItems.Count == 0; - bool groupStillExists = currentGroupedBeatmap?.Group != null && grouping.GroupItems.ContainsKey(currentGroupedBeatmap.Group); - if (groupingRemainsOff || groupStillExists) + bool groupStillValid = false; + + if (currentGroupedBeatmap?.Group != null) + { + groupStillValid = grouping.GroupItems.TryGetValue(currentGroupedBeatmap.Group, out var items) + && items.Any(i => CheckModelEquality(i.Model, currentGroupedBeatmap)); + } + + if (groupingRemainsOff || groupStillValid) { // Only update the visual state of the selected item. HandleItemSelected(currentGroupedBeatmap); } else if (currentGroupedBeatmap != null) { - // If the group no longer exists, grab an arbitrary other instance of the beatmap under the first group encountered. + // If the group no longer exists (or the item no longer exists in the previous group), grab an arbitrary other instance of the beatmap under the first group encountered. var newSelection = GetCarouselItems()?.Select(i => i.Model).OfType().FirstOrDefault(gb => gb.Beatmap.Equals(currentGroupedBeatmap.Beatmap)); + // Only change the selection if we actually got a positive hit. // This is necessary so that selection isn't lost if the panel reappears later due to e.g. unapplying some filter criteria that made it disappear in the first place. if (newSelection != null) + { CurrentSelection = newSelection; + groupForReselection = newSelection.Group; + } } // If a group was selected that is not the one containing the selection, attempt to reselect it. - // If the original group was not found, ExpandedGroup will already have been updated to a valid value in `HandleItemSelected` above. if (groupForReselection != null && grouping.GroupItems.TryGetValue(groupForReselection, out _)) setExpandedGroup(groupForReselection); }