Fix player restarting on fail while the triggering mod is not configured as such #28825

Peter-io · 2024-07-11T13:15:41Z

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.

osu.Game/Rulesets/Mods/IApplicableFailOverride.cs

This reverts commit de7c90d.

This reverts commit 8da673e.

This reverts commit 674d6ee.

osu.Game/Screens/Play/Player.cs

osu.Game/Rulesets/Scoring/HealthProcessor.cs

not sure to what yet

Peter-io · 2024-07-11T20:20:32Z

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 FailConditions and keeping things mostly the same.
Could use this one for PR instead, some likely changes would be to replace object type to something else for FailTriggers list, unless anything could become a trigger in future and not just mods?

osu.Game/Rulesets/Scoring/HealthProcessor.cs

osu.Game/Rulesets/Mods/IHasFailCondition.cs

osu.Game/Rulesets/Scoring/HealthProcessor.cs

smoogipoo · 2024-07-16T01:53:12Z

osu.Game/Rulesets/Scoring/HealthProcessor.cs

@@ -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))


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.

I got an idea for that

smoogipoo · 2024-07-16T02:00:15Z

osu.Game/Rulesets/Mods/IHasFailCondition.cs

+ /// <remarks>
+ /// This method should only be used to trigger failures based on <paramref name="result"/>
+ /// </remarks>
+ bool FailCondition(JudgementResult result);


Isn't this basically the same as IApplicableOverride.PerformFail()? I feel like these two interfaces can't exist together...

I don't think they are the same, however i think they could be somehow combined?

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.

Co-authored-by: Dan Balasescu <[email protected]>

frenzibyte · 2024-07-17T11:58:20Z

osu.Game/Screens/Play/Player.cs

@@ -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();


I proposed this at first, but I think just remove this at this point until someone notices it.

peppy · 2024-07-24T11:02:39Z

Proposal 1: two methods

diff --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: combined

diff --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);

Second is a but of a mouthful, not sure.

cc @smoogipoo @frenzibyte

peppy

As commented

frenzibyte · 2024-07-24T19:46:38Z

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 HealthProcessor (which is the meetsAnyFailCondition method).

I meant to open a PR of the above but there were test failures and I couldn't get to it in time.

smoogipoo · 2024-07-24T23:11:44Z

From the look of it I definitely prefer the second proposal.

peppy · 2024-07-25T14:52:17Z

I'll tidy up the second proposal and commit it to this branch.

peppy · 2024-08-06T14:18:55Z

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.

smoogipoo · 2024-08-08T06:45:52Z

osu.Game/Rulesets/Mods/IApplicableFailOverride.cs

 /// </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>


The null condition really destroys my brain :/

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.

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...)

```diff diff --git a/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs b/osu.Game.Rulesets.Mania/Mods/ManiaModPerfect.cs index 24dcf67bca..450f2c07f2 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 FailState CheckFail(JudgementResult? result) + public override FailState CheckFail(bool failTriggered, JudgementResult? result) { if (result == null) return FailState.Allow; diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs index 5afaec4868..6f7fb4606a 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs +++ b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs @@ -102,7 +102,7 @@ public class OsuModTargetPractice : ModWithVisibilityAdjustment, IApplicableToDr
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>

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 on CheckFail().

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.

smoogipoo

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.

smoogipoo · 2024-08-09T05:52:52Z

osu.Game/Rulesets/Scoring/HealthProcessor.cs

 if (Failed?.Invoke() != false)
 HasFailed = true;
+
+ if (triggeringMod != null)
+ ModTriggeringFailure = triggeringMod;


The setup here is weird to me:

Why is ModTriggeringFailure set after invoking the callback, when it's used inside the callback (albeit inside a Schedule()).

Why is ModTriggeringFailure set even if HasFailed is not set to true?

smoogipoo · 2024-08-09T06:00:33Z

osu.Game/Rulesets/Scoring/HealthProcessor.cs

+ Mods.BindValueChanged(mods =>
+ {
+ OrderedFailOverrideMods = mods.NewValue.OfType<IApplicableFailOverride>().OrderByDescending(m => m.RestartOnFail).ToArray();
+ });


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

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.

smoogipoo · 2024-08-09T06:37:44Z

osu.Game/Rulesets/Mods/IApplicableFailOverride.cs

 /// </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>


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 on CheckFail().

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.

Peter-io added 3 commits July 11, 2024 14:27

Add TriggeredFail to IApplicableFailOverride

674d6ee

Include TriggeredFail in mods

8da673e

Add additional check in Player

de7c90d

pull-request-size bot added the size/M label Jul 11, 2024

bdach requested changes Jul 11, 2024

View reviewed changes

osu.Game/Rulesets/Mods/IApplicableFailOverride.cs Outdated Show resolved Hide resolved

Peter-io added 6 commits July 11, 2024 15:47

Revert "Add additional check in Player"

3a73fa4

This reverts commit de7c90d.

Revert "Include TriggeredFail in mods"

7fd6f3f

This reverts commit 8da673e.

Revert "Add TriggeredFail to IApplicableFailOverride"

307a0e8

This reverts commit 674d6ee.

Store FailTrigger in HealthProcessor

2aeda54

Pass mod class that cause fail

7e1ee61

Add additional condition

00d65f9

pull-request-size bot added size/S and removed size/M labels Jul 11, 2024

Peter-io changed the title ~~Check which IApplicableFailOverride mod triggers fail~~ Check which mod triggers fail Jul 11, 2024

Remove unused using

527e143

frenzibyte requested changes Jul 11, 2024

View reviewed changes

osu.Game/Screens/Play/Player.cs Outdated Show resolved Hide resolved

osu.Game/Rulesets/Scoring/HealthProcessor.cs Outdated Show resolved Hide resolved

Peter-io added 5 commits July 11, 2024 20:20

Implement IHasFailCondition interface

0a8d909

Use IHasFailCondition in HealthProcessor

4a473f4

Implement IHasFailCondition in mods

751006c

Change condition in Player

97cde4f

Change summaries

d1323bb

not sure to what yet

pull-request-size bot added size/M and removed size/S labels Jul 11, 2024

frenzibyte added area:gameplay area:mods labels Jul 12, 2024

frenzibyte requested changes Jul 12, 2024

View reviewed changes

Peter-io added 3 commits July 12, 2024 15:40

Apply suggestions to HealthProcessor

b21d47e

Apply suggestions to mods

2a7124c

Change condition in Player

d95db40

frenzibyte added 4 commits July 15, 2024 08:08

Let IHasFailCondition inherit from IApplicableFailOverride

37b12fe

Reword documentations

3739067

Update existing tests

4aaf477

Add test covering restart-on-fail preference

126a39b

pull-request-size bot added size/L and removed size/M labels Jul 15, 2024

frenzibyte previously approved these changes Jul 15, 2024

View reviewed changes

frenzibyte requested a review from a team July 15, 2024 05:49

frenzibyte changed the title ~~Check which mod triggers fail~~ Fix player restarting on fail while the triggering mod is not configured as such Jul 15, 2024

smoogipoo reviewed Jul 16, 2024

View reviewed changes

Peter-io dismissed frenzibyte’s stale review via b21473a July 16, 2024 12:40

Pass already ordered mods to HealthProcessor

54c12c2

Peter-io force-pushed the check-fail-triggerer branch from 56dfae9 to 54c12c2 Compare July 16, 2024 12:59

Use get/set for ModTriggeringFailure

950b001

Co-authored-by: Dan Balasescu <[email protected]>

frenzibyte reviewed Jul 17, 2024

View reviewed changes

peppy self-requested a review July 24, 2024 10:39

peppy reviewed Jul 24, 2024

View reviewed changes

peppy self-requested a review August 6, 2024 13:11

peppy added 2 commits August 6, 2024 23:17

Combine fail blocking and forcing logic into a singular mod interface

9ed8528

Fix fail check ordering not being enforced correctly

278e227

peppy requested a review from a team August 6, 2024 14:28

Merge branch 'master' into check-fail-triggerer

7272747

smoogipoo reviewed Aug 8, 2024

View reviewed changes

smoogipoo reviewed Aug 9, 2024

View reviewed changes

smoogipoo mentioned this pull request Aug 10, 2024

Rework the way mod fails are done #29379

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix player restarting on fail while the triggering mod is not configured as such #28825

Fix player restarting on fail while the triggering mod is not configured as such #28825

Peter-io commented Jul 11, 2024

Peter-io commented Jul 11, 2024 •

edited

Loading

smoogipoo Jul 16, 2024

Peter-io Jul 16, 2024

smoogipoo Jul 16, 2024 •

edited

Loading

Peter-io Jul 16, 2024 •

edited

Loading

frenzibyte Jul 16, 2024

frenzibyte Jul 17, 2024 •

edited

Loading

peppy commented Jul 24, 2024 •

edited

Loading

peppy left a comment

frenzibyte commented Jul 24, 2024 •

edited

Loading

smoogipoo commented Jul 24, 2024 •

edited

Loading

peppy commented Jul 25, 2024

peppy commented Aug 6, 2024 •

edited

Loading

smoogipoo Aug 8, 2024

peppy Aug 8, 2024

peppy Aug 8, 2024 •

edited

Loading

smoogipoo Aug 9, 2024

smoogipoo left a comment

smoogipoo Aug 9, 2024

smoogipoo Aug 9, 2024

peppy Aug 9, 2024

smoogipoo Aug 9, 2024

Fix player restarting on fail while the triggering mod is not configured as such #28825

Are you sure you want to change the base?

Fix player restarting on fail while the triggering mod is not configured as such #28825

Conversation

Peter-io commented Jul 11, 2024

Peter-io commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Peter-io Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

peppy commented Jul 24, 2024 • edited Loading

peppy left a comment

Choose a reason for hiding this comment

frenzibyte commented Jul 24, 2024 • edited Loading

smoogipoo commented Jul 24, 2024 • edited Loading

peppy commented Jul 25, 2024

peppy commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter-io commented Jul 11, 2024 •

edited

Loading

smoogipoo Jul 16, 2024 •

edited

Loading

Peter-io Jul 16, 2024 •

edited

Loading

frenzibyte Jul 17, 2024 •

edited

Loading

peppy commented Jul 24, 2024 •

edited

Loading

frenzibyte commented Jul 24, 2024 •

edited

Loading

smoogipoo commented Jul 24, 2024 •

edited

Loading

peppy commented Aug 6, 2024 •

edited

Loading

peppy Aug 8, 2024 •

edited

Loading