From 259be976e870db1fcc6f64f1382a23be02919ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 21 Feb 2024 11:42:34 +0100 Subject: [PATCH 1/5] Adjust test to fail --- .../SongSelect/TestSceneBeatmapCarousel.cs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index aa4c879468e4..de2ae3708faa 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -629,7 +629,8 @@ public void TestSorting() { var sets = new List(); - const string zzz_string = "zzzzz"; + const string zzz_lowercase = "zzzzz"; + const string zzz_uppercase = "ZZZZZ"; AddStep("Populuate beatmap sets", () => { @@ -640,10 +641,16 @@ public void TestSorting() var set = TestResources.CreateTestBeatmapSetInfo(); if (i == 4) - set.Beatmaps.ForEach(b => b.Metadata.Artist = zzz_string); + set.Beatmaps.ForEach(b => b.Metadata.Artist = zzz_uppercase); + + if (i == 8) + set.Beatmaps.ForEach(b => b.Metadata.Artist = zzz_lowercase); + + if (i == 12) + set.Beatmaps.ForEach(b => b.Metadata.Author.Username = zzz_uppercase); if (i == 16) - set.Beatmaps.ForEach(b => b.Metadata.Author.Username = zzz_string); + set.Beatmaps.ForEach(b => b.Metadata.Author.Username = zzz_lowercase); sets.Add(set); } @@ -652,9 +659,11 @@ public void TestSorting() loadBeatmaps(sets); AddStep("Sort by author", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Author }, false)); - AddAssert($"Check {zzz_string} is at bottom", () => carousel.BeatmapSets.Last().Metadata.Author.Username == zzz_string); + AddAssert($"Check {zzz_uppercase} is last", () => carousel.BeatmapSets.Last().Metadata.Author.Username == zzz_uppercase); + AddAssert($"Check {zzz_lowercase} is second last", () => carousel.BeatmapSets.SkipLast(1).Last().Metadata.Author.Username == zzz_lowercase); AddStep("Sort by artist", () => carousel.Filter(new FilterCriteria { Sort = SortMode.Artist }, false)); - AddAssert($"Check {zzz_string} is at bottom", () => carousel.BeatmapSets.Last().Metadata.Artist == zzz_string); + AddAssert($"Check {zzz_uppercase} is last", () => carousel.BeatmapSets.Last().Metadata.Artist == zzz_uppercase); + AddAssert($"Check {zzz_lowercase} is second last", () => carousel.BeatmapSets.SkipLast(1).Last().Metadata.Artist == zzz_lowercase); } /// From 59235d6c50a796469bb29440745eec9e13d0e926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 21 Feb 2024 12:07:18 +0100 Subject: [PATCH 2/5] Implement custom comparer for expected carousel sort behaviour Co-authored-by: Salman Ahmed --- .../Utils/OrdinalSortByCaseStringComparer.cs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 osu.Game/Utils/OrdinalSortByCaseStringComparer.cs diff --git a/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs b/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs new file mode 100644 index 000000000000..99d73f644f8d --- /dev/null +++ b/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs @@ -0,0 +1,49 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Collections.Generic; + +namespace osu.Game.Utils +{ + /// + /// This string comparer is something of a cross-over between and . + /// is used first, but is used as a tie-breaker. + /// + /// + /// This comparer's behaviour somewhat emulates , + /// but non-ordinal comparers - both culture-aware and culture-invariant - have huge performance overheads due to i18n factors (up to 5x slower). + /// + /// + /// Given the following strings to sort: [A, B, C, D, a, b, c, d, A] and a stable sorting algorithm: + /// + /// + /// would return [A, A, B, C, D, a, b, c, d]. + /// This is undesirable as letters are interleaved. + /// + /// + /// would return [A, a, A, B, b, C, c, D, d]. + /// Different letters are not interleaved, but because case is ignored, the As are left in arbitrary order. + /// + /// + /// + /// would return [a, A, A, b, B, c, C, d, D], which is the expected behaviour. + /// + /// + public class OrdinalSortByCaseStringComparer : IComparer + { + public static readonly OrdinalSortByCaseStringComparer INSTANCE = new OrdinalSortByCaseStringComparer(); + + private OrdinalSortByCaseStringComparer() + { + } + + public int Compare(string? a, string? b) + { + int result = StringComparer.OrdinalIgnoreCase.Compare(a, b); + if (result == 0) + result = -StringComparer.Ordinal.Compare(a, b); // negative to place lowercase letters before uppercase. + return result; + } + } +} From 04a2ac3df332fda91ae41c2ee9bc5e1300a60d36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 21 Feb 2024 12:07:28 +0100 Subject: [PATCH 3/5] Add benchmarking for custom string comparer --- .../BenchmarkStringComparison.cs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 osu.Game.Benchmarks/BenchmarkStringComparison.cs diff --git a/osu.Game.Benchmarks/BenchmarkStringComparison.cs b/osu.Game.Benchmarks/BenchmarkStringComparison.cs new file mode 100644 index 000000000000..78e4130abe30 --- /dev/null +++ b/osu.Game.Benchmarks/BenchmarkStringComparison.cs @@ -0,0 +1,48 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Collections.Generic; +using BenchmarkDotNet.Attributes; +using osu.Game.Utils; + +namespace osu.Game.Benchmarks +{ + public class BenchmarkStringComparison + { + private string[] strings = null!; + + [GlobalSetup] + public void GlobalSetUp() + { + strings = new string[10000]; + + for (int i = 0; i < strings.Length; ++i) + strings[i] = Guid.NewGuid().ToString(); + + for (int i = 0; i < strings.Length; ++i) + { + if (i % 2 == 0) + strings[i] = strings[i].ToUpperInvariant(); + } + } + + [Benchmark] + public void OrdinalIgnoreCase() => compare(StringComparer.OrdinalIgnoreCase); + + [Benchmark] + public void OrdinalSortByCase() => compare(OrdinalSortByCaseStringComparer.INSTANCE); + + [Benchmark] + public void InvariantCulture() => compare(StringComparer.InvariantCulture); + + private void compare(IComparer comparer) + { + for (int i = 0; i < strings.Length; ++i) + { + for (int j = i + 1; j < strings.Length; ++j) + _ = comparer.Compare(strings[i], strings[j]); + } + } + } +} From 929858226ad208e29746f1b22b80399623f07d52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 21 Feb 2024 12:09:37 +0100 Subject: [PATCH 4/5] Use custom comparer in beatmap carousel for expected sort behaviour --- osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs index 6c41bc3805f6..bf0d7dcbdefc 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs @@ -7,6 +7,7 @@ using osu.Framework.Extensions.IEnumerableExtensions; using osu.Game.Beatmaps; using osu.Game.Screens.Select.Filter; +using osu.Game.Utils; namespace osu.Game.Screens.Select.Carousel { @@ -67,19 +68,19 @@ public override int CompareTo(FilterCriteria criteria, CarouselItem other) { default: case SortMode.Artist: - comparison = string.Compare(BeatmapSet.Metadata.Artist, otherSet.BeatmapSet.Metadata.Artist, StringComparison.Ordinal); + comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Artist, otherSet.BeatmapSet.Metadata.Artist); break; case SortMode.Title: - comparison = string.Compare(BeatmapSet.Metadata.Title, otherSet.BeatmapSet.Metadata.Title, StringComparison.Ordinal); + comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Title, otherSet.BeatmapSet.Metadata.Title); break; case SortMode.Author: - comparison = string.Compare(BeatmapSet.Metadata.Author.Username, otherSet.BeatmapSet.Metadata.Author.Username, StringComparison.Ordinal); + comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Author.Username, otherSet.BeatmapSet.Metadata.Author.Username); break; case SortMode.Source: - comparison = string.Compare(BeatmapSet.Metadata.Source, otherSet.BeatmapSet.Metadata.Source, StringComparison.Ordinal); + comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Source, otherSet.BeatmapSet.Metadata.Source); break; case SortMode.DateAdded: From fb593470d553c68b77e7ec129c47cfd9a21097d0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 21 Feb 2024 21:02:20 +0800 Subject: [PATCH 5/5] Use `DEFAULT` instead of `INSTANCE` or static field Matches other similar comparers. --- osu.Game.Benchmarks/BenchmarkStringComparison.cs | 2 +- osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs | 8 ++++---- osu.Game/Utils/OrdinalSortByCaseStringComparer.cs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game.Benchmarks/BenchmarkStringComparison.cs b/osu.Game.Benchmarks/BenchmarkStringComparison.cs index 78e4130abe30..d40b92db5f06 100644 --- a/osu.Game.Benchmarks/BenchmarkStringComparison.cs +++ b/osu.Game.Benchmarks/BenchmarkStringComparison.cs @@ -31,7 +31,7 @@ public void GlobalSetUp() public void OrdinalIgnoreCase() => compare(StringComparer.OrdinalIgnoreCase); [Benchmark] - public void OrdinalSortByCase() => compare(OrdinalSortByCaseStringComparer.INSTANCE); + public void OrdinalSortByCase() => compare(OrdinalSortByCaseStringComparer.DEFAULT); [Benchmark] public void InvariantCulture() => compare(StringComparer.InvariantCulture); diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs index bf0d7dcbdefc..43c9c621e87a 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs @@ -68,19 +68,19 @@ public override int CompareTo(FilterCriteria criteria, CarouselItem other) { default: case SortMode.Artist: - comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Artist, otherSet.BeatmapSet.Metadata.Artist); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(BeatmapSet.Metadata.Artist, otherSet.BeatmapSet.Metadata.Artist); break; case SortMode.Title: - comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Title, otherSet.BeatmapSet.Metadata.Title); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(BeatmapSet.Metadata.Title, otherSet.BeatmapSet.Metadata.Title); break; case SortMode.Author: - comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Author.Username, otherSet.BeatmapSet.Metadata.Author.Username); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(BeatmapSet.Metadata.Author.Username, otherSet.BeatmapSet.Metadata.Author.Username); break; case SortMode.Source: - comparison = OrdinalSortByCaseStringComparer.INSTANCE.Compare(BeatmapSet.Metadata.Source, otherSet.BeatmapSet.Metadata.Source); + comparison = OrdinalSortByCaseStringComparer.DEFAULT.Compare(BeatmapSet.Metadata.Source, otherSet.BeatmapSet.Metadata.Source); break; case SortMode.DateAdded: diff --git a/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs b/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs index 99d73f644f8d..6c1532eef569 100644 --- a/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs +++ b/osu.Game/Utils/OrdinalSortByCaseStringComparer.cs @@ -32,7 +32,7 @@ namespace osu.Game.Utils /// public class OrdinalSortByCaseStringComparer : IComparer { - public static readonly OrdinalSortByCaseStringComparer INSTANCE = new OrdinalSortByCaseStringComparer(); + public static readonly OrdinalSortByCaseStringComparer DEFAULT = new OrdinalSortByCaseStringComparer(); private OrdinalSortByCaseStringComparer() {