Skip to content

Commit

Permalink
Merge pull request #27292 from bdach/carousel-sorting-again
Browse files Browse the repository at this point in the history
Fix beatmap carousel string carousel not matching expectations
  • Loading branch information
peppy authored Feb 21, 2024
2 parents cd1acf1 + fb59347 commit 67626e5
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 9 deletions.
48 changes: 48 additions & 0 deletions osu.Game.Benchmarks/BenchmarkStringComparison.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. 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.DEFAULT);

[Benchmark]
public void InvariantCulture() => compare(StringComparer.InvariantCulture);

private void compare(IComparer<string> comparer)
{
for (int i = 0; i < strings.Length; ++i)
{
for (int j = i + 1; j < strings.Length; ++j)
_ = comparer.Compare(strings[i], strings[j]);
}
}
}
}
19 changes: 14 additions & 5 deletions osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,8 @@ public void TestSorting()
{
var sets = new List<BeatmapSetInfo>();

const string zzz_string = "zzzzz";
const string zzz_lowercase = "zzzzz";
const string zzz_uppercase = "ZZZZZ";

AddStep("Populuate beatmap sets", () =>
{
Expand All @@ -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);
}
Expand All @@ -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);
}

/// <summary>
Expand Down
9 changes: 5 additions & 4 deletions osu.Game/Screens/Select/Carousel/CarouselBeatmapSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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.DEFAULT.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.DEFAULT.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.DEFAULT.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.DEFAULT.Compare(BeatmapSet.Metadata.Source, otherSet.BeatmapSet.Metadata.Source);
break;

case SortMode.DateAdded:
Expand Down
49 changes: 49 additions & 0 deletions osu.Game/Utils/OrdinalSortByCaseStringComparer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. 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
{
/// <summary>
/// This string comparer is something of a cross-over between <see cref="StringComparer.Ordinal"/> and <see cref="StringComparer.OrdinalIgnoreCase"/>.
/// <see cref="StringComparer.OrdinalIgnoreCase"/> is used first, but <see cref="StringComparer.Ordinal"/> is used as a tie-breaker.
/// </summary>
/// <remarks>
/// This comparer's behaviour somewhat emulates <see cref="StringComparer.InvariantCulture"/>,
/// but non-ordinal comparers - both culture-aware and culture-invariant - have huge performance overheads due to i18n factors (up to 5x slower).
/// </remarks>
/// <example>
/// Given the following strings to sort: <c>[A, B, C, D, a, b, c, d, A]</c> and a stable sorting algorithm:
/// <list type="bullet">
/// <item>
/// <see cref="StringComparer.Ordinal"/> would return <c>[A, A, B, C, D, a, b, c, d]</c>.
/// This is undesirable as letters are interleaved.
/// </item>
/// <item>
/// <see cref="StringComparer.OrdinalIgnoreCase"/> would return <c>[A, a, A, B, b, C, c, D, d]</c>.
/// Different letters are not interleaved, but because case is ignored, the As are left in arbitrary order.
/// </item>
/// </list>
/// <item>
/// <see cref="OrdinalSortByCaseStringComparer"/> would return <c>[a, A, A, b, B, c, C, d, D]</c>, which is the expected behaviour.
/// </item>
/// </example>
public class OrdinalSortByCaseStringComparer : IComparer<string>
{
public static readonly OrdinalSortByCaseStringComparer DEFAULT = 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;
}
}
}

0 comments on commit 67626e5

Please sign in to comment.