Pass list of active mods when calculating score multiplier

[tsunyoku]

Required to support some changes in the ongoing mod multipliers work.

With the existing set of multipliers there are no changes expected here, but have changed some mods locally and tested this works as expected.

To support this server-side, a game bump alongside any multiplier changes should be enough. Score reverification is the only place this matters, and it falls back onto StandardisedScoreMigrationTools so no code changes necessary.

[bdach]

@tsunyoku For transparency, this PR was briefly discussed last week internally (me, @peppy, @smoogipoo) and I wasn't sold on this approach because for the changes you want wherein mod A's multiplier affects mod B it requires arbitrarily choosing whether you ascribe the different behaviour to A or B and it becomes weirdly asymmetrical. Pretty sure I wasn't alone in that concern. I was probably going to try writing a counterproposal wherein the mod multiplier calculation was centralized to one object and it would handle the combinations somehow.

This also ties into a somewhat-frequent feature request of having mod multipliers be adjustable in certain multiplayer rooms, which could be facilitated by such a centralized object if it could be replaceable depending on context.

[bdach]

So I typed up a counterproposal and it's here: https://github.com/ppy/osu/compare/master...bdach:osu:mod-multiplier-refactor?expand=1

Notably that branch is VERY UNTESTED, VERY UNBENCHMARKED, AND ONLY AIMS TO BE A SIMPLE, SINGLE-RULESET, NON-VALUE-CHANGING DEMONSTRATION OF A POSSIBLE REFACTOR OF MASTER MULTIPLIERS. ALL PASSERBY GITHUB VULTURES ARE HEREFORE ASKED TO LEAVE.

Having gotten that out of the way...

---

I'm generally not that convinced that what I typed up is better than this PR.

Upsides:

• Easier to check / cross-reference against Ruleset.GetAllMods().

• No need to chase down 3 levels of inheritance to find a mod's multiplier.

• Marginally more explicit when e.g. having mods A and B active is supposed to turn A's multiplier completely off while preserving B's, because then you do

  Register<ModA, ModB>(static (a, _) => getMultiplierForModA(a));

• Results in better / easier reuse of logic across mods (see GetMultiplierForRateAdjustment()).

• One big upside that this PR does nothing to help with is that e.g. if you wanted to change Difficulty Adjust's multiplier so that it depends on the map's baseline difficulty settings. This is something that is not accessible to mods reliably right now. An "unchanged" difficulty setting in DA terms is serialised as null generally speaking.

  The closest thing to accessing the original value is DifficultyBindable.ReadCurrentFromDifficulty but that still needs an instance of IBeatmapDifficultyInfo to actually work. Therefore you'd probably have to change this PR to pass in those baseline difficulty settings to ScoreMultiplier() too and I think that'd make this PR much less palatable than currently. Mine would get uglier too, but not by much.

Downsides:

• Generics abuse.
• Casting abuse.
• You have to register mods in the right order (combinations first, then individual mods) or else it's going to do the wrong thing.
• Probably slow.
• When wanting to do more complicated stuff you still end up having to factor out methods like getMultiplierForModA() mentioned above. This matters when wanting to implement stuff like "if mods A and B are active, decrease A's multiplier by X amount".

@tsunyoku @peppy @smoogipoo would like your comments at this stage if possible.

[tsunyoku]

I generally agree with the upsides and downsides mentioned.

I think the general idea is in the right direction - the fact that all multipliers are visible in a central place, especially when combinations are involved, is incredibly valuable. Like you mentioned, the alternative is several levels of inheritance and even then you might be unsure which of the mods in the combination is actually responsible for the reduction. This way of doing things completely avoids that debate.

Not to mention that it may help with other multiplier work later down the line (i.e. multiplayer lobbies).

The only 2 things I would point to being potential issues (for me) is the casting and performance. The casting I can look past, and performance I am expecting to be worse but hopefully not considerably.

[peppy]

I definitely prefer @bdach's branch over this PR.

The following is a bit of a stream of thoughts, and I don't expect a reply to each of my points (feel free to dismiss any which make zero sense, and apply any which make sense in a future iteration of the branch).

---

Regarding performance concerns, is it for the registration part? Because this should fix that without causing any problems:

diff --git a/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs b/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
index 1ad28b338f..b3bac5655f 100644
--- a/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
+++ b/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
@@ -13,22 +13,22 @@ public class OsuModMultiplierCalculator
     {
         private record ModCombination(Type[] ModTypes, Func<Mod[], double> CalculateMultiplier);
 
-        private readonly List<ModCombination> modCombinations = [];
+        private static readonly List<ModCombination> mod_combinations = [];
 
-        public void Register<TMod>(Func<TMod, double> multiplier)
+        public static void Register<TMod>(Func<TMod, double> multiplier)
             where TMod : Mod
         {
-            modCombinations.Add(new ModCombination([typeof(TMod)], mods => multiplier((TMod)mods[0])));
+            mod_combinations.Add(new ModCombination([typeof(TMod)], mods => multiplier((TMod)mods[0])));
         }
 
-        public void Register<T1, T2>(T1 first, T2 second, Func<T1, T2, double> multiplier)
+        public static void Register<T1, T2>(T1 first, T2 second, Func<T1, T2, double> multiplier)
             where T1 : Mod
             where T2 : Mod
         {
-            modCombinations.Add(new ModCombination([typeof(T1), typeof(T2)], mods => multiplier((T1)mods[0], (T2)mods[1])));
+            mod_combinations.Add(new ModCombination([typeof(T1), typeof(T2)], mods => multiplier((T1)mods[0], (T2)mods[1])));
         }
 
-        public OsuModMultiplierCalculator()
+        static OsuModMultiplierCalculator()
         {
             #region Difficulty Reduction
 
@@ -116,7 +116,7 @@ public double CalculateFor(IEnumerable<Mod> mods)
 
             double multiplier = 1;
 
-            foreach (var combination in modCombinations)
+            foreach (var combination in mod_combinations)
             {
                 if (remainingModTypes.IsSupersetOf(combination.ModTypes))
                 {

If it's for the IsSupersetOf and other reflection in the check method, then yeah, maybe that's of concern. Benchmarking setup should be simple to get some answers to this question. I can see ways to optimise things (and I'm sure you can too) if it doesn't meet required perf goals.

I'd probably see us having tests to ensure mods aren't missed in the registration. Especially for cases like DT/NC where – at least with the current code – you have to register each derived version.

You have to register mods in the right order (combinations first, then individual mods) or else it's going to do the wrong thing.

I think this can be easily asserted.

When wanting to do more complicated stuff you still end up having to factor out methods like getMultiplierForModA() mentioned above. This matters when wanting to implement stuff like "if mods A and B are active, decrease A's multiplier by X amount".

I am not sure I'm seeing the concern here.

---

I'd also want to consider whether things can be simplified by splitting out the two paths:

• Base multiplier when mod is not considered as a compound with other mods
• Compound cases

And what I mean by this is potentially storing them in separate arrays and adjusting a bit like this:

diff --git a/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs b/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
index 1ad28b338f..12a299ecc5 100644
--- a/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
+++ b/osu.Game.Rulesets.Osu/OsuModMultiplierCalculator.cs
@@ -11,45 +11,52 @@ namespace osu.Game.Rulesets.Osu
 {
     public class OsuModMultiplierCalculator
     {
-        private record ModCombination(Type[] ModTypes, Func<Mod[], double> CalculateMultiplier);
+        private static readonly List<(Type mod, Func<Mod, double> calculate)> single = [];
+        private static readonly List<(Type[] mods, Func<Mod[], double> calculate)> compound = [];
 
-        private readonly List<ModCombination> modCombinations = [];
+        public static void Single<TMod>(double multiplier)
+            where TMod : Mod
+        {
+            single.Add((typeof(TMod), _ => multiplier));
+        }
 
-        public void Register<TMod>(Func<TMod, double> multiplier)
+        public static void Single<TMod>(Func<TMod, double> multiplier)
             where TMod : Mod
         {
-            modCombinations.Add(new ModCombination([typeof(TMod)], mods => multiplier((TMod)mods[0])));
+            single.Add((typeof(TMod), mod => multiplier((TMod)mod)));
         }
 
-        public void Register<T1, T2>(T1 first, T2 second, Func<T1, T2, double> multiplier)
+        public static void Compound<T1, T2>(Func<T1, T2, double> multiplier)
             where T1 : Mod
             where T2 : Mod
         {
-            modCombinations.Add(new ModCombination([typeof(T1), typeof(T2)], mods => multiplier((T1)mods[0], (T2)mods[1])));
+            compound.Add(([typeof(T1), typeof(T2)], mods => multiplier((T1)mods[0], (T2)mods[1])));
         }
 
-        public OsuModMultiplierCalculator()
+        static OsuModMultiplierCalculator()
         {
+            Compound<OsuModEasy, OsuModNoFail>((e, nf) => 0.1);
+
             #region Difficulty Reduction
 
-            Register<OsuModEasy>(static _ => 0.5);
-            Register<OsuModNoFail>(static _ => 0.5);
-            Register<OsuModHalfTime>(static ht => GetMultiplierForRateAdjustment(ht.SpeedChange.Value));
-            Register<OsuModDaycore>(static dc => GetMultiplierForRateAdjustment(dc.SpeedChange.Value));
+            Single<OsuModEasy>(0.5);
+            Single<OsuModNoFail>(0.5);
+            Single<OsuModHalfTime>(static ht => GetMultiplierForRateAdjustment(ht.SpeedChange.Value));
+            Single<OsuModDaycore>(static dc => GetMultiplierForRateAdjustment(dc.SpeedChange.Value));
 
             #endregion
 
             #region Difficulty Increase
 
-            Register<OsuModHardRock>(static hr => hr.UsesDefaultConfiguration ? 1.06 : 1);
+            Single<OsuModHardRock>(static hr => hr.UsesDefaultConfiguration ? 1.06 : 1);
             // Sudden Death
             // Perfect
-            Register<OsuModDoubleTime>(static dt => GetMultiplierForRateAdjustment(dt.SpeedChange.Value));
-            Register<OsuModNightcore>(static nc => GetMultiplierForRateAdjustment(nc.SpeedChange.Value));
-            Register<OsuModHidden>(static hd => hd.UsesDefaultConfiguration ? 1.06 : 1);
+            Single<OsuModDoubleTime>(static dt => GetMultiplierForRateAdjustment(dt.SpeedChange.Value));
+            Single<OsuModNightcore>(static nc => GetMultiplierForRateAdjustment(nc.SpeedChange.Value));
+            Single<OsuModHidden>(static hd => hd.UsesDefaultConfiguration ? 1.06 : 1);
             // Traceable
-            Register<OsuModFlashlight>(static fl => fl.UsesDefaultConfiguration ? 1.12 : 1);
-            Register<OsuModBlinds>(static bl => bl.UsesDefaultConfiguration ? 1.12 : 1);
+            Single<OsuModFlashlight>(static fl => fl.UsesDefaultConfiguration ? 1.12 : 1);
+            Single<OsuModBlinds>(static bl => bl.UsesDefaultConfiguration ? 1.12 : 1);
             // Strict Tracking
             // Accuracy Challenge
 
@@ -57,9 +64,9 @@ public OsuModMultiplierCalculator()
 
             #region Conversion
 
-            Register<OsuModTargetPractice>(static _ => 0.1);
-            Register<OsuModDifficultyAdjust>(static _ => 0.5);
-            Register<OsuModClassic>(static _ => 0.96);
+            Single<OsuModTargetPractice>(0.1);
+            Single<OsuModDifficultyAdjust>(0.5);
+            Single<OsuModClassic>(0.96);
             // Random
             // Mirror
             // Alternate
@@ -71,9 +78,9 @@ public OsuModMultiplierCalculator()
 
             // Autoplay
             // Cinema
-            Register<OsuModRelax>(static _ => 0.1);
-            Register<OsuModAutopilot>(static _ => 0.1);
-            Register<OsuModSpunOut>(static _ => 0.9);
+            Single<OsuModRelax>(0.1);
+            Single<OsuModAutopilot>(0.1);
+            Single<OsuModSpunOut>(0.9);
 
             #endregion
 
@@ -84,18 +91,18 @@ public OsuModMultiplierCalculator()
             // Spin In
             // Grow
             // Deflate
-            Register<ModWindUp>(static _ => 0.5);
-            Register<ModWindDown>(static _ => 0.5);
+            Single<ModWindUp>(0.5);
+            Single<ModWindDown>(0.5);
             // Barrel Roll
             // Approach Different
             // Muted
             // No Scope
-            Register<OsuModMagnetised>(static _ => 0.5);
+            Single<OsuModMagnetised>(0.5);
             // Repel
-            Register<ModAdaptiveSpeed>(static _ => 0.5);
+            Single<ModAdaptiveSpeed>(0.5);
             // Freeze Frame
             // Bubbles
-            Register<OsuModSynesthesia>(static _ => 0.8);
+            Single<OsuModSynesthesia>(0.8);
             // Depth
             // Bloom
 
@@ -116,13 +123,25 @@ public double CalculateFor(IEnumerable<Mod> mods)
 
             double multiplier = 1;
 
-            foreach (var combination in modCombinations)
+            foreach (var combination in compound)
+            {
+                if (remainingModTypes.IsSupersetOf(combination.mods))
+                {
+                    var instances = combination.mods.Select(t => allModsByType[t]).ToArray();
+                    multiplier *= combination.calculate(instances);
+                    remainingModTypes.ExceptWith(combination.mods);
+                }
+            }
+
+            // doing as a second pass guarantees ordering
+            foreach (var mod in remainingModTypes)
             {
-                if (remainingModTypes.IsSupersetOf(combination.ModTypes))
+                var found = single.FirstOrDefault(m => m.mod == mod);
+
+                if (found != default)
                 {
-                    var instances = combination.ModTypes.Select(t => allModsByType[t]).ToArray();
-                    multiplier *= combination.CalculateMultiplier(instances);
-                    remainingModTypes.ExceptWith(combination.ModTypes);
+                    multiplier *= found.calculate(allModsByType[mod]);
+                    remainingModTypes.Remove(found.mod);
                 }
             }
 

Note that this is half-assed but hopefully gives some food for thought.

---

I'd also consider using mod names rather than types for this whole thing. It may make things more performant and easier to manage (can always work with Mod instances rather than types). Register calls would need to be provided a basic instance to get the name from but this may work okay.

May somehow allow the case to be removed, but not sure.

[tsunyoku]

Agree with the above. @bdach if you're happy, I can close this PR and let you work on your branch separately.

[bdach]

Regarding performance concerns, is it for the registration part?

Don't know yet. static is an option, sure, though I wasn't sure I wanted to go there.

I'd probably see us having tests to ensure mods aren't missed in the registration. Especially for cases like DT/NC where – at least with the current code – you have to register each derived version.

I'm not sure how these tests would look. To my awareness there is no higher-level way to assert that those two mods should always have the same multiplier than to just test them manually. Which can be done but it's a chicken-and-egg problem: the test will fail to catch errors if it isn't remembered that the test needs to be updated for some specific new future scenario.

I'd also want to consider whether things can be simplified by splitting out the two paths:

• Base multiplier when mod is not considered as a compound with other mods
• Compound cases

And what I mean by this is potentially storing them in separate arrays and adjusting a bit like this

Sure.

I'd also consider using mod names rather than types for this whole thing. It may make things more performant and easier to manage (can always work with Mod instances rather than types). Register calls would need to be provided a basic instance to get the name from but this may work okay.

Note that the dictionaries in the discussed variant cannot be easily keyed by mod instances because Mod equality members take mod settings into account. Would require custom comparer specifications at least.

---

I will work on finalizing the proof of concept of the variant I proposed today.