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 instant spinners giving insane amounts of strain #15009

Merged
merged 4 commits into from
Oct 15, 2021

Conversation

stanriders
Copy link
Member

@stanriders stanriders commented Oct 9, 2021

Fixes maps such as https://osu.ppy.sh/beatmapsets/814850#osu/1901200 (9.98* -> 6.54*)
https://osu.ppy.sh/beatmapsets/388378#osu/847257 (13.88* -> 6.20*)

Should not affect normal maps, even extremes such as https://osu.ppy.sh/beatmapsets/132#osu/296

@bdach
Copy link
Collaborator

bdach commented Oct 9, 2021

the modified block of code - as far as i can gather - is trying to calculate an angle between three subsequent positions. would it not make more sense to not run any of it if any of the three objects involved was a spinner?

i'm especially thinking about lastLastObject potentially being a spinner as well. it's just a thought that occurred to me given that there are two disparate spinner checks (one nested in the other to boot) which seem to unnecessarily complicate the code when a potentially simpler alternative would be to discount the entire three-object sequence based on the presence of a spinner.

@bdach bdach requested a review from a team October 9, 2021 12:07
@stanriders
Copy link
Member Author

Yeah that makes sense

@joseph-ireland
Copy link
Contributor

It was mentioned on discord that this still leaves room for StrainTime to give speed PP. Maybe a better approach is to filter out spinners from OsuDifficultyCalculator so they're fully excluded from all skills. Seems all the skills were trying (unsuccessfully) to filter them out anyway.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 15, 2021

Spreadsheet of SR changes (PP changes not listed).

Looks good to me.

@smoogipoo smoogipoo merged commit b1989e6 into ppy:master Oct 15, 2021
@stanriders stanriders deleted the fix-invisible-sinners branch November 9, 2021 17:02
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.

5 participants