Merge pull request #33773 from peppy/fix-carousel-random-not-respecting-group

SongSelectV2: Refine random selection to currently open group (and support difficulty split panels better)

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<BeatmapSetInfo> BeatmapSetRequestedSelections = new Stack<BeatmapSetInfo>();
+        protected readonly Stack<BeatmapInfo> BeatmapRequestedSelections = new Stack<BeatmapInfo>();
 
         protected readonly BindableList<BeatmapSetInfo> BeatmapSets = new BindableList<BeatmapSetInfo>();
 
@@ -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 =>

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
 {
@@ -36,6 +37,49 @@ 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();
+
+            nextRandom();
+            ensureRandomDidNotRepeat();
+
+            AddStep("store selection", () => originalSelected = (BeatmapInfo)Carousel.CurrentSelection!);
+
+            SortAndGroupBy(SortMode.Artist, GroupMode.Difficulty);
+            WaitForFiltering();
+
+            AddAssert("selection not changed", () => Carousel.CurrentSelection, () => Is.EqualTo(originalSelected));
+
+            storeExpandedGroup();
+
+            for (int i = 0; i < 5; i++)
+            {
+                nextRandom();
+                ensureRandomDidNotRepeat();
+                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));
+        }
+
         /// <summary>
         /// Test random non-repeating algorithm
         /// </summary>
@@ -47,57 +91,139 @@ namespace osu.Game.Tests.Visual.SongSelectV2
             AddBeatmaps(10, 3, true);
             WaitForDrawablePanels();
 
-            nextRandom();
-            ensureRandomDidNotRepeat();
-            nextRandom();
-            ensureRandomDidNotRepeat();
-            nextRandom();
-            ensureRandomDidNotRepeat();
+            GroupDefinition? expanded = null;
 
-            prevRandom();
+            for (int i = 0; i < 2; i++)
+            {
+                nextRandom();
+                expanded ??= storeExpandedGroup();
+
+                ensureSetRandomDidNotRepeat();
+                checkExpandedGroupUnchanged();
+            }
+
+            nextRandom();
+            ensureSetRandomDidRepeat();
+            checkExpandedGroupUnchanged();
+
+            prevRandomSet();
             checkRewindCorrectSet();
-            prevRandom();
+            checkExpandedGroupUnchanged();
+            prevRandomSet();
             checkRewindCorrectSet();
+            checkExpandedGroupUnchanged();
 
             nextRandom();
-            ensureRandomDidNotRepeat();
+            ensureSetRandomDidNotRepeat();
+            checkExpandedGroupUnchanged();
             nextRandom();
-            ensureRandomDidNotRepeat();
+            ensureSetRandomDidRepeat();
+            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));
         }
 
         /// <summary>
         /// Test random non-repeating algorithm
         /// </summary>
         [Test]
-        public void TestRandomDifficultyGrouping()
+        public void TestRandomDifficultyGroupingRewindsCorrectly()
         {
             SortAndGroupBy(SortMode.Difficulty, GroupMode.Difficulty);
 
-            AddBeatmaps(10, 3, true);
+            AddBeatmaps(3, 3, true);
             WaitForDrawablePanels();
 
-            nextRandom();
-            ensureRandomDidNotRepeat();
-            nextRandom();
-            ensureRandomDidNotRepeat();
-            nextRandom();
-            ensureRandomDidNotRepeat();
+            GroupDefinition? expanded = null;
 
-            prevRandom();
-            checkRewindCorrectSet();
-            prevRandom();
-            checkRewindCorrectSet();
+            for (int i = 0; i < 3; i++)
+            {
+                nextRandom();
+                expanded ??= storeExpandedGroup();
+
+                ensureRandomDidNotRepeat();
+                checkExpandedGroupUnchanged();
+            }
+
+            for (int i = 0; i < 2; i++)
+            {
+                prevRandom();
+                checkRewindCorrect();
+                checkExpandedGroupUnchanged();
+            }
+
+            for (int i = 0; i < 2; i++)
+            {
+                nextRandom();
+                ensureRandomDidNotRepeat();
+                checkExpandedGroupUnchanged();
+            }
 
             nextRandom();
-            ensureRandomDidNotRepeat();
-            nextRandom();
-            ensureRandomDidNotRepeat();
+            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));
+        }
+
+        /// <summary>
+        /// Test random non-repeating algorithm
+        /// </summary>
+        [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();
-            AddAssert("ensure repeat", () => BeatmapSetRequestedSelections.Contains(Carousel.SelectedBeatmapSet!));
+            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]
@@ -116,7 +242,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2
 
             for (int i = 0; i < random_select_count; i++)
             {
-                prevRandom();
+                prevRandomSet();
                 checkRewindCorrectSet();
             }
         }
@@ -167,20 +293,40 @@ 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));
         }
 
         private void nextRandom() =>
             AddStep("select random next", () => Carousel.NextRandom());
 
+        private void ensureRandomDidRepeat() =>
+            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();

osu.Game/Screens/SelectV2/BeatmapCarousel.cs

@@ -272,8 +272,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);
@@ -294,8 +293,9 @@ 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 (groupForReselection != null)
+            // 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);
         }
 
@@ -368,8 +368,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)
@@ -625,8 +628,8 @@ namespace osu.Game.Screens.SelectV2
         #region Random selection handling
 
         private readonly Bindable<RandomSelectAlgorithm> randomAlgorithm = new Bindable<RandomSelectAlgorithm>();
-        private readonly List<BeatmapSetInfo> previouslyVisitedRandomSets = new List<BeatmapSetInfo>();
-        private readonly List<BeatmapInfo> randomSelectedBeatmaps = new List<BeatmapInfo>();
+        private readonly List<BeatmapInfo> previouslyVisitedRandomBeatmaps = new List<BeatmapInfo>();
+        private readonly List<BeatmapInfo> randomHistory = new List<BeatmapInfo>();
 
         private Sample? spinSample;
         private Sample? randomSelectSample;
@@ -638,39 +641,129 @@ namespace osu.Game.Screens.SelectV2
             if (carouselItems?.Any() != true)
                 return false;
 
-            // This is the fastest way to retrieve sets for randomisation.
-            ICollection<BeatmapSetInfo> visibleSets = grouping.SetItems.Keys;
+            var selectionBefore = CurrentSelectionItem;
+            var beatmapBefore = selectionBefore?.Model as BeatmapInfo;
 
-            if (CurrentSelection is BeatmapInfo beatmapInfo)
+            bool success;
+
+            if (beatmapBefore != null)
             {
-                randomSelectedBeatmaps.Add(beatmapInfo);
-
-                // 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!);
+                // 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<BeatmapInfo> 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.
+                ? grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType<BeatmapInfo>().ToArray()
+                : GetCarouselItems()!.Select(i => i.Model).OfType<BeatmapInfo>().ToArray();
+
+            BeatmapInfo beatmap;
+
+            switch (randomAlgorithm.Value)
+            {
+                case RandomSelectAlgorithm.RandomPermutation:
+                {
+                    ICollection<BeatmapInfo> notYetVisitedBeatmaps = visibleBeatmaps.Except(previouslyVisitedRandomBeatmaps).ToList();
+
+                    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<BeatmapSetInfo> 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<BeatmapSetInfo>().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<BeatmapSetInfo> notYetVisitedSets = visibleSets.Except(previouslyVisitedRandomSets).ToList();
-
-                if (!notYetVisitedSets.Any())
+                case RandomSelectAlgorithm.RandomPermutation:
                 {
-                    previouslyVisitedRandomSets.RemoveAll(s => visibleSets.Contains(s));
-                    notYetVisitedSets = visibleSets;
+                    ICollection<BeatmapSetInfo> 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;
@@ -683,10 +776,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));
 
@@ -696,7 +789,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);

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;