Medical Status - Add isDamageAllowed check to setDead

[LinkIsGrim]

When merged this pull request will:

• Title.

IMPORTANT

• If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
• Development Guidelines are read, understood and applied.
• Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

[kymckay]

Just to clarify because this is really hard to review without context:

1. What error in the existing logic is causing units to remain alive but be in the dead state? Where is this function being called from a remote location?
2. Is this function intended to run on remote units (i.e. is this fix valid)? It's been a while since I worked on the state machine, but does it not run on all machines only for the units local to each?
3. Why is the frame delay necessary? What's happening without it?

[LinkIsGrim]

1. Good question. This shouldn't ever be called from a remote location under normal working conditions.
2. Function header would imply this was originally local effect only. Need to ask commy on what intention was. Yes, statemachine should only run for local units.
3. We're shooting in the dark here. In theory, setting damage twice on a unit in the same frame works perfectly fine: the unit is killed by the first call of setHitPointDamage then the second call returns damage to previous so as to remove visual changes. No idea if this is the part that's failling.

Veteran's community has had issues with players being moved to dead state while remaining alive. As entering dead state would require this function to be called, I can only see three things failling:

1. The unit has damage disabled on it. I find it hard to believe this is the case.
2. The unit isn't local. setHitPointDamage on a remote unit has no effect. Thus, the unit remains alive but is in the dead state.
3. The second call of setHitPointDamage on the unit cancels the first if done in the same frame. This doesn't seem to be the case in my testing, but I've also seen no negative change in delaying a frame anyway.

To clarify, I've never seen the bug myself, so these are just my best guesses on the subject. Could be some other obscure interaction or something else entirely for all I know.

[kymckay]

Thanks, that's perfect! ☺️ As a general bit of feedback on development habits: try to make exactly those intentions/assumptions clear in both the code comments and in the git history. It saves reviewers having to reverse engineer the coding intention (or take your word that it's sound), but also is vital for anyone who looks at it a year from now and wants to know why it's there.

Personally I'm unsure about these changes, if we have no evidence to suggest they're the issue then they seem like a bit of a red herring. I do seem to recall we used to have a delay on the damage resetting, may be worth checking git history to see why it was removed (if true) 🤔 While there would be no harm in adding remote support to the function, if the intention is that it only ever runs local to the unit then the right fix would be to figure out where it is running remotely (if it is).

Edit: Don't be discouraged though, let's see what others think too.

[kymckay]

Ah looks like previously when we had a frame delay it was back when we were using setStructuralDamage, during the rewrite I removed it - but it's not analogous to the delay here because it was applied to the actual kill damage, not the damage afterwards.

[veteran29]

@kymckay see the linked PR from my unit mod, the PR was made before I was actually able to evaluate where the issue is. @Salluci can you convert this PR to the draft for now?

[veteran29]

After further investigation, I found out that the medical issue we were facing in my unit is by us enabling BIS EG Spectator the moment unit dies, this started causing issues due to setHitPointDamage respecting that flag.

@Salluci could you simplify this PR to include only the:

if (!isDamageAllowed _unit) then {
    WARNING_1("setDead executed on unit with damage blocked - %1",_unit);
    _unit allowDamage true;
};

bit.

[kymckay]

@veteran29 Are you hooking into the local event of this function which runs before the setHitPointDamage?

In any case I think this check should be moved to after that since other code using that event could be falsely preventing damage, would also be good to see a code comment explaining that very reasoning for future reference.