Extract leaderboard fetch logic from song select beatmap leaderboard drawable

[bdach]

RFC. Another attempt at this.

• Supersedes Extract leaderboard fetch logic from song select beatmap leaderboard drawable #31881
• Supersedes / closes Remove fetching logic from leaderboard #31355
• Closes Leaderboards do not show when a user goes into gameplay immediately #29861
• Closes Personal best should be shown on gameplay leaderboards #26716
• Closes Watching a replay does not show leaderboard #27609

This is a weird diff because I am feeling rather boxed in by all the constraints, namely that:

• Leaderboard state should be global state
• But the global state is essentially managed by song select and namely BeatmapLeaderboard itself. That's because trying to e.g. not have BeatmapLeaderboard pass the beatmap and the ruleset to the global leaderboard manager is worse, as it essentially introduces two parallel paths of execution that need to be somehow merged into one (as in I'd have to somehow sync LeaderboardManager responding to global beatmap/ruleset changes with BeatmapLeaderboard which is also doing the same via RefetchScores() inheritance hell)
• Also local leaderboard fetching is data-push (as in the scores can change under the leaderboard manager), and online leaderboard fetching is data-pull (as in the scores do not change unless the leaderboard manager does something). Also online leaderboard fetching can fail. Which is why I need to still have the weird setup wherein there's a FetchWithCriteriaAsync() (because I need to be able to respond to online requests taking time, or failing), but also the BeatmapLeaderboard only uses the public Scores bindable to actually read the scores (because it needs to respond to new local scores arriving).
• Another thing to think about here is what happens when a retrieval fails because e.g. the user requested friend leaderboards without having supporter. With how this diff is written, that special condition is handled local to BeatmapLeaderboard, and LeaderboardManager's state will remain as whatever it was before that scope change was requested, which may be considered good or it may not (I imagine it's better to show any scores in gameplay than not in this case, but maybe I'm wrong?)

[smoogipoo]

This looks better to me on a brief pass. I'll defer full review to @peppy.

[peppy]

Sorry for the late response. I am also on board with this direction. It seems that this is a soft dependency for moving forward with song select, so would be interested in seeing things move forward so it's not blocking (although keep in mind there's still a lot of other things blocking release so it's not super urgent).

[peppy]

As commented.

[bdach]

Alright, well, I fixed test failures as of 0209129, also fixed some more breakage that only showed in-game, and also actually changed this so that the players use the new global state in 16b817b, which fixes like three separate issues.

Should be ready for proper review. I'll check back on CI later.

[peppy]

I noticed there's still two locations doing local leaderboard requests:

osu/osu.Game/Overlays/BeatmapSet/Scores/ScoresContainer.cs

Lines 279 to 288 in 9630002

	getScoresRequest = new GetScoresRequest(Beatmap.Value, Beatmap.Value.Ruleset, scope.Value, modSelector.SelectedMods);
	getScoresRequest.Success += scores =>
	{
	Scores = scores;
	if (!scores.Scores.Any())
	noScoresPlaceholder.ShowWithScope(scope.Value);
	};
	api.Queue(getScoresRequest);

osu/osu.Game/Screens/Ranking/SoloResultsScreen.cs

Lines 44 to 47 in 3dde024

	getScoreRequest = new GetScoresRequest(Score.BeatmapInfo, Score.Ruleset);
	getScoreRequest.Success += requestTaskSource.SetResult;
	getScoreRequest.Failure += requestTaskSource.SetException;
	api.Queue(getScoreRequest);

One of these makes sense (the beatmap overlay), but the case of the results screen seems like it could potentially use this new flow with very little effort. Is this something that could/should be attempted in this PR?

(based off the OP close issue list, I think this may have been intended to be covered)

Also, while we're here it probably makes sense to update the endpoint URL to use the new one. I've confirmed with nanaya that this should not change behaviour:

diff --git a/osu.Game/Online/API/Requests/GetScoresRequest.cs b/osu.Game/Online/API/Requests/GetScoresRequest.cs
index f2a2daccb5..ed26c77dd9 100644
--- a/osu.Game/Online/API/Requests/GetScoresRequest.cs
+++ b/osu.Game/Online/API/Requests/GetScoresRequest.cs
@@ -36,7 +36,7 @@ public GetScoresRequest(IBeatmapInfo beatmapInfo, IRulesetInfo ruleset, BeatmapL
             this.mods = mods ?? Array.Empty<IMod>();
         }
 
-        protected override string Target => $@"beatmaps/{beatmapInfo.OnlineID}/solo-scores{createQueryParameters()}";
+        protected override string Target => $@"beatmaps/{beatmapInfo.OnlineID}/scores{createQueryParameters()}";
 
         private string createQueryParameters()
         {

[bdach]

I noticed there's still two locations doing local requests. One of these makes sense (the beatmap overlay), but the case of the results screen seems like it could potentially use this new flow with very little effort. Is this something that could/should be attempted in this PR?

(based off the OP close issue list, I think this may have been intended to be covered)

The inclusion in the issue close list is a mistake. I've retracted that now. Not sure what happened there but I was not intending to close this here. Mainly because results screen is likely going to prove to be more annoying and I don't want to involve it in this PR. I probably meant gameplay leaderboards but forgot that was actually already working on master.

Also, while we're here it probably makes sense to update the endpoint URL to use the new one. I've confirmed with nanaya that this should not change behaviour:

Sure, applied.

[bdach]

As I typed my previous reply I realised there is probably an issue here with the replay player usage. Blocking temporarily until confirmation.

[bdach]

Issue in question should be resolved by 31b98ac. See commit message for details what the issue was.