fix restore bug

[vafada]

fixes #45

I believe this will fix the restore bug in LSL and PQ

Fixes involves

1. Do not exit the LOGIC evaluating loop when restoring game as discussed here:

https://github.com/lanceewing/agile/issues/45#issuecomment-1304645654

2. the restore.game command should also return 0 so LOGIC 0 can be evaluated in the beginning (this fixes the PQ1 bug) where the restore game logic is at the beginning of LOGIC 0

3. Matches also the logic in the while-loop and evaluating new room to match the original code. In MAIN.C:

while (_CallLogic(0) == NULL) {
            var[OBJHIT] = var[OBJEDGE] = var[UNKNOWN_WORD] = 0;
            Reset(INPUT);
            previousScore = var[SCORE];
            }

This also fixes the PQ1 bug because in the current code this evaluates to true (state.Vars[Defines.SCORE] != previousScore)

// Update the status line, if the score or sound status have changed.
                if ((state.Vars[Defines.SCORE] != previousScore) || (soundStatus != state.Flags[Defines.SOUNDON]))
                {
                    // If the SOUND ON flag is off, then immediately stop any currently playing sound.
                    if (!state.Flags[Defines.SOUNDON]) soundPlayer.StopSound();

                    textGraphics.UpdateStatusLine();
                }

and overrides the status line and remove the map coordinate

[vafada]

not sure how to translate

while (_CallLogic(0) == NULL) {
            var[OBJHIT] = var[OBJEDGE] = var[UNKNOWN_WORD] = 0;
            Reset(INPUT);
            previousScore = var[SCORE];
            }

[lanceewing]

Let me try to work it out...

[lanceewing]

I think when I came up with that line of code:

    while (NewRoom(commands.ExecuteLogic(0))) ;

I was trying to do something clever that is now demonstrating its inflexibility. It does seem like we'll need to expand this similar to how you've done, but will post back here with my full thoughts later on.

[lanceewing]

I think the underlying problem in AGILE is that NewRoom is executed outside of ExecuteLogic method, and that was a poor design on my part. Hmmm, maybe I inherited that thinking from my earlier MEKA project, but I think it is clear looking at the original AGI interpreter source, that the code I have in NewRoom should be executed immediately when the new.room or new.room.v commands are executed.

In NEWROOM.C, we can see the following:

  STRPTR
  LNewRoom(lp)
  STRPTR    lp;
  {
      return (NewRoom(*lp));
  }

  STRPTR
  LNewRoomF(lp)
  STRPTR    lp;
  {
      return (NewRoom(var[*lp]));
  }

which I am fairly sure indicates that NewRoom was executed by the logic command execution code, i.e. within the _CallLogic call.

So my proposed change is that we change ExecuteAction to execute NewRoom immediately (a slightly modified version of NewRoom). That would mean that there is no need to return the room number back, which means we can use the return value for something else, e.g. a boolean indicating logic rescan.

I might give this a quick go now on a branch to show what I mean.

[lanceewing]

I also noticed this line in the restore game code:

    newRoom = state.CurrentRoom = state.Vars[Defines.CURROOM];

which may have confused things, as newRoom was being used to track whether to run the new room code. Not sure what it would have done for a restore game. The original AGI interpreter code doesn't appear to have run NewRoom on a restore.

[lanceewing]

I'm going to try something that I'm not 100% sure will work for all scenarios, but it is in an attempt to make the code a little cleaner.

So my interpretation is going to be that if our ExecuteAction method returns 0, it means a rescan of Logic.0, by exiting out to the outer while loop, and that if ExecuteAction returns a -1, then it has reached the end of the Logic, e.g. when it hits command 0, which is the AGI return command. It's not exactly what the original interpreter does, which returns the scan offset that it was up to when it gets to the end, but that doesn't appear to have been used for anything. Assuming that is true, then it means we won't need the "exit" out var, as we can get away with just the return value from ExecuteAction.

(if it doesn't work out, then we'll have to reintroduce the exit var)

[lanceewing]

I've added PR #49 to show the refactor I was proposing above. It did a very quick test, and I think it hasn't broken anything obvious. Would be interesting to see if it fixes your restore issue. I haven't yet been able to test that.

[vafada]

closing in favor of https://github.com/lanceewing/agile/pull/49