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

Fix incorrect DifficultyBindable logic #29198

Merged
merged 2 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions osu.Game.Tests/Mods/ModDifficultyAdjustTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void TestFollowsBeatmapDefaultsVisually()
}

[Test]
public void TestOutOfRangeValueStillApplied()
public void TestValueAboveRangeStillApplied()
{
AddStep("set override cs to 11", () => modDifficultyAdjust.CircleSize.Value = 11);

Expand All @@ -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()
{
Expand All @@ -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);
Expand Down
17 changes: 14 additions & 3 deletions osu.Game/Rulesets/Mods/DifficultyBindable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public float Precision

public float MinValue
{
get => minValue;
set
{
if (value == minValue)
Expand All @@ -52,6 +53,7 @@ public float MinValue

public float MaxValue
{
get => maxValue;
set
{
if (value == maxValue)
Expand All @@ -69,6 +71,7 @@ public float MaxValue
/// </summary>
public float? ExtendedMinValue
{
get => extendedMinValue;
set
{
if (value == extendedMinValue)
Expand All @@ -86,6 +89,7 @@ public float? ExtendedMinValue
/// </summary>
public float? ExtendedMaxValue
{
get => extendedMaxValue;
set
{
if (value == extendedMaxValue)
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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)
Expand Down
Loading