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

Allow "Difficulty Adjust" mod's extended AR selection to go below zero #24736

Merged
merged 7 commits into from Sep 7, 2023

Conversation

isakvik
Copy link
Contributor

@isakvik isakvik commented Sep 7, 2023

Discussion reference: #22898

This PR adds negative ARs down to -10 to the difficulty adjust mod for osu!standard (for the extended range), and adds millisecond value information to the selection slider.

Negative ARs are fun, and well supported in the gamemode, so this simple implementation has been made to enable eventual sanity checking of even lower ARs, or with a more complicated UI solution if the slider is deemed insufficient.

Adjustment screen

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 7, 2023

private double millisecondsFromApproachRate(float value, float clockRate)
{
return Math.Round(1800 - Math.Min(value, 5) * 120 - (value >= 5 ? (value - 5) * 150 : 0) / clockRate);
Copy link
Sponsor Member

@peppy peppy Sep 7, 2023

Choose a reason for hiding this comment

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

I don't know where you got this from, but it doesn't match any existing implementations and is probably wrong. I'll @bdach will fix it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed by just creating a hitcircle and reading the raw value out of it.

I also nuked the clockRate param (I get what this was going for, but we're not handling "effective" AR anywhere yet, so this should be done separately and comprehensively), and made the ms thing display only when extended limits are active. That last one is kinda style, kinda not wanting to overwhelm users / prompt questions "why does only osu! ruleset get this ms display".

@peppy peppy changed the title osu!std: Extend AR selection of Difficulty Adjust to AR -10 Allow "Difficulty Adjust" mod's extended AR selection to go below zero Sep 7, 2023
And just use a hitcircle, and read the actual value. Comes with 100%
less chance of forgetting to update either place in the future.
@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 7, 2023
@bdach bdach requested a review from peppy September 7, 2023 06:36
@apollo-dw
Copy link
Member

I just want to quickly step in and say we could consider adjusting the AR -> visibility time formula as suggested in the discussion. Above AR5 and below AR5 have different formulas IIRC, so there is precedent for adjusting the formula for below 0.

What this would do in effect is allow for way longer visibility times, without making the scaling of the slider super weird. The millisecond information appearing in the tooltip only makes this more feasible IMO. I've seen reading players do ARs lower than even -20...

@peppy
Copy link
Sponsor Member

peppy commented Sep 7, 2023

I just want to quickly step in and say we could consider adjusting the AR -> visibility time formula as suggested in the discussion. Above AR5 and below AR5 have different formulas IIRC, so there is precedent for adjusting the formula for below 0.

This becomes a performance and UX issue. Let's track that separately.

I'm only pushing this one through so we can handle the seemingly major case and cease the discussions as to keep focus on the more important tasks at hand.

@peppy peppy merged commit b1a9f50 into ppy:master Sep 7, 2023
15 of 17 checks passed
@bdach
Copy link
Collaborator

bdach commented Sep 7, 2023

Scores are subject to a wipe anyway. So we can probably rescale in the future as seen fit.

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.

None yet

4 participants