Make HealthIncreaseFor(JudgementResult) virtual

[LumpBloom7]

By making Judgement.HealthIncreaseFor(JudgementResult), rulesets are able to implement different health penalties / rewards based on information encoded in JudgementResult.

Rush in particular only penalizes players when colliding with hitobjects, like in MuseDash, but this is achieved via workarounds in HealthProcessor(RushJudgement, RushHealthProcessor). This doesn't play nice with the updated HealthDisplay, which uses Judgement.HealthIncreaseFor(JudgementResult) to determine whether the miss animation should happen.

An alternative would be to make HealthProcessor.HealthIncreaseFor(JudgementResult) public and let HealthDisplay use that to determine whether a miss occurred.

[bdach]

This suggestion is likely to be closely correlated to #24365 and I'm not sure whether it should be considered until we understand what it would take to match stable's HP system closer in the default rulesets in order to avoid breaking ruleset API multiple times.

[smoogipoo]

I suppose we could go with this in any case? I think "mathing stable's HP system" is going to involve changes to the HealthProcessor and not judgements themselves.

[bdach]

"mathing stable's HP system" is going to involve changes to the HealthProcessor and not judgements themselves

I'm not sure I agree, especially with things like #25111 which is now mainline. Right now perfects likely give way too much HP due to being split into 2 judgements (perfect + small bonus). See #24238 (comment).

[frenzibyte]

If we're being iffy about the overall structure of judgements, then we could change HealthDisplay to avoid interacting with such methods and use an alternative way of identifying a "hurting" judgement. Something like:

diff --git a/osu.Game/Screens/Play/HUD/HealthDisplay.cs b/osu.Game/Screens/Play/HUD/HealthDisplay.cs
index fdbce15b40..d850d75bdf 100644
--- a/osu.Game/Screens/Play/HUD/HealthDisplay.cs
+++ b/osu.Game/Screens/Play/HUD/HealthDisplay.cs
@@ -122,7 +122,7 @@ private void onNewJudgement(JudgementResult judgement)
         {
             if (judgement.IsHit && judgement.Type != HitResult.IgnoreHit)
                 Scheduler.AddOnce(Flash);
-            else if (judgement.Judgement.HealthIncreaseFor(judgement) < 0)
+            else if (Current.Value < judgement.HealthAtJudgement)
                 Scheduler.AddOnce(Miss);
         }
 

@bdach @smoogipoo thoughts?

[bdach]

My thought is if there are two ways to check what ostensibly is the same thing and they give two different results, it's not good. So in that respect applying the above would be papering over the cracks.

[frenzibyte]

The current method is technically inaccurate at identifying the effect of the judgement, because HealthProcessor is officially allowed to freely manipulate the health change of the judgement with the virtual GetHealthIncreaseFor method (which was the reason why this PR was open in the first place, because the ruleset was relying on HealthProcessor's methods).

I would go as far as to say either Judgement.HealthIncreaseFor should be internal and not usable anywhere else besides HealthProcessor, or HealthProcessor.GetHealthIncreaseFor should not be overridable for custom rulesets (and a different method should be set up for them).

To me, the simplest solution would be doing the former (marking HealthIncreaseFor internal) and changing HealthDisplay to do something similar to #25052 (comment) and call it a day for this PR.

[smoogipoo]

In implementing #25418, which extracts all knowledge of the health increase to the health processor itself, I've become unsure of where this PR now fits. Could the same technique be used here?

[LumpBloom7]

I think moving all health related values to the Health Processor is good, since it keeps all health related information in a place I would expect it to be. (and also allow different values to be used based on the active health processor)

[bdach]

This doesn't play nice with the updated HealthDisplay, which uses Judgement.HealthIncreaseFor(JudgementResult) to determine whether the miss animation should happen.

This is no longer the case since #25672 so I don't know if this PR has any reason to continue being open.

[smoogipoo]

Let's drop this for now. I think at the time HealthProcessor didn't include a virtual method itself, but it now does and it should probably suffice for this.