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

January 2025 Diffcalc/PP release #31595

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

January 2025 Diffcalc/PP release #31595

wants to merge 39 commits into from

Conversation

stanriders
Copy link
Member

@stanriders stanriders commented Jan 20, 2025

This PR includes all changes listed in the diffcalc project marked as Ready for deploy. Some changes were already merged to master before pp-dev branch was created

Deployment considerations:

  • Includes 3 rulesets: osu, taiko and catch. Mania only has a small refactor change that doesn't affect values
  • All rulesets have difficulty changes, catch doesn't have performance changes
  • 4 new difficulty attributes for osu: 2 (GreatHitWindow/OkHitWindow) are reusing IDs from other rulesets, 2 are completely new
  • New taiko attributes are NOT to be databased

osu! changes: https://pp.huismetbenen.nl/rankings/players/master
Taiko changes: https://pp.huismetbenen.nl/rankings/players/master_taiko
Newspost: ppy/osu-wiki#12699

Lawtrohux and others added 30 commits December 18, 2024 18:16
…o being too inflated (#31067)

* low sr

* merge two line

* update decimal

* fix formatting

---------

Co-authored-by: StanR <[email protected]>
…utes (#31191)

* refactor + countdifficultstrain

* norm in utils

* adjust scaling shift

* fix comment

* revert all value changes

* add the else back

* remove cds comments
…31195)

* revert acc scaling shift to previous values

* increase variance in accuracy values across od

* move return values, move nullcheck into return

---------

Co-authored-by: James Wilson <[email protected]>
* Simplify osu! high-bpm acute angle jumps bonus

* Add aim wiggle bonus

* Add hitwindow-based aim velocity decrease

* Revert "Add hitwindow-based aim velocity decrease"

This reverts commit bcebe96.

* Move wiggle multiplier to a const, slightly decrease acute bonus multiplier

* Make sure the previous object in the wiggle bonus is also part of the wiggle

* Scale the wiggle bonus multiplayer down

* Increase the acute angle jump bonus multiplier

* Make wiggle bonus only apply on >150 bpm streams, make repetitive angle penalty

* Reduce wiggle bonus multiplier to not break velocity>difficulty relation

* Adjust wiggle falloff function to fix stability issues

* Adjust wiggle consts

* Update tests
… fix slider drop penalty being too lenient (#31055)

* Change slider drop penalty to use actual number of difficult sliders, fix slider nerf being too lenient

* Move cubing to performance calculation

* Add separate list for slider strains

* Rename difficulty atttribute

* Rename attribute in perfcalc

* Check if AimDifficultSliderCount is more than 0, code quality fixes

* Add `AimDifficultSliderCount` to the list of databased attributes

* Code quality

---------

Co-authored-by: James Wilson <[email protected]>
* Use `lastAngle` when nerfing repeated angles on acute bonus

* Bump acute multiplier

* Correct outdated wiggle bonus comment

* Update test

---------

Co-authored-by: StanR <[email protected]>
* implement bell curve into diffcalcutils

* remove unneeded attributes

* implement new rhythm skill

* change dho variables

* update dho rhythm

* interval interface

* implement rhythmevaluator

* evenhitobjects

* evenpatterns

* evenrhythm

* change attribute ordering

* initial balancing

* change naming to Same instead of Even

* remove attribute bump for display

* Fix diffcalc tests

---------

Co-authored-by: StanR <[email protected]>
…su! ruleset (#21211)

* Set speed distance to 0

* Reduce speed & flashlight, remove aim

* Remove speed AR bonus

* cleanup autopilot mod check in `SpeedEvaluator`

* further decrease speed rating for extra hand availability

* Pass all mods to the speed evaluator, zero out distance bonus instead of distance

---------

Co-authored-by: tsunyoku <[email protected]>
Co-authored-by: StanR <[email protected]>
…scaling to wide bonus (#31320)

* Fix angle bonuses calculating repetition incorrectly, apply distance scaling to wide bonus

* Buff speed to compensate for streams losing pp

* Adjust speed multiplier

* Adjust wide scaling

* Fix tests
* fix colour

* review fix

Co-authored-by: StanR <[email protected]>

* remove cancelled out operand

* increase nerf, adjust tests

* fix automated spacing issues

* up penalty

* adjust tests

* apply review changes

* fix nullable hell

---------

Co-authored-by: StanR <[email protected]>
* Simplify angle bonus formula

* Simplify further

* Simplify acute too

* Tests
* Apply a bunch of balancing changes to aim

* Update tests

---------

Co-authored-by: James Wilson <[email protected]>
* Make aim accuracy scaling harsher

* Use deviation-based scaling

* Bring the balancing multiplier down

* Adjust multipliers, fix incorrect deviation when using slider accuracy

* Adjust multipliers

* Update osu.Game.Rulesets.Osu/Difficulty/OsuPerformanceAttributes.cs

Co-authored-by: James Wilson <[email protected]>

* Change high speed deviation threshold to 22-27 instead of 20-24

* Update tests

---------

Co-authored-by: James Wilson <[email protected]>
* Remove problematic total deviation scaling, rebalance aim

* Fix tests
* stamina considerations

* remove consecutive note count

* adjust multiplier

* add back comment

* adjust tests

* adjusts tests post merge

* use diffcalcutils

---------

Co-authored-by: StanR <[email protected]>
Code quality CI runs have suddenly started failing out of nowhere:

- Passing run: https://github.com/ppy/osu/actions/runs/12806242929/job/35704267944#step:10:1
- Failing run: https://github.com/ppy/osu/actions/runs/12807108792/job/35707131634#step:10:1

In classic github fashion, they began rolling out another runner change
wherein `ubuntu-latest` has started meaning `ubuntu-24.04` rather than
`ubuntu-22.04`. `ubuntu-24.04` no longer has .NET 6 bundled.

Therefore, upgrade NVika to 4.0.0 because that version is compatible
with .NET 8.
* rebalance

* revert pp scaling change

* further rebalancing

* comment

* adjust tests
* further considerations for rhythm

* new rhythm balancing

* fix license header

* use isNormal to validate ratio

* adjust tests

---------

Co-authored-by: StanR <[email protected]>
…ty.Utils (#31520)

* Move error function implementation to osu.Game.Rulesets.Difficulty.Utils

* Rename ErrorFunction.cs to DifficultyCalculationUtils_ErrorFunction.cs
…nsity" (#31512)

* Penalise reading difficulty of high velocity notes at high densities

* Use System for math functions

* Lawtrohux changes

* Clean up density penalty comment

* Swap midVelocity and highVelocity back around

* code quality pass

---------

Co-authored-by: Jay Lawton <[email protected]>
Co-authored-by: StanR <[email protected]>
* Implement fix for catch buzz sliders SR abuse

* Run formatting

---------

Co-authored-by: StanR <[email protected]>
Lawtrohux and others added 5 commits January 19, 2025 21:55
* return a higher finger count

* implement isConvert

* diffcalc cleanup

* harshen monostaminafactor accuracy curve

* readd comment

* adjusts tests
* adjust straincount to assume 1300

* remove comment

---------

Co-authored-by: StanR <[email protected]>
…lty (#31573)

* Replace long interval nerf with a new one that uses stamina difficulty

* Turn tabs into spaces

* Update unit tests

---------

Co-authored-by: StanR <[email protected]>
…0034)

* Replace indexed skill access with `skills.First(s is ...)`

* Fix comment

* Further refactoring to remove casts

---------

Co-authored-by: Dan Balasescu <[email protected]>
Co-authored-by: Bartłomiej Dach <[email protected]>
…y calculator (#31579)

* Use correct `HitWindows` class for osu!taiko hit windows in difficulty calculator

* Remove redundant (and incorrect) hit window creation

* Balance rhythm against hit window changes
@stanriders
Copy link
Member Author

!diffcalc
RULESET=osu

@stanriders
Copy link
Member Author

!diffcalc
RULESET=taiko

@stanriders
Copy link
Member Author

!diffcalc
RULESET=catch

Copy link

Copy link

Copy link

@peppy peppy self-requested a review January 21, 2025 04:21
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

initial review. taiko code is weird.

@@ -30,6 +30,8 @@ public class DifficultyAttributes
protected const int ATTRIB_ID_AIM_DIFFICULT_STRAIN_COUNT = 25;
protected const int ATTRIB_ID_OK_HIT_WINDOW = 27;
protected const int ATTRIB_ID_MONO_STAMINA_FACTOR = 29;
protected const int ATTRIB_ID_AIM_DIFFICULT_SLIDER_COUNT = 31;
Copy link
Member

Choose a reason for hiding this comment

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

Are these new attributes documented somewhere? Which ruleset are they for? Should they be persisted to the database (apparently some shouldn't now?)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the osu's new attributes, I'm not sure how we should document them code wise other than just through the usage

@@ -73,7 +73,10 @@ protected override PerformanceAttributes CreatePerformanceAttributes(ScoreInfo s

private double computeDifficultyValue(ScoreInfo score, TaikoDifficultyAttributes attributes)
{
double difficultyValue = Math.Pow(5 * Math.Max(1.0, attributes.StarRating / 0.115) - 4.0, 2.25) / 1150.0;
double baseDifficulty = 5 * Math.Max(1.0, attributes.StarRating / 0.110) - 4.0;
double difficultyValue = Math.Min(Math.Pow(baseDifficulty, 3) / 69052.51, Math.Pow(baseDifficulty, 2.25) / 1250.0);
Copy link
Member

Choose a reason for hiding this comment

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

These inline undocumented random numbers are something.

Could we just add a single liner explaining where they come from?

Copy link
Member

Choose a reason for hiding this comment

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

#31067

image

@YaniFR could you link a desmos link for the modified curve here please?

Copy link

Choose a reason for hiding this comment

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

/// </summary>
public interface IHasInterval
{
double Interval { get; }
Copy link
Member

Choose a reason for hiding this comment

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

What's an interval?

Copy link
Member

@Lawtrohux Lawtrohux Jan 21, 2025

Choose a reason for hiding this comment

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

Interval referring the rhythmic value, startime to startime of current and previous objects, so we can garner the changes in rhythm (a change in interval // spacing of an object), and being outputted eventually as a ratio. Originally having a tiny class for solely an interface was questionable, however it was the best outcome I could think of at the time.

/// <summary>
/// Represents <see cref="SameRhythmHitObjects"/> grouped by their <see cref="SameRhythmHitObjects.StartTime"/>'s interval.
/// </summary>
public class SamePatterns : SameRhythm<SameRhythmHitObjects>
Copy link
Member

Choose a reason for hiding this comment

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

this is oop hell. without reading any implementation, none of these class names sound sane.

Copy link
Member

Choose a reason for hiding this comment

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

Originally, this was labelled as Even instead of Same, with both referring to the objects being grouped by their ratio (rhythm), thus being a same rhythm object, or a same rhythm pattern.

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.

Initial review (catch and osu!)

private double lastStrainTime;
private bool isBuzzSliderTriggered;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about isInBuzzSection?

Comment on lines +121 to +123
GreatHitWindow = hitWindowGreat,
OkHitWindow = hitWindowOk,
MehHitWindow = hitWindowMeh,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, I'm not keen on these being databased. Will be removing them either after this branch is merged or after deploy. Needs a few considerations (including server-side).

Copy link
Member

Choose a reason for hiding this comment

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

What would you want to do in it's place? Databasing OD and calculating them in the performance calculator?

Copy link
Contributor

@smoogipoo smoogipoo Jan 21, 2025

Choose a reason for hiding this comment

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

Correct, they're already in the database as osu_beatmaps.diff_overall etc:

https://github.com/ppy/osu-queue-score-statistics/blob/ce1032b7cac62a5fc1e028e54134cf1970cedc11/osu.Server.Queues.ScoreStatisticsProcessor/Models/Beatmap.cs#L30-L33

In theory, the only thing required is something like this:

diff --git a/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs b/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs
index 260ad39..147777b 100644
--- a/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs
+++ b/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs
@@ -87,7 +87,8 @@ namespace osu.Server.Queues.ScoreStatisticsProcessor.Models
             var beatmapInfo = new LightweightBeatmapInfo
             {
                 OnlineID = (int)beatmap_id,
-                Ruleset = new RulesetInfo { OnlineID = beatmap!.playmode }
+                Ruleset = new RulesetInfo { OnlineID = beatmap!.playmode },
+                Difficulty = new BeatmapDifficulty(beatmap.GetLegacyBeatmapConversionDifficultyInfo())
             };
 
             return new LightweightScoreInfo

After which it should arrive in the performance calculator's score.Beatmap.

(May be better to copy values directly than via the helper method, so don't treat that part as gospel).

Copy link
Member

@tsunyoku tsunyoku Jan 21, 2025

Choose a reason for hiding this comment

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

That's not going to adjust for mods though, right? Would we have to do that ourselves? That's already how it works, this seems simple enough.

{
// We add tick misses here since they too mean that the player didn't follow the slider properly
// We however aren't adding misses here because missing slider heads has a harsh penalty by itself and doesn't mean that the rest of the slider wasn't followed properly
estimateImproperlyFollowedDifficultSliders = Math.Min(countSliderEndsDropped + countSliderTickMiss, attributes.AimDifficultSliderCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous code, this was a Math.Clamp(). This was brought up in a PR at the time (#30544) so I'd like that to still be the case here.

@tsunyoku
Copy link
Member

tsunyoku commented Jan 21, 2025

After reading initial review I am not particularly fond of some of the Taiko code, specifically the SameRhythm abstraction and related parts. I spent some time cleaning things up on another branch but I'm not sure if trying to push a refactor through while reviewing for a deploy is a good idea, although it would probably be easier to review the taiko code with it.

PR: #31636

@tsunyoku
Copy link
Member

tsunyoku commented Jan 21, 2025

Also, @stanriders and I cannot address any review comments directly as we can't push to the pp-dev branch (we'd have to separately PR to resolve any of them I guess)

@smoogipoo
Copy link
Contributor

We can't fix that with branch rules. I think a separate PR to pp-dev is fine? We can merge that PR only when everything's reviewed + fixed (peppy and I haven't finished our reviews yet) and maintain the single commit quality?

@peppy peppy self-requested a review January 23, 2025 14:44
Copy link
Member

@BabySnakes101 BabySnakes101 left a comment

Choose a reason for hiding this comment

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

Taiko looks good, some code and magic could be clearer but hopefully some refactoring can be done in the near future.

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.