Merge pull request #6414 from peppy/fix-song-select-sort-when-filtering

Fix beatmap carousel including filtered items in sort calculations

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

@@ -58,107 +58,6 @@ namespace osu.Game.Tests.Visual.SongSelect
             });
         }
 
-        private void loadBeatmaps(List<BeatmapSetInfo> beatmapSets = null)
-        {
-            if (beatmapSets == null)
-            {
-                beatmapSets = new List<BeatmapSetInfo>();
-
-                for (int i = 1; i <= set_count; i++)
-                    beatmapSets.Add(createTestBeatmapSet(i));
-            }
-
-            bool changed = false;
-            AddStep($"Load {beatmapSets.Count} Beatmaps", () =>
-            {
-                carousel.Filter(new FilterCriteria());
-                carousel.BeatmapSetsChanged = () => changed = true;
-                carousel.BeatmapSets = beatmapSets;
-            });
-
-            AddUntilStep("Wait for load", () => changed);
-        }
-
-        private void ensureRandomFetchSuccess() =>
-            AddAssert("ensure prev random fetch worked", () => selectedSets.Peek() == carousel.SelectedBeatmapSet);
-
-        private void waitForSelection(int set, int? diff = null) =>
-            AddUntilStep($"selected is set{set}{(diff.HasValue ? $" diff{diff.Value}" : "")}", () =>
-            {
-                if (diff != null)
-                    return carousel.SelectedBeatmap == carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff.Value - 1).First();
-
-                return carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Contains(carousel.SelectedBeatmap);
-            });
-
-        private void setSelected(int set, int diff) =>
-            AddStep($"select set{set} diff{diff}", () =>
-                carousel.SelectBeatmap(carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First()));
-
-        private void advanceSelection(bool diff, int direction = 1, int count = 1)
-        {
-            if (count == 1)
-                AddStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () =>
-                    carousel.SelectNext(direction, !diff));
-            else
-            {
-                AddRepeatStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () =>
-                    carousel.SelectNext(direction, !diff), count);
-            }
-        }
-
-        private void checkVisibleItemCount(bool diff, int count) =>
-            AddAssert($"{count} {(diff ? "diffs" : "sets")} visible", () =>
-                carousel.Items.Count(s => (diff ? s.Item is CarouselBeatmap : s.Item is CarouselBeatmapSet) && s.Item.Visible) == count);
-
-        private void checkNoSelection() => AddAssert("Selection is null", () => currentSelection == null);
-
-        private void nextRandom() =>
-            AddStep("select random next", () =>
-            {
-                carousel.RandomAlgorithm.Value = RandomSelectAlgorithm.RandomPermutation;
-
-                if (!selectedSets.Any() && carousel.SelectedBeatmap != null)
-                    selectedSets.Push(carousel.SelectedBeatmapSet);
-
-                carousel.SelectNextRandom();
-                selectedSets.Push(carousel.SelectedBeatmapSet);
-            });
-
-        private void ensureRandomDidntRepeat() =>
-            AddAssert("ensure no repeats", () => selectedSets.Distinct().Count() == selectedSets.Count);
-
-        private void prevRandom() => AddStep("select random last", () =>
-        {
-            carousel.SelectPreviousRandom();
-            selectedSets.Pop();
-        });
-
-        private bool selectedBeatmapVisible()
-        {
-            var currentlySelected = carousel.Items.Find(s => s.Item is CarouselBeatmap && s.Item.State.Value == CarouselItemState.Selected);
-            if (currentlySelected == null)
-                return true;
-
-            return currentlySelected.Item.Visible;
-        }
-
-        private void checkInvisibleDifficultiesUnselectable()
-        {
-            nextRandom();
-            AddAssert("Selection is visible", selectedBeatmapVisible);
-        }
-
-        private void checkNonmatchingFilter()
-        {
-            AddStep("Toggle non-matching filter", () =>
-            {
-                carousel.Filter(new FilterCriteria { SearchText = "Dingo" }, false);
-                carousel.Filter(new FilterCriteria(), false);
-                eagerSelectedIDs.Add(carousel.SelectedBeatmapSet.ID);
-            });
-        }
-
         /// <summary>
         /// Test keyboard traversal
         /// </summary>
@@ -346,6 +245,30 @@ namespace osu.Game.Tests.Visual.SongSelect
             AddAssert($"Check #{set_count} is at bottom", () => carousel.BeatmapSets.Last().Metadata.Title.EndsWith($"#{set_count}!"));
         }
 
+        [Test]
+        public void TestSortingWithFiltered()
+        {
+            List<BeatmapSetInfo> sets = new List<BeatmapSetInfo>();
+
+            for (int i = 0; i < 3; i++)
+            {
+                var set = createTestBeatmapSet(i);
+                set.Beatmaps[0].StarDifficulty = 3 - i;
+                set.Beatmaps[2].StarDifficulty = 6 + i;
+                sets.Add(set);
+            }
+
+            loadBeatmaps(sets);
+
+            AddStep("Filter to normal", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Difficulty, SearchText = "Normal" }, false));
+            AddAssert("Check first set at end", () => carousel.BeatmapSets.First() == sets.Last());
+            AddAssert("Check last set at start", () => carousel.BeatmapSets.Last() == sets.First());
+
+            AddStep("Filter to insane", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Difficulty, SearchText = "Insane" }, false));
+            AddAssert("Check first set at start", () => carousel.BeatmapSets.First() == sets.First());
+            AddAssert("Check last set at end", () => carousel.BeatmapSets.Last() == sets.Last());
+        }
+
         [Test]
         public void TestRemoveAll()
         {
@@ -482,6 +405,107 @@ namespace osu.Game.Tests.Visual.SongSelect
             AddAssert("Selection was random", () => eagerSelectedIDs.Count > 1);
         }
 
+        private void loadBeatmaps(List<BeatmapSetInfo> beatmapSets = null)
+        {
+            if (beatmapSets == null)
+            {
+                beatmapSets = new List<BeatmapSetInfo>();
+
+                for (int i = 1; i <= set_count; i++)
+                    beatmapSets.Add(createTestBeatmapSet(i));
+            }
+
+            bool changed = false;
+            AddStep($"Load {beatmapSets.Count} Beatmaps", () =>
+            {
+                carousel.Filter(new FilterCriteria());
+                carousel.BeatmapSetsChanged = () => changed = true;
+                carousel.BeatmapSets = beatmapSets;
+            });
+
+            AddUntilStep("Wait for load", () => changed);
+        }
+
+        private void ensureRandomFetchSuccess() =>
+            AddAssert("ensure prev random fetch worked", () => selectedSets.Peek() == carousel.SelectedBeatmapSet);
+
+        private void waitForSelection(int set, int? diff = null) =>
+            AddUntilStep($"selected is set{set}{(diff.HasValue ? $" diff{diff.Value}" : "")}", () =>
+            {
+                if (diff != null)
+                    return carousel.SelectedBeatmap == carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff.Value - 1).First();
+
+                return carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Contains(carousel.SelectedBeatmap);
+            });
+
+        private void setSelected(int set, int diff) =>
+            AddStep($"select set{set} diff{diff}", () =>
+                carousel.SelectBeatmap(carousel.BeatmapSets.Skip(set - 1).First().Beatmaps.Skip(diff - 1).First()));
+
+        private void advanceSelection(bool diff, int direction = 1, int count = 1)
+        {
+            if (count == 1)
+                AddStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () =>
+                    carousel.SelectNext(direction, !diff));
+            else
+            {
+                AddRepeatStep($"select {(direction > 0 ? "next" : "prev")} {(diff ? "diff" : "set")}", () =>
+                    carousel.SelectNext(direction, !diff), count);
+            }
+        }
+
+        private void checkVisibleItemCount(bool diff, int count) =>
+            AddAssert($"{count} {(diff ? "diffs" : "sets")} visible", () =>
+                carousel.Items.Count(s => (diff ? s.Item is CarouselBeatmap : s.Item is CarouselBeatmapSet) && s.Item.Visible) == count);
+
+        private void checkNoSelection() => AddAssert("Selection is null", () => currentSelection == null);
+
+        private void nextRandom() =>
+            AddStep("select random next", () =>
+            {
+                carousel.RandomAlgorithm.Value = RandomSelectAlgorithm.RandomPermutation;
+
+                if (!selectedSets.Any() && carousel.SelectedBeatmap != null)
+                    selectedSets.Push(carousel.SelectedBeatmapSet);
+
+                carousel.SelectNextRandom();
+                selectedSets.Push(carousel.SelectedBeatmapSet);
+            });
+
+        private void ensureRandomDidntRepeat() =>
+            AddAssert("ensure no repeats", () => selectedSets.Distinct().Count() == selectedSets.Count);
+
+        private void prevRandom() => AddStep("select random last", () =>
+        {
+            carousel.SelectPreviousRandom();
+            selectedSets.Pop();
+        });
+
+        private bool selectedBeatmapVisible()
+        {
+            var currentlySelected = carousel.Items.Find(s => s.Item is CarouselBeatmap && s.Item.State.Value == CarouselItemState.Selected);
+            if (currentlySelected == null)
+                return true;
+
+            return currentlySelected.Item.Visible;
+        }
+
+        private void checkInvisibleDifficultiesUnselectable()
+        {
+            nextRandom();
+            AddAssert("Selection is visible", selectedBeatmapVisible);
+        }
+
+        private void checkNonmatchingFilter()
+        {
+            AddStep("Toggle non-matching filter", () =>
+            {
+                carousel.Filter(new FilterCriteria { SearchText = "Dingo" }, false);
+                carousel.Filter(new FilterCriteria(), false);
+                eagerSelectedIDs.Add(carousel.SelectedBeatmapSet.ID);
+            });
+        }
+
         private BeatmapSetInfo createTestBeatmapSet(int id)
         {
             return new BeatmapSetInfo

osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs

@@ -49,16 +49,33 @@ namespace osu.Game.Screens.Select.Carousel
                     return otherSet.BeatmapSet.DateAdded.CompareTo(BeatmapSet.DateAdded);
 
                 case SortMode.BPM:
-                    return BeatmapSet.MaxBPM.CompareTo(otherSet.BeatmapSet.MaxBPM);
+                    return compareUsingAggregateMax(otherSet, b => b.BPM);
 
                 case SortMode.Length:
-                    return BeatmapSet.MaxLength.CompareTo(otherSet.BeatmapSet.MaxLength);
+                    return compareUsingAggregateMax(otherSet, b => b.Length);
 
                 case SortMode.Difficulty:
-                    return BeatmapSet.MaxStarDifficulty.CompareTo(otherSet.BeatmapSet.MaxStarDifficulty);
+                    return compareUsingAggregateMax(otherSet, b => b.StarDifficulty);
             }
         }
 
+        /// <summary>
+        /// All beatmaps which are not filtered and valid for display.
+        /// </summary>
+        protected IEnumerable<BeatmapInfo> ValidBeatmaps => Beatmaps.Where(b => !b.Filtered.Value).Select(b => b.Beatmap);
+
+        private int compareUsingAggregateMax(CarouselBeatmapSet other, Func<BeatmapInfo, double> func)
+        {
+            var ourBeatmaps = ValidBeatmaps.Any();
+            var otherBeatmaps = other.ValidBeatmaps.Any();
+
+            if (!ourBeatmaps && !otherBeatmaps) return 0;
+            if (!ourBeatmaps) return -1;
+            if (!otherBeatmaps) return 1;
+
+            return ValidBeatmaps.Max(func).CompareTo(other.ValidBeatmaps.Max(func));
+        }
+
         public override void Filter(FilterCriteria criteria)
         {
             base.Filter(criteria);

osu.Game/Screens/Select/Carousel/CarouselGroup.cs

@@ -83,8 +83,8 @@ namespace osu.Game.Screens.Select.Carousel
 
             var children = new List<CarouselItem>(InternalChildren);
 
-            children.Sort((x, y) => x.CompareTo(criteria, y));
             children.ForEach(c => c.Filter(criteria));
+            children.Sort((x, y) => x.CompareTo(criteria, y));
 
             InternalChildren = children;
         }