-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 combo scaling from Aim and Speed from osu! performance calculation #16280
Conversation
…l (targets diffspikes)
int speedDifficultyStrainCount = ((OsuStrainSkill)skills[2]).RelevantDifficultStrains(); | ||
|
||
// Total number of strains in a map can vary by clockrate, and this needs to be corrected for. | ||
aimDifficultyStrainCount = (int)(aimDifficultyStrainCount * clockRate); |
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.
Make RelevantDifficultStrains
have clock rate as param instead? So that it'll be ((OsuStrainSkill)skills[0]).RelevantDifficultStrains(clockRate);
and public int RelevantDifficultStrains(double clockRate)
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.
Yup, good idea.
|
||
private double calculateMissPenalty(double missCount, double strainCount) | ||
{ | ||
return 0.95 / ((missCount / (3 * Math.Sqrt(strainCount))) + 1); |
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 probably need some comments
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 really sure what I could comment here, and I don't think it'd end up being very helpful anyway? have you got anything specific in mind
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.
Something about why exactly you need difficult strains here would be good I think. Basically just explain the idea so that people that have no idea what's going on could understand the motivation for this penalty
@@ -57,5 +57,12 @@ public override double DifficultyValue() | |||
|
|||
return difficulty * DifficultyMultiplier; | |||
} | |||
|
|||
public int RelevantDifficultStrains() |
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.
Xmldoc?
aimValue *= 0.97 * Math.Pow(1 - Math.Pow((double)effectiveMissCount / totalHits, 0.775), effectiveMissCount); | ||
|
||
aimValue *= getComboScalingFactor(); | ||
aimValue *= calculateMissPenalty(effectiveMissCount, Attributes.AimDifficultStrainCount); |
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 should calculate miss penalty only once and save it into a variable in my opinion.
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.
the penalties for aim and speed are different because AimDifficultStrainCount
and SpeedDifficultStrainCount
are different, so there is no repetition...
just for clarity, here are the effects the miss penalty are having on the maps here compared to their FC values (no misses):
these plays are atleast a full star above one of the FCs there (make a move is 6.11*, and harumachi clover expert is 4.74*), so you can make a judgement there whether those plays with misses are a similar skill level to the harumachi clover FC. also worth noting that farm maps will be farm either way :P |
Here's a passer-by not really familiar with performance calculation algorithms, but here are some potential issues I see (just in case they're missed): (1) How would sb counts be evaluated? It seems that you use (2) Following (1), because the (3) A hard threshold (66%) is used in calculating the Anyway it's great to see scores with unlucky combobreaks in the middle getting what they deserve. Good job! |
These two issues are out of the scope of this pull request. This pr is for removing the combo scaling. The issues you mentioned are already present in the live version, this pr is not bringing them in. |
Revert "Remove abusable 0.66 threshold by averaging"
i've implemented this this with massive support from @Luminiscental, thanks! point 1 is unfortunately unsolveable without a sliderbreak metric, and @stanriders might be able to give some insight into point 2 the strain count is now weighted against the top strain with a power so that low strain counts have less weighting, should take care of potential abuse cases now O_O |
Fix adding a diffspike reducing PP
Change miss penalty (nerf longer maps)
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.
🎉
double aimDifficultyStrainCount = ((OsuStrainSkill)skills[0]).CountDifficultStrains(); | ||
double speedDifficultyStrainCount = ((OsuStrainSkill)skills[2]).CountDifficultStrains(); |
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.
At some point I'd really like to rid of the indexing of skills because it makes me anxious but future problem..
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.
The time has come.
@smoogipoo this PR is considered approved by osu pp committee and is ready for merge |
@ppy/team-client pinging on request of bdach (sorry for double ping smoogi, not intended) to make it clear these changes are ready but also to provide some direction. This will require 2 new difficulty attributes as a bare minimum to deploy for lazer, I don't know if it's viable to add any new attributes to the database but they're quite small so I'm hoping that it might be possible even if just for this rework. If the added storage requirements are not negotiable then it will have to wait for real time difficulty calculation since there's no way to proceed without the new attributes. To deploy to stable, it's going to require that the score performance processor in osu-queue-score-statistics is in use to prevent the scenario where a lower pp score will overwrite since the old infrastructure is overwriting based on score. I know this isn't new information, I'm just reminding. I'm hoping with this rework being approved alongside a few other reworks being stacked up for review that focus on solving pp infra concerns and eventually running recalculations will be higher priority than so far. This rework is pretty huge in value and in general is resulting in large changes for a lot of users, so it would be great if we could see it deployed and recalculated sooner rather than later. I'm highly aware the infra work is no small task but it would be nice to see it worked on! It'd be nice if we could get some time estimation on how viable it is for these changes to be deployed and recalculated, as if it's going to take a while then we may focus on getting a few other smaller reworks merged in if a recalculation isn't viable soon - equally, I'm keen to push specifically these changes to deploy & recalculation as soon as realistically possible. |
Deployment considerations as far as I can tell:
|
@@ -25,6 +25,8 @@ public class DifficultyAttributes | |||
protected const int ATTRIB_ID_SCORE_MULTIPLIER = 15; | |||
protected const int ATTRIB_ID_FLASHLIGHT = 17; | |||
protected const int ATTRIB_ID_SLIDER_FACTOR = 19; | |||
protected const int ATTRIB_ID_AIM_DIFFICULT_STRAIN_COUNT = 21; |
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.
is this really intended to be duplicate of ATTRIB_ID_SPEED_NOTE_COUNT
?
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 a good (and very scary) catch. I can see how this could have happened, the speed note count thing was added in #15035 which precedes the open date of this PR.
That said I'm not sure I would trust any sheet results at this point given this revelation because I'm not sure what this means for correct calculation. Probably needs a full diffcalc re-run and re-verification of results after fixing.
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.
Oh dear.. luckily no sheets were generated for the latest changes. osu-tools isn't using these attribute IDs and the huismetbenen website that is used by users also doesn't use these so I'm not concerned about the values - but obviously, this needs to be fixed before anything which does use these can be relied upon.
Co-authored-by: James Wilson <[email protected]>
What's going on here? This still has conflicts. Should I look to resolve them or is someone going to handle that? |
To play devil's advocate these conflicts 100% weren't there before. They're probably from #29291. |
Yeah, those conflicts were fresh from recent refactors (not the first time these refactors have caused me issues 🫠). I've resolved them now. |
!diffcalc |
This proposal removes combo scaling from the Aim and Speed skills in osu!, and overhauls their miss penalties. Currently, the performance calculator does not know where the player missed in a map - for example whether it was on the hardest section or an easier section. Therefore, the most fair way to treat misses is to assume they are equal across the map, i.e.
500x/1000x 1 miss
vs.750x/1000x 1 miss
should be weighted similarly.Misses, in general, have been made harsher. The initial miss is going to have a harsher penalty to differentiate between FCs and non-FCs. The scaling is no longer based on the total object count, but instead the number of relevant difficult strains. This is measured by counting the number of strains, dividing them by the top strain and raising to power 4. This results in maps with consistent difficulty being more lenient, and long maps with a short spike in difficulty being treated similarly to shorter maps.
There have been concerns about consistency supposedly mattering less, and I don't think that's the case. Miss counts are a fine metric to rely on here, and again, using combo here doesn't make much sense if we can't tell what parts of the map are hard from performance side, or where the player has missed. I hope that the strain count stuff described in the miss penalty change above can at least rest some of the concerns :P
Values (and a very rough Q&A thingy) can be found at https://pp.huismetbenen.nl/rankings/players/apollo_visual