-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 player restarting on fail while the triggering mod is not configured as such #28825
base: master
Are you sure you want to change the base?
Conversation
Alternatively I did separate branch from scratch https://github.com/Peter-io/osu/tree/alternative-fail-trigger which solves problems mentioned above in simplier way without replacing type of |
@@ -86,13 +102,12 @@ private bool meetsAnyFailCondition(JudgementResult result) | |||
if (CheckDefaultFailCondition(result)) | |||
return true; | |||
|
|||
if (FailConditions != null) | |||
foreach (var condition in Mods.Value.OfType<IHasFailCondition>().OrderByDescending(m => m.RestartOnFail)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do we really need the
OrderByDescending
? - Let's try to not use Linq in potentially hot spots where performance could matter, like on every judgement. I don't know if we've enforced this up until now, but I think it's good to start being wary of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got an idea for that
/// <remarks> | ||
/// This method should only be used to trigger failures based on <paramref name="result"/> | ||
/// </remarks> | ||
bool FailCondition(JudgementResult result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this basically the same as IApplicableOverride.PerformFail()
? I feel like these two interfaces can't exist together...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they are the same, however i think they could be somehow combined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PerformFail()
blocks fail when true, otherwise doesn't. FailCondition
triggers fail when true, otherwise doesn't. I agree with merging the two together, I tried doing that but I felt I'm going through this moment again where I'm refactoring things because of a minor PR, I dunno.
56dfae9
to
54c12c2
Compare
Co-authored-by: Dan Balasescu <[email protected]>
osu.Game/Screens/Play/Player.cs
Outdated
@@ -244,7 +244,7 @@ private void load(OsuConfigManager config, OsuGameBase game, CancellationToken c | |||
|
|||
HealthProcessor = gameplayMods.OfType<IApplicableHealthProcessor>().FirstOrDefault()?.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime); | |||
HealthProcessor ??= ruleset.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime); | |||
HealthProcessor.Mods.Value = gameplayMods; | |||
HealthProcessor.Mods.Value = gameplayMods.OrderByDescending(m => m is IHasFailCondition mod && mod.RestartOnFail).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed this at first, but I think just remove this at this point until someone notices it.
Proposal 1: two methodsdiff --git a/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs b/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
index abcee50e82..3e4ae9c68f 100644
--- a/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
+++ b/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
@@ -9,7 +9,7 @@ namespace osu.Game.Rulesets.Mania.Mods
{
public class ManiaModPerfect : ModPerfect
{
- public override bool FailCondition(JudgementResult result)
+ public override bool ForceFail(JudgementResult result)
{
if (!isRelevantResult(result.Judgement.MinResult) && !isRelevantResult(result.Judgement.MaxResult) && !isRelevantResult(result.Type))
return false;
diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
index 56b3feb7c4..71e80e8bf7 100644
--- a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
+++ b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
@@ -33,7 +33,7 @@
namespace osu.Game.Rulesets.Osu.Mods
{
public class OsuModTargetPractice : ModWithVisibilityAdjustment, IApplicableToDrawableRuleset<OsuHitObject>,
- IApplicableToDifficulty, IHasSeed, IHidesApproachCircles, IHasFailCondition
+ IApplicableToDifficulty, IHasSeed, IHidesApproachCircles, IApplicableFailOverride
{
public override string Name => "Target Practice";
public override string Acronym => "TP";
@@ -100,12 +100,12 @@ public class OsuModTargetPractice : ModWithVisibilityAdjustment, IApplicableToDr
#region Sudden Death (IApplicableFailOverride)
- public bool PerformFail() => true;
+ public bool AllowFail => true;
public bool RestartOnFail => false;
// Sudden death
- public bool FailCondition(JudgementResult result)
+ public bool ForceFail(JudgementResult result)
=> result.Type.AffectsCombo()
&& !result.IsHit;
diff --git a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
index be4429b283..c22a10d5af 100644
--- a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
+++ b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
@@ -440,7 +440,7 @@ public ModFailOnResult(HitResult type)
this.type = type;
}
- public override bool FailCondition(JudgementResult result) => result.Type == type;
+ public override bool ForceFail(JudgementResult result) => result.Type == type;
}
}
}
diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
index a5931b98e9..247565b2a7 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
@@ -61,7 +61,7 @@ private class ModFailOnFirstJudgement : ModFailCondition
public override double ScoreMultiplier => 1.0;
public override string Acronym => "";
- public override bool FailCondition(JudgementResult result) => true;
+ public override bool ForceFail(JudgementResult result) => true;
}
}
}
diff --git a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
index 8c99d739cb..59c95f8911 100644
--- a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
+++ b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
@@ -1,6 +1,8 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
+using osu.Game.Rulesets.Judgements;
+
namespace osu.Game.Rulesets.Mods
{
/// <summary>
@@ -11,12 +13,23 @@ public interface IApplicableFailOverride : IApplicableMod
/// <summary>
/// Whether we should allow failing at the current point in time.
/// </summary>
+ /// <remarks>
+ /// This won't trigger a failure itself, but allow other failure triggers to proceed.
+ /// The main usage is to forcefully block a failure from occurring (ie. "no fail" mods).
+ /// </remarks>
/// <returns>Whether the fail should be allowed to proceed. Return false to block.</returns>
- bool PerformFail();
+ bool AllowFail { get; }
/// <summary>
- /// Whether we want to restart on fail. Only used if <see cref="PerformFail"/> returns true.
+ /// Whether we want to restart on fail. Only used if <see cref="AllowFail"/> returns true.
/// </summary>
bool RestartOnFail { get; }
+
+ /// <summary>
+ /// Whether to force a failure based on a new judgement result.
+ /// </summary>
+ /// <param name="result">The judgement result which should be considered.</param>
+ /// <returns>Whether the fail condition has been met.</returns>
+ bool ForceFail(JudgementResult result);
}
}
diff --git a/osu.Game/Rulesets/Mods/IHasFailCondition.cs b/osu.Game/Rulesets/Mods/IHasFailCondition.cs
deleted file mode 100644
index 73c734f858..0000000000
--- a/osu.Game/Rulesets/Mods/IHasFailCondition.cs
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
-// See the LICENCE file in the repository root for full licence text.
-
-using osu.Game.Rulesets.Judgements;
-using osu.Game.Rulesets.Scoring;
-
-namespace osu.Game.Rulesets.Mods
-{
- /// <summary>
- /// Interface for a <see cref="Mod"/> that specifies its own conditions for failure.
- /// </summary>
- // todo: maybe IHasFailCondition and IApplicableFailOverride should be combined into a single interface.
- public interface IHasFailCondition : IApplicableFailOverride
- {
- /// <summary>
- /// Determines whether <paramref name="result"/> should trigger a failure. Called every time a
- /// judgement is applied to <see cref="HealthProcessor"/>.
- /// </summary>
- /// <param name="result">The latest <see cref="JudgementResult"/>.</param>
- /// <returns>Whether the fail condition has been met.</returns>
- /// <remarks>
- /// This method should only be used to trigger failures based on <paramref name="result"/>
- /// </remarks>
- bool FailCondition(JudgementResult result);
- }
-}
diff --git a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
index 1ee0b8c466..791486f2f5 100644
--- a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
+++ b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
@@ -74,7 +74,7 @@ public void ApplyToScoreProcessor(ScoreProcessor scoreProcessor)
public ScoreRank AdjustRank(ScoreRank rank, double accuracy) => rank;
- public override bool FailCondition(JudgementResult result) => false;
+ public override bool ForceFail(JudgementResult result) => false;
public enum AccuracyMode
{
diff --git a/osu.Game/Rulesets/Mods/ModCinema.cs b/osu.Game/Rulesets/Mods/ModCinema.cs
index 0c00eb6ae0..5408034b47 100644
--- a/osu.Game/Rulesets/Mods/ModCinema.cs
+++ b/osu.Game/Rulesets/Mods/ModCinema.cs
@@ -6,6 +6,7 @@
using osu.Framework.Graphics.Sprites;
using osu.Framework.Localisation;
using osu.Game.Graphics;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Objects;
using osu.Game.Rulesets.UI;
using osu.Game.Screens.Play;
@@ -48,8 +49,10 @@ public void ApplyToPlayer(Player player)
player.BreakOverlay.Hide();
}
- public bool PerformFail() => false;
+ public bool AllowFail => false;
public bool RestartOnFail => false;
+
+ public bool ForceFail(JudgementResult result) => false;
}
}
diff --git a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
index e101ac440e..843e797381 100644
--- a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
+++ b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
@@ -7,6 +7,7 @@
using osu.Framework.Bindables;
using osu.Game.Beatmaps;
using osu.Game.Configuration;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Scoring;
namespace osu.Game.Rulesets.Mods
@@ -33,17 +34,21 @@ public override void ApplyToDifficulty(BeatmapDifficulty difficulty)
retries = Retries.Value;
}
- public bool PerformFail()
+ public bool AllowFail
{
- if (retries == 0) return true;
+ get
+ {
+ if (retries == 0) return true;
- health.Value = health.MaxValue;
- retries--;
+ health.Value = health.MaxValue;
+ retries--;
- return false;
+ return false;
+ }
}
public bool RestartOnFail => false;
+ public bool ForceFail(JudgementResult result) => false;
public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
{
diff --git a/osu.Game/Rulesets/Mods/ModFailCondition.cs b/osu.Game/Rulesets/Mods/ModFailCondition.cs
index 8f3dec2d2b..0586986d5e 100644
--- a/osu.Game/Rulesets/Mods/ModFailCondition.cs
+++ b/osu.Game/Rulesets/Mods/ModFailCondition.cs
@@ -9,14 +9,14 @@
namespace osu.Game.Rulesets.Mods
{
- public abstract class ModFailCondition : Mod, IApplicableToHealthProcessor, IHasFailCondition
+ public abstract class ModFailCondition : Mod, IApplicableToHealthProcessor, IApplicableFailOverride
{
public override Type[] IncompatibleMods => new[] { typeof(ModNoFail), typeof(ModCinema) };
[SettingSource("Restart on fail", "Automatically restarts when failed.")]
public BindableBool Restart { get; } = new BindableBool();
- public virtual bool PerformFail() => true;
+ public virtual bool AllowFail => true;
public virtual bool RestartOnFail => Restart.Value;
@@ -32,6 +32,6 @@ public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
/// </summary>
protected void TriggerFailure() => triggerFailureDelegate?.Invoke(this);
- public abstract bool FailCondition(JudgementResult result);
+ public abstract bool ForceFail(JudgementResult result);
}
}
diff --git a/osu.Game/Rulesets/Mods/ModNoFail.cs b/osu.Game/Rulesets/Mods/ModNoFail.cs
index 1aaef8eac4..3a955ecde7 100644
--- a/osu.Game/Rulesets/Mods/ModNoFail.cs
+++ b/osu.Game/Rulesets/Mods/ModNoFail.cs
@@ -7,6 +7,7 @@
using osu.Framework.Localisation;
using osu.Game.Configuration;
using osu.Game.Graphics;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Screens.Play;
namespace osu.Game.Rulesets.Mods
@@ -27,9 +28,10 @@ public abstract class ModNoFail : Mod, IApplicableFailOverride, IApplicableToHUD
/// <summary>
/// We never fail, 'yo.
/// </summary>
- public bool PerformFail() => false;
+ public bool AllowFail => false;
public bool RestartOnFail => false;
+ public bool ForceFail(JudgementResult result) => false;
public void ReadFromConfig(OsuConfigManager config)
{
diff --git a/osu.Game/Rulesets/Mods/ModPerfect.cs b/osu.Game/Rulesets/Mods/ModPerfect.cs
index d8020905da..40da4fad70 100644
--- a/osu.Game/Rulesets/Mods/ModPerfect.cs
+++ b/osu.Game/Rulesets/Mods/ModPerfect.cs
@@ -28,7 +28,7 @@ protected ModPerfect()
Restart.Value = Restart.Default = true;
}
- public override bool FailCondition(JudgementResult result)
+ public override bool ForceFail(JudgementResult result)
=> (isRelevantResult(result.Judgement.MinResult) || isRelevantResult(result.Judgement.MaxResult) || isRelevantResult(result.Type))
&& result.Type != result.Judgement.MaxResult;
diff --git a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
index 070818e1c3..4e8bddf11c 100644
--- a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
+++ b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
@@ -23,7 +23,7 @@ public abstract class ModSuddenDeath : ModFailCondition
public override Type[] IncompatibleMods => base.IncompatibleMods.Append(typeof(ModPerfect)).ToArray();
- public override bool FailCondition(JudgementResult result)
+ public override bool ForceFail(JudgementResult result)
=> result.Type.AffectsCombo()
&& !result.IsHit;
}
diff --git a/osu.Game/Rulesets/Scoring/HealthProcessor.cs b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
index aba78c4379..13cb413714 100644
--- a/osu.Game/Rulesets/Scoring/HealthProcessor.cs
+++ b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
@@ -102,9 +102,9 @@ private bool meetsAnyFailCondition(JudgementResult result)
if (CheckDefaultFailCondition(result))
return true;
- foreach (var condition in Mods.Value.OfType<IHasFailCondition>())
+ foreach (var condition in Mods.Value.OfType<IApplicableFailOverride>())
{
- if (condition.FailCondition(result))
+ if (condition.ForceFail(result))
{
ModTriggeringFailure = condition as Mod;
return true;
diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index bf9ca1c5bd..39b1dffd56 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -152,7 +152,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
/// Whether failing should be allowed.
/// By default, this checks whether all selected mods allow failing.
/// </summary>
- protected virtual bool CheckModsAllowFailure() => GameplayState.Mods.OfType<IApplicableFailOverride>().All(m => m.PerformFail());
+ protected virtual bool CheckModsAllowFailure() => GameplayState.Mods.OfType<IApplicableFailOverride>().All(m => m.AllowFail);
public readonly PlayerConfiguration Configuration;
@@ -244,7 +244,7 @@ private void load(OsuConfigManager config, OsuGameBase game, CancellationToken c
HealthProcessor = gameplayMods.OfType<IApplicableHealthProcessor>().FirstOrDefault()?.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime);
HealthProcessor ??= ruleset.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime);
- HealthProcessor.Mods.Value = gameplayMods.OrderByDescending(m => m is IHasFailCondition mod && mod.RestartOnFail).ToArray();
+ HealthProcessor.Mods.Value = gameplayMods.OrderByDescending(m => m is IApplicableFailOverride mod && mod.RestartOnFail).ToArray();
HealthProcessor.ApplyBeatmap(playableBeatmap);
dependencies.CacheAs(HealthProcessor);
Proposal 2: combineddiff --git a/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs b/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
index abcee50e82..5460ae982f 100644
--- a/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
+++ b/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs
@@ -9,16 +9,19 @@ namespace osu.Game.Rulesets.Mania.Mods
{
public class ManiaModPerfect : ModPerfect
{
- public override bool FailCondition(JudgementResult result)
+ public override FailState CheckFail(JudgementResult? result)
{
+ if (result == null)
+ return FailState.Allow;
+
if (!isRelevantResult(result.Judgement.MinResult) && !isRelevantResult(result.Judgement.MaxResult) && !isRelevantResult(result.Type))
- return false;
+ return FailState.Allow;
// Mania allows imperfect "Great" hits without failing.
if (result.Judgement.MaxResult == HitResult.Perfect)
- return result.Type < HitResult.Great;
+ return result.Type < HitResult.Great ? FailState.Allow : FailState.Force;
- return result.Type != result.Judgement.MaxResult;
+ return result.Type != result.Judgement.MaxResult ? FailState.Force : FailState.Allow;
}
private bool isRelevantResult(HitResult result) => result.AffectsAccuracy() || result.AffectsCombo();
diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
index 56b3feb7c4..5afaec4868 100644
--- a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
+++ b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs
@@ -33,7 +33,7 @@
namespace osu.Game.Rulesets.Osu.Mods
{
public class OsuModTargetPractice : ModWithVisibilityAdjustment, IApplicableToDrawableRuleset<OsuHitObject>,
- IApplicableToDifficulty, IHasSeed, IHidesApproachCircles, IHasFailCondition
+ IApplicableToDifficulty, IHasSeed, IHidesApproachCircles, IApplicableFailOverride
{
public override string Name => "Target Practice";
public override string Acronym => "TP";
@@ -100,14 +100,15 @@ public class OsuModTargetPractice : ModWithVisibilityAdjustment, IApplicableToDr
#region Sudden Death (IApplicableFailOverride)
- public bool PerformFail() => true;
-
public bool RestartOnFail => false;
- // Sudden death
- public bool FailCondition(JudgementResult result)
- => result.Type.AffectsCombo()
- && !result.IsHit;
+ public FailState CheckFail(JudgementResult? result)
+ {
+ if (result == null)
+ return FailState.Allow;
+
+ return result.Type.AffectsCombo() && !result.IsHit ? FailState.Force : FailState.Allow;
+ }
#endregion
diff --git a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
index be4429b283..7b575183c8 100644
--- a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
+++ b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
@@ -440,7 +440,7 @@ public ModFailOnResult(HitResult type)
this.type = type;
}
- public override bool FailCondition(JudgementResult result) => result.Type == type;
+ public override FailState CheckFail(JudgementResult result) => result?.Type == type ? FailState.Force : FailState.Allow;
}
}
}
diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
index a5931b98e9..964a674108 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
@@ -61,7 +61,7 @@ private class ModFailOnFirstJudgement : ModFailCondition
public override double ScoreMultiplier => 1.0;
public override string Acronym => "";
- public override bool FailCondition(JudgementResult result) => true;
+ public override FailState CheckFail(JudgementResult? result) => FailState.Force;
}
}
}
diff --git a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
index 8c99d739cb..1809e6acdd 100644
--- a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
+++ b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
@@ -1,6 +1,8 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
+using osu.Game.Rulesets.Judgements;
+
namespace osu.Game.Rulesets.Mods
{
/// <summary>
@@ -9,14 +11,33 @@ namespace osu.Game.Rulesets.Mods
public interface IApplicableFailOverride : IApplicableMod
{
/// <summary>
- /// Whether we should allow failing at the current point in time.
+ /// Whether we want to restart on fail.
/// </summary>
- /// <returns>Whether the fail should be allowed to proceed. Return false to block.</returns>
- bool PerformFail();
+ bool RestartOnFail { get; }
/// <summary>
- /// Whether we want to restart on fail. Only used if <see cref="PerformFail"/> returns true.
+ /// Check the current failure allowance for this mod.
/// </summary>
- bool RestartOnFail { get; }
+ /// <param name="result">The judgement result which should be considered. May be null if a failure is already being triggered.</param>
+ /// <returns>The current failure allowance (see <see cref="FailState"/>).</returns>
+ FailState CheckFail(JudgementResult? result);
+ }
+
+ public enum FailState
+ {
+ /// <summary>
+ /// Failure is being blocked by this mod.
+ /// </summary>
+ Block,
+
+ /// <summary>
+ /// Failure is allowed as per default.
+ /// </summary>
+ Allow,
+
+ /// <summary>
+ /// Failure should be forced immediately.
+ /// </summary>
+ Force
}
}
diff --git a/osu.Game/Rulesets/Mods/IHasFailCondition.cs b/osu.Game/Rulesets/Mods/IHasFailCondition.cs
deleted file mode 100644
index 73c734f858..0000000000
--- a/osu.Game/Rulesets/Mods/IHasFailCondition.cs
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
-// See the LICENCE file in the repository root for full licence text.
-
-using osu.Game.Rulesets.Judgements;
-using osu.Game.Rulesets.Scoring;
-
-namespace osu.Game.Rulesets.Mods
-{
- /// <summary>
- /// Interface for a <see cref="Mod"/> that specifies its own conditions for failure.
- /// </summary>
- // todo: maybe IHasFailCondition and IApplicableFailOverride should be combined into a single interface.
- public interface IHasFailCondition : IApplicableFailOverride
- {
- /// <summary>
- /// Determines whether <paramref name="result"/> should trigger a failure. Called every time a
- /// judgement is applied to <see cref="HealthProcessor"/>.
- /// </summary>
- /// <param name="result">The latest <see cref="JudgementResult"/>.</param>
- /// <returns>Whether the fail condition has been met.</returns>
- /// <remarks>
- /// This method should only be used to trigger failures based on <paramref name="result"/>
- /// </remarks>
- bool FailCondition(JudgementResult result);
- }
-}
diff --git a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
index 1ee0b8c466..1176a452e0 100644
--- a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
+++ b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
@@ -74,7 +74,7 @@ public void ApplyToScoreProcessor(ScoreProcessor scoreProcessor)
public ScoreRank AdjustRank(ScoreRank rank, double accuracy) => rank;
- public override bool FailCondition(JudgementResult result) => false;
+ public override FailState CheckFail(JudgementResult? result) => FailState.Allow;
public enum AccuracyMode
{
diff --git a/osu.Game/Rulesets/Mods/ModCinema.cs b/osu.Game/Rulesets/Mods/ModCinema.cs
index 0c00eb6ae0..70a0138f8a 100644
--- a/osu.Game/Rulesets/Mods/ModCinema.cs
+++ b/osu.Game/Rulesets/Mods/ModCinema.cs
@@ -6,6 +6,7 @@
using osu.Framework.Graphics.Sprites;
using osu.Framework.Localisation;
using osu.Game.Graphics;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Objects;
using osu.Game.Rulesets.UI;
using osu.Game.Screens.Play;
@@ -48,8 +49,8 @@ public void ApplyToPlayer(Player player)
player.BreakOverlay.Hide();
}
- public bool PerformFail() => false;
-
public bool RestartOnFail => false;
+
+ public FailState CheckFail(JudgementResult? result) => FailState.Block;
}
}
diff --git a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
index e101ac440e..c22b6ad08d 100644
--- a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
+++ b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
@@ -7,6 +7,7 @@
using osu.Framework.Bindables;
using osu.Game.Beatmaps;
using osu.Game.Configuration;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Scoring;
namespace osu.Game.Rulesets.Mods
@@ -33,17 +34,34 @@ public override void ApplyToDifficulty(BeatmapDifficulty difficulty)
retries = Retries.Value;
}
- public bool PerformFail()
+ public bool AllowFail
{
- if (retries == 0) return true;
+ get
+ {
+ if (retries == 0) return true;
- health.Value = health.MaxValue;
- retries--;
+ health.Value = health.MaxValue;
+ retries--;
- return false;
+ return false;
+ }
}
public bool RestartOnFail => false;
+ public bool ForceFail(JudgementResult result) => false;
+
+ public FailState CheckFail(JudgementResult? result)
+ {
+ if (result != null)
+ return FailState.Block;
+
+ if (retries == 0) return FailState.Allow;
+
+ health.Value = health.MaxValue;
+ retries--;
+
+ return FailState.Block;
+ }
public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
{
diff --git a/osu.Game/Rulesets/Mods/ModFailCondition.cs b/osu.Game/Rulesets/Mods/ModFailCondition.cs
index 8f3dec2d2b..9cb9d25be4 100644
--- a/osu.Game/Rulesets/Mods/ModFailCondition.cs
+++ b/osu.Game/Rulesets/Mods/ModFailCondition.cs
@@ -9,17 +9,16 @@
namespace osu.Game.Rulesets.Mods
{
- public abstract class ModFailCondition : Mod, IApplicableToHealthProcessor, IHasFailCondition
+ public abstract class ModFailCondition : Mod, IApplicableToHealthProcessor, IApplicableFailOverride
{
public override Type[] IncompatibleMods => new[] { typeof(ModNoFail), typeof(ModCinema) };
[SettingSource("Restart on fail", "Automatically restarts when failed.")]
public BindableBool Restart { get; } = new BindableBool();
- public virtual bool PerformFail() => true;
+ public virtual bool AllowFail => true;
public virtual bool RestartOnFail => Restart.Value;
-
private Action<Mod>? triggerFailureDelegate;
public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
@@ -32,6 +31,6 @@ public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
/// </summary>
protected void TriggerFailure() => triggerFailureDelegate?.Invoke(this);
- public abstract bool FailCondition(JudgementResult result);
+ public abstract FailState CheckFail(JudgementResult? result);
}
}
diff --git a/osu.Game/Rulesets/Mods/ModNoFail.cs b/osu.Game/Rulesets/Mods/ModNoFail.cs
index 1aaef8eac4..f630ee6a3d 100644
--- a/osu.Game/Rulesets/Mods/ModNoFail.cs
+++ b/osu.Game/Rulesets/Mods/ModNoFail.cs
@@ -7,6 +7,7 @@
using osu.Framework.Localisation;
using osu.Game.Configuration;
using osu.Game.Graphics;
+using osu.Game.Rulesets.Judgements;
using osu.Game.Screens.Play;
namespace osu.Game.Rulesets.Mods
@@ -24,13 +25,10 @@ public abstract class ModNoFail : Mod, IApplicableFailOverride, IApplicableToHUD
private readonly Bindable<bool> showHealthBar = new Bindable<bool>();
- /// <summary>
- /// We never fail, 'yo.
- /// </summary>
- public bool PerformFail() => false;
-
public bool RestartOnFail => false;
+ public FailState CheckFail(JudgementResult? result) => FailState.Block;
+
public void ReadFromConfig(OsuConfigManager config)
{
config.BindWith(OsuSetting.ShowHealthDisplayWhenCantFail, showHealthBar);
diff --git a/osu.Game/Rulesets/Mods/ModPerfect.cs b/osu.Game/Rulesets/Mods/ModPerfect.cs
index d8020905da..a4c6192f55 100644
--- a/osu.Game/Rulesets/Mods/ModPerfect.cs
+++ b/osu.Game/Rulesets/Mods/ModPerfect.cs
@@ -28,9 +28,16 @@ protected ModPerfect()
Restart.Value = Restart.Default = true;
}
- public override bool FailCondition(JudgementResult result)
- => (isRelevantResult(result.Judgement.MinResult) || isRelevantResult(result.Judgement.MaxResult) || isRelevantResult(result.Type))
- && result.Type != result.Judgement.MaxResult;
+ public override FailState CheckFail(JudgementResult? result)
+ {
+ if (result == null)
+ return FailState.Allow;
+
+ return (isRelevantResult(result.Judgement.MinResult) || isRelevantResult(result.Judgement.MaxResult) || isRelevantResult(result.Type))
+ && result.Type != result.Judgement.MaxResult
+ ? FailState.Force
+ : FailState.Allow;
+ }
private bool isRelevantResult(HitResult result) => result.AffectsAccuracy() || result.AffectsCombo();
}
diff --git a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
index 070818e1c3..49b3430f24 100644
--- a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
+++ b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
@@ -23,8 +23,12 @@ public abstract class ModSuddenDeath : ModFailCondition
public override Type[] IncompatibleMods => base.IncompatibleMods.Append(typeof(ModPerfect)).ToArray();
- public override bool FailCondition(JudgementResult result)
- => result.Type.AffectsCombo()
- && !result.IsHit;
+ public override FailState CheckFail(JudgementResult? result)
+ {
+ if (result == null)
+ return FailState.Allow;
+
+ return result.Type.AffectsCombo() && !result.IsHit ? FailState.Force : FailState.Allow;
+ }
}
}
diff --git a/osu.Game/Rulesets/Scoring/HealthProcessor.cs b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
index aba78c4379..6d8d3963e0 100644
--- a/osu.Game/Rulesets/Scoring/HealthProcessor.cs
+++ b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
@@ -102,9 +102,9 @@ private bool meetsAnyFailCondition(JudgementResult result)
if (CheckDefaultFailCondition(result))
return true;
- foreach (var condition in Mods.Value.OfType<IHasFailCondition>())
+ foreach (var condition in Mods.Value.OfType<IApplicableFailOverride>())
{
- if (condition.FailCondition(result))
+ if (condition.CheckFail(result) == FailState.Force)
{
ModTriggeringFailure = condition as Mod;
return true;
diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index bf9ca1c5bd..2f52a4df14 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -152,7 +152,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
/// Whether failing should be allowed.
/// By default, this checks whether all selected mods allow failing.
/// </summary>
- protected virtual bool CheckModsAllowFailure() => GameplayState.Mods.OfType<IApplicableFailOverride>().All(m => m.PerformFail());
+ protected virtual bool CheckModsAllowFailure() => GameplayState.Mods.OfType<IApplicableFailOverride>().All(m => m.CheckFail(null) != FailState.Block);
public readonly PlayerConfiguration Configuration;
@@ -244,7 +244,7 @@ private void load(OsuConfigManager config, OsuGameBase game, CancellationToken c
HealthProcessor = gameplayMods.OfType<IApplicableHealthProcessor>().FirstOrDefault()?.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime);
HealthProcessor ??= ruleset.CreateHealthProcessor(playableBeatmap.HitObjects[0].StartTime);
- HealthProcessor.Mods.Value = gameplayMods.OrderByDescending(m => m is IHasFailCondition mod && mod.RestartOnFail).ToArray();
+ HealthProcessor.Mods.Value = gameplayMods.OrderByDescending(m => m is IApplicableFailOverride mod && mod.RestartOnFail).ToArray();
HealthProcessor.ApplyBeatmap(playableBeatmap);
dependencies.CacheAs(HealthProcessor);
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented
My opinion might be biased because I've already gone through something similar to this (see master...frenzibyte:osu:refactor-fail-and-fix-issue), but I prefer the second proposal from a code-quality perspective because both blocking & triggering fail is decided by a single point in I meant to open a PR of the above but there were test failures and I couldn't get to it in time. |
From the look of it I definitely prefer the second proposal. |
I'll tidy up the second proposal and commit it to this branch. |
Sorry for the delay, lost track of this one. I've applied the diff with multiple fixes and improvements. Will require a re-review from someone else with fresh eyes. |
/// </summary> | ||
bool RestartOnFail { get; } | ||
/// <param name="result">The judgement result which should be considered. Importantly, will be <c>null</c> if a failure has already being triggered.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null
condition really destroys my brain :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and I'd like to be able to avoid it, but I couldn't find a good way to get the responsible judgement for already triggered cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding a bool
but I'm honestly not sure it helps much. Updating a lot of the usages from result == null
to failTriggered
requires a secondary null check regardless (so unless there's some fancy cross-parameter NotNullWhenParamIsTrue
attribute I'm missing...)
public bool RestartOnFail => false;
-
public FailState CheckFail(JudgementResult? result)
-
public FailState CheckFail(bool failTriggered, JudgementResult? result) { if (result == null) return FailState.Allow;
diff --git a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
index 7b575183c8..299f1cc41d 100644
--- a/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
+++ b/osu.Game.Tests/Gameplay/TestSceneDrainingHealthProcessor.cs
@@ -440,7 +440,7 @@ public ModFailOnResult(HitResult type)
this.type = type;
}
-
public override FailState CheckFail(JudgementResult result) => result?.Type == type ? FailState.Force : FailState.Allow;
-
}
public override FailState CheckFail(bool failTriggered, JudgementResult result) => result?.Type == type ? FailState.Force : FailState.Allow; }
}
diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
index 964a674108..e88130dfcc 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneFailJudgement.cs
@@ -61,7 +61,7 @@ private class ModFailOnFirstJudgement : ModFailCondition
public override double ScoreMultiplier => 1.0;
public override string Acronym => "";
-
public override FailState CheckFail(JudgementResult? result) => FailState.Force;
-
}
public override FailState CheckFail(bool failTriggered, JudgementResult? result) => FailState.Force; } }
diff --git a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
index 7c47261a21..013788a368 100644
--- a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
+++ b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs
@@ -18,9 +18,10 @@ public interface IApplicableFailOverride : IApplicableMod
///
/// Check the current failure allowance for this mod.
/// -
/// <param name="failTriggered">Whether a failure has been triggered. In this state, only a <see cref="FailState.Block"/> will change the outcome.</param> /// <param name="result">The judgement result which should be considered. Importantly, will be <c>null</c> if a failure has already being triggered.</param> /// <returns>The current failure allowance (see <see cref="FailState"/>).</returns>
-
FailState CheckFail(JudgementResult? result);
-
FailState CheckFail(bool failTriggered, JudgementResult? result);
}
public enum FailState
diff --git a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
index 1176a452e0..f82f8ed344 100644
--- a/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
+++ b/osu.Game/Rulesets/Mods/ModAccuracyChallenge.cs
@@ -74,7 +74,7 @@ public void ApplyToScoreProcessor(ScoreProcessor scoreProcessor)public ScoreRank AdjustRank(ScoreRank rank, double accuracy) => rank;
-
public override FailState CheckFail(JudgementResult? result) => FailState.Allow;
-
public override FailState CheckFail(bool failTriggered, JudgementResult? result) => FailState.Allow; public enum AccuracyMode {
diff --git a/osu.Game/Rulesets/Mods/ModCinema.cs b/osu.Game/Rulesets/Mods/ModCinema.cs
index 70a0138f8a..b45889ca27 100644
--- a/osu.Game/Rulesets/Mods/ModCinema.cs
+++ b/osu.Game/Rulesets/Mods/ModCinema.cs
@@ -51,6 +51,6 @@ public void ApplyToPlayer(Player player)
public bool RestartOnFail => false;
-
public FailState CheckFail(JudgementResult? result) => FailState.Block;
-
public FailState CheckFail(bool failTriggered, JudgementResult? result) => FailState.Block;
}
}
diff --git a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
index 891d76e48a..6825d661cc 100644
--- a/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
+++ b/osu.Game/Rulesets/Mods/ModEasyWithExtraLives.cs
@@ -36,9 +36,9 @@ public override void ApplyToDifficulty(BeatmapDifficulty difficulty)public bool RestartOnFail => false;
-
public FailState CheckFail(JudgementResult? result)
-
public FailState CheckFail(bool failTriggered, JudgementResult? result) {
-
if (result != null)
-
if (!failTriggered) return FailState.Block; if (retries == 0)
diff --git a/osu.Game/Rulesets/Mods/ModFailCondition.cs b/osu.Game/Rulesets/Mods/ModFailCondition.cs
index 9cb9d25be4..448a1bb9dd 100644
--- a/osu.Game/Rulesets/Mods/ModFailCondition.cs
+++ b/osu.Game/Rulesets/Mods/ModFailCondition.cs
@@ -31,6 +31,6 @@ public void ApplyToHealthProcessor(HealthProcessor healthProcessor)
///
protected void TriggerFailure() => triggerFailureDelegate?.Invoke(this);
-
public abstract FailState CheckFail(JudgementResult? result);
-
}
public abstract FailState CheckFail(bool failTriggered, JudgementResult? result);
}
diff --git a/osu.Game/Rulesets/Mods/ModNoFail.cs b/osu.Game/Rulesets/Mods/ModNoFail.cs
index e64c2a078c..1f3b448246 100644
--- a/osu.Game/Rulesets/Mods/ModNoFail.cs
+++ b/osu.Game/Rulesets/Mods/ModNoFail.cs
@@ -30,7 +30,7 @@ public abstract class ModNoFail : Mod, IApplicableFailOverride, IApplicableToHUD
///
/// We never fail, 'yo.
///
-
public FailState CheckFail(JudgementResult? result) => FailState.Block;
-
public FailState CheckFail(bool failTriggered, JudgementResult? result) => FailState.Block; public void ReadFromConfig(OsuConfigManager config) {
diff --git a/osu.Game/Rulesets/Mods/ModPerfect.cs b/osu.Game/Rulesets/Mods/ModPerfect.cs
index a4c6192f55..07be8ef79a 100644
--- a/osu.Game/Rulesets/Mods/ModPerfect.cs
+++ b/osu.Game/Rulesets/Mods/ModPerfect.cs
@@ -28,7 +28,7 @@ protected ModPerfect()
Restart.Value = Restart.Default = true;
}
-
public override FailState CheckFail(JudgementResult? result)
-
public override FailState CheckFail(bool failTriggered, JudgementResult? result) { if (result == null) return FailState.Allow;
diff --git a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
index 49b3430f24..a462e2bbdb 100644
--- a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
+++ b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs
@@ -23,7 +23,7 @@ public abstract class ModSuddenDeath : ModFailCondition
public override Type[] IncompatibleMods => base.IncompatibleMods.Append(typeof(ModPerfect)).ToArray();
-
public override FailState CheckFail(JudgementResult? result)
-
public override FailState CheckFail(bool failTriggered, JudgementResult? result) { if (result == null) return FailState.Allow;
diff --git a/osu.Game/Rulesets/Scoring/HealthProcessor.cs b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
index 793256abb4..69b9e7f519 100644
--- a/osu.Game/Rulesets/Scoring/HealthProcessor.cs
+++ b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
@@ -117,7 +117,7 @@ private bool meetsAnyFailCondition(JudgementResult result)
foreach (var condition in OrderedFailOverrideMods)
{
-
if (condition.CheckFail(result) == FailState.Force)
-
if (condition.CheckFail(false, result) == FailState.Force) { ModTriggeringFailure = condition as Mod; return true;
diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index 4d8e4d1383..1c29580bdb 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -152,7 +152,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
/// Whether failing should be allowed.
/// By default, this checks whether all selected mods allow failing.
///
-
protected virtual bool CheckModsAllowFailure() => HealthProcessor.OrderedFailOverrideMods.All(m => m.CheckFail(null) != FailState.Block);
-
protected virtual bool CheckModsAllowFailure() => HealthProcessor.OrderedFailOverrideMods.All(m => m.CheckFail(true, null) != FailState.Block); public readonly PlayerConfiguration Configuration;
</details>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explain a bit further, my problem with this occurs when reading ModEasyWithExtraLives
. It reads as "if any judgement has occurred, then we block fail", and, with my prior knowledge that failure can only occur as a result of a judgement, the rest of the code in that method looks like an oxymoron.
It's only when you realise that regardless of any of the mod code running, TriggerFail()
will still be invoked via the CheckDefaultFailCondition()
path, and that later on there's a precondition inside Player
calling the method a second time, that it all starts to make sense.
It's very hidden and non-obvious logic, and I would expect a first time reader/user of this code to go along with my initial thought process.
Then we have to consider the second path that this can happen, which is if a mod triggers failure ModAccuracyChallenge
, which is truly a path "without a judgement".
If my other comments in HealthProcessor
sound reasonable, then maybe a simple solution is:
- Invert the logic:
if (check_default_condition)
TriggerFailure()
if (any_mod_forces_fail)
TriggerFailure()
to:
if (force || allow && check_default_condition)
TriggerFailure()
-
Replace the precondition inside
Player.onFail()
, so that it doesn't do a second lookup onCheckFail()
. -
Make
HealthProcessor.TriggerFailure()
private. Remove the ability for mods to randomly trigger failure. We only allow failure on a judgement in principle, and the new state logic should be able to handle that.ModAccuracyChallenge
could be implemented as forcing the fail provided the condition matches during a judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to apply these changes myself and see how it goes. I'm mostly putting the review here for comments on anything I might have missed - for example perhaps there's a reason for the ordering that means it can't be split into an aggregate condition as I propose, which almost entirely nukes this review.
if (Failed?.Invoke() != false) | ||
HasFailed = true; | ||
|
||
if (triggeringMod != null) | ||
ModTriggeringFailure = triggeringMod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup here is weird to me:
- Why is
ModTriggeringFailure
set after invoking the callback, when it's used inside the callback (albeit inside aSchedule()
). - Why is
ModTriggeringFailure
set even ifHasFailed
is not set totrue
?
Mods.BindValueChanged(mods => | ||
{ | ||
OrderedFailOverrideMods = mods.NewValue.OfType<IApplicableFailOverride>().OrderByDescending(m => m.RestartOnFail).ToArray(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the implications of this ordering, and I would like to see if it can be removed. It's also not documented anywhere as this seems like very specific logic for RestartOnFail
.
The way I envision it is as an aggregate condition, in other words (don't use linq if actually implementing this):
bool allow = Mods.All(m => m.CheckFail() != Block);
bool force = Mods.Any(m => m.CheckFail() == Force);
if (force || allow)
// do fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering is the crux of this PR, to fix the underlying issue (#28434). It may need better documentation, but reading the issue should explain why it's there, I think.
/// </summary> | ||
bool RestartOnFail { get; } | ||
/// <param name="result">The judgement result which should be considered. Importantly, will be <c>null</c> if a failure has already being triggered.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explain a bit further, my problem with this occurs when reading ModEasyWithExtraLives
. It reads as "if any judgement has occurred, then we block fail", and, with my prior knowledge that failure can only occur as a result of a judgement, the rest of the code in that method looks like an oxymoron.
It's only when you realise that regardless of any of the mod code running, TriggerFail()
will still be invoked via the CheckDefaultFailCondition()
path, and that later on there's a precondition inside Player
calling the method a second time, that it all starts to make sense.
It's very hidden and non-obvious logic, and I would expect a first time reader/user of this code to go along with my initial thought process.
Then we have to consider the second path that this can happen, which is if a mod triggers failure ModAccuracyChallenge
, which is truly a path "without a judgement".
If my other comments in HealthProcessor
sound reasonable, then maybe a simple solution is:
- Invert the logic:
if (check_default_condition)
TriggerFailure()
if (any_mod_forces_fail)
TriggerFailure()
to:
if (force || allow && check_default_condition)
TriggerFailure()
-
Replace the precondition inside
Player.onFail()
, so that it doesn't do a second lookup onCheckFail()
. -
Make
HealthProcessor.TriggerFailure()
private. Remove the ability for mods to randomly trigger failure. We only allow failure on a judgement in principle, and the new state logic should be able to handle that.ModAccuracyChallenge
could be implemented as forcing the fail provided the condition matches during a judgement.
Should fix #28434
Not sure if those mods should just have one bool for that? but you could argue this gives more flexibility to player.