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

[WillLem]

12.13 Exp (Build 4098 | Commit 00a392b)

[Simon]

A few bugs in the options management, and I shed light on the lingering mismatching config (where we see X in the config menu but the game does Y). We'll start with the easiest.


Wrong default out of box.

1. Freshly extract Exp 6 from your full release.
2. Run NL.
3. Play a level (e.g., the included Mazulems level 1).
4. Fast-forward into death.

Expected: We get the panel message to rewind or quit.
Observed: We exit to postview.

IIRC, we agreed that the default should be to exit if we have won, and show the panel message when we lose. This is the most appropriate default. We had newbies (Darkshoxx in one of his streams) shout in surprise when the level exited on losing.


File contains three options, not one

1. Freshly extract Exp 6 from your full release.
2. Run NL.
3. Open the options menu (click on the cog in the main menu).
4. Press OK to save options.
5. Exit NL.
6. Open in a text editor: ./settings/settings.ini

Expected: One of the lines is for the end-of-level behavior.
Observed: Three of the lines are for the end-of-level behavior, e.g.:

AlwaysEndGameplay=1
EndIfLevelPassed=0
NeverEndGameplay=0

This invites to put nonsensical values in the file, e.g., set them all to 0, or set several of them to 1.

I recommend to store one line, e.g., with values 0, 1, 2. Call it, e.g., ExitToPostviewBehavior, or, if you'd like 0 to mean always-exit and want to keep that nuance in the text file, call it, e.g., StayInGameAfterLemmingsDied, although that's contrived.

Certainly, the user can still put nonsense values even when we use only a single line. E.g., he can text-edit the line to put negative values, or 3 or greater. It's probably okay to do whatever you want then, as long as it means the same as exactly one out of 0, 1, or 2. Maybe fix it under the hood, e.g., convert it to 0, 1, or 3 at options-loading time?

In particular, avoid the following mismatching bug for such nonsense inputs. It's the uncanny bug from months ago:


Mismatch from more than one 1

1. Produce a settings file with the Exp 6. (E.g., do all steps (1-6) from previous bug: File contains three option lines for the end-of-level behavior.)
2. Text-edit the file: Make all three lines end in 1 (none in 0).
3. Run NL.
4. Play a level and lose, e.g., kill all lemmings.
5. You automatically exit to postview.
6. Return to the NL main menu.
7. Open the options menu and go to the interface tab.

Expected: "Always Exit to Postview" is selected, matching our step 5.
Observed: "Never Exit to Postview" is selected.

Certainly, the user had to do something nonsensical to get here, and you can probably treat the garbage as what you like. But be consistent across game and options dialog.

-- Simon

[Simon]

In Exp 6 the following are all oneframediff (= the good, expected result):

• All replays for Lemmings United non-bonus ranks,
  
• all replays for NeoLemmix Introduction Pack,
  
• all replays for PimoLems except for the a single replay for Straight Forward (in the rank called "One", level 7).
  
• the 3 of your 5 replays from reply #102 ("Replay Check") that passed or won talismans.
  

All of these replays (including Straight Forward) test identical (down to the frame) in Exp 6 as they tested in 4 pm. These two NL versions generate byte-for-byte identical verification summary files.

Your undetermined replay from reply #102 is still undetermined, with the same number of physics updates, not oneframediff. That's probably fine? Your level-not-found replay from reply #102 still finds no level, good.

The only eyebrow-raiser is Straight Forward. This is a one-minute level in PimoLems. The replay is attached: This replay wins, then has only one blocker left standing that is never nuked, then runs over the time limit. WillLem, do you think that it's plausible that the NL replay verification takes the same number of physics updates, 1021 updates, in both NL 12.12.5 and Exp 6? We have:

1021 physics updates / 17 physics updates per second
= 60.0588 seconds
= 1 minute + 1/17 seconds
= 1 minute + 1 extra physics update

This sounds reasonable for a one-minute level. I think there is no need to worry here, especially since your undetermined replay from #102 also ran for identical physics update numbers in 12.12.5 and in Exp 6.

-- Simon

Edit 23:10 UTC: Added results for the 5 replays from WillLem's reply #102, a.k.a., "Replay Check" replays.

[WillLem]

Wrong default out of box.

...

we agreed that the default should be to exit if we have won, and show the panel message when we lose. This is the most appropriate default.

Agreed, "End if Level Passed" should be the default - this has now been fixed, committed and pushed. I think I set it to something else during testing and forgot to switch it back. Good catch.

File contains three options, not one

...

Expected: One of the lines is for the end-of-level behavior.
Observed: Three of the lines are for the end-of-level behavior

...

I recommend to store one line

Yes, very good shout.

The option is now stored as "ExitToPostviewOption" with 3 possible strings, one for each option ['Always', 'IfLevelPassed', 'Never']. If the string in the config file is one of these 3, the correct option is loaded. If it's is anything other that these 3 (so, blank or nonsensical), we simply load the default. I believe this should also address the third of the concerns in Reply #113. Committed and pushed.

EDIT - Also, if the user writes multiple lines into the text file for the same option, existing behaviour is that only the first one is loaded & handled, and the rest are discarded on saving. Perfectly elegant, does the job.

Here's V6-A which implements these fixes (to Commit a2610dc). Meanwhile, it sounds as if the MRC is behaving as we'd like (apart from the bug you reported here, which I'll probably fix myself in SLX and can always port across to NL at a later date).

I'm thinking we're ready for the PR now. Do let me know if there's any other last-minute tweaks you'd like to get sorted before we next review.

[Simon]

"End if Level Passed" should be the default

3 possible strings, one for each option ['Always', 'IfLevelPassed', 'Never'].
blank or nonsensical), we simply load the default.

multiple lines [...] only the first one is loaded & handled

Yes, all of this sounds great. Thanks!

MRC is behaving as we'd like (apart from the bug you reported here

I agree, it all feels correct.

I agree, we shouldn't worry about that bug (= talismans are not reported if we verify the replays under another username) because it sounds unrelated to our exit-to-postview work. I've already worked around it by changing my username.

The option is now stored as "ExitToPostviewOption"

With text strings as values (they are better than my earlier-suggested 0, 1, 2), I'd now name the option in the text file "ExitToPostview" (without the redundant "Option" at its end; everything in the file is an option). This is not a big worry.

Here's V6-A which implements these fixes (to Commit a2610dc).
ready for the PR now. Do let me know if there's any other last-minute tweaks

I can spare 2-3 hours tonight for testing; I'll post findings before UTC midnight.

After that, I'll be busy the next 4 days.

-- Simon

[Simon]

You still have 3 internal variables, which can be a similar source of bugs as the 3 exposed options from the text file.

For example, here:

Code: [Select]

if not MatchStr(sOption, sPossibleOptions) then
    EndifLevelPassed := True
else begin
    AlwaysEndGameplay := (sOption = 'Always');
    EndIfLevelPassed := (sOption = 'IfLevelPassed');
    NeverEndGameplay := (sOption = 'Never');
end;
... you risk a bug whenever this runs while ((AlwaysEndGameplay is true) or (NeverEndGameplay is true)) and you run into the 'if'. I don't know if the code ever runs in that case. I know that it parses options during startup. But if it does run at another time, you'd again violate the invariant that exactly one of the three is true. This is a maintenance trap: The option-loading code now has a silent requirement that it may only be called once.

One way to avoid that worry is to remove the if/else entirely and write:

Code: [Select]

AlwaysEndGameplay := (sOption = 'Always');
NeverEndGameplay := (sOption = 'Never');
EndIfLevelPassed := not AlwaysEndGameplay and not NeverEndGameplay;

But even better: Track the option internally with one variable. Then it's absolutely impossible to have more than one of them true, it'll be obvious anywhere in the program and to any maintainer. Consider an enumeration; Delphi 6 should support them:

Code: [Select]

type TExitToPostview = (
    etpAlways,
    etpIfPassed,
    etpNever
);
var ExitToPostview : TExitToPostview;

Code: [Select]

if sOption = 'Always' then
    ExitToPostview := etpAlways;
else if sOption = 'Never' then
    ExitToPostview := etpNever;
else
    ExitToPostview := etpIfPassed;

I'll sit in Mumble for the night from 19:00 UTC to around 22:00 UTC. Catch me in Mumble spontaneously; otherwise, we'll review next week.

-- Simon

[Simon]

I tested the Exp 6-A. Everything looks good:

• Default option out-of-box is exit if save requirement met.
  
• All three choices of the option behave during play as they should.
  
• Text file contains one line only, which loads back as it should.
  
• Mass replay verification produces byte-for-byte-identical files like the Exp 6, at least for Lemmings United and PimoLems. Haven't re-tested the others yet with the Exp 6-A.
  

In the end, it's up to you how much of today's feedback (my 2 earlier posts today) you still want to implement.

I can't estimate, e.g., how hard it is in the NL source (mostly in the code for the options dialog) to turn the 3 bools into an enum. Maybe it's sensible to address only the worry ("One way to avoid that worry is to remove the if/else entirely and write:").

If you touch the source at all before making the PR, then I'd rename the text-file option from "ExitToPostviewOption" to "ExitToPostview". It's easy now to rename it (there are 4 occurrences in the code), it'll be harder after release when people have the old string in their files.

Remember to rebase/to add the recent commits to the ExitToPostview upstream branch when you make the PR. They're still sitting on top of the Graphics32 update, but that was fine for me today to review.

Happy to schedule the evening on Monday, 13th for a review session in Mumble. Anything from Monday, 16:00 UTC through Monday, 22:00 UTC I can make.

-- Simon

[WillLem]

I'd now name the option in the text file "ExitToPostview" (without the redundant "Option" at its end; everything in the file is an option)

Agreed, this is now done. Committed and pushed.

You still have 3 internal variables, which can be a similar source of bugs as the 3 exposed options from the text file.
...
Track the option internally with one variable ... Consider an enumeration

I tested the Exp 6-A. Everything looks good:
...
In the end, it's up to you how much of today's feedback ... you still want to implement.

This might be a case of "if it ain't broke, don't fix it!" The options all play nicely now, and I've tested thoroughly for nonsensical values, setting all three to true, etc. The codebase already has it built-in to ignore all but the first declaration of the option in settings; this should be sufficient for preventing the option from being set to two things at once.

And OK, there's some risk that it might be set elsewhere during gameplay, but tbh I can't see how that would happen. If the user changes the options between levels, for example, it's saved correctly. If they change the option via the text Editor, they must then re-load NL for it to acknowledge the option change (or at least keep using NL until the options are re-loaded, at which point it will be correctly set). AFAIK, default options are only ever explicitly set upon loading a brand new copy of NL, and never again after that.

I understand your desire for the singular option, but there are benefits to keeping them separate (and, identified by strings rather than numbers). My instinct on this one is, let's keep it as it is at least for now.

That's not to rush things, it's genuinely what I think is best.

Monday is fine for another Mumble sesh. I'll need help doing the PR anyway, and we can go over anything else that might need to be looked at first. Speak to you then if not before, W.