Change unstable rate calculation to account for rate-change mods

[Poyo-SSB]

In community parlance, cv. UR (converted unstable rate) is unstable rate divided by the rate-change of the mods used. This accounts for UR not scaling against rate-change, making it possible to compare UR on scores with or without rate-change mods. PR #24812 proposed adding a toggle to the unstable rate HUD counter which would display cv. UR, but it was ultimately decided in discussion that cv. UR shouldn't need to exist and that UR should always correspond to milliseconds no matter the rate change.

This PR does just that by storing the local gameplay rate in HitEvent and JudgementResult and accounting for it when calculating UR. This approach has the benefit of working with any type of rate change, including static (e.g. Double Time), linear (e.g. Wind Up), and dynamic (e.g. Adaptive Speed).

[bdach]

I'm still not sure this is correct applied at this high a level, because of mania. mania adjusts hitwindows so they are no longer actually dependent on gameplay rate, so adjusting offset based on rate there may not make sense anymore...

@smoogipoo you probably know the most about mania of all of us, do you have an opinion on this?

[Poyo-SSB]

My understanding is that the way mania rate adjustments work is that the hit windows are scaled to cancel out the rate adjustment's effect on the offsets (see IManiaRateAdjustmentMod). So if I'm understanding how things work correctly, the offsets themselves scale the same as in other rulesets, and the UR calculations should still be correct.

[peppy]

I do believe the above holds true, as UR does not use hit windows to calculate.

[bdach]

I was going to apply some follow-up changes to this myself to speed the proceedings up, but the way this PR and the fork it comes from are set up make this impossible. I am not sure what ever the reason for having the osu fork live under https://github.com/Poyo-SSB-forks is, if there even is one, but this makes it not possible for the maintainer(s) to apply their own changes. If this is by design then this is annoying.

So, @Poyo-SSB, if you would be so inclined as to apply the following:

commit 96746816dccaa3594468c3a3e27855cfe3b38e24
Author: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Date:   Mon Nov 20 18:25:49 2023 +0900

    Use better fallback
    
    Seems better to use the rate from a non-gameplay clock than to
    arbitrarily apply 1.

diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
index 5abca168ed..1daaa24d57 100644
--- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
+++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
@@ -704,7 +704,7 @@ protected void ApplyResult(Action<JudgementResult> application)
             }
 
             Result.RawTime = Time.Current;
-            Result.GameplayRate = (Clock as IGameplayClock)?.GetTrueGameplayRate() ?? 1.0;
+            Result.GameplayRate = (Clock as IGameplayClock)?.GetTrueGameplayRate() ?? Clock.Rate;
 
             if (Result.HasResult)
                 updateState(Result.IsHit ? ArmedState.Hit : ArmedState.Miss);

commit 6a62a56e67863b49b06a11cc2a1f83ea294c8910
Author: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Date:   Mon Nov 20 18:29:06 2023 +0900

    Rewrite backwards assertion

diff --git a/osu.Game/Rulesets/Scoring/HitEventExtensions.cs b/osu.Game/Rulesets/Scoring/HitEventExtensions.cs
index 70a11ae760..9fb61c6cd9 100644
--- a/osu.Game/Rulesets/Scoring/HitEventExtensions.cs
+++ b/osu.Game/Rulesets/Scoring/HitEventExtensions.cs
@@ -19,7 +19,7 @@ public static class HitEventExtensions
         /// </returns>
         public static double? CalculateUnstableRate(this IEnumerable<HitEvent> hitEvents)
         {
-            Debug.Assert(!hitEvents.Any(ev => ev.GameplayRate == null));
+            Debug.Assert(hitEvents.All(ev => ev.GameplayRate != null));
 
             // Division by gameplay rate is to account for TimeOffset scaling with gameplay rate.
             double[] timeOffsets = hitEvents.Where(affectsUnstableRate).Select(ev => ev.TimeOffset / ev.GameplayRate!.Value).ToArray();

then that would be swell thanks. Author under your name please, I don't want to have to deal with the commit showing as unverified or something either.

[Poyo-SSB]

Sorry about the fork not being editable by maintainers. The organization was supposed to keep my personal page tidy, but I guess that functionality being disabled makes it a nonstarter. I'll contribute from a personal-account fork in future to avoid this.

[smoogipoo]

Seems ok to me

[huyenden]

At first, when I applied this update, I believed it would be beneficial. However, I've come to deeply regret it, as the Unstable Rate (UR) no longer accurately reflects the player's skill level.

Prior to this update, players using the Half Time (HT) modifier typically achieved lower UR scores compared to those playing with no modifiers (NM), and NM players had lower UR than Double Time (DT) players. This made it clear that when playing with HT, a player's ability to perceive music would be superior to playing with NM, leading to the expectation that as HT decreased UR, NM would follow suit, similar to the principle applied to DT.

However, following this update, HT now yields higher UR scores than DT, suggesting that a player's performance with HT is inferior to that with DT. This inference is inaccurate.

I earnestly desire the option to revert this UR/speed coefficient back to its original state through a toggle button. This would restore the integrity of UR as an indicator of player skill, allowing for fairer comparisons across different speed modifiers.

[peppy]

At the end of the day it really depends on what you want. With this PR applied, comparing UR is "fair" across all rates. Without this PR, it is unfair to compare UR without also mentioning which mods were played with, because getting a lower UR with half time applied would have been basically guaranteed.

It's subjective and the one which the community finds most intuitive will likely be the one that is continued to be adopted.

[huyenden]

At the end of the day it really depends on what you want. With this PR applied, comparing UR is "fair" across all rates. Without this PR, it is unfair to compare UR without also mentioning which mods were played with, because getting a lower UR with half time applied would have been basically guaranteed.

It's subjective and the one which the community finds most intuitive will likely be the one that is continued to be adopted.

While I agree that community input is crucial, I believe many players would favor having lower URs in Double Time (DT) simply because they look aesthetically pleasing. This preference may override considerations about whether the rhythm perception in UR is better than No Mod (NM) or not. If there's a possibility of incorporating a conversion calculation in the UR HUD element as mentioned in issue #24812, it could address these concerns. However, I understand this is just my personal preference and may not align with the broader community sentiment.

[Rekunan]

suggesting that a player's performance with HT is inferior to that with DT. This inference is inaccurate.

I would actually disagree with this, as on the same map, HT is going to be inherently easier than DT.

I believe many players would favor having lower URs in Double Time (DT) simply because they look aesthetically pleasing.

I also disagree with this, the reason converted UR exists is that it's accurate as a direct translation, whereas unconverted isn't.

If someone gets a larger UR on HT than NM, and NM than DT, then perhaps the player actually has a lower perception of music with it, which is possible since they also lower the AR as a side effect.

[huyenden]

suggesting that a player's performance with HT is inferior to that with DT. This inference is inaccurate.

I would actually disagree with this, as on the same map, HT is going to be inherently easier than DT.

I believe many players would favor having lower URs in Double Time (DT) simply because they look aesthetically pleasing.

I also disagree with this, the reason converted UR exists is that it's accurate as a direct translation, whereas unconverted isn't.

If someone gets a larger UR on HT than NM, and NM than DT, then perhaps the player actually has a lower perception of music with it, which is possible since they also lower the AR as a side effect.

Oh, actually you're agreeing with what I was thinking the same way

[huyenden]

Maybe it's just me who feels sad about this, UR no longer seems to have any meaning as a reference for speed and awareness anymore because cvUR is gone.