-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove estimations where score data is available for osu! difficulty calculations #27691
base: master
Are you sure you want to change the base?
Conversation
Score data for non-CL scores includes sliderends dropped, meaning no need to estimate. CL scores are still estimated.
…or classic" This reverts commit 941c048.
No need to estimate misses for non-CL scores.
Miss count fix
Use actual sliderends dropped instead of estimating
Very much open to discussion on if these should be weighed differently
I am not sure the "available" score data in question can be used. We've previously expressed worry that this was going to spiral into two separate implementations of pp for classic and non-classic scores. |
My bad, I was not aware of these conversations. Could you drop a link to them so I can read through them? |
#21938 would be a good starting point I guess |
Stan's decision of splitting the calculator into 2 parts for classic and non-classic is a direction I personally dislike, and the way Fina is doing it here is the way I would approach it myself. I don't see any harm in using the new metrics, and PP spiraling into 2 calculators should only happen if per note judgements ever become available, and prove to give much better results than without them. These new metrics are easily estimable, and the existing estimations can just be made harsher if they prove to give an advantage to stable scores. |
Why is this only using additional data if slideracc enabled? |
The implications of a change like this are massive. If I set scores with sliderbreaks, I will only ever be punished more on lazer than I would be if I played on stable. I play on lazer as my main client, but a change like this on its own would make me consider moving back to stable because I don't want to play with something like this that is just a clear disadvantage. You could try tuning the estimations for stable scores to be more harsh, but I doubt people would be happy with that and I'm honestly not sure if that's even possible. The complexity to balance this kind of change is so high that I don't think it's worth the trouble to begin with. |
@Flamiii you’re already at an objective disadvantage if you play on lazer. This arguably helps you since it means miss estimations can no longer assume you may have sliderbroke where you didn’t. Any non-cl 1 miss plays where the break was in the middle are buffed. Same for plays on hard slider maps where you broke but didn’t drop any sliderends. |
If by "objective disadvantage" you mean slideracc, I play on lazer because I'm certain that will be taken into account in the future (#27063). The scenario I'm thinking of which happens quite a bit is the one where a player misses near the end of a map but also sliderbreaks 2 times afterwards. In stable, this play is counted as a 1 miss and the sliderbreaks only negatively impact your accuracy. With these changes in lazer, this play would be treated as if it was a 3 miss. As far as I know, there really isn't a way to adjust stable's estimation to make this scenario balanced because the relevant information just isn't there. There are some situations where you're punished a bit less, but the scenario I mentioned is extremely common and you would be punished way more for playing on lazer with these changes. Edit: It's also worth noting that using HitResults.LargeTickMiss to increase the effective miss count means that missing buzz sliders or any fast sliders with many sliderticks will be incredibly punishing. For example, misaiming a buzz slider with 5 repeats and missing it entirely would increase the effective miss count by 6, which would basically kill any score. |
There actually is a way, if you take a look at the function in question, we could, for example, lower fullComboThreshold to make it assume there were more misses than before, and if you look at the changes in this pr, this function is only used if CL with slideracc is on.
This is a valid point, however, I can assure you that future work will be done to account for this. |
I see. As long as these changes are balanced properly, I have no issues with them. |
shoot I hit the wrong button |
Ok but again this is already a thing. Sliderbreaks by missing the slider head are already counted as misses in lazer, so this scenario can already happen without any pp changes.
I know, I'm more apprehensive about that part and wanted to spark discussion for that specifically. The crux of the matter is, some scores will be punished by being unable to slip through the cracks. That's inevitable. The gain of scores no longer being undeservingly punished from estimations counteracts this and, at least in my personal opinion, is a fair trade off. |
As per suggestion by givikap, I was not aware that non-legacy cl scores stored this data
…into estimation-removal
After letting the comments @Flamiii left brew for a while, I realized they were very much right about the buzz slider thing. As such, I've implemented a quick and dirty untested fix that will hopefully have zero unintended side-effects :) I don't see this as a permanent or final solution yet. There's definitely some potential issues/inaccuracies that could arise with maps like Notch Hell or IOException's Black Rover, but afaik this implementation would not cause any issues that stable doesn't already have.
if (!score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value)) | ||
effectiveMissCount = countMiss; | ||
else | ||
effectiveMissCount = calculateEffectiveMissCount(osuAttributes); |
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.
using raw effectiveMissCount
for anything other than stable scores is bad
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.
Any reason why…?
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.
you already used amount missed sliderends in lazer CL scores
but now you're trying to use estimator (that's almost always outputting lower value than normal misscount)
this doesn't makes any sense
@@ -39,8 +41,14 @@ protected override PerformanceAttributes CreatePerformanceAttributes(ScoreInfo s | |||
countOk = score.Statistics.GetValueOrDefault(HitResult.Ok); | |||
countMeh = score.Statistics.GetValueOrDefault(HitResult.Meh); | |||
countMiss = score.Statistics.GetValueOrDefault(HitResult.Miss); | |||
effectiveMissCount = calculateEffectiveMissCount(osuAttributes); | |||
countLargeTickMiss = score.Statistics.GetValueOrDefault(HitResult.LargeTickMiss); | |||
countSliderEndsDropped = score.Statistics.GetValueOrDefault(HitResult.SmallTickMiss); |
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.
HitResult.SmallTickMiss
equals to lost sliderends only in Slideracc disabled scores
consider using HitResult.SliderTailHit
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.
This thing works
if (!score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value))
{
countSliderEndsDropped = osuAttributes.SliderCount - score.Statistics.GetValueOrDefault(HitResult.SliderTailHit);
}
if (score.IsLegacyScore) | ||
estimateSliderEndsDropped = Math.Clamp(Math.Min(countOk + countMeh + countMiss, attributes.MaxCombo - scoreMaxCombo), 0, estimateDifficultSliders); | ||
else | ||
estimateSliderEndsDropped = countSliderEndsDropped; |
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.
this is incorrect
you should also use HitResult.LargeTickMiss
here
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.
-
What about this is incorrect?
-
I’m hesitant to do that since I’d imagine values using
estimateSliderEndsDropped
are balanced around sliderend drops, not sliderend drops + other stuff
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.
what if you hit all sliderends but missed all reverses?
you will still get full slideraim value?
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.
- What about this is incorrect?
- I’m hesitant to do that since I’d imagine values using
estimateSliderEndsDropped
are balanced around sliderend drops, not sliderend drops + other stuff
also, it's not balanced around sliderend drops, because in case where you not dropped sliderend but sliderbroke - you almost instantly loosing almost all slider pp, so adding sliderbreaks here is actually always same or more lenient than using estimator
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.
also change it to Math.Min(countSliderEndsDropped + countLargeTickMiss, estimateDifficultSliders);
because otherwise you can get negative pp by missing to many sliderends
Actually, new score data can allow using 3 types of aim instead of 2:
This will make slider judgements even more accurate. But it's out of scope of this PR |
I think we can add a
|
useClassicSlider may better if it not only affects the slider head? |
This reverts commit 1f55c14.
@@ -32,14 +36,23 @@ public OsuPerformanceCalculator() | |||
protected override PerformanceAttributes CreatePerformanceAttributes(ScoreInfo score, DifficultyAttributes attributes) | |||
{ | |||
var osuAttributes = (OsuDifficultyAttributes)attributes; | |||
useClassicSlider = score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value); |
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.
usingClassicSliderMechanics
sounds better to me
if (useClassicSlider) | ||
estimateSliderEndsDropped = Math.Clamp(Math.Min(countOk + countMeh + countMiss, attributes.MaxCombo - scoreMaxCombo), 0, estimateDifficultSliders); | ||
else | ||
estimateSliderEndsDropped = Math.Min(countSliderEndsDropped + countLargeTickMiss, estimateDifficultSliders); |
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'm not sure min(drops, difficultSliders)
is necessary here since we definitely know how many dropped sliders there are. It'd be better to adjust slider nerf curve instead imo
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 just did it this way to match live since stable scores are clamped in the same way
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'm not sure
min(drops, difficultSliders)
is necessary here since we definitely know how many dropped sliders there are. It'd be better to adjust slider nerf curve instead imo
it is necessary because amount of dropped sliderends can be higher than difficultSliders
, as difficultSliders
is only 15% of the whole sliders count
if (!useClassicSlider) | ||
countSliderEndsDropped = osuAttributes.SliderCount - score.Statistics.GetValueOrDefault(HitResult.SliderTailHit); | ||
|
||
if (useClassicSlider) |
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.
Do stable scores have NoSliderHeadAccuracy
set?
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.
idk but the desired effects are achieved when using the perfcalc tools so it seems to be fine?
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.
If we are worried about this.
we can add score.IsLegacyScore
to conditions to just in case.
usingClassicSliderMechanics = score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value) || score.IsLegacyScore
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.
If we are worried about this. we can add
score.IsLegacyScore
to conditions to just in case.usingClassicSliderMechanics = score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value) || score.IsLegacyScore
no need to worry about this
you can't have stable score without CL unless someone replay-edited it specifically
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.
just curious, is there any real reason to not have it? (just as an extra safety guard)
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.
just curious, is there any real reason to not have it? (just as an extra safety guard)
the potential reason is when you have some score simulator like Osu-Tools, and you don't wanna see additional IsLegacy
checkboxes everywhere (you don't have to if everything is done correctly but still)
Also please update the op because I got really confused by what am I even looking at at first |
done, new one should be far less confusing |
@Finadoggie for my own understanding while I try to trample through these changes, do your changes account for CL (on default configuration, which matches stable and doesn't have slider head accuracy) being 1:1 with stable? |
This PR replaces estimations with score data where applicable for Lazer scores. More specifically, it…
=== original post ===
(contains outdated info)
On non-CL lazer scores, extra slider data means certain estimations are no longer required.
For effective miss count, HitResults.Miss + HitResults.LargeTickMiss is used.
For sliderends dropped, HitResults.SmallTickMiss is used.
To my understanding, LargeTickMiss includes slider ticks and reverse arrows, the two parts that can break your combo, while SmallTickMiss is for sliderends. Please correct me if I am wrong.