Fix multiplayer results screen displaying same beatmap for all users #32109

smoogipoo · 2025-02-26T10:14:17Z

Fixes #31951
Fixes #16480

Three important changes are made here...

Refactoring results screens to use `Task`s

On master, results screens are very closely tied to their API requests. Implementations are required to return an APIRequest to be handled by the base class, which doesn't work well when the playlists implementation needs to do a second lookup for the beatmaps:

osu/osu.Game/Screens/Ranking/ResultsScreen.cs

Lines 240 to 243 in 38d807e

    
           var req = FetchScores(fetchScoresCallback); 
        
           if (req != null) 
        
               api.Queue(req);

Furthermore, callbacks are passed around that are expected to be called in just the right way by implementations to actually add new scores to the list:

osu/osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs

Lines 116 to 120 in 38d807e

    
           Schedule(() => 
        
           { 
        
               PerformSuccessCallback(scoresCallback, allScores); 
        
               hideLoadingSpinners(); 
        
           });

This abstraction turned out to be quite a mess that refactoring it to use Tasks seemed appropriate. Now, the FetchScores() methods both return a Task<ScoreInfo[]> which also indicates the completion, allowing removal of the extra lastFetchCompleted state.

Important things to look out for: The code for querying when scrolled to the left/right of the list has changed significantly. There should be no functional changes.

Making `PlaylistItemResultsScreen` query beatmaps

Aside from querying scores appropriately, this screen also handles transforming from MultiplayerScore models to ScoreInfo. The former model contains a BeatmapId parameter that was previously unused.

Now, the screen will perform a second request to fetch corresponding beatmaps before performing the transformation. Of particular importance:

Local beatmaps are preferred if they exist.
Online beatmaps are allowed to return null, in which case the score will default to displaying the global beatmap while logging an error. This is done primarily to simplify testing flows, and I don't expect this to be hit in practice.

Making score panel difficulty rating display not depend on local beatmap availability

Hopefully the explanation in 9029099 is sufficient, but the gist is that getting this to work means essentially ferrying APIBeatmap through BeatmapInfo so that it can be attached to a ScoreInfo. BeatmapDifficultyCache does not understand this scenario and will not return the expected fallback value, as it expects BeatmapInfo to always represent a database-local model.

I've experimented with making it understand this scenario in https://github.com/smoogipoo/osu/tree/fix-bdc-fallback, but I'm unsure of that path.

Testing

I've updated TestScenePlaylistsResultsScreen to show multiple beatmaps, but most of my testing has been on the live servers. In particular:

The ongoing FA playlist to ensure scrolling/basic lookups are still working as expected.
This freestyle room to check that multiple beatmaps are displayed correctly on the screen.

This is a very special case where online beatmap/ruleset models are being ferried via `ScoreInfo` in what appear to `BeatmapDifficultyCache` as local `BeatmapInfo`/`RulesetInfo` models. Here, BDC will incorrectly attempt to proceed with calculating true difficulty where it cannot, and return 0. This is fixed locally because `ScoreInfo` is a very weird model, and I'm not sure whether BDC should contain logic to work around this.

- Handling all errors matches master a little bit better. Logging exceptions in any case. - Not throwing when beatmaps are missing simplifies tests.

Somewhat informal because it isn't super easy to handle.

bdach · 2025-02-28T08:33:32Z

osu.Game/Screens/Ranking/ResultsScreen.cs

Remark, no action required: I can't understate how much I stand behind these changes, the coupling of the results screen to API requests is so terrible

bdach · 2025-02-28T08:46:22Z

osu.Game/Screens/Ranking/Expanded/ExpandedPanelMiddleContent.cs

+            // In some cases, the beatmap ferried through ScoreInfo actually represents an online beatmap.
+            // If it isn't, we may be able to compute a more accurate difficulty from the ruleset and mods.
+            if (realmAccess.Run(r => r.Find<BeatmapInfo>(score.BeatmapInfo!.ID)) != null)
+                starDifficulty = beatmapDifficultyCache.GetDifficultyAsync(score.BeatmapInfo!, score.Ruleset, score.Mods).GetResultSafely() ?? starDifficulty;


Remark, no action required: I am wincing at this a lot because as far as I can tell this working predicates on GUID conflicts never happening out of thin air. But also I know how tightly everything in the results screen hierarchy is coupled to ScoreInfo and have tried and failed to refactor everything so that it's not so I'll probably just bite the bullet and ignore this...

bdach · 2025-02-28T09:11:21Z

osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs

+                    Difficulty = new BeatmapDifficulty(beatmap.Difficulty),
+                    Metadata =
+                    {
+                        Author = new RealmUser
+                        {
+                            Username = beatmap.Metadata.Author.Username,
+                            OnlineID = beatmap.Metadata.Author.OnlineID,
+                        }
+                    },
+                    DifficultyName = beatmap.DifficultyName,
+                    StarRating = beatmap.StarRating,
+                    Length = beatmap.Length,
+                    BPM = beatmap.BPM
+                };


Suggestion / moderate severity concern: Think this may be missing a few? At least

diff --git a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs index 572bf535f7..184de2f50c 100644 --- a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistItemResultsScreen.cs @@ -223,6 +223,8 @@ private async Task<ScoreInfo[]> transformScores(List<MultiplayerScore> scores) Difficulty = new BeatmapDifficulty(beatmap.Difficulty), Metadata = { + Artist = beatmap.Metadata.Artist, + Title = beatmap.Metadata.Title, Author = new RealmUser { Username = beatmap.Metadata.Author.Username,

The aforementioned patch would "fix" a particular white whale of mine in #16480. It's a much more direct fix than what I was going for (which was weaning off the entirety of the results monstrosity off of ScoreInfo and then giving up seeing the scope of work required, at which point I decided to just belay that until the "inevitable" redesign of resutls), but it appears to work well enough?

Applied in 993473c.

One thing I forgot to mention in the OP, is I originally started going down the path of adding copy constructors to all these models, similar to BeatmapDifficulty except inlined instead. In theory it would catch this right out of the gate.
I didn't know how to feel about that from the outset/thought it would be heavily contested in review, so I didn't end up doing it and went with this local solution instead.

Curious on what your take would be.

going down the path of adding copy constructors to all these models

You mean a copy constructor to BeatmapInfo I guess? I dunno where I'd be with that.

We already have

osu/osu.Game/Beatmaps/BeatmapInfo.cs

Line 236 in 89b6d7c

public BeatmapInfo Clone() => (BeatmapInfo)this.Detach().MemberwiseClone();

which is half-baked; we already have #25467 which was pronounced too ugly to live; and in general I'm skeptical as to having cloning be a solution to any of this. The ideal way out for me would be to redo everything so that we can start using IScoreInfo on these screens and skip all the weird games with model instances and collating data and so on, but as I said like twice here, I tried that and it was painful.

I dunno. I'd just get this in as is rather than go down that conversation, so I tentatively agree with the decision of keeping that out of this pull.

bdach · 2025-02-28T09:58:07Z

Operationally speaking, it should be fine to merge this immediately without looking at the other multiplayer PRs, right?

smoogipoo · 2025-02-28T09:58:51Z

it should be fine to merge this immediately without looking at the other multiplayer PRs

Yep, this should be independent of all the others.

smoogipoo added 14 commits February 25, 2025 21:54

Make results screens use tasks to fetch scores

13ca8c2

Populate playlists results screen with online beatmaps

dfae111

Replace virtual async method with better abstraction

8a27b66

Arrays instead of enumerables

3b5bf39

ConfigureAwait(false) everywhere

116b5a3

Clean up completion handling

bb457ca

Fix loading spinners not hiding correctly

baf20d8

Prefer local models where available

59cfcb3

Include author

b7d431f

Add missing ConfigureAwait

c7fd7cf

Replace error handling with logs

3dde024

- Handling all errors matches master a little bit better. Logging exceptions in any case. - Not throwing when beatmaps are missing simplifies tests.

Add support to tests

c280c8f

Somewhat informal because it isn't super easy to handle.

Cleanup score fetching a bit

c287542

smoogipoo added area:multiplayer area:results area:playlists labels Feb 26, 2025

pull-request-size bot added the size/L label Feb 26, 2025

Fix typo

d3c4afe

bdach self-requested a review February 28, 2025 08:23

bdach reviewed Feb 28, 2025

View reviewed changes

Pass through artist/title in beatmap transform

993473c

bdach approved these changes Feb 28, 2025

View reviewed changes

bdach merged commit c4e37a1 into ppy:master Feb 28, 2025
8 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiplayer results screen displaying same beatmap for all users #32109

Fix multiplayer results screen displaying same beatmap for all users #32109

smoogipoo commented Feb 26, 2025 •

edited by bdach

Loading

bdach Feb 28, 2025

bdach Feb 28, 2025

bdach Feb 28, 2025

smoogipoo Feb 28, 2025 •

edited

Loading

bdach Feb 28, 2025

bdach commented Feb 28, 2025

smoogipoo commented Feb 28, 2025

	var req = FetchScores(fetchScoresCallback);

	if (req != null)
	api.Queue(req);

	Schedule(() =>
	{
	PerformSuccessCallback(scoresCallback, allScores);
	hideLoadingSpinners();
	});

Fix multiplayer results screen displaying same beatmap for all users #32109

Fix multiplayer results screen displaying same beatmap for all users #32109

Conversation

smoogipoo commented Feb 26, 2025 • edited by bdach Loading

Refactoring results screens to use Tasks

Making PlaylistItemResultsScreen query beatmaps

Making score panel difficulty rating display not depend on local beatmap availability

Testing

bdach Feb 28, 2025

Choose a reason for hiding this comment

bdach Feb 28, 2025

Choose a reason for hiding this comment

bdach Feb 28, 2025

Choose a reason for hiding this comment

smoogipoo Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

bdach Feb 28, 2025

Choose a reason for hiding this comment

bdach commented Feb 28, 2025

smoogipoo commented Feb 28, 2025

smoogipoo commented Feb 26, 2025 •

edited by bdach

Loading

Refactoring results screens to use `Task`s

Making `PlaylistItemResultsScreen` query beatmaps

smoogipoo Feb 28, 2025 •

edited

Loading