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 Slideraim penalty for missing slider ends #29991

Closed
wants to merge 6 commits into from

Conversation

Givikap120
Copy link
Contributor

Part of this PR - #27303

This PR is fixing a mistake made back in 2021, when xexxar forgot that SliderFactor needs to be cubed before applying it to pp.

The main effect of this is correct penalty for missing slider ends.

Side-effect of this is nerfing all non-FC scores, as they assumed to miss all sliderends. This is unfixable with stable scores but wouldn't be an issue with lazer scores, because you have info about exact amount of missed sliderends.

@peppy
Copy link
Member

peppy commented Sep 27, 2024

Seems like if what is written in the OP is correct, this should go in with the current batch on changes? @ppy/osu-pp-committee

@smoogipoo
Copy link
Contributor

@ppy/osu-pp-committee Any updates on what to do with this one?

@tsunyoku
Copy link
Member

tsunyoku commented Oct 22, 2024

I'd like to see this in at some point. However, with #27691 being merged I do not see this as something super critical. I'd be more comfortable pushing it in with the current deploy if not for the fact that it causes changes on any non-FC score, since this means we need to consider balancing.

@@ -181,8 +175,8 @@ private double computeAimValue(ScoreInfo score, OsuDifficultyAttributes attribut
estimateImproperlyFollowedDifficultSliders = Math.Min(countSliderEndsDropped + countSliderTickMiss, estimateDifficultSliders);
}

double sliderNerfFactor = (1 - attributes.SliderFactor) * Math.Pow(1 - estimateImproperlyFollowedDifficultSliders / estimateDifficultSliders, 3) + attributes.SliderFactor;
aimValue *= sliderNerfFactor;
double sliderNerfFactor = (1 - attributes.SliderFactor) * (1 - estimateImproperlyFollowedDifficultSliders / estimateDifficultSliders) + attributes.SliderFactor;
Copy link
Member

Choose a reason for hiding this comment

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

Considering the known issue with this nerf is the lack of cubing resolved by the line below, I don't think touching the core nerf factor should be in scope of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the known issue with this nerf is the lack of cubing resolved by the line below, I don't think touching the core nerf factor should be in scope of this change.

it's the opposite of what you think
the way I've done this is not touching core nerf factor, when any other variation of this are changing the current curve

https://www.desmos.com/calculator/z2bdvr5iht
Black curve is live, Red line is this PR, Purple line is what you proposed

double fullComboThreshold = attributes.MaxCombo - countSliderEndsDropped;

if (scoreMaxCombo < fullComboThreshold)
effectiveMissCount = fullComboThreshold / Math.Max(1.0, scoreMaxCombo);

// Combine regular misses with tick misses since tick misses break combo as well
effectiveMissCount = Math.Min(effectiveMissCount, countSliderTickMiss + countMiss);
effectiveMissCount = countMiss;
Copy link
Member

Choose a reason for hiding this comment

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

What is your reason for changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the usage of estimators for lazer non-CL scores was added as a bandaid to nerf oshama scramble without merging this change

@stanriders
Copy link
Member

#31055

@stanriders stanriders closed this Dec 22, 2024
@Givikap120 Givikap120 deleted the slider_fix_end_penalty branch December 22, 2024 11:35
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