Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use beatmap cache to populate beatmap information in tournament client #24037

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

cdwcgt
Copy link
Contributor

@cdwcgt cdwcgt commented Jun 25, 2023

In many different tournaments, the beatmap information in the seedingBeatmaps of different teams is the same, because each team use the same set of seedingBeatmaps for seeding, and the cache should be used.

Can greatly speed up populate when the number of beatmap that should be requested is small and the team is large

var req = new GetBeatmapRequest(new APIBeatmap { OnlineID = b.ID });
API.Perform(req);
b.Beatmap = new TournamentBeatmap(req.Response ?? new APIBeatmap());
b.Beatmap = new TournamentBeatmap(beatmapCache.GetBeatmapAsync(b.ID).GetAwaiter().GetResult() ?? new APIBeatmap());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this GetAwaiter().GetResult() stuff an attempt to bypass the fact that .Result is banned? If yes, then did you try calling .GetResultSafely() which is a framework extension for this express purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this should be my oversight, sorry

already fix

@bdach bdach changed the title Use BeatmapCache for populate beatmap information Use beatmap cache to populate beatmap information in tournament client Jun 25, 2023
@peppy peppy self-requested a review June 26, 2023 04:41
@peppy
Copy link
Member

peppy commented Jun 26, 2023

This seems okay. But I did notice one other issue: if you're not logged in when this population process runs, it still attempts each and every request, but they all fail due to Unauthorized. We probably need to fix that in a future..

@peppy peppy merged commit 719ac75 into ppy:master Jun 26, 2023
peppy added a commit to peppy/osu that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants