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

Fix daily challenge results screen fetching scores beginning from the user's highest #30852

Merged
merged 8 commits into from
Nov 28, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Nov 23, 2024

Note that using the scoreid-based endpoint for fetching scores excludes the user's highest score for some reason, this can be seen in the video below as my highest score with total score 118,509 is omitted from the results screen. It's sort of a minor detail to care about so I'm not considering it a blocker for this p:0 issue.

Video snippet of testing the new code on dev server:

CleanShot.2024-11-23.at.15.42.14.mp4

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

This made more sense in playlists where the results screen shows the average scores of all players

That's... not how it works? In playlists the results screen shows everyone's best score on the particular playlist item. You're confusing that with the leaderboard on the playlists lounge, which shows the sum of best plays across all playlist items, not the average. Nothing ever shows the average. I dunno where it would ever make sense to show an "average score".

In light of that I question the entire premise of this pull request in that it's doing something special to daily challenge. What's the guarantee that this doesn't happen on standard playlists? From testing on production (with a release build) I'm pretty sure this will not be a problem that is unique to daily challenge.

What I believe to be happening here is that the request fetches the user's best score on the playlist, while the results screen shows the completed play, and there's a gap of missing scores in between, so the automatic page fetch logic attempts to load every available score between user's best and current to fill the gap. And if that is the case then this is only monkey patching the issue in daily challenge, without fixing the same issue in playlists, for no reason whatsoever.

@frenzibyte
Copy link
Member Author

I used the wrong word, I meant the sum not the average, you could have assumed that part.

What I believe to be happening here is that the request fetches the user's best score on the playlist, while the results screen shows the completed play, and there's a gap of missing scores in between, so the automatic page fetch logic attempts to load every available score between user's best and current to fill the gap.

I will assume you're not implying that I should've stated this in the PR description, this part should be clear by the title alone.

And if that is the case then this is only monkey patching the issue in daily challenge, without fixing the same issue in playlists, for no reason whatsoever.

Well hey then I am utterly confused about why are we even using the ShowPlaylistUserScoreRequest endpoint at all.

@bdach
Copy link
Collaborator

bdach commented Nov 26, 2024

Well hey then I am utterly confused about why are we even using the ShowPlaylistUserScoreRequest endpoint at all.

This bug has probably been in playlists all these years but nobody either noticed or bothered to report it. The other request that takes in a score ID was added for daily challenge specifically.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 26, 2024
@bdach
Copy link
Collaborator

bdach commented Nov 26, 2024

Not sure how this happened but test failures in the test that was specifically adjusted here

@frenzibyte
Copy link
Member Author

My bad, I rushed out a push yesterday without double-checking tests.

I wasn't feeling well last night.
As we're moving towards using the `/playlist/<id>/scores/<id>` endpoint,
the existing playlists results screen classes needed some restructuring.
@pull-request-size pull-request-size bot added size/L and removed size/S labels Nov 27, 2024
@frenzibyte
Copy link
Member Author

Fixed test failure and also took some time to rethink the PlaylistItemResultsScreen structure to make more sense with the changes applied to the endpoint. Tested to work correctly on all of playlists, multiplayer, and daily challenge.

@peppy peppy requested a review from bdach November 28, 2024 06:48
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

seems fine, even for the code quality improvement aspect of it (no more weird nullable score ctor params)

@bdach bdach enabled auto-merge November 28, 2024 08:43
@bdach bdach merged commit d218c19 into ppy:master Nov 28, 2024
7 of 9 checks passed
@frenzibyte frenzibyte deleted the fix-daily-challenge-leaderboard branch November 28, 2024 09:27
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.

Daily Challenge results start loading from personal best on subsequent scores
2 participants