Fix ArgonHealthDisplay sometimes behaving weirdly on miss judgements (alternative)

[frenzibyte]

• Supersedes and closes Fix ArgonHealthDisplay sometimes behaving weirdly on miss judgements #25592
• Addresses Argon health drain effect doesn't animate correctly on fail #25351

Took a completely different approach using schedules, works pretty well and 10x simpler than #25592.

Before:

CleanShot.2023-12-04.at.23.57.13.mp4

After:

CleanShot.2023-12-04.at.23.56.08.mp4

[bdach]

Subscription is not cleaned up on disposal.

[bdach]

I dunno. This appears to be assuming that if the player missed, then the pendingJudgementResult here will be a miss. But the delay introduced by the schedule makes it very difficult to say that it will be for sure. What happens if a miss and a hit get judged and only then the schedule runs? (This isn't necessarily farfetched - see e.g. replays and FrameStabilityContainer processing multiple replay frames in one game frame.) This would presumably break?

Rather than store the latest judgement result this should probably store a bool flag that determines if there are any misses that haven't been "animated" yet. The completion of the glow bar "animation" would clear the flag.

[frenzibyte]

At the moment where multiple judgements are processed at once, I'm not sure the health display should be as accurate as it can. The only case I can think of where this can happen is when the user seeks forward and gameplay starts to move fast, at which point it sounds redundant to look into this? Unless there's another case that I cannot think of, and is noticeable in action.

The proposal you have feels equally odd to me, if a miss occurs but multiple hits occur after it, then showing a red bar while health is increasing might look wrong.

[bdach]

The proposal you have feels equally odd to me, if a miss occurs but multiple hits occur after it, then showing a red bar while health is increasing might look wrong.

I dunno, seems like it'd be less wrong for it to animate than for it to just instantly go away, would it not? The instant transitions is what this PR is trying to fix, isn't it?

[frenzibyte]

Well, I still cannot imagine a specific scenario where the current logic needs to be changed, but that aside, does the following match your proposal, or is there more to it?

diff --git a/osu.Game/Screens/Play/HUD/ArgonHealthDisplay.cs b/osu.Game/Screens/Play/HUD/ArgonHealthDisplay.cs
index e27466e811..f831b05e8f 100644
--- a/osu.Game/Screens/Play/HUD/ArgonHealthDisplay.cs
+++ b/osu.Game/Screens/Play/HUD/ArgonHealthDisplay.cs
@@ -151,7 +151,7 @@ private void load()
             };
         }
 
-        private JudgementResult? pendingJudgementResult;
+        private bool pendingMissAnimation;
 
         protected override void LoadComplete()
         {
@@ -171,7 +171,7 @@ protected override void LoadComplete()
             BarHeight.BindValueChanged(_ => updatePath(), true);
         }
 
-        private void onNewJudgement(JudgementResult result) => pendingJudgementResult = result;
+        private void onNewJudgement(JudgementResult result) => pendingMissAnimation |= !result.IsHit;
 
         private void onCurrentChanged(ValueChangedEvent<double> valueChangedEvent)
             // schedule display updates one frame later to ensure we know the judgement result causing this change (if there is one).
@@ -179,8 +179,6 @@ private void onCurrentChanged(ValueChangedEvent<double> valueChangedEvent)
 
         private void updateDisplay()
         {
-            var result = pendingJudgementResult;
-
             if (Current.Value >= GlowBarValue)
                 finishMissDisplay();
 
@@ -189,12 +187,13 @@ private void updateDisplay()
             // TODO: this should probably use interpolation in update.
             this.TransformTo(nameof(HealthBarValue), Current.Value, time, Easing.OutQuint);
 
-            if (result != null && !result.IsHit)
+            if (pendingMissAnimation)
+            {
                 triggerMissDisplay();
+                pendingMissAnimation = false;
+            }
             else if (!displayingMiss)
                 this.TransformTo(nameof(GlowBarValue), Current.Value, time, Easing.OutQuint);
-
-            pendingJudgementResult = null;
         }
 
         protected override void Update()

[bdach]

That looks roughly as I'd expect it to yes. If it works.

[frenzibyte]

After writing a test case of a miss judgement followed by a hit in the same update frame, I can see what you mean now. Looks better indeed 👍🏻