Skip to content

Fix legacy score calculators using incorrect mod multipliers #24988

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 5 commits into from
Oct 9, 2023

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Oct 2, 2023

Supersedes #24685
Fixes #24653

For those that have access to it, you can refer to this method in the osu-stable source: https://github.com/peppy/osu-stable-reference/blob/1531237b63392e82c003c712faa028406073aa8f/osu!/GameplayElements/Scoring/ModManager.cs#L180-L194

There's no real good way to test this right now, except by manually setting DifficultyCalculator.ComputeLegacyScoringValues to true and then breakpointing in the SV1 calculation block. The reason is because score by itself is kind-of meaningless on its own unless you're drilling down into something specific/you know what you're looking for - in this case the calculated total legacy score, which isn't exposed yet (maybe an osu-tools command is a good fit).

Also, I was told not to worry about allocations of new ManiaRuleset().RulesetInfo (both in the allocation of the objects and the finalisable RealmObject that RulesetInfo inherits).

Spreadsheets:

Spreadsheet TBD.

@bdach
Copy link
Collaborator

bdach commented Oct 2, 2023

From the source alone this seems pretty much okay, no major objections.

  • Not sure why this is in draft state - is that for the spreadsheet?
  • Is there any particular testing worth running, aside from like smoke tests to make sure the game launches?

@smoogipoo
Copy link
Contributor Author

Yeah, draft state because I want to get some spreadsheets out.

@smoogipoo
Copy link
Contributor Author

smoogipoo commented Oct 4, 2023

Moving this into ready for review because the osu spreadsheet (added to description) looks okay and fairly closely matches the previous PR (differences between spreadsheets are that the previous one was processed over top-10000 whereas this one is top-1000, and the previous one contained the scoreMultiplier prevision bug).

I'll add the rest of the spreadsheets as they arrive (runs: osu | taiko | catch | mania).

@smoogipoo smoogipoo marked this pull request as ready for review October 4, 2023 00:54
@smoogipoo smoogipoo added the blocked Don't merge this. label Oct 4, 2023
@smoogipoo
Copy link
Contributor Author

Something does not look right with catch. Almost like the HT multiplier isn't added in. Blocked while investigating.

@smoogipoo
Copy link
Contributor Author

Okay, turns out this is fine to go ahead. I justified some of the changes in the description.

@smoogipoo smoogipoo removed the blocked Don't merge this. label Oct 5, 2023
@bdach
Copy link
Collaborator

bdach commented Oct 9, 2023

So I've looked at this from a few directions. I don't think there's much reason left not to get this in anymore.

The sheets generally look good. The only thing approaching suspicious there is this row on the catch sheet:

1696848583

as it generally should not be possible for a HT score to reach 1 million (save for egregious bonus, and even then). But upon closer investigation I believe this is likely another case of the rounding issue I found a while back, which underestimates the map-dependent "score multiplier" and thus has a major impact on maximum total. The calculated difficultyPeppyStars in this case before rounding is 4.5, which is the same as I had in my case. And manually forcing the rounding to return 5 (via debugger) caused a way more reasonable total to be spit out.

I'm not sure how much this rounding issue is going to be a problem for us. Maybe we should reconsider fixing it. I'm not entirely sure how we would do it exactly, though...

@bdach bdach merged commit b3d3ae8 into ppy:master Oct 9, 2023
@smoogipoo smoogipoo deleted the fix-legacy-score-multipliers-2 branch October 10, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

osu! ruleset ScoreV1 simulation underestimates total score
2 participants