Skip to content

Conversation

@tsunyoku
Copy link
Member

totalPlayableHits is a new field which uses MaximumStatistics to sum the amount of hits in the beatmap, regardless of how far into the beatmap the given score was. My assumption is that using IsBasic is good enough to get what is effectively a sum of the circles, sliders and spinners in a beatmap - based on reading that appears to be true, but please scrutinise this line of code.

This allows for a proper fix of what was attempted in #31741 as it prevents the cases where these variables could go negative, rather than just pushing them to zero if it happened.

My logic for where I kept totalHits was that those places are better using the hits in the score thus far - miss penalty, length bonus etc. all make sense to react to the amount of hits the score actually achieved rather than what's in the beatmap.

Testing against the replay provided in the attached PR still gives zero with the relevant guards removed due to these changes, and I'll run a sheet to confirm that this doesn't affect submitted plays. PP counter in-game also appears to progress as expected.

@tsunyoku
Copy link
Member Author

!diffcalc
RULESET=osu
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#32137

@github-actions
Copy link

@tsunyoku tsunyoku moved this from In Progress to Pending Review in Difficulty calculation changes Mar 4, 2025
@stanriders
Copy link
Member

I do like the idea, but this will 100% be a major pain for any third-party consumers. Not sure I enjoy this fact as one of those consumers lol

Another thing is - shouldn't this be done for all rulesets?

@Givikap120
Copy link
Contributor

I don't sure why this is necessary. I always thought that pp counter should represent the pp if map would've ended right here. And this adds a difference to this, so pp counter is always aware of full beatmap. Don't see how it would be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Pending Review

Development

Successfully merging this pull request may close these issues.

3 participants