Include sliders in accuracy pp if slider head accuracy is in use #27063

tsunyoku · 2024-02-06T13:21:04Z

Existed originally at #25722 but slider judgment decisions were premature at the time; as far as I'm aware, that's not the case anymore.

In cases where slider accuracy is used, accuracy pp should be adjusted to include sliders too. This should be fine to merge as-is as the change is purely on pp and no changes to diff calc attributes are required.

This isn't the only change required for slider accuracy to be accounted for, however the remaining ones require difficulty calculation changes (i.e extra attributes) so I'm leaving those out until we're at a point that those can be facilitated.

apollo-dw · 2024-02-06T13:37:04Z

@bdach The linked PR was closed for being premature. Could we be assured now that slider mechanics won't be changing?

bdach · 2024-02-06T13:46:28Z

I'm not aware of any complaints about slider mechanics after the changes we've applied over the course of December so I suppose I don't see any reason that they'd be changed.

Note that I'm not saying that this is going to merge imminently - we've still got work to do to stabilise infra. Review was requested just to ensure you (as in the pp committee) are on board with the change.

tsunyoku · 2024-02-06T13:57:12Z

As requested, some data to showcase the changes:

Beatmap ID	master total PP	PR total PP	master accuracy PP	PR accuracy PP
1893461	201.48	214.14	60.64	72.92
1892257	326.38	337.46	103.30	114.02
3718752	227.68	244.01	77.61	93.26
1537566	647.73	661.44	203.26	216.56
1256809	493.05	493.05	130.86	130.86

This shows that accuracy PP caps out as expected.

apollo-dw

LGTM, no real wiggle room to dispute this IMO.

stanriders · 2024-02-12T08:41:13Z

osu.Game.Rulesets.Osu/Difficulty/OsuPerformanceCalculator.cs

@@ -192,6 +192,8 @@ private double computeAccuracyValue(ScoreInfo score, OsuDifficultyAttributes att
            // This percentage only considers HitCircles of any value - in this part of the calculation we focus on hitting the timing hit window.
            double betterAccuracyPercentage;
            int amountHitObjectsWithAccuracy = attributes.HitCircleCount;
+            if (!score.Mods.Any(h => h is OsuModClassic cl && cl.NoSliderHeadAccuracy.Value))


The change itself is fine, but I'll raise the same point I keep bringing in all other lazer-related pp PRs - I strongly believe we need to split calculators into classic and non-classic because of the code bloat (lots of possible if lazer or classic all around the calc) and more complete score data on new scores that we can (I hope?) utilise.

all the difference lazer have is new information, so it's not a lot of code bloat, around 10-15 additional lines to covers this i think
also, this thing is not a lazer vs stable, it's CL vs non CL

All stable scores are CL by default and all lazer scores are non-CL by default. Treating lazer CL differently compared to stable auto-CL doesn't make much sense therefore CL scores must always use stable score data structure.

Mod ifs are already a big mess in pp calc now, adding CL handling on top would make it even more convoluted. Keep in mind that we have to handle both extended score data and legacy score data since stable is here to stay

I agree that it might get a little messy to sprinkle CL checks everywhere, however I equally see it being messy having to maintain 2 separate calculators that will share (mostly) the same logic. Having to make identical changes in 2 places whenever PP changes doesn't sound like a particularly great alternative.

Someone brought this up during discussion, and from what I can tell, there's no real reason why this wasn't done either. All this does is use the number of circles instead of the number of objects for accuracy calculations, since circles are the only objects that check for accuracy. Could also be merged with ppy#27063 to use circles + sliders when sliders check for accuracy (aka when classic mod is disabled). Currently the code for that is included but commented out.

stanriders

👍

@apollo-dw would you mind confirming that you still approve this PR?

apollo-dw · 2024-05-30T10:13:28Z

@apollo-dw would you mind confirming that you still approve this PR?

Good to go on my end 👍

stanriders · 2024-05-30T13:15:19Z

@smoogipoo this one is good to go

smoogipoo · 2024-05-30T13:27:17Z

!diffcalc

github-actions · 2024-05-30T13:27:37Z

Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/9303393736

bdach · 2024-05-31T05:41:23Z

Diffcalc failure looks to be because of outdated master:

2024-05-30T17:23:20.7103922Z execution-9303393736-9659-1-verify-1     | /tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Models/Beatmap.cs(47,13): error CS0117: 'LegacyBeatmapConversionDifficultyInfo' does not contain a definition for 'DrainRate' [/tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/osu.Server.Queues.ScoreStatisticsProcessor.csproj]
2024-05-30T17:23:20.7117650Z execution-9303393736-9659-1-verify-1     | /tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Models/Beatmap.cs(48,13): error CS0117: 'LegacyBeatmapConversionDifficultyInfo' does not contain a definition for 'ApproachRate' [/tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/osu.Server.Queues.ScoreStatisticsProcessor.csproj]
2024-05-30T17:23:20.7121806Z execution-9303393736-9659-1-verify-1     | /tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Helpers/LegacyDatabaseHelper.cs(83,17): warning CS0162: Unreachable code detected [/tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/osu.Server.Queues.ScoreStatisticsProcessor.csproj]
2024-05-30T17:23:20.7126117Z execution-9303393736-9659-1-verify-1     | /tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs(112,17): error CS0117: 'ScoreInfo' does not contain a definition for 'Ranked' [/tmp/tmp.3uZQZH05Qn/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/osu.Server.Queues.ScoreStatisticsProcessor.csproj]
2024-05-30T17:23:20.7209999Z execution-9303393736-9659-1-verify-1     | 
2024-05-30T17:23:20.7210796Z execution-9303393736-9659-1-verify-1     | The build failed. Fix the build errors and run again.

Merging master again and re-running:

!diffcalc

github-actions · 2024-05-31T05:41:43Z

Target: #27063
Spreadsheet: https://docs.google.com/spreadsheets/d/150RFYxVH1aRQ1wh5NOp2tqYiPjuK1fdUtZGMvpmlgZs/edit

apollo-dw · 2024-07-13T10:36:28Z

@smoogipoo I believe this is good to go, since this PR affects the performance calculator only?

EDIT: NVM just noticed this is on "pending review"

smoogipoo · 2024-07-16T03:24:50Z

!diffcalc

github-actions · 2024-07-16T03:25:13Z

Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/9950229982

smoogipoo · 2024-07-18T04:54:58Z

!diffcalc

github-actions · 2024-07-18T04:55:15Z

Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/9985739398

smoogipoo · 2024-07-19T04:50:19Z

!diffcalc

github-actions · 2024-07-19T04:50:40Z

Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/10002753749

smoogipoo · 2024-07-19T07:45:26Z

!diffcalc

github-actions · 2024-07-19T07:45:48Z

Difficulty calculation queued -- please wait! (https://github.com/ppy/osu/actions/runs/10004491429)

This comment will update on completion

include slider count in accuracy pp if slider head accuracy is in use

8ccb14f

pull-request-size bot added the size/XS label Feb 6, 2024

bdach added ruleset/osu! area:difficulty labels Feb 6, 2024

bdach requested a review from a team February 6, 2024 13:25

apollo-dw approved these changes Feb 12, 2024

View reviewed changes

stanriders reviewed Feb 12, 2024

View reviewed changes

Flamiii mentioned this pull request Mar 22, 2024

Remove estimations where score data is available for osu! difficulty calculations #27691

Open

tsunyoku mentioned this pull request May 20, 2024

Don't punish slider hit windows in difficulty calculations when slider head accuracy is in use #28267

Open

stanriders approved these changes May 27, 2024

View reviewed changes

Merge branch 'master' into account-for-sliders-in-accuracy-pp

b272252

Invert condition to reduce number of brain flips required

fcc8e7b

smoogipoo previously approved these changes Jul 16, 2024

View reviewed changes

Even better readability

ced11e6

smoogipoo dismissed their stale review via ced11e6 July 16, 2024 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include sliders in accuracy pp if slider head accuracy is in use #27063

Include sliders in accuracy pp if slider head accuracy is in use #27063

tsunyoku commented Feb 6, 2024 •

edited

Loading

apollo-dw commented Feb 6, 2024

bdach commented Feb 6, 2024

tsunyoku commented Feb 6, 2024

apollo-dw left a comment

stanriders Feb 12, 2024

Givikap120 Feb 12, 2024

stanriders Feb 12, 2024

tsunyoku Feb 12, 2024

stanriders left a comment

apollo-dw commented May 30, 2024

stanriders commented May 30, 2024

smoogipoo commented May 30, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

bdach commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

apollo-dw commented Jul 13, 2024 •

edited

Loading

smoogipoo commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 •

edited

Loading

smoogipoo commented Jul 18, 2024

github-actions bot commented Jul 18, 2024 •

edited

Loading

smoogipoo commented Jul 19, 2024

github-actions bot commented Jul 19, 2024 •

edited

Loading

smoogipoo commented Jul 19, 2024

github-actions bot commented Jul 19, 2024

Include sliders in accuracy pp if slider head accuracy is in use #27063

Are you sure you want to change the base?

Include sliders in accuracy pp if slider head accuracy is in use #27063

Conversation

tsunyoku commented Feb 6, 2024 • edited Loading

apollo-dw commented Feb 6, 2024

bdach commented Feb 6, 2024

tsunyoku commented Feb 6, 2024

apollo-dw left a comment

Choose a reason for hiding this comment

stanriders Feb 12, 2024

Choose a reason for hiding this comment

Givikap120 Feb 12, 2024

Choose a reason for hiding this comment

stanriders Feb 12, 2024

Choose a reason for hiding this comment

tsunyoku Feb 12, 2024

Choose a reason for hiding this comment

stanriders left a comment

Choose a reason for hiding this comment

apollo-dw commented May 30, 2024

stanriders commented May 30, 2024

smoogipoo commented May 30, 2024

github-actions bot commented May 30, 2024 • edited Loading

bdach commented May 31, 2024

github-actions bot commented May 31, 2024 • edited Loading

apollo-dw commented Jul 13, 2024 • edited Loading

smoogipoo commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 • edited Loading

smoogipoo commented Jul 18, 2024

github-actions bot commented Jul 18, 2024 • edited Loading

smoogipoo commented Jul 19, 2024

github-actions bot commented Jul 19, 2024 • edited Loading

smoogipoo commented Jul 19, 2024

github-actions bot commented Jul 19, 2024

tsunyoku commented Feb 6, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

apollo-dw commented Jul 13, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading