Implement "rank achieved" grouping mode #34548

frenzibyte · 2025-08-07T08:16:54Z

Closes Add "rank achieved" grouping mode to song select #34495
Depends on Implement "my maps" grouping mode #34539 for FilterCriteria.LocalUserId

RFC. This does not cover online scores not present in the database. Implementation is done as discussed, using a dictionary to achieve O(n) time complexity, as proposed by @peppy.

Performance is tested through TestSceneSongSelectGrouping.TestPerformance, which loads a total of 10k difficulties and 1.2k scores to a temporary realm database (going any higher takes too long to load). I've also confirmed the generated databases are in a temporary folder (/var/folders/...) since the database grows up to 7MB to contain all of the imported models.

Code-wise this PR violates rules to achieve the end result, by including realm directly in the grouping class. I think we're at the point where it doesn't really matter if realm lookups are done directly inside the grouping class or encapsulated by a delegate, and if delegates are still favoured then we'll need a delegate which wraps realm.Run, and the signature for that won't be pretty (I proposed something similar for collection grouping before and it was faced with disgust).

I've also took the time to tidy up TestSceneSongSelectGrouping as it keeps containing more realm/API-based tests.

peppy · 2025-08-07T08:56:24Z

(going any higher takes too long to load)

pretty scary since users will hit this number in no time

frenzibyte · 2025-08-07T09:00:22Z

Sorry, I meant it will take too long for the test to import all those beatmaps and scores. I've still waited patiently and tested 12k scores, it had almost no effect on the filter time.

peppy · 2025-08-07T14:53:18Z

osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectGrouping.cs

+        #region Rank Achieved grouping
+
+        [Test]
+        public void TestRankAchievedGrouping()


I'm not too sure why tests have started to be added to this class and not the non-test-scene one. Did you run into an issue which meant you couldn't put them there..?

It's also literally the only reason the realm instance is provided as nullable. I don't like making arguments nullable just for tests.

I've set up this test scene to take away the complications of setting up a local temporary database and API access, I've also stated that in the xmldoc of the test scene class.

This test scene also helps confirming that the grouping filter is correctly hooked up with those components (especially cases like a user logging out or collections being modified, which isn't necessarily identified by the grouping filter class itself, but rather signaled by FilterControl / components of the screen).

peppy · 2025-08-07T14:54:50Z

osu.Game/Beatmaps/BeatmapInfo.cs


        public bool Equals(IBeatmapInfo? other) => other is BeatmapInfo b && Equals(b);

+        public override int GetHashCode()


~~What is this..?~~ Reading on I can see why this might have been added, but I do not think it's a good idea.

I've went with this approach following BeatmapSetInfo and the SetItems dictionary set up in the grouping filter, which uses BeatmapSetInfo as a key.

That dictionary is a bit different because usages need to read the BeatmapSetInfo content. That's not the case for this private usage so let's not. It's just extra complexity to think about. I see a class being used as a dinctionary key and my brain goes into panic.

peppy · 2025-08-07T14:55:25Z

osu.Game/Screens/SelectV2/BeatmapCarousel.cs

                new BeatmapCarouselFilterMatching(() => Criteria!),
                new BeatmapCarouselFilterSorting(() => Criteria!),
-                grouping = new BeatmapCarouselFilterGrouping(() => Criteria!, () => detachedCollections())
+                grouping = new BeatmapCarouselFilterGrouping(() => Criteria!, () => realm)


why was this moved?

Since I'm including realm directly, I thought there's not really any reason to still have the detachedCollections func. If you don't think that's the correct approach and the realm func will eventually be removed, then I'll revert it.

peppy · 2025-08-07T14:55:39Z

osu.Game/Screens/SelectV2/BeatmapCarousel.cs

        private Sample? spinSample;
        private Sample? randomSelectSample;

-        private Func<List<BeatmapCollection>> detachedCollections = null!;


peppy · 2025-08-07T14:56:51Z

osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs

+                    {
+                        var allLocalScores = r.All<ScoreInfo>()
+                                              .Filter($"{nameof(ScoreInfo.User)}.{nameof(RealmUser.OnlineID)} == $0"
+                                                      + $" && {nameof(ScoreInfo.BeatmapInfo)}.{nameof(BeatmapInfo.Hash)} == {nameof(ScoreInfo.BeatmapHash)}"


What's this conditional?

Carried across from TopLocalRank (also exists in LeaderboardManager), it seems to be for discarding scores of a beatmap version that doesn't match the current one.

Inline comment mentioning this is the case, please. I can't read brains.

peppy · 2025-08-07T14:57:15Z

osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs

+                                                      + $" && {nameof(ScoreInfo.Ruleset)}.{nameof(RulesetInfo.ShortName)} == $1"
+                                                      + $" && {nameof(ScoreInfo.DeletePending)} == false", criteria.LocalUserId, criteria.Ruleset?.ShortName)
+                                              .OrderByDescending(s => s.TotalScore)
+                                              .ThenBy(s => s.Date);


Date has limited precision. Online score id should be preferred whenever available.

Also carried from https://github.com/ppy/osu/blob/cb354685ca323f4e429e60617077ed5a5175ee3d/osu.Game/Screens/Select/Carousel/TopLocalRank.cs#L78-L77, I can switch it to fall back to ID but we should probably update that code as well? If so, should I bundle it in this PR or in a separate one.

Update both places, and add a comment inline here mentioning the important fact that this code is shared.

peppy · 2025-08-07T14:58:19Z

osu.Game/Screens/SelectV2/BeatmapCarouselFilterGrouping.cs

+                        {
+                            Debug.Assert(score.BeatmapInfo != null);
+
+                            if (topRankMapping.ContainsKey(score.BeatmapInfo))


Is there a reason to not use hash here?

I was debating between hash and the database ID, but the latter seemed to represent the beatmap in the local user's database better. This seems to be an either/or situation to me?

The issue I have here is using a model class as the key. I don't care if you use ID or hash, but please use it directly.

Rank animation is played for new panels when scrolling down, showing the previous rank belonging to the previous beatmap assigned to that specific panel instance.

peppy · 2025-08-12T06:12:53Z

osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelectGrouping.cs

+        #region Benchmarks
+
+        [Test]
+        public void TestPerformance()


just for your own improvement, when i asked for a benchmark i hoped to be able to run something headless via nunit.

frenzibyte added the area:song-select label Aug 7, 2025

pull-request-size bot added the size/L label Aug 7, 2025

peppy self-requested a review August 7, 2025 10:14

frenzibyte added 2 commits August 7, 2025 19:16

Add "Rank Achieved" grouping mode

3e9df00

Add test and benchmark coverage

c23856c

peppy force-pushed the rank-achieved-grouping branch from 8d1ae6e to c23856c Compare August 7, 2025 10:16

Simplify LINQ usage to appease inspectcode

8d11f4d

peppy reviewed Aug 7, 2025

View reviewed changes

frenzibyte added 2 commits August 11, 2025 19:19

Replace realm access with delegates

f8049a8

Fix incorrect rank animation when beatmap panels retrieved from pool

522f942

Rank animation is played for new panels when scrolling down, showing the previous rank belonging to the previous beatmap assigned to that specific panel instance.

peppy self-requested a review August 12, 2025 05:08

Revert unnecessary changes

8239294

peppy force-pushed the rank-achieved-grouping branch from 49bb206 to 8239294 Compare August 12, 2025 05:26

Merge branch 'master' into rank-achieved-grouping

e7f9f62

peppy reviewed Aug 12, 2025

View reviewed changes

Use ExplicitAttribute instead of manual nunit ignore

ade7641

peppy approved these changes Aug 12, 2025

View reviewed changes

peppy merged commit 263fb20 into ppy:master Aug 12, 2025
4 of 8 checks passed

frenzibyte deleted the rank-achieved-grouping branch August 12, 2025 07:26

This was referenced Sep 5, 2025

Bump ppy.osu.Game from 2025.605.1 to 2025.904.0 LumpBloom7/hishigata#293

Closed

Bump ppy.osu.Game from 2025.605.1 to 2025.905.0 LumpBloom7/hishigata#294

Closed

This was referenced Oct 3, 2025

Bump ppy.osu.Game from 2025.605.1 to 2025.1002.0 LumpBloom7/hishigata#296

Closed

Bump ppy.osu.Game from 2025.605.1 to 2025.1007.0 LumpBloom7/hishigata#298

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump ppy.osu.Game from 2025.605.1 to 2025.1013.0 LumpBloom7/hishigata#299

Closed

dependabot bot mentioned this pull request Oct 29, 2025

Bump ppy.osu.Game from 2025.605.1 to 2025.1029.0 LumpBloom7/hishigata#300

Merged

dependabot bot mentioned this pull request Dec 14, 2025

Bump ppy.osu.Game from 2025.710.0-lazer to 2025.1209.0 goodtrailer/soyokaze#241

Open


		public bool Equals(IBeatmapInfo? other) => other is BeatmapInfo b && Equals(b);

		public override int GetHashCode()

Uh oh!

Implement "rank achieved" grouping mode #34548

Implement "rank achieved" grouping mode #34548

Uh oh!

Conversation

frenzibyte commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peppy commented Aug 7, 2025

Uh oh!

frenzibyte commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peppy Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

frenzibyte commented Aug 7, 2025 •

edited

Loading

frenzibyte commented Aug 7, 2025 •

edited

Loading

peppy Aug 7, 2025 •

edited

Loading