From d200a5990213e1568cb9e9d00da201e1972b3e52 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Jun 2025 19:18:44 +0900 Subject: [PATCH 1/7] SongSelectV2: Fix random button not respecting open group Closes https://github.com/ppy/osu/issues/33569. --- .../TestSceneBeatmapCarouselRandom.cs | 74 ++++++++++++++----- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 20 +++++ 2 files changed, 77 insertions(+), 17 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs index 858c314904..17d31634fc 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs @@ -6,6 +6,7 @@ using NUnit.Framework; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Screens.Select.Filter; +using osu.Game.Screens.SelectV2; namespace osu.Game.Tests.Visual.SongSelectV2 { @@ -47,25 +48,42 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddBeatmaps(10, 3, true); WaitForDrawablePanels(); + GroupDefinition? expanded = null; + + for (int i = 0; i < 2; i++) + { + nextRandom(); + expanded ??= storeExpandedGroup(); + + ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); + } + nextRandom(); - ensureRandomDidNotRepeat(); - nextRandom(); - ensureRandomDidNotRepeat(); - nextRandom(); - ensureRandomDidNotRepeat(); + ensureRandomDidRepeat(); + checkExpandedGroupUnchanged(); prevRandom(); checkRewindCorrectSet(); + checkExpandedGroupUnchanged(); prevRandom(); checkRewindCorrectSet(); + checkExpandedGroupUnchanged(); nextRandom(); ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); nextRandom(); - ensureRandomDidNotRepeat(); + ensureRandomDidRepeat(); + checkExpandedGroupUnchanged(); - nextRandom(); - AddAssert("ensure repeat", () => BeatmapSetRequestedSelections.Contains(Carousel.SelectedBeatmapSet!)); + GroupDefinition? storeExpandedGroup() + { + AddStep("store open group", () => expanded = Carousel.ExpandedGroup); + return null; + } + + void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); } /// @@ -76,28 +94,47 @@ namespace osu.Game.Tests.Visual.SongSelectV2 { SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty); - AddBeatmaps(10, 3, true); + AddBeatmaps(3, 3, true); WaitForDrawablePanels(); + GroupDefinition? expanded = null; + + for (int i = 0; i < 3; i++) + { + nextRandom(); + expanded ??= storeExpandedGroup(); + + ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); + } + nextRandom(); - ensureRandomDidNotRepeat(); - nextRandom(); - ensureRandomDidNotRepeat(); - nextRandom(); - ensureRandomDidNotRepeat(); + ensureRandomDidRepeat(); + checkExpandedGroupUnchanged(); prevRandom(); checkRewindCorrectSet(); + checkExpandedGroupUnchanged(); + prevRandom(); checkRewindCorrectSet(); + checkExpandedGroupUnchanged(); nextRandom(); ensureRandomDidNotRepeat(); - nextRandom(); - ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); nextRandom(); - AddAssert("ensure repeat", () => BeatmapSetRequestedSelections.Contains(Carousel.SelectedBeatmapSet!)); + ensureRandomDidRepeat(); + checkExpandedGroupUnchanged(); + + GroupDefinition? storeExpandedGroup() + { + AddStep("store open group", () => expanded = Carousel.ExpandedGroup); + return null; + } + + void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); } [Test] @@ -174,6 +211,9 @@ namespace osu.Game.Tests.Visual.SongSelectV2 private void nextRandom() => AddStep("select random next", () => Carousel.NextRandom()); + private void ensureRandomDidRepeat() => + AddAssert("did repeat", () => BeatmapSetRequestedSelections.Distinct().Count(), () => Is.LessThan(BeatmapSetRequestedSelections.Count)); + private void ensureRandomDidNotRepeat() => AddAssert("no repeats", () => BeatmapSetRequestedSelections.Distinct().Count(), () => Is.EqualTo(BeatmapSetRequestedSelections.Count)); diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 24092b8ecd..077a0fb9f8 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -634,6 +634,26 @@ namespace osu.Game.Screens.SelectV2 // This is the fastest way to retrieve sets for randomisation. ICollection visibleSets = grouping.SetItems.Keys; + if (ExpandedGroup != null) + { + // In the case of grouping, users expect random to only operate on the expanded group. + // This is going to incur some overhead as we don't have a group-beatmapset mapping currently. + // + // If this becomes an issue, we could either store a mapping, or run the random algorithm many times + // using the `SetItems` method until we get a group HIT. + if (grouping.BeatmapSetsGroupedTogether) + visibleSets = grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().ToArray(); + else + { + // Note that this is probably not correct in all cases. + // When we aren't grouping sets together, we might want to randomise by beatmaps, not sets. + // + // Imagine the scenario where a single beatmap set has multiple difficulties in the same difficulty grouping, where this + // would always choose the set's user recommended difficulty rather than the visible ones. + visibleSets = grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().Select(b => b.BeatmapSet!).Distinct().ToArray(); + } + } + if (CurrentSelection is BeatmapInfo beatmapInfo) { randomSelectedBeatmaps.Add(beatmapInfo); From f676206331cfa43485f6b62f81282eefc50b3ae6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Jun 2025 19:42:13 +0900 Subject: [PATCH 2/7] SongSelectV2: Fix random selection not working as expected when displaying individual difficulties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, random selection would always be done at a *set* level. The final operation of a random action would be "select the user's recommended difficulty from this randomly selected set". This makes no sense when sets are not grouped together at song select. In fact, it is completely broken with the previous commit which adds group-isolated random support – if we're grouping by difficulty and the user's recommendation is not in the current group it would throw the user into another group unexpectedly. This fixes the issue by splitting out the random implementation into two separate pathways depending on the carousel display mode. --- .../SongSelectV2/BeatmapCarouselTestScene.cs | 3 + .../TestSceneBeatmapCarouselRandom.cs | 113 ++++++++++--- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 150 +++++++++++++----- 3 files changed, 201 insertions(+), 65 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/BeatmapCarouselTestScene.cs b/osu.Game.Tests/Visual/SongSelectV2/BeatmapCarouselTestScene.cs index 3943b13286..e6ba6a904d 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/BeatmapCarouselTestScene.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/BeatmapCarouselTestScene.cs @@ -36,6 +36,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 public abstract partial class BeatmapCarouselTestScene : OsuManualInputManagerTestScene { protected readonly Stack BeatmapSetRequestedSelections = new Stack(); + protected readonly Stack BeatmapRequestedSelections = new Stack(); protected readonly BindableList BeatmapSets = new BindableList(); @@ -73,6 +74,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 { AddStep("create components", () => { + BeatmapRequestedSelections.Clear(); BeatmapSetRequestedSelections.Clear(); BeatmapRecommendationFunction = null; NewItemsPresentedInvocationCount = 0; @@ -113,6 +115,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 NewItemsPresented = _ => NewItemsPresentedInvocationCount++, RequestSelection = b => { + BeatmapRequestedSelections.Push(b); Carousel.CurrentSelection = b; }, RequestRecommendedSelection = beatmaps => diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs index 17d31634fc..739fc23ed5 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs @@ -55,26 +55,26 @@ namespace osu.Game.Tests.Visual.SongSelectV2 nextRandom(); expanded ??= storeExpandedGroup(); - ensureRandomDidNotRepeat(); + ensureSetRandomDidNotRepeat(); checkExpandedGroupUnchanged(); } nextRandom(); - ensureRandomDidRepeat(); + ensureSetRandomDidRepeat(); checkExpandedGroupUnchanged(); - prevRandom(); + prevRandomSet(); checkRewindCorrectSet(); checkExpandedGroupUnchanged(); - prevRandom(); + prevRandomSet(); checkRewindCorrectSet(); checkExpandedGroupUnchanged(); nextRandom(); - ensureRandomDidNotRepeat(); + ensureSetRandomDidNotRepeat(); checkExpandedGroupUnchanged(); nextRandom(); - ensureRandomDidRepeat(); + ensureSetRandomDidRepeat(); checkExpandedGroupUnchanged(); GroupDefinition? storeExpandedGroup() @@ -90,7 +90,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 /// Test random non-repeating algorithm /// [Test] - public void TestRandomDifficultyGrouping() + public void TestRandomDifficultyGroupingRewindsCorrectly() { SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty); @@ -108,21 +108,19 @@ namespace osu.Game.Tests.Visual.SongSelectV2 checkExpandedGroupUnchanged(); } - nextRandom(); - ensureRandomDidRepeat(); - checkExpandedGroupUnchanged(); + for (int i = 0; i < 2; i++) + { + prevRandom(); + checkRewindCorrect(); + checkExpandedGroupUnchanged(); + } - prevRandom(); - checkRewindCorrectSet(); - checkExpandedGroupUnchanged(); - - prevRandom(); - checkRewindCorrectSet(); - checkExpandedGroupUnchanged(); - - nextRandom(); - ensureRandomDidNotRepeat(); - checkExpandedGroupUnchanged(); + for (int i = 0; i < 2; i++) + { + nextRandom(); + ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); + } nextRandom(); ensureRandomDidRepeat(); @@ -137,6 +135,54 @@ namespace osu.Game.Tests.Visual.SongSelectV2 void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); } + /// + /// Test random non-repeating algorithm + /// + [Test] + public void TestRandomDifficultyGroupingRepeatsWhenExhausted() + { + SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty); + + AddBeatmaps(3, 3, true); + WaitForDrawablePanels(); + + GroupDefinition? expanded = null; + + for (int i = 0; i < 3; i++) + { + nextRandom(); + expanded ??= storeExpandedGroup(); + + ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); + } + + for (int i = 0; i < 3; i++) + { + nextRandom(); + ensureRandomDidRepeat(); + } + + for (int i = 0; i < 5; i++) + { + prevRandom(); + checkRewindCorrect(); + checkExpandedGroupUnchanged(); + } + + nextRandom(); + checkExpandedGroupUnchanged(); + // can't assert repeat or otherwise as we went through multiple permutations. + + GroupDefinition? storeExpandedGroup() + { + AddStep("store open group", () => expanded = Carousel.ExpandedGroup); + return null; + } + + void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); + } + [Test] public void TestRewindOverMultipleIterations() { @@ -153,7 +199,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 for (int i = 0; i < random_select_count; i++) { - prevRandom(); + prevRandomSet(); checkRewindCorrectSet(); } } @@ -204,7 +250,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddAssert("selection not changed", () => Carousel.CurrentSelection, () => Is.EqualTo(postRandomSelection)); - prevRandom(); + prevRandomSet(); AddAssert("selection not changed", () => Carousel.CurrentSelection, () => Is.EqualTo(postRandomSelection)); } @@ -212,15 +258,32 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddStep("select random next", () => Carousel.NextRandom()); private void ensureRandomDidRepeat() => - AddAssert("did repeat", () => BeatmapSetRequestedSelections.Distinct().Count(), () => Is.LessThan(BeatmapSetRequestedSelections.Count)); + AddAssert("did repeat", () => BeatmapRequestedSelections.Distinct().Count(), () => Is.LessThan(BeatmapRequestedSelections.Count)); private void ensureRandomDidNotRepeat() => + AddAssert("no repeats", () => BeatmapRequestedSelections.Distinct().Count(), () => Is.EqualTo(BeatmapRequestedSelections.Count)); + + private void ensureSetRandomDidRepeat() => + AddAssert("did repeat", () => BeatmapSetRequestedSelections.Distinct().Count(), () => Is.LessThan(BeatmapSetRequestedSelections.Count)); + + private void ensureSetRandomDidNotRepeat() => AddAssert("no repeats", () => BeatmapSetRequestedSelections.Distinct().Count(), () => Is.EqualTo(BeatmapSetRequestedSelections.Count)); + private void checkRewindCorrect() => + AddAssert("rewind matched expected beatmap", () => BeatmapRequestedSelections.Peek(), () => Is.EqualTo(Carousel.SelectedBeatmapInfo)); + private void checkRewindCorrectSet() => AddAssert("rewind matched expected set", () => BeatmapSetRequestedSelections.Peek(), () => Is.EqualTo(Carousel.SelectedBeatmapSet)); - private void prevRandom() => AddStep("select random last", () => + private void prevRandom() => AddStep("select last random", () => + { + Carousel.PreviousRandom(); + BeatmapRequestedSelections.Pop(); + // Pop twice because the PreviousRandom call also requests selection. + BeatmapRequestedSelections.Pop(); + }); + + private void prevRandomSet() => AddStep("select last random set", () => { Carousel.PreviousRandom(); BeatmapSetRequestedSelections.Pop(); diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 077a0fb9f8..cd5ab68e6f 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -618,8 +618,8 @@ namespace osu.Game.Screens.SelectV2 #region Random selection handling private readonly Bindable randomAlgorithm = new Bindable(); - private readonly List previouslyVisitedRandomSets = new List(); - private readonly List randomSelectedBeatmaps = new List(); + private readonly List previouslyVisitedRandomBeatmaps = new List(); + private readonly List randomHistory = new List(); private Sample? spinSample; private Sample? randomSelectSample; @@ -631,59 +631,129 @@ namespace osu.Game.Screens.SelectV2 if (carouselItems?.Any() != true) return false; - // This is the fastest way to retrieve sets for randomisation. - ICollection visibleSets = grouping.SetItems.Keys; + var selectionBefore = CurrentSelectionItem; + var beatmapBefore = selectionBefore?.Model as BeatmapInfo; - if (ExpandedGroup != null) + bool success; + + if (beatmapBefore != null) { + // keep track of visited beatmaps and sets for rewind + randomHistory.Add(beatmapBefore); + // keep track of visited beatmaps for "RandomPermutation" random tracking. + // note that this is reset when we run out of beatmaps, while `randomHistory` is not. + previouslyVisitedRandomBeatmaps.Add(beatmapBefore); + } + + if (grouping.BeatmapSetsGroupedTogether) + success = nextRandomSet(); + else + success = nextRandomBeatmap(); + + if (!success) + { + if (beatmapBefore != null) + randomHistory.RemoveAt(randomHistory.Count - 1); + return false; + } + + Scheduler.Add(() => + { + if (selectionBefore != null && CurrentSelectionItem != null) + playSpinSample(distanceBetween(selectionBefore, CurrentSelectionItem), carouselItems.Count); + }); + + return true; + } + + private bool nextRandomBeatmap() + { + ICollection visibleBeatmaps = ExpandedGroup != null // In the case of grouping, users expect random to only operate on the expanded group. // This is going to incur some overhead as we don't have a group-beatmapset mapping currently. // // If this becomes an issue, we could either store a mapping, or run the random algorithm many times // using the `SetItems` method until we get a group HIT. - if (grouping.BeatmapSetsGroupedTogether) - visibleSets = grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().ToArray(); - else - { - // Note that this is probably not correct in all cases. - // When we aren't grouping sets together, we might want to randomise by beatmaps, not sets. - // - // Imagine the scenario where a single beatmap set has multiple difficulties in the same difficulty grouping, where this - // would always choose the set's user recommended difficulty rather than the visible ones. - visibleSets = grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().Select(b => b.BeatmapSet!).Distinct().ToArray(); - } - } + ? grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().ToArray() + : GetCarouselItems()!.Select(i => i.Model).OfType().ToArray(); - if (CurrentSelection is BeatmapInfo beatmapInfo) + BeatmapInfo beatmap; + + switch (randomAlgorithm.Value) { - randomSelectedBeatmaps.Add(beatmapInfo); + case RandomSelectAlgorithm.RandomPermutation: + { + ICollection notYetVisitedBeatmaps = visibleBeatmaps.Except(previouslyVisitedRandomBeatmaps).ToList(); - // when performing a random, we want to add the current set to the previously visited list - // else the user may be "randomised" to the existing selection. - if (previouslyVisitedRandomSets.LastOrDefault()?.Equals(beatmapInfo.BeatmapSet) != true) - previouslyVisitedRandomSets.Add(beatmapInfo.BeatmapSet!); + if (!notYetVisitedBeatmaps.Any()) + { + previouslyVisitedRandomBeatmaps.RemoveAll(b => visibleBeatmaps.Contains(b)); + notYetVisitedBeatmaps = visibleBeatmaps; + if (CurrentSelection is BeatmapInfo beatmapInfo) + notYetVisitedBeatmaps = notYetVisitedBeatmaps.Except([beatmapInfo]).ToList(); + } + + if (notYetVisitedBeatmaps.Count == 0) + return false; + + beatmap = notYetVisitedBeatmaps.ElementAt(RNG.Next(notYetVisitedBeatmaps.Count)); + break; + } + + case RandomSelectAlgorithm.Random: + beatmap = visibleBeatmaps.ElementAt(RNG.Next(visibleBeatmaps.Count)); + break; + + default: + throw new ArgumentOutOfRangeException(); } + RequestSelection(beatmap); + return true; + } + + private bool nextRandomSet() + { + ICollection visibleSets = ExpandedGroup != null + // In the case of grouping, users expect random to only operate on the expanded group. + // This is going to incur some overhead as we don't have a group-beatmapset mapping currently. + // + // If this becomes an issue, we could either store a mapping, or run the random algorithm many times + // using the `SetItems` method until we get a group HIT. + ? grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType().ToArray() + // This is the fastest way to retrieve sets for randomisation. + : grouping.SetItems.Keys; + BeatmapSetInfo set; - if (randomAlgorithm.Value == RandomSelectAlgorithm.RandomPermutation) + switch (randomAlgorithm.Value) { - ICollection notYetVisitedSets = visibleSets.Except(previouslyVisitedRandomSets).ToList(); - - if (!notYetVisitedSets.Any()) + case RandomSelectAlgorithm.RandomPermutation: { - previouslyVisitedRandomSets.RemoveAll(s => visibleSets.Contains(s)); - notYetVisitedSets = visibleSets; + ICollection notYetVisitedSets = visibleSets.Except(previouslyVisitedRandomBeatmaps.Select(b => b.BeatmapSet!)).ToList(); + + if (!notYetVisitedSets.Any()) + { + previouslyVisitedRandomBeatmaps.RemoveAll(b => visibleSets.Contains(b.BeatmapSet!)); + notYetVisitedSets = visibleSets; + if (CurrentSelection is BeatmapInfo beatmapInfo) + notYetVisitedSets = notYetVisitedSets.Except([beatmapInfo.BeatmapSet!]).ToList(); + } + + if (notYetVisitedSets.Count == 0) + return false; + + set = notYetVisitedSets.ElementAt(RNG.Next(notYetVisitedSets.Count)); + break; } - set = notYetVisitedSets.ElementAt(RNG.Next(notYetVisitedSets.Count)); - previouslyVisitedRandomSets.Add(set); - } - else - set = visibleSets.ElementAt(RNG.Next(visibleSets.Count)); + case RandomSelectAlgorithm.Random: + set = visibleSets.ElementAt(RNG.Next(visibleSets.Count)); + break; - if (CurrentSelectionItem != null) - playSpinSample(distanceBetween(carouselItems.First(i => !ReferenceEquals(i.Model, set)), CurrentSelectionItem), visibleSets.Count); + default: + throw new ArgumentOutOfRangeException(); + } selectRecommendedDifficultyForBeatmapSet(set); return true; @@ -696,10 +766,10 @@ namespace osu.Game.Screens.SelectV2 if (carouselItems?.Any() != true) return; - while (randomSelectedBeatmaps.Any()) + while (randomHistory.Any()) { - var previousBeatmap = randomSelectedBeatmaps[^1]; - randomSelectedBeatmaps.RemoveAt(randomSelectedBeatmaps.Count - 1); + var previousBeatmap = randomHistory[^1]; + randomHistory.RemoveAt(randomHistory.Count - 1); var previousBeatmapItem = carouselItems.FirstOrDefault(i => i.Model is BeatmapInfo b && b.Equals(previousBeatmap)); @@ -709,7 +779,7 @@ namespace osu.Game.Screens.SelectV2 if (CurrentSelection is BeatmapInfo beatmapInfo) { if (randomAlgorithm.Value == RandomSelectAlgorithm.RandomPermutation) - previouslyVisitedRandomSets.Remove(beatmapInfo.BeatmapSet!); + previouslyVisitedRandomBeatmaps.Remove(beatmapInfo); if (CurrentSelectionItem == null) playSpinSample(0, carouselItems.Count); From 873d62291899cfb55605995209c10117b1365b44 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 19 Jun 2025 16:57:32 +0900 Subject: [PATCH 3/7] Fix global beatmap validity potentially being checked on a stale beatmap Noticed in a flaky test with the changes to random, where the debounce may be delayed to the point of thinking the selection is invalid even though it's valid (timing woes, I cannot explain in words but I would highly recommend smiling and nodding approach). --- osu.Game/Screens/SelectV2/SongSelect.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/SelectV2/SongSelect.cs b/osu.Game/Screens/SelectV2/SongSelect.cs index fc15090a5b..58de111324 100644 --- a/osu.Game/Screens/SelectV2/SongSelect.cs +++ b/osu.Game/Screens/SelectV2/SongSelect.cs @@ -467,6 +467,9 @@ namespace osu.Game.Screens.SelectV2 if (!this.IsCurrentScreen()) return false; + if (selectionDebounce?.State == ScheduledDelegate.RunState.Waiting) + selectionDebounce?.RunTask(); + // While filtering, let's not ever attempt to change selection. // This will be resolved after the filter completes, see `newItemsPresented`. bool carouselStateIsValid = filterDebounce?.State != ScheduledDelegate.RunState.Waiting && !carousel.IsFiltering; From a1ec71e677a35a4fb453340034db705a6ac629e4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 24 Jun 2025 17:48:38 +0900 Subject: [PATCH 4/7] Fix potential crash when attempting random selection after changing grouping mode --- .../TestSceneBeatmapCarouselRandom.cs | 17 +++++++++++++++++ osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 9 +++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs index 739fc23ed5..b7e169964d 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs @@ -37,6 +37,23 @@ namespace osu.Game.Tests.Visual.SongSelectV2 } } + [Test] + public void TestGroupingModeChangeStillWorks() + { + SortAndGroupBy(SortMode.Artist, GroupMode.Artist); + AddBeatmaps(10, 3, true); + WaitForDrawablePanels(); + + nextRandom(); + ensureRandomDidNotRepeat(); + + SortAndGroupBy(SortMode.Artist, GroupMode.None); + WaitForFiltering(); + + nextRandom(); + ensureRandomDidNotRepeat(); + } + /// /// Test random non-repeating algorithm /// diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index cd5ab68e6f..ccd7e52ed1 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -289,9 +289,14 @@ namespace osu.Game.Screens.SelectV2 // This will update the visual state of the selected item. HandleItemSelected(CurrentSelection); - // If a group was selected that is not the one containing the selection, reselect it. + // If a group was selected that is not the one containing the selection, attempt to reselect it. if (groupForReselection != null) - setExpandedGroup(groupForReselection); + { + if (!grouping.GroupItems.TryGetValue(groupForReselection, out _)) + ExpandedGroup = null; + else + setExpandedGroup(groupForReselection); + } } private void selectRecommendedDifficultyForBeatmapSet(BeatmapSetInfo beatmapSet) From 7b6ecbd10b4bab72ab8a919a7d0399f126dc27e6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 24 Jun 2025 19:21:04 +0900 Subject: [PATCH 5/7] Expand test to cover more potential weirdness and fix said weirdness --- .../TestSceneBeatmapCarouselRandom.cs | 23 ++++++++++++++++--- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 10 +++----- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs index b7e169964d..ef142e6253 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs @@ -40,6 +40,9 @@ namespace osu.Game.Tests.Visual.SongSelectV2 [Test] public void TestGroupingModeChangeStillWorks() { + BeatmapInfo originalSelected = null!; + GroupDefinition? expanded = null; + SortAndGroupBy(SortMode.Artist, GroupMode.Artist); AddBeatmaps(10, 3, true); WaitForDrawablePanels(); @@ -47,11 +50,25 @@ namespace osu.Game.Tests.Visual.SongSelectV2 nextRandom(); ensureRandomDidNotRepeat(); - SortAndGroupBy(SortMode.Artist, GroupMode.None); + AddStep("store selection", () => originalSelected = (BeatmapInfo)Carousel.CurrentSelection!); + + SortAndGroupBy(SortMode.Artist, GroupMode.Difficulty); WaitForFiltering(); - nextRandom(); - ensureRandomDidNotRepeat(); + AddAssert("selection not changed", () => Carousel.CurrentSelection, () => Is.EqualTo(originalSelected)); + + storeExpandedGroup(); + + for (int i = 0; i < 5; i++) + { + nextRandom(); + ensureRandomDidNotRepeat(); + checkExpandedGroupUnchanged(); + } + + void storeExpandedGroup() => AddStep("store open group", () => expanded = Carousel.ExpandedGroup); + + void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); } /// diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index ccd7e52ed1..c5fb363f3b 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -290,13 +290,9 @@ namespace osu.Game.Screens.SelectV2 HandleItemSelected(CurrentSelection); // If a group was selected that is not the one containing the selection, attempt to reselect it. - if (groupForReselection != null) - { - if (!grouping.GroupItems.TryGetValue(groupForReselection, out _)) - ExpandedGroup = null; - else - setExpandedGroup(groupForReselection); - } + // 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); } private void selectRecommendedDifficultyForBeatmapSet(BeatmapSetInfo beatmapSet) From 0de964b10b461609fdec36eb46c99116e3305d7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 24 Jun 2025 13:32:42 +0200 Subject: [PATCH 6/7] Expand test even further to cover even more potential weirdness --- .../SongSelectV2/TestSceneBeatmapCarouselRandom.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs index ef142e6253..ed694c9e3d 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselRandom.cs @@ -66,6 +66,15 @@ namespace osu.Game.Tests.Visual.SongSelectV2 checkExpandedGroupUnchanged(); } + SortAndGroupBy(SortMode.Artist, GroupMode.None); + WaitForFiltering(); + + for (int i = 0; i < 5; i++) + { + nextRandom(); + ensureRandomDidNotRepeat(); + } + void storeExpandedGroup() => AddStep("store open group", () => expanded = Carousel.ExpandedGroup); void checkExpandedGroupUnchanged() => AddAssert("expanded did not change", () => Carousel.ExpandedGroup, () => Is.EqualTo(expanded)); From 8781901d6bc691a44d99e8bde1f0475afb44b79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 24 Jun 2025 13:38:32 +0200 Subject: [PATCH 7/7] Ensure expanded group is cleared when grouping is turned off --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index c5fb363f3b..e19cdd20c7 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -267,8 +267,7 @@ namespace osu.Game.Screens.SelectV2 // Find any containing group. There should never be too many groups so iterating is efficient enough. GroupDefinition? containingGroup = grouping.GroupItems.SingleOrDefault(kvp => kvp.Value.Any(i => CheckModelEquality(i.Model, beatmapInfo))).Key; - if (containingGroup != null) - setExpandedGroup(containingGroup); + setExpandedGroup(containingGroup); if (grouping.BeatmapSetsGroupedTogether) setExpandedSet(beatmapInfo); @@ -362,8 +361,11 @@ namespace osu.Game.Screens.SelectV2 { if (ExpandedGroup != null) setExpansionStateOfGroup(ExpandedGroup, false); + ExpandedGroup = group; - setExpansionStateOfGroup(group, true); + + if (ExpandedGroup != null) + setExpansionStateOfGroup(group, true); } private void setExpansionStateOfGroup(GroupDefinition group, bool expanded)