Fixed VitalChangeDetector UI synchronization issue.

[John-Leitch]

This PR fixes the following issue: #2593 (Changing Vitals while HUDVitals is not Active)

RCA can be found in my comment on the issue. This was the least hacky fix I could think of without resorting to updating HUDVitals every tick from DaggerfallUI.Update, or doing more complicated state tracking in VitalsChangeDetector to ensure VitalsChangeDetector.HealthLost wasn't missed. The latter approach would have been difficult to make generic to cover all of the states that can be missed e.g. FatigueLost, MagickaLost, etc., and may have involved modifications to referencing code as well.

Instead, I opted to modify VitalsChangeDetector to delay updates a single tick after each unpause. This should cover cases like those observed in #2593, where only a single tick passes between pauses, wherein a window is pushed to the front that causes HUDVitals to miss the VitalsChangeDetector update. This approach also has the added benefit of triggering the view bob, which probably wouldn't occur if side channels such as HUDVitals.SynchronizeImmediately were used instead.

In code review and testing, nothing seemed to stick out to me, and everything worked as expected. However, this probably warrants careful review of somebody more familiar with the code. If there's a better way of doing this that I missed, let me know and I will update appropriately.

[KABoissonneault]

I would have preferred an approach where HUDVitals remembers its last current health / fatigue / spell points, and forces its own refresh if it doesn't match the current vitals.

The VitalsChangeDetector is used in three places: the camera recoiler, the HUD Flicker Controller, and the HUD Vitals. I don't think any of these are critical when it comes to your changes, but also, I don't like adding hacky "wait one frame" solutions.

The only problem here is that the HUD Vitals relies on frame-to-frame changes to detect if it should update at all, and is not resilient to missing a frame. If it could miss a frame for reasons other than pause, your solution wouldn't work.

I really think we should just take VitalsChangeDetector as a first indicator of vitals change (since its HealthLost properties help control how much we should react to direct changes), and if the detector returns nothing new, then we should fallback to checking for disparity between current player vitals and current HUD vitals.

[John-Leitch]

@KABoissonneault, if you think this is better handled in HUDVitals, I can fire off a PR that handle it there. However, in terms of player experience, that will mean no more recoil or anything, so the only feedback will be the bars changing. Perhaps not that big of a deal, but I was hoping to keep the behaviors consistent.

[KABoissonneault]

I think one concern I have with your approach is, what if there's 2 frames between pauses? Do we recreate the issue?

What I want to avoid here is the HUD stuck on a bad value. The rest is pretty minor. Mods should not damage players on one frame between two pauses if they want a proper reaction. With your fix, the recoil occurs after accepting the quest reward right?

[John-Leitch]

Regarding >1 frame between pauses, that depends on whether or not DaggerfallHUD is on top. If it's not on top, then yes, the issue will return. Currently, with the way windowing is handled, I don't see how that can happen; UserInterfaceManager.PushWindow is what ultimately triggers the pause. Reviewing the code again, I'm not seeing any way for >1 frame to occur with the game unpaused and DaggerfallHUD not on top. However, I do see what you're saying, and even if I'm not missing anything, future changes could lead to less-than-obvious regressions.

As for the behavior, yes, with the provided test case it is triggered after accepting the quest reward. From the outside in, I'm not sure if I would have expected an unpaused frame between the dialog and the quest reward. While an even more extreme edge case than what we're talking about right now, I could see one arguing that is the actual bug. Should an enemy be standing near the player while completing a question, wouldn't it be possible for the enemy to damage the player in that frame?

[John-Leitch]

Okay, upon further review, it seems that CameraRecoiler was actually catching the update from VitalsChangeDetector, so my delay hack probably isn't necessary. Out of curiosity I looked to see if catching the frames slipping through between pauses was a viability, but GameManager.PauseGame is being called in too many disparate ways, and high risk whackamole changes for what is otherwise a minor issue didn't seem worth it.

The new commit reverts the delay hack, and adds extra checks in HUDVitals to catch missed updates from VitalsChangeDetector, as recommended.

[petchema]

If HUDVitals tracks vitals' changes on his own, what becomes the purpose of VitalsChangeDetector?

[John-Leitch]

Based on a quick look at references, VitalsChangeDetector is still used by CameraRecoiler and HUDFlickerController, so more than just HUDVitals. However, from the perspective of best practices (specifically separation-of-concerns), it seems clear this type of state tracking is better suited for VitalsChangeDetector. Perhaps adding some events to VitalsChangeDetector and subscribing from HUDVitals would be a better approach.

[John-Leitch]

Updated VitalsChangeDetector to trigger events on change. Unfortunately, the level of abstraction did not easily permit genericizing of the code, so there's a good deal of duplication on both the event and subscriber sides.

Thinking aloud, I wonder if there's any value in abstracting the vitals and making the types dynamic. Not only would it allow for refactoring of the code to make it more maintainable by more easily enforcing DRY, it could also allow an easier extension point for modders e.g. adding hunger/thirst bars.