Merge pull request #8481 from LittleEndu/rewrite-select-next

Rewrite beatmap carousel's select next logic to not use drawables

osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs

@@ -83,6 +83,82 @@ namespace osu.Game.Tests.Visual.SongSelect
             waitForSelection(set_count, 3);
         }
 
+        [TestCase(true)]
+        [TestCase(false)]
+        public void TestTraversalBeyondVisible(bool forwards)
+        {
+            var sets = new List<BeatmapSetInfo>();
+
+            const int total_set_count = 200;
+
+            for (int i = 0; i < total_set_count; i++)
+                sets.Add(createTestBeatmapSet(i + 1));
+
+            loadBeatmaps(sets);
+
+            for (int i = 1; i < total_set_count; i += i)
+                selectNextAndAssert(i);
+
+            void selectNextAndAssert(int amount)
+            {
+                setSelected(forwards ? 1 : total_set_count, 1);
+
+                AddStep($"{(forwards ? "Next" : "Previous")} beatmap {amount} times", () =>
+                {
+                    for (int i = 0; i < amount; i++)
+                    {
+                        carousel.SelectNext(forwards ? 1 : -1);
+                    }
+                });
+
+                waitForSelection(forwards ? amount + 1 : total_set_count - amount);
+            }
+        }
+
+        [Test]
+        public void TestTraversalBeyondVisibleDifficulties()
+        {
+            var sets = new List<BeatmapSetInfo>();
+
+            const int total_set_count = 20;
+
+            for (int i = 0; i < total_set_count; i++)
+                sets.Add(createTestBeatmapSet(i + 1));
+
+            loadBeatmaps(sets);
+
+            // Selects next set once, difficulty index doesn't change
+            selectNextAndAssert(3, true, 2, 1);
+
+            // Selects next set 16 times (50 \ 3 == 16), difficulty index changes twice (50 % 3 == 2)
+            selectNextAndAssert(50, true, 17, 3);
+
+            // Travels around the carousel thrice (200 \ 60 == 3)
+            // continues to select 20 times (200 \ 60 == 20)
+            // selects next set 6 times (20 \ 3 == 6)
+            // difficulty index changes twice (20 % 3 == 2)
+            selectNextAndAssert(200, true, 7, 3);
+
+            // All same but in reverse
+            selectNextAndAssert(3, false, 19, 3);
+            selectNextAndAssert(50, false, 4, 1);
+            selectNextAndAssert(200, false, 14, 1);
+
+            void selectNextAndAssert(int amount, bool forwards, int expectedSet, int expectedDiff)
+            {
+                // Select very first or very last difficulty
+                setSelected(forwards ? 1 : 20, forwards ? 1 : 3);
+
+                AddStep($"{(forwards ? "Next" : "Previous")} difficulty {amount} times", () =>
+                {
+                    for (int i = 0; i < amount; i++)
+                        carousel.SelectNext(forwards ? 1 : -1, false);
+                });
+
+                waitForSelection(expectedSet, expectedDiff);
+            }
+        }
+
         /// <summary>
         /// Test filtering
         /// </summary>

osu.Game/Screens/Select/BeatmapCarousel.cs

@@ -253,46 +253,37 @@ namespace osu.Game.Screens.Select
         /// <param name="skipDifficulties">Whether to skip individual difficulties and only increment over full groups.</param>
         public void SelectNext(int direction = 1, bool skipDifficulties = true)
         {
-            var visibleItems = Items.Where(s => !s.Item.Filtered.Value).ToList();
-
-            if (!visibleItems.Any())
+            if (beatmapSets.All(s => s.Filtered.Value))
                 return;
 
-            DrawableCarouselItem drawable = null;
+            if (skipDifficulties)
+                selectNextSet(direction, true);
+            else
+                selectNextDifficulty(direction);
+        }
 
-            if (selectedBeatmap != null && (drawable = selectedBeatmap.Drawables.FirstOrDefault()) == null)
-                // if the selected beatmap isn't present yet, we can't correctly change selection.
-                // we can fix this by changing this method to not reference drawables / Items in the first place.
-                return;
+        private void selectNextSet(int direction, bool skipDifficulties)
+        {
+            var unfilteredSets = beatmapSets.Where(s => !s.Filtered.Value).ToList();
 
-            int originalIndex = visibleItems.IndexOf(drawable);
-            int currentIndex = originalIndex;
+            var nextSet = unfilteredSets[(unfilteredSets.IndexOf(selectedBeatmapSet) + direction + unfilteredSets.Count) % unfilteredSets.Count];
 
-            // local function to increment the index in the required direction, wrapping over extremities.
-            int incrementIndex() => currentIndex = (currentIndex + direction + visibleItems.Count) % visibleItems.Count;
+            if (skipDifficulties)
+                select(nextSet);
+            else
+                select(direction > 0 ? nextSet.Beatmaps.First(b => !b.Filtered.Value) : nextSet.Beatmaps.Last(b => !b.Filtered.Value));
+        }
 
-            while (incrementIndex() != originalIndex)
-            {
-                var item = visibleItems[currentIndex].Item;
+        private void selectNextDifficulty(int direction)
+        {
+            var unfilteredDifficulties = selectedBeatmapSet.Children.Where(s => !s.Filtered.Value).ToList();
 
-                if (item.Filtered.Value || item.State.Value == CarouselItemState.Selected) continue;
+            int index = unfilteredDifficulties.IndexOf(selectedBeatmap);
 
-                switch (item)
-                {
-                    case CarouselBeatmap beatmap:
-                        if (skipDifficulties) continue;
-
-                        select(beatmap);
-                        return;
-
-                    case CarouselBeatmapSet set:
-                        if (skipDifficulties)
-                            select(set);
-                        else
-                            select(direction > 0 ? set.Beatmaps.First(b => !b.Filtered.Value) : set.Beatmaps.Last(b => !b.Filtered.Value));
-                        return;
-                }
-            }
+            if (index + direction < 0 || index + direction >= unfilteredDifficulties.Count)
+                selectNextSet(direction, false);
+            else
+                select(unfilteredDifficulties[index + direction]);
         }
 
         /// <summary>