use of WF_NEVER_EXISTED

[Goober5000]

Prompted by The E's comment here: https://github.com/scp-fs2open/fs2open.github.com/pull/369#discussion_r42224096

The only place this flag is checked (as opposed to set) is in the sexp_query_has_yet_to_arrive() function. This is used to return SEXP_CANT_EVAL in sexps like is-destroyed, is-disabled, has-arrived, and so forth. SEXP_CANT_EVAL makes sense for wings which haven't arrived yet. However, for wings that never existed, they won't ever arrive and therefore SEXP_CANT_EVAL is inappropriate.

Now, the use of WF_NEVER_EXISTED appears redundant in sexp_query_has_yet_to_arrive() because if a wing never existed, its total_arrived_count should also be zero.

The strange thing is that in parse_wing_create_ships(), if the mothership is destroyed then WF_NEVER_EXISTED is set to true and the total_arrived_count is set to a nonzero value. I think this is a mistake, and this was supposed to be wrapped in an if() block (as the comment "if the current wave is zero" implies) where it checks that EITHER the wing never arrived in the first place, it is marked as never existing, OR it did (partially) arrive, and therefore the entire wing is marked destroyed, including the waves yet to appear, since they were all onboard the destroyed mothership. It should be two mutually exclusive cases for two blocks of code, rather than both blocks of code applying to both cases.

Thinking on this issue a bit, I believe this may be the source of an actual bug that dates back to retail, to wit: when a mothership is destroyed before all waves of a wing have arrived, sexps involving that wing do not fire. This can be seen in High Noon when the Sathanas is destroyed before both waves of Cancer wing arrive, and the RTB directive is never shown. I believe this is because the WF_NEVER_EXISTED flag is incorrectly set, causing the sexps checking Cancer wing to return SEXP_CANT_EVAL, causing the RTB conditions to perpetually be incomplete.

I believe all of these issues can be resolved by adding the missing if() block to parse_wing_create_ships(). The new code would look like this:

if (wingp->current_wave == 0)
{
    // if the current wave is zero, it never existed
    wingp->flags |= WF_NEVER_EXISTED;
}
else
{
    // mark the number of waves and number of ships destroyed equal to the last wave and the number
    // of ships yet to arrive
    num_remaining = ( (wingp->num_waves - wingp->current_wave) * wingp->wave_count);
    wingp->total_arrived_count += num_remaining;
    wingp->current_wave = wingp->num_waves;
}

[MageKing17]

If WF_NEVER_EXISTED is only ever checked in that one place, why should we have it at all? Why does it make sense to mark a wing as "never having existed" if its mothership is destroyed before its first wave can launch?

[Goober5000]

Well, conceptually, it makes sense that a wing has never existed (from the point of view of a mission) if the mothership is destroyed before it can launch.

Presumably, sexp_query_has_yet_to_arrive() tests the WF_NEVER_EXISTED flag because wingp->total_arrived_count is not 0 when it is expected to be. However, I believe a better solution is to not set wingp->total_arrived_count at all when the wing has not yet arrived.

We could, in fact, entirely remove the WF_NEVER_EXISTED flag and change the missing if() block in parse_wing_create_ships() to mark the number of waves and ships if wingp->current_wave != 0.

[MageKing17]

Well, conceptually, it makes sense that a wing has never existed (from the point of view of a mission) if the mothership is destroyed before it can launch.

Why?

[The-E]

There's a few things you could do with such information. A debrief could talk (or not talk, as the case may be) about wings the player has shot down, campaigns could alter themselves based on the enemies faced etc.

Not that that's at all relevant to FS2 retail, or for that matter most user-made campaigns, but it is something that could be useful.

That said, I do believe we'd be better off cutting this flag entirely, as there are other ways to implement this behaviour if we really do need it.

[MageKing17]

Yeah, there are things it could be used for, but at the moment, FREDers have no way of checking for the flag's presence, and I'm not clear on what practical purpose flagging wings as having never existed serves.

[The-E]

Nor do I, really. My working assumption is that this flag was used at some point for scoring purposes, but I really have no idea (which is why I am in favour of cutting it)

[Goober5000]

In the original public source code release, the flag only appears in three places. Once in the #define, once in parse_wing_create_ships() where it is set, and once in sexp_query_has_yet_to_arrive() where it is checked. The usage of the flag has been unchanged since then.

[MageKing17]

Uh, what about the other time it's set, in redalert.cpp?

uses Git Blame to answer his own question Turns out that was added by our very own Goober5000, in a04e83ab012045207ab6f4e39c4249230a1eb109.

Anyway, regardless of what it could be used for, or what it may have been intended to be used for, it doesn't seem to be serving a practical purpose now, except to make destroyed/departed SEXPs not work on wings that haven't fully arrived before their motherships are destroyed.

[Goober5000]

Hence why I said "in the original public source code release". :)

[The-E]

I think that's a consensus to remove the flag then?

[MageKing17]

Hence why I said "in the original public source code release". :)

You also said, "The usage of the flag has been unchanged since then", which is what I was responding to. ;)

I think that's a consensus to remove the flag then?

I'm still worried about breaking some obscure retail event somewhere; although if any such examples are comparable to the High Noon one in Goober's original comment, I don't think anyone would miss them...

[Goober5000]

Touche. What I meant was that the usage was unchanged in the original code. All that happened since then was adding an additional use of the flag. But I should have said what I meant. :)

The only place sexp_query_has_yet_to_arrive() is used is in the sexps that check the mission log: is-destroyed, is-subsystem-destroyed, has-docked, has-undocked, is-disabled, is-disarmed, has-departed, depart-node, are-waypoints-done. For all of those, returning SEXP_CANT_EVAL for a wing whose mothership is destroyed is the wrong approach. So removing the flag from that function is the right call. And if it's removed from that function, that removes the one and only place it's checked.

I'll make a separate PR for the flag removal. I don't think this should be lumped in with PR #369.

[Goober5000]

I created PR #380. I came to the conclusion that my original plan for adding an if() block in parse_wing_create_ships() was not necessary, as the following calculations will properly handle all of the cleanup for getting rid of the wing, even if no fighters of that wing have arrived. All that was necessary was to remove the obsolete flag.

As for sexp_query_has_yet_to_arrive(), there is no flag hackery needed there, because even though the wing has not yet arrived, the word "yet" does not apply. Since the wingp->total_arrived_count is not zero, the check will fail and SEXP_CANT_EVAL will not be returned.

[niffiwan]

380 is merged, I think we can close this