mirror of
https://github.com/ppy/osu.git
synced 2026-06-04 13:44:26 +08:00
master
3 Commits
-
Add beatmap difficulty before mods as context for score multiplier calculations (#37921)
- Part of https://github.com/ppy/osu/issues/37818 Access to difficulty info is required for the upcoming multiplier proposals. All places providing difficulty info intentionally use `IBeatmapInfo` as the difficulty info exposed to the calculator should _always_ be pre-mods for our usecase. There's a couple of quirks: - The usage in `ScoreProcessor` is a bit troubling to me but I can't see a way to make it better without refactoring it. Essentially, we don't have a beatmap until `ApplyBeatmap` is called, but most usages of `ScoreProcessor` are setting `Mods` prior to `ApplyBeatmap` so there is a `null` check in the logic for when mods change. Additionally, this means a new bindable of the beatmap via `ApplyBeatmap` which also feels a bit dirty. Open to suggestions. - ~~`BeatmapLeaderboardScore.Tooltip` is using a null-forgiving on the `BeatmapInfo`, but there's basically no context available on if this is an issue - the only code path which sets the score is `SetContent` which has no callers, so it's essentially dead code. Makes sense given it's Select V1.~~ --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
James Wilson ·
2026-05-28 12:44:59 +02:00 -
Replace usages of
Mod.ScoreMultiplierwith new score multiplier API (#37845)- Part of https://github.com/ppy/osu/issues/37818 During review, I would like to direct particular attention to the following changes: ## [Migrate song select to new score multiplier API](https://github.com/ppy/osu/commit/945fd78539da3ae57d1550a5bbfb0f859d153cc4) This was a confusing change to write because of the way song selects hook their mod overlays up to global bindables. In particular different things happen in different circumstances. - When going through `SongSelect.CreateModOverlay()`, which is called by the base `SongSelect`, the mod overlay is automatically bound to global bindables via `SongSelect.on{ArrivingAt,Leaving}Screen()`. - For multiplayer user mod select overlays, which are bolted on by subclasses of `SongSelect`, manual hook-up is required. - As for free mod select overlays, they don't show mod multipliers at all, and don't have easy access to the ruleset, and thus the hookup is skipped entirely as redundant. ## [Fix score multiplier registrations being shared between implementations via superclass static fields](https://github.com/ppy/osu/commit/ba0a7ad421e0c84c2d8162b6bbdd3a0683f5a6a6) Revealed by `ScoreMultiplierCalculatorTest` starting to fail due to interference from `OsuScoreMultiplierCalculator`. It's not ideal from a performance standpoint but it's the simplest choice for now. Tricks could be pulled to salvage the static. One is ```csharp public class ScoreMultiplierCalculator<T> where T : ScoreMultiplierCalculator<T> { } ``` This works because of generics internals; static instance members are not shared between different specialisations of a generic class. It is also very unintuitive, so I would rather not. (It trips a ReSharper inspection too, which would have to be silenced.) From a performance standpoint this is not ideal, but a significant chunk of migrated usages already precede the construction of the calculator via the known-expensive `RulesetInfo.CreateInstance()`, and the paths that actually construct the calculator do not appear to be that hot. If need be, this can be handled by actually caching ruleset instances and their derivative subcomponents. ## [Introduce passing of context to score multiplier calculator](https://github.com/ppy/osu/pull/37845/changes/9e9242b3221dddacd226f4b3b9c5632d7350e998) This is required for two reasons: - The upcoming mod rebalance will require out-of-band supplementary information that is not available for reading from the mod instances themselves for calculating the multiplier. - This context, namely passing of `ScoreInfo`, will be used for implementing backwards compatibility with old scores and their score multipliers. This is required because it has turned out under inspection that all server-side lazer replays recorded until now are missing `TotalScoreWithoutMods` due to an omission of not sending it across the wire to spectator server. Because the score import flow uses replays, filtered through `LegacyScoreDecoder`, to populate total score in the realm database, it is basically impossible to ignore scores that are missing `TotalScoreWithoutMods`, because that will result in bug reports that the scores do not have the new score multipliers applied. Thus, passing of `ScoreInfo` will facilitate implementation of versioning score multipliers, which should result in less breakage than not doing so. An example of this is added in
341b2d6e55, which should handle the case of mania mod multipliers having been changed without any attempt to facilitate for it in https://github.com/ppy/osu/pull/30506. --------- Co-authored-by: Dean Herbert <pe@ppy.sh>Bartłomiej Dach ·
2026-05-26 18:02:15 +09:00 -
Add score multiplier calculator API (#37822)
- Part of https://github.com/ppy/osu/issues/37818 - Continued from https://github.com/ppy/osu/pull/37355#issuecomment-4449248107 ## Overview This PR introduces an alternative API, `ScoreMultiplierCalculator`, to be used going forward for calculating mod multipliers. The reason for introducing this new API is that it has been requested that: - For any two given mods, it should be possible to have the combined mod multipliers of them in combination be *different* than the product of the individual mods' multipliers in isolation, i.e. $mult( \\{ A, B \\} ) \neq mult( \\{ A \\} ) \cdot mult( \\{ B \\} )$. - For an individual mod, it should be possible to have the mod multipliers depend on a quantity that is *not* the presence of another mod or the direct value of a setting on the mod. This capability is being demonstrated in this PR via the `osu.Game.Tests.Rulesets.Scoring.ScoreMultiplierCalculatorTest` test fixture. ## Parity with `Mod.ScoreMultiplier` This PR contains a `ScoreMultiplierCalculator` implementation for each of the built-in four rulesets. The abstract `osu.Game.Tests.Rulesets.RulesetScoreMultiplierTest` and its four derived ruleset-specific test fixtures were written to ensure that the new implementations do not diverge from the current state of affairs. `Mod.ScoreMultiplier` is not removed in this diff to keep size low. It will be removed as a follow-up. ## Performance This PR contains a benchmark comparing the current implementation via `Mod.ScoreMultiplier` and the new `ScoreMultiplierCalculator` API. Results below. <details> | Method | Times | Mods | Mean | Error | StdDev | Gen0 | Allocated | |---------------------- |------ |--------------------- |--------------:|------------:|------------:|--------:|----------:| | ViaModScoreMultiplier | 1 | mods (...)tings [27] | 121.171 ns | 1.5284 ns | 1.4297 ns | 0.0782 | 656 B | | ViaCalculator | 1 | mods (...)tings [27] | 248.509 ns | 1.9313 ns | 1.6127 ns | 0.1364 | 1144 B | | ViaModScoreMultiplier | 1 | multiple mods | 128.357 ns | 0.4282 ns | 0.4006 ns | 0.0782 | 656 B | | ViaCalculator | 1 | multiple mods | 252.953 ns | 1.2860 ns | 1.2029 ns | 0.1364 | 1144 B | | ViaModScoreMultiplier | 1 | no mods | 3.007 ns | 0.0345 ns | 0.0288 ns | - | - | | ViaCalculator | 1 | no mods | 14.802 ns | 0.0616 ns | 0.0576 ns | 0.0134 | 112 B | | ViaModScoreMultiplier | 1 | single mod | 40.271 ns | 0.1238 ns | 0.1098 ns | 0.0258 | 216 B | | ViaCalculator | 1 | single mod | 113.033 ns | 0.3140 ns | 0.2937 ns | 0.0842 | 704 B | | ViaModScoreMultiplier | 1 | single mod 2 | 3.653 ns | 0.0384 ns | 0.0359 ns | 0.0038 | 32 B | | ViaCalculator | 1 | single mod 2 | 78.172 ns | 0.0680 ns | 0.0603 ns | 0.0621 | 520 B | | ViaModScoreMultiplier | 10 | mods (...)tings [27] | 1,169.609 ns | 4.3058 ns | 4.0276 ns | 0.7839 | 6560 B | | ViaCalculator | 10 | mods (...)tings [27] | 2,575.264 ns | 21.2705 ns | 19.8964 ns | 1.3657 | 11440 B | | ViaModScoreMultiplier | 10 | multiple mods | 1,171.775 ns | 6.2332 ns | 5.2050 ns | 0.7839 | 6560 B | | ViaCalculator | 10 | multiple mods | 2,579.593 ns | 22.1010 ns | 20.6733 ns | 1.3657 | 11440 B | | ViaModScoreMultiplier | 10 | no mods | 35.943 ns | 0.1665 ns | 0.1476 ns | - | - | | ViaCalculator | 10 | no mods | 154.980 ns | 0.2381 ns | 0.1988 ns | 0.1338 | 1120 B | | ViaModScoreMultiplier | 10 | single mod | 404.185 ns | 1.3190 ns | 1.2338 ns | 0.2580 | 2160 B | | ViaCalculator | 10 | single mod | 1,167.279 ns | 6.1641 ns | 5.7659 ns | 0.8411 | 7040 B | | ViaModScoreMultiplier | 10 | single mod 2 | 42.128 ns | 0.2878 ns | 0.2692 ns | 0.0382 | 320 B | | ViaCalculator | 10 | single mod 2 | 775.435 ns | 2.3318 ns | 2.1811 ns | 0.6208 | 5200 B | | ViaModScoreMultiplier | 100 | mods (...)tings [27] | 11,623.346 ns | 51.7174 ns | 43.1863 ns | 7.8430 | 65600 B | | ViaCalculator | 100 | mods (...)tings [27] | 25,252.987 ns | 44.4352 ns | 39.3906 ns | 13.6719 | 114400 B | | ViaModScoreMultiplier | 100 | multiple mods | 11,928.536 ns | 35.2079 ns | 32.9334 ns | 7.8430 | 65600 B | | ViaCalculator | 100 | multiple mods | 25,399.378 ns | 152.4597 ns | 127.3108 ns | 13.6719 | 114400 B | | ViaModScoreMultiplier | 100 | no mods | 328.158 ns | 0.5827 ns | 0.5165 ns | - | - | | ViaCalculator | 100 | no mods | 1,517.485 ns | 10.2304 ns | 9.5695 ns | 1.3390 | 11200 B | | ViaModScoreMultiplier | 100 | single mod | 3,986.251 ns | 24.2523 ns | 21.4991 ns | 2.5787 | 21600 B | | ViaCalculator | 100 | single mod | 11,479.514 ns | 23.3738 ns | 20.7203 ns | 8.4076 | 70400 B | | ViaModScoreMultiplier | 100 | single mod 2 | 385.679 ns | 3.5190 ns | 3.2917 ns | 0.3824 | 3200 B | | ViaCalculator | 100 | single mod 2 | 7,658.646 ns | 21.8274 ns | 19.3494 ns | 6.2103 | 52000 B | </details> While the calculator is obviously slower, in my view it is not egregiously so. The main overheads both time- and memory-wise are collection allocations for the dictionary and the set which I consider to be directly caused by the requested additional complexity and as such I don't really consider them eliminable. I have tried and applied some micro-optimisations (
e2469ce338,cb33abec17), albeit with negligible effect. I have also tried to key the mods by `Acronym` instead of by `Type` and the difference was basically nil. <details> <summary>patch for keying by acronym</summary> ```diff diff --git a/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs b/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs index 772f9d178b..7f5907cbda 100644 --- a/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs +++ b/osu.Game/Rulesets/Scoring/ScoreMultiplierCalculator.cs @@ -13,26 +13,26 @@ namespace osu.Game.Rulesets.Scoring /// </summary> public class ScoreMultiplierCalculator { - private static readonly List<(Type[] mods, Func<Mod[], double> multiplier)> combination_multipliers = []; - private static readonly Dictionary<Type, Func<Mod, ScoreMultiplierCalculator, double>> single_multipliers_with_context = []; - private static readonly Dictionary<Type, Func<Mod, double>> single_multipliers = []; + private static readonly List<(string[] modAcronyms, Func<Mod[], double> multiplier)> combination_multipliers = []; + private static readonly Dictionary<string, Func<Mod, ScoreMultiplierCalculator, double>> single_multipliers_with_context = []; + private static readonly Dictionary<string, Func<Mod, double>> single_multipliers = []; /// <summary> /// Defines a flat, setting-independent score multiplier for the given <typeparamref name="TMod"/>. /// </summary> public static void Single<TMod>(double hasMultiplier) - where TMod : Mod + where TMod : Mod, new() { - single_multipliers[typeof(TMod)] = _ => hasMultiplier; + single_multipliers[new TMod().Acronym] = _ => hasMultiplier; } /// <summary> /// Defines a setting-dependent score multiplier for the given <typeparamref name="TMod"/>. /// </summary> public static void Single<TMod>(Func<TMod, double> hasMultiplier) - where TMod : Mod + where TMod : Mod, new() { - single_multipliers[typeof(TMod)] = mod => hasMultiplier.Invoke((TMod)mod); + single_multipliers[new TMod().Acronym] = mod => hasMultiplier.Invoke((TMod)mod); } /// <summary> @@ -40,20 +40,20 @@ public static void Single<TMod>(Func<TMod, double> hasMultiplier) /// The multiplier calculation is given additional context to calculate the multiplier via the <typeparamref name="TContext"/> type instance. /// </summary> public static void Single<TMod, TContext>(Func<TMod, TContext, double> hasMultiplier) - where TMod : Mod + where TMod : Mod, new() where TContext : ScoreMultiplierCalculator { - single_multipliers_with_context[typeof(TMod)] = (mod, context) => hasMultiplier.Invoke((TMod)mod, (TContext)context); + single_multipliers_with_context[new TMod().Acronym] = (mod, context) => hasMultiplier.Invoke((TMod)mod, (TContext)context); } /// <summary> /// Defines a score multiplier specific to when both <typeparamref name="T1"/> and <typeparamref name="T2"/> mods are present. /// </summary> public static void Combination<T1, T2>(Func<T1, T2, double> hasMultiplier) - where T1 : Mod - where T2 : Mod + where T1 : Mod, new() + where T2 : Mod, new() { - combination_multipliers.Add(([typeof(T1), typeof(T2)], mods => hasMultiplier((T1)mods[0], (T2)mods[1]))); + combination_multipliers.Add(([new T1().Acronym, new T2().Acronym], mods => hasMultiplier((T1)mods[0], (T2)mods[1]))); } /// <summary> @@ -61,7 +61,7 @@ public static void Combination<T1, T2>(Func<T1, T2, double> hasMultiplier) /// </summary> public double CalculateFor(IEnumerable<Mod> mods) { - var allModsByType = mods.ToDictionary(m => m.GetType()); + var allModsByType = mods.ToDictionary(m => m.Acronym); if (allModsByType.Count == 0) return 1; @@ -83,7 +83,7 @@ public double CalculateFor(IEnumerable<Mod> mods) } } - foreach (var modType in remainingModTypes) + foreach (string modType in remainingModTypes) { if (single_multipliers.TryGetValue(modType, out var multiplier)) result *= multiplier(allModsByType[modType]); ``` </details> One particular parallel thread that may warrant follow-up is that `Mod.UsesDefaultConfiguration` is disproportionately expensive due to calling into regexes via Humanizer internals. <img width="1517" height="517" alt="Screenshot_2026-05-19_at_10 58 30" src="https://github.com/user-attachments/assets/68309a8c-74e7-4f96-8ef9-62868eeca337" />2026-05-19 21:37:17 +09:00