Skip to content

Fix inconsistent rounding strategy for PP #33830

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

Merged
merged 3 commits into from
Jun 24, 2025
Merged

Conversation

EYHN
Copy link
Contributor

@EYHN EYHN commented Jun 23, 2025

I've noticed that after a game, the UserStatisticsUpdateDisplay next to the user's avatar directly truncates the decimal places of the PP value instead of rounding it like everywhere else.

@EYHN EYHN force-pushed the eyhn/fix/pp-rounding branch from 288acc0 to c61ec0b Compare June 23, 2025 06:29
smoogipoo
smoogipoo previously approved these changes Jun 24, 2025
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Just from the code itself, I think this looks okay?

@smoogipoo smoogipoo requested a review from bdach June 24, 2025 09:00
@bdach
Copy link
Collaborator

bdach commented Jun 24, 2025

If we're being pedantic, this was not entirely correct. The reason why is that while applying rounding as it was applied here was correct to match the initial and final values as displayed everywhere else, it would not end up being correct for the delta, as demonstrated in the a78dc31 test (one display would show +1pp, the other would show +2pp).

Which I fixed in b1e86aa.

Notably while flooring was explicitly introduced in #29512 rather than rounding, I'm fine with rounding because it appears the website is already rounding anyway, so I'm falling in line with everything else rather than inventing new behaviours.

@bdach bdach merged commit 1c2e56c into ppy:master Jun 24, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants