Error in the Default template's save system

Started by Crimson Wizard, Fri 15/01/2016 23:49:23

Previous topic - Next topic

Crimson Wizard

My attention was brought to this when I was testing someones game and found bizzare savesystem behavior. I cannot remember if I've seen this before, but assuming a lot of people around use Default template for their games, I believe this situation relates to many games, just had not been noticed or covered yet (?).

Okay, so here is how Default template saves a game when user already typed saves name and clicks on Save button:
Code: ags

////////////////////////////////////////////////////////////////
  int gameSlotToSaveInto = lstSaveGamesList.ItemCount + 1;
  int i = 0;
  while (i < lstSaveGamesList.ItemCount)
  {
    if (lstSaveGamesList.Items[i] == txtNewSaveName.Text)
    {
      gameSlotToSaveInto = lstSaveGamesList.SaveGameSlots[i];
    }
    i++;
  }
  SaveGameSlot(gameSlotToSaveInto, txtNewSaveName.Text);
////////////////////////////////////////////////////////////////

(I removed some lines that are not important.)

What we have here is an algorithm of finding a slot to save the game into. How does default template do that?
It searches through all saves it found in the directory, until the name of the save matches with what user typed in the text box, or until it hits the end.
In the first case it saves game in the slot number, equal to the slot number with matching name, and in the second case it saves to the slot number equals to the total number of files in the list + 1.

Now, consider two test cases.

Case 1: we have several saves with properly ordered slot numbers:
Quote
agssave.001 -- named "Save 1"
agssave.002 -- named "Save 2"
agssave.003 -- named "Save 3"
agssave.004 -- named "Save 4"

User types in "Save 3" and game finds it in the slot 3. The game gets properly written into agssave.003 again.
User types in "New Save", games cannot find such slot and assigns slot number to the number of saves + 1, that is 5. The new game gets properly written into agssave.005.

Case 2: we have some save numbers missing in the beginning. How did this happen? Well, for instance, we were copying saves to other computer and missed some. Or maybe person uploaded single save to demonstrate a bug in the game, or something like this.
Quote
agssave.003 -- named "Save 3"
agssave.004 -- named "Save 4"

User types in "Save 3" and game finds it in the slot 3. The game gets properly written into agssave.003 again.
User types in "New Save", game cannot find such slot and assigns slot number to the number of saves + 1, that is... 3.
And the game is written to the existing file named agssave.003, overwriting game which we did not want to overwrite.
This also means, that player will now have a very low limit of save slots.

Khris

Regarding case 2, I can confirm the bug of overwriting an existing slot with a new name.

It does properly overwrite existing saves though, since it uses lstSaveGamesList.SaveGameSlots[i], not i as save slot.

Crimson Wizard

#2
Quote from: Khris on Sat 16/01/2016 13:45:48
It does properly overwrite existing saves though, since it uses lstSaveGamesList.SaveGameSlots[i], not i as save slot.
Right, I incorrectly diagnosed a cause of some other effect. I will fix the post.

Crimson Wizard

#3
What about changing the script to following:

Code: ags

int find_save_slot(String name)
{
  bool slots[999];
  int i = 0;
  while (i < lstSaveGamesList.ItemCount)
  {
    if (lstSaveGamesList.Items[i] == txtNewSaveName.Text)
    {
      // found existing save with matching name
      return lstSaveGamesList.SaveGameSlots[i];
    }
    // remember which slots are already taken
    slots[lstSaveGamesList.SaveGameSlots[i]] = true;
    i++;
  }

  // Find first free save slot, starting with slot 1
  i = 1;
  while (i < 999)
  {
    if (!slots[i])
      return i;
    i++;
  }
  // no free slots found
  return -1;
}

function btnSaveGame_OnClick(GUIControl *control, MouseButton button)
{
  int gameSlotToSaveInto = find_save_slot(txtNewSaveName.Text);
  if (gameSlotToSaveInto < 0)
  {
    Display("No more free save slots!");
    return;
  }
  SaveGameSlot(gameSlotToSaveInto, txtNewSaveName.Text);
  close_save_game_dialog();
}

Gurok

#4
I would just obtain the maximum gameSlotToSaveInto inside the existing loop (untested).

Code: ags

////////////////////////////////////////////////////////////////
  int gameSlotToSaveInto = lstSaveGamesList.ItemCount + 1;
  int i = 0;
  while (i < lstSaveGamesList.ItemCount)
  {
    if (lstSaveGamesList.Items[i] == txtNewSaveName.Text)
    {
      gameSlotToSaveInto = lstSaveGamesList.SaveGameSlots[i];
      i = lstSaveGamesList.ItemCount; // break;
    }
    else
    {
      if(lstSaveGamesList.SaveGameSlots[i] >= gameSlotToSaveInto)
      {
        gameSlotToSaveInto = lstSaveGamesList.SaveGameSlots[i] + 1;
      }
      i++;
    }
  }
  SaveGameSlot(gameSlotToSaveInto, txtNewSaveName.Text);
////////////////////////////////////////////////////////////////
[img]http://7d4iqnx.gif;rWRLUuw.gi

Crimson Wizard

#5
Quote from: Gurok on Fri 22/01/2016 00:36:11
I would just obtain the maximum gameSlotToSaveInto inside the existing loop (untested).
I had that idea first, but what if there is only agssave998.sav lying in folder? No more saves.

BTW, thinking about it, my script is faulty, I will fix it.

Gurok

Quote from: Crimson Wizard on Fri 22/01/2016 01:01:30
Quote from: Gurok on Fri 22/01/2016 00:36:11
I would just obtain the maximum gameSlotToSaveInto inside the existing loop (untested).
I had that idea first, but what if there is only agssave998.sav lying in folder? No more saves.

BTW, thinking about it, my script is faulty, I will fix it.

Ah yes, fair enough. It does have shortcomings. Best to stick with the array then.
[img]http://7d4iqnx.gif;rWRLUuw.gi

Crimson Wizard

#7
Ugm... this was rather lucky that 999 bools accurately fit into the memory that AGS Script's allocates for arrays on stack. Which means that if this function (find_save_slot) is called from another function which has some more arrays allocated locally, game will stop with "stack overflow" error.

The array should probably be dynamic, or global. Latter will be slightly faster (it is not easy to predict this when dealing with AGS script interpreter) but have 4 kb of memory constantly allocated. ...I guess either is not a big deal anyway.

SMF spam blocked by CleanTalk