-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Adjust extended OD limits for mania difficulty change mod to reflect HR and EZ values #35249
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much in way of source to look at here, but I don't foresee merging this on my own without a second check with @ppy/team-client that nobody has major ideological oppositions to entertaining this type of change.
| ExtendedMaxValue = 13.61f, | ||
| ExtendedMinValue = -14.93f, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An inline comment describing where these specific values come from is probably warranted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a comment explaining it even though I rounded the values, since I figure it's probably useful to know why the limits are even being overriden in the first place
| MinValue = 0, | ||
| MaxValue = 10, | ||
| ExtendedMaxValue = 13.61f, | ||
| ExtendedMinValue = -14.93f, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative OD looks very wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, but you actually can get into "negative OD" territory when applying Easy mod because of how special mania Easy / Hard Rock are. See #34227.
(And again, we're kinda already there elsewhere - osu! Difficulty Adjust permits negative AR already...)
| ExtendedMaxValue = 13.61f, | ||
| ExtendedMinValue = -14.93f, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use round numbers. 13.61 is meaningless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually not that meaningless. -14.93 is what the "effective" OD is when EZ is applied to a beatmap with baseline OD 0, and 13.61 is what the "effective" OD is when HR is applied to a beatmap with baseline OD 10. (Which is why I asked for inline comments explaining what these magic numbers are.)
Whether this should be exposed to people as an option is the debate I'm interested in, but do note we're kind of already halfway there given #34227 is a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the above comment from the user's perspective.
In other words, round it to -15/+15?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no real objections left from me, not sure about everyone else
smoogipoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is fine from me too
Resolves #35186
This change sets the OD bindable in ModDifficultyAdjust to virtual and the mania specific difficulty adjust mod is changed to override the bindable and set extended OD limits to their EZ minimum and HR maximum.