Merge pull request #29198 from bdach/fix-difficulty-bindable

Fix incorrect `DifficultyBindable` logic

osu.Game.Tests/Mods/ModDifficultyAdjustTest.cs

@@ -8,6 +8,8 @@
 using osu.Game.Rulesets;
 using osu.Game.Rulesets.Difficulty;
 using osu.Game.Rulesets.Mods;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Rulesets.Osu.Mods;
 using osu.Game.Rulesets.UI;
 
 namespace osu.Game.Tests.Mods
@@ -105,9 +107,6 @@ public void TestChangedSettingsRevertedToDefault()
             testMod.ResetSettingsToDefaults();
 
             Assert.That(testMod.DrainRate.Value, Is.Null);
-
-            // ReSharper disable once HeuristicUnreachableCode
-            // see https://youtrack.jetbrains.com/issue/RIDER-70159.
             Assert.That(testMod.OverallDifficulty.Value, Is.Null);
 
             var applied = applyDifficulty(new BeatmapDifficulty
@@ -119,6 +118,48 @@ public void TestChangedSettingsRevertedToDefault()
             Assert.That(applied.OverallDifficulty, Is.EqualTo(10));
         }
 
+        [Test]
+        public void TestDeserializeIncorrectRange()
+        {
+            var apiMod = new APIMod
+            {
+                Acronym = @"DA",
+                Settings = new Dictionary<string, object>
+                {
+                    [@"circle_size"] = -727,
+                    [@"approach_rate"] = -727,
+                }
+            };
+            var ruleset = new OsuRuleset();
+
+            var mod = (OsuModDifficultyAdjust)apiMod.ToMod(ruleset);
+
+            Assert.Multiple(() =>
+            {
+                Assert.That(mod.CircleSize.Value, Is.GreaterThanOrEqualTo(0).And.LessThanOrEqualTo(11));
+                Assert.That(mod.ApproachRate.Value, Is.GreaterThanOrEqualTo(-10).And.LessThanOrEqualTo(11));
+            });
+        }
+
+        [Test]
+        public void TestDeserializeNegativeApproachRate()
+        {
+            var apiMod = new APIMod
+            {
+                Acronym = @"DA",
+                Settings = new Dictionary<string, object>
+                {
+                    [@"approach_rate"] = -9,
+                }
+            };
+            var ruleset = new OsuRuleset();
+
+            var mod = (OsuModDifficultyAdjust)apiMod.ToMod(ruleset);
+
+            Assert.That(mod.ApproachRate.Value, Is.GreaterThanOrEqualTo(-10).And.LessThanOrEqualTo(11));
+            Assert.That(mod.ApproachRate.Value, Is.EqualTo(-9));
+        }
+
         /// <summary>
         /// Applies a <see cref="BeatmapDifficulty"/> to the mod and returns a new <see cref="BeatmapDifficulty"/>
         /// representing the result if the mod were applied to a fresh <see cref="BeatmapDifficulty"/> instance.

osu.Game.Tests/Visual/UserInterface/TestSceneModDifficultyAdjustSettings.cs

@@ -70,7 +70,7 @@ public void TestFollowsBeatmapDefaultsVisually()
         }
 
         [Test]
-        public void TestOutOfRangeValueStillApplied()
+        public void TestValueAboveRangeStillApplied()
         {
             AddStep("set override cs to 11", () => modDifficultyAdjust.CircleSize.Value = 11);
 
@@ -91,6 +91,28 @@ public void TestOutOfRangeValueStillApplied()
             checkBindableAtValue("Circle Size", 11);
         }
 
+        [Test]
+        public void TestValueBelowRangeStillApplied()
+        {
+            AddStep("set override cs to -5", () => modDifficultyAdjust.ApproachRate.Value = -5);
+
+            checkSliderAtValue("Approach Rate", -5);
+            checkBindableAtValue("Approach Rate", -5);
+
+            // this is a no-op, just showing that it won't reset the value during deserialisation.
+            setExtendedLimits(false);
+
+            checkSliderAtValue("Approach Rate", -5);
+            checkBindableAtValue("Approach Rate", -5);
+
+            // setting extended limits will reset the serialisation exception.
+            // this should be fine as the goal is to allow, at most, the value of extended limits.
+            setExtendedLimits(true);
+
+            checkSliderAtValue("Approach Rate", -5);
+            checkBindableAtValue("Approach Rate", -5);
+        }
+
         [Test]
         public void TestExtendedLimits()
         {
@@ -109,6 +131,11 @@ public void TestExtendedLimits()
             checkSliderAtValue("Circle Size", 11);
             checkBindableAtValue("Circle Size", 11);
 
+            setSliderValue("Approach Rate", -5);
+
+            checkSliderAtValue("Approach Rate", -5);
+            checkBindableAtValue("Approach Rate", -5);
+
             setExtendedLimits(false);
 
             checkSliderAtValue("Circle Size", 10);

osu.Game/Rulesets/Mods/DifficultyBindable.cs

@@ -38,6 +38,7 @@ public float Precision
 
         public float MinValue
         {
+            get => minValue;
             set
             {
                 if (value == minValue)
@@ -52,6 +53,7 @@ public float MinValue
 
         public float MaxValue
         {
+            get => maxValue;
             set
             {
                 if (value == maxValue)
@@ -69,6 +71,7 @@ public float MaxValue
         /// </summary>
         public float? ExtendedMinValue
         {
+            get => extendedMinValue;
             set
             {
                 if (value == extendedMinValue)
@@ -86,6 +89,7 @@ public float? ExtendedMinValue
         /// </summary>
         public float? ExtendedMaxValue
         {
+            get => extendedMaxValue;
             set
             {
                 if (value == extendedMaxValue)
@@ -114,9 +118,14 @@ public override float? Value
             {
                 // Ensure that in the case serialisation runs in the wrong order (and limit extensions aren't applied yet) the deserialised value is still propagated.
                 if (value != null)
-                    CurrentNumber.MaxValue = MathF.Max(CurrentNumber.MaxValue, value.Value);
-
-                base.Value = value;
+                {
+                    CurrentNumber.MinValue = Math.Clamp(MathF.Min(CurrentNumber.MinValue, value.Value), ExtendedMinValue ?? MinValue, MinValue);
+                    CurrentNumber.MaxValue = Math.Clamp(MathF.Max(CurrentNumber.MaxValue, value.Value), MaxValue, ExtendedMaxValue ?? MaxValue);
+
+                    base.Value = Math.Clamp(value.Value, CurrentNumber.MinValue, CurrentNumber.MaxValue);
+                }
+                else
+                    base.Value = value;
             }
         }
 
@@ -138,6 +147,8 @@ public override void CopyTo(Bindable<float?> them)
             // the following max value copies are only safe as long as these values are effectively constants.
             otherDifficultyBindable.MaxValue = maxValue;
             otherDifficultyBindable.ExtendedMaxValue = extendedMaxValue;
+            otherDifficultyBindable.MinValue = minValue;
+            otherDifficultyBindable.ExtendedMinValue = extendedMinValue;
         }
 
         public override void BindTo(Bindable<float?> them)