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 mania SR inflation for hold note releases in quick succession #17913

Merged
merged 7 commits into from
Apr 26, 2022

Conversation

molneya
Copy link
Contributor

@molneya molneya commented Apr 21, 2022

This change fixes 2 critical issues with mania release holds:

  • Hold releases can change difficulty depending on what column they are in (e.g. mirroring a map changes star rating in some circumstances)
  • Hold releases in quick succession can inflate the difficulty as they are treated independently, despite how the player might actually play them

Visual explanation for the new LN release nerf created by @Eve-ning

Both of these changes should just be nerfing maps with holds and nothing else.

@stanriders
Copy link
Member

Looks good from code and conceptual side for me. Can't really speak about how well that translates to gameplay tho, hope someone familiar with mania can take a look

@Kominaru
Copy link

Kominaru commented Apr 21, 2022

Can't really speak about how well that translates to gameplay tho, hope someone familiar with mania can take a look

Hi mania NAT here. Can confirm this fixes a lot of the issues with SR bloat caused from uneven releases, so all good on that end.
We are currently looking onto whether this overnerfs some actually difficult LN patterns, but in hindsight this is all good and benefitial.

@stanriders
Copy link
Member

We are currently looking onto whether this overnerfs some actually difficult LN patterns

As we briefly discussed with @molneya in pp dev discord, it's pretty much out of scope of this change anyway. Unless there is a quick way to balance them back, fixing LN maps can be done later in a different PR.

stanriders
stanriders previously approved these changes Apr 21, 2022
@smoogipoo
Copy link
Contributor

@smoogipoo
Copy link
Contributor

Since we don't really have a pp committee for mania, I'm going to rely on approvals from @abraker95 @Eve-ning and @Kominaru for merge.

@Kominaru
Copy link

Since we don't really have a pp committee for mania, I'm going to rely on approvals from @abraker95 @Eve-ning and @Kominaru for merge.

All good on my end!
On that note, I think it would be a good idea to actually conform a formal pp committee knowing all four of us are pretty much among the pack of people that are actively contributing to pp dev

@smoogipoo smoogipoo requested a review from a team April 22, 2022 14:10
WindowsMeosu
WindowsMeosu previously approved these changes Apr 22, 2022
abraker95
abraker95 previously approved these changes Apr 23, 2022
Copy link

@abraker95 abraker95 left a comment

Choose a reason for hiding this comment

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

looks good

@molneya molneya dismissed stale reviews from abraker95 and WindowsMeosu via 6cca56a April 23, 2022 02:46
Copy link
Member

@Eve-ning Eve-ning left a comment

Choose a reason for hiding this comment

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

Looks good, while it may underrate some LN maps, this is an essential step forward as it corrects unexpected behavior from hold_addition, good job

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

Not yet sure when the next diffcalc release will be due to changes in the infrastructure.

@smoogipoo smoogipoo changed the title Fix for mania hold release difficulty Fix mania SR inflation for hold note releases in quick succession Apr 26, 2022
@smoogipoo smoogipoo merged commit b19e738 into ppy:master Apr 26, 2022
@molneya molneya deleted the hold-addition-fix branch November 11, 2023 12:05
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.

9 participants