Rework the way mod fails are done

[smoogipoo]

RFC. I'm PRing this for easier team review and to make sure that we're all on-board before I continue working on it. The intention is to replace #28825, following the commentary that I provided in that PR.

Please do not review this commit-by-commit as it is not in a final or clean state, and instead look at the full diff.

The HasFailed property has been moved to JudgementProcessor, and any such class can cause the fail. The immediate reason for this is because ModAccuracyChallenge triggers fail based on ScoreProcessor parameters, which are updated after HealthProcessor.
One way around this may be to provide helper methods that compute certain values (in this case Accuracy + MaximumAccuracy) given a result, without actually changing said values inside ScoreProcessor. I backed out of this path because I'm not sure what the scope would be - just accuracy, or score too? combo? highestcombo? maximum remaining achievable combo? The scope seems endless.

As in #28825, IApplicableFailOverride has a new method providing an enum fail state - Block, Allow and Force. Unlike that PR, the result parameter is not nullable (the method is only triggered on a result by JudgementProcessor) and whether a fail is triggered at the end of the day is instead somewhat of a decision tree (see JudgementProcessor.meetsAnyFailCondition()).

[peppy]

Is this supposed to compile?

[smoogipoo]

Game will compile, yes. Tests no because I want feedback on the direction before spending time dealing with them.

[bdach]

I'm probably not that deep into this code as the others are but I do prefer this over #28825. I share the concern about introducing ordering to determining failure, as well as the concern about mod code being less readable in that other PR in comparison to this.

A couple of sidebar comments but in general I do prefer this.

[bdach]

Not sure on the naming here, I'd probably adjust to something like PreventFail / NoEffect / ForceFail or something. But the idea is sound, so no major objections here.

[peppy]

I'd agree with the proposed renaming. The current naming was very hard to work with when I was looking at this.

[bdach]

The only part I'm not sure is this thing, because it's dropping the information identifying the mod that called for a restart.

That said, I think it is fine? My thinking is that this handles cases where two mods would allow or force a failure at the same instant, so at that point it's probably fine to decide to do whatever, and allowing the mod that forces restart to take precedence is probably a better idea.

[bdach]

This being here is a tad weird... In the large scale of things it's probably fine, but I wouldn't expect to see this here.

[smoogipoo]

How do you imagine this working? I'm struggling to understand what your suggestions are.

I pondered RestartOnFail quite a bit because the semantics are somewhat up in the air. My thinking is that if a mod forces fail then it should have the say as to whether to restart or not, leading to the above logic (including your comment above).

Are you seeing:

1. Making this an interface-implemented property (implemented as => false by default). (I'm not sure how to feel on these in general right now).
2. Making the enum a bitwise flag?

Otherwise, please describe the suggestions / your perspective a bit more.

[bdach]

(1) would work, but in general I'm not super sure into trying to force ModNoFail and similar mods into the IApplicableFailOverride hole. The slightly-left-of-field off-the-cuff suggestion here would be to give fail-blocking mods a separate interface (IBlockFail?) instead. That would probably make FailState disappear again too as it could be a bool again on IApplicableFailOverride (which would probably become IForceFail or something).

[smoogipoo]

Based on above comments, I've split IApplicableFailOverride into two interfaces:

IBlockFail
    bool AllowFail()

IForceFail
    bool RestartOnFail { get; }
    bool ShouldFail(JudgementResult)

Curious on the naming/thoughts.

Also, this PR is still not ready because there's another issue to be resolved: AllowFail will be triggered twice, once by HealthProcessor and by ScoreProcessor, which means ModEasy isn't working correctly. I'm not 100% sure on the solution, and request opinions:

• A method returning whether the mod should apply to a specific processor: bool IBlockFail.ApplyToJudgementProcessor()
• Pass triggering JudgementProcessor to both methods (e.g. bool AllowFail(JudgementProcessor))
• Register the callbacks to a JudgementProcessor instead? Similar to how it was originally with FailConditions?
• Maybe AllowFail() should only be checked by HealthProcessor as it was originally, and ShouldFail only be checked by ScoreProcessor?

[smoogipoo]

I applied one of my proposals from the above - to check IBlockFail in HealthProcessor and IForceFail in ScoreProcessor.

[bdach]

I applied one of my proposals from the above - to check IBlockFail in HealthProcessor and IForceFail in ScoreProcessor.

I'm not sure I would have gone with that. It seems rather arbitrary and dependent on specific ordering, and avoiding specific ordering was initially the point of this PR.

[peppy]

I didn't see the most recent comments, but came here to review as a whole and also look into test failures.

Something like this should fix failures:

diff --git a/osu.Game/Rulesets/Scoring/HealthProcessor.cs b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
index b6bb3a98d8..09aad81f94 100644
--- a/osu.Game/Rulesets/Scoring/HealthProcessor.cs
+++ b/osu.Game/Rulesets/Scoring/HealthProcessor.cs
@@ -26,14 +26,14 @@ protected override void ApplyResultInternal(JudgementResult result)
 
             if (CheckDefaultFailCondition(result))
             {
-                bool allowFail = false;
+                bool allowFail = true;
 
                 for (int i = 0; i < Mods.Value.Count; i++)
                 {
                     if (Mods.Value[i] is IBlockFail blockMod)
                     {
                         // Intentionally does not early return so that all mods have a chance to update internal states (e.g. ModEasyWithExtraLives).
-                        allowFail |= blockMod.AllowFail();
+                        allowFail &= blockMod.AllowFail();
                         break;
                     }
                 }
diff --git a/osu.Game/Tests/Visual/ModForceFailTestScene.cs b/osu.Game/Tests/Visual/ModForceFailTestScene.cs
index 4c6988f5ed..86b5527a95 100644
--- a/osu.Game/Tests/Visual/ModForceFailTestScene.cs
+++ b/osu.Game/Tests/Visual/ModForceFailTestScene.cs
@@ -40,12 +40,12 @@ public ModFailConditionTestPlayer(ModTestData data, bool allowFail)
 
             protected override bool CheckModsAllowFailure() => true;
 
-            public bool CheckFailed(bool failed)
+            public bool CheckFailed(bool shouldHaveFailed)
             {
-                if (!failed)
+                if (!shouldHaveFailed)
                     return ScoreProcessor.HasCompleted.Value && !HealthProcessor.HasFailed;
 
-                return HealthProcessor.HasFailed;
+                return HealthProcessor.HasFailed || ScoreProcessor.HasFailed;
             }
         }
 

But without reading the above, I was going to bring up the fact that now HealthProcessor and ScoreProcessor both have separate fail states (as noted in the test fix). I'm not 100% sure about this either..

Apart from this, the code does seem quite legible.

[peppy]

As commented

[smoogipoo]

I'm not sure I would have gone with that. It seems rather arbitrary and dependent on specific ordering, and avoiding specific ordering was initially the point of this PR.

If you have a preference I'm all ears. I took a path to keep the ball rolling in one of the possible directions as I would like to try things out to see what looks good and sticks without engaging in bureaucratic criticism of 4 diffs (one for each suggestion) at once.

---

This is still avoiding the ordering to me. Here's my reasoning:

• HealthProcessor can only trigger a fail after the judgement has been applied. This is so a player can remain at 0 health getting HP-increase judgements, and still keep living.
• In a similar fashion, I reason that IForceFail can only be run after all scoring parameters have been updated because it is a very undirected way of forcing a fail (i.e. it can depend on any scoring parameter). It just so happens that this time is after ScoreProcessor runs.
• Could there be an IForceFail that runs after HealthProcessor (before ScoreProcessor)? Sure, but I don't think it'd change much besides making certain things harder to handle (imagine binding IForceFail to a particular processor, sorta thing). At least, I can't imagine a reason why you'd want it to fail there specifically.

Perhaps the way we've split the processors out is a bad idea. I say this because of two reasons. First, I understand the above concern, and it would be nice to have everything in one place. Second, I really don't like that the fail state is split between the two - it means one has HasFailed = true and the other HasFailed = false.
I'm not sure if the solution to that is a part of this PR (or a prerequisite), but some way of composing the "gameplay processor"?

An alternative solution may be to add a third processor, like a FailProcessor, that runs after both these other ones. As I alluded to above, it doesn't really matter imo whether an HP-based fail occurs after HealthProcessor or after ScoreProcessor.

[peppy]

Splitting the two out felt good for keep the classes small, but I agree that maybe they should be combined.

I'm fine with that being done elsewhere / later. Curious on @bdach's thoughts.

[bdach]

An alternative solution may be to add a third processor, like a FailProcessor, that runs after both these other ones.

If we must enforce ordering somehow then I'd prefer this. It feels much less shoddy to me than arbitrarily deciding that one processor does half of the fail processing and the other processor does the other half.

I have vague unsubstantiated reservations to combining the two existing processors, because I think it will result in a class that is in totality too large to live.

[smoogipoo]

Moving things into a FailProcessor gets weird, because the health-based fail condition has 4 implementations 1 2 3 4.

Sure, to simplify things I could try to move the judgement-based conditions inside JudgementResult (even though that would already be somewhat of a challenge imo), but I'm not sure what to do about the other conditions and CheckDefaultFailCondition(JudgementResult) itself which references [Health] and a private var in AccumulatingHealthProcessor...

Is the solution that FailProcessor shall receive a HealthProcessor and 🙈 reference its things? It's a very weird coupling.

This change looks kind of weird to me:

diff --git a/osu.Game.Rulesets.Catch.Tests/CatchHealthProcessorTest.cs b/osu.Game.Rulesets.Catch.Tests/CatchHealthProcessorTest.cs
index 1b46be01fb..c086ac8f1f 100644
--- a/osu.Game.Rulesets.Catch.Tests/CatchHealthProcessorTest.cs
+++ b/osu.Game.Rulesets.Catch.Tests/CatchHealthProcessorTest.cs
@@ -6,6 +6,7 @@
 using osu.Game.Rulesets.Catch.Judgements;
 using osu.Game.Rulesets.Catch.Objects;
 using osu.Game.Rulesets.Catch.Scoring;
+using osu.Game.Rulesets.Scoring;
 
 namespace osu.Game.Rulesets.Catch.Tests
 {
@@ -25,18 +26,20 @@ public class CatchHealthProcessorTest
         [TestCaseSource(nameof(test_cases))]
         public void TestFailAfterMinResult(CatchHitObject hitObject, double startingHealth, bool failExpected)
         {
-            var healthProcessor = new CatchHealthProcessor(0);
-            healthProcessor.ApplyBeatmap(new CatchBeatmap
-            {
-                HitObjects = { hitObject }
-            });
-            healthProcessor.Health.Value = startingHealth;
-
+            var beatmap = new CatchBeatmap { HitObjects = { hitObject } };
             var result = new CatchJudgementResult(hitObject, hitObject.CreateJudgement());
             result.Type = result.Judgement.MinResult;
+
+            var healthProcessor = new CatchHealthProcessor(0);
+            healthProcessor.ApplyBeatmap(beatmap);
+            healthProcessor.Health.Value = startingHealth;
             healthProcessor.ApplyResult(result);
 
-            Assert.That(healthProcessor.HasFailed, Is.EqualTo(failExpected));
+            var failProcessor = new FailProcessor(healthProcessor);
+            failProcessor.ApplyBeatmap(beatmap);
+            failProcessor.ApplyResult(result);
+
+            Assert.That(failProcessor.HasFailed, Is.EqualTo(failExpected));
         }
 
         [TestCaseSource(nameof(test_cases))]

[bdach]

Is the solution that FailProcessor shall receive a HealthProcessor and 🙈 reference its things?

I'm not super opposed to this mind. If it stops there being a god Processor class where everything can be intertwined, then I'd prefer concern isolation to at least that level? Like health processor will process health, score processor will process score, and then the fail processor can decide what to do based on what the two preceding do.

That said, in the provided diff, why does failProcessor need to have beatmap applied? I'd just hope it can just access whatever state it needs from the two other processors.