Don't exit on losing all lemmings (feature development)

[WillLem]

OK, this fixes the problem. We specifically check for (StateIsUnplayable and a non-Success result) at the point that the replay check stops because of StateIsUnplayable, and return a CR_FAIL result:

Code: [Select]

            if (Game.CurrentIteration > CutoffFrame) or
               Game.IsOutOfTime or Game.StateIsUnplayable then
            begin
              Game.Finish(GM_FIN_TERMINATE);
              if Game.GameResultRec.gSuccess then
                fReplays[i].ReplayResult := CR_PASS;
              if Game.GameResultRec.gGotTalisman then
                fReplays[i].ReplayResult := CR_PASS_TALISMAN;
              if (not Game.GameResultRec.gSuccess) and Game.StateIsUnplayable then  <----- added these lines---
                fReplays[i].ReplayResult := CR_FAIL;                              <-----------------------------
              Break;
            end;

New .exe attached. We'll call this one Exp 13 (1/5/24).

EDIT: Maybe IsOutOfTime should also be factored in, and return a CR_FAIL result?

[WillLem]

OK, even better.

This time, we factor in IsOutOfTime as well.

Code: [Select]

            if (Game.CurrentIteration > CutoffFrame) or
               Game.IsOutOfTime or Game.StateIsUnplayable then
            begin
              Game.Finish(GM_FIN_TERMINATE);
              if Game.GameResultRec.gSuccess then
                fReplays[i].ReplayResult := CR_PASS;
              if Game.GameResultRec.gGotTalisman then
                fReplays[i].ReplayResult := CR_PASS_TALISMAN;
              if (Game.StateIsUnplayable or Game.IsOutOfTime) <------------------------------------
                and not Game.GameResultRec.gSuccess then      <--------changed these lines------
                  fReplays[i].ReplayResult := CR_FAIL;        <------------------------------------
              Break;
            end;

This now only returns CR_UNDETERMINED if the level has an infinite time limit. Otherwise, if the time limit has been reached and the result is non-Success, it returns CR_FAIL. Not sure why it wasn't always like this tbh, there are probably reasons. But, this works nicely.

New .exe attached. We'll call this one The 4pm Version

[Simon]

Thanks. Both packs that I tested yesterday (the NL Introduction pack and Lemmings United) produce identical text outputs during mass-replay-verification in your new nl-4pm as they produced in nl-2024-04-08. In other words, in nl-4pm, the replays are still oneframediff, which is good.

Your code (that you posted with nl-4pm) looks reasonable. I've only looked at your snippet, I haven't looked at the codebase. In those GameResultRec that you get there, does gGotTalisman always imply gSuccess? Then I don't see any problems.

I haven't looked at your test cases yet. The idea is excellent. I wouldn't worry much about the "Error" high-level result.

-- Simon

[WillLem]

does gGotTalisman always imply gSuccess?

As far as I can tell, yes. We first check for Success, then check for a Talisman, wherever it's relevant. So, if we're checking for a Talisman then we have to have reached a Success result.

[Simon]

As far as I can tell, yes. We first check for Success, then check for a Talisman, wherever it's relevant. So, if we're checking for a Talisman then we have to have reached a Success result.

That doesn't answer my worry.

Reason: You're not exiting the monster, you're continuing into tests that care for (not success), but not for (not talisman) and overwrite the result that depended on your test for success:

Code: [Select]

... things that depend on success but don't exit early ...
if (Game.StateIsUnplayable or Game.IsOutOfTime)
    and not Game.GameResultRec.gSuccess then
        fReplays[i].ReplayResult := CR_FAIL;
This is a problem when the supplier of that result struct GameResultRec allows to have (gSuccess false at the same time as g.GotTalisman true). When I wrote "imply", I meant this supplier's side of concerns, not the consuming code that you showed. Your code seems to assume that the supplier always has gSuccess whenever he has g.GotTalisman. I ask you if that assumption is justified.

Your unassuming line would be:

Code: [Select]

and not (Game.GameResultRec.gSuccess or Game.GameResultRec.gGotTalisman) then
I'll have time for code review tonight, Friday 3rd, 17:00 UTC, or anytime on the weekend.

-- Simon

[WillLem]

You're not exiting the monster, you're continuing into tests that care for (not success), but not for (not talisman) and overwrite the result that depended on your test for success:

I see what you mean, but this happens in the very next block:

Code: [Select]

                if Game.GameResultRec.gSuccess then
                begin
                  if Game.GameResultRec.gGotTalisman then

This makes me wonder whether gGotTalisman does indeed imply gSuccess. However, a quick search elsewhere in the codebase doesn't make it clear that these conditions are definitely mutually inclusive.

So, I've committed and pushed your suggested change. You're right, let's not assume anything.

I'll have time for code review tonight, Friday 3rd, 17:00 UTC, or anytime on the weekend.

Weekend is good for me too, shall we aim for 1700 UTC Saturday?

[Simon]

All right, 17:00 UTC on Saturday is it!

-- Simon