Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAXSAVEGAMES 50 #816

Closed
rofl0r opened this issue Jun 5, 2019 · 20 comments · Fixed by #2541
Closed

MAXSAVEGAMES 50 #816

rofl0r opened this issue Jun 5, 2019 · 20 comments · Fixed by #2541
Labels
context: game logic res: in consideration type: enhancement a suggestion or necessity to have something improved

Comments

@rofl0r
Copy link
Contributor

rofl0r commented Jun 5, 2019

with quest for infamy, i'm constantly hitting this limit. this is pretty annoying, i constantly have to delete old savegames.

the gui part even seems to be unable to display more than the latest 20 savegames (macro gets redefined). can't really figure out a good reason why it shouldn't be possible to display all savegames in the listbox.

@ivan-mogilko ivan-mogilko added the ags3 related to ags3 (version with full backward compatibility) label Jun 5, 2019
@ivan-mogilko ivan-mogilko added this to the 3.5.0 milestone Jun 5, 2019
@ghost
Copy link

ghost commented Jun 5, 2019

This is one of those things that continiously get postponed because they are "less important". (This limit only persists if developer used built-in functions to fill the save list)

It should be trivial to remove this limit.

IMPORTANT UPDATE: not so trivial, please read this:
#816 (comment)

@ivan-mogilko ivan-mogilko self-assigned this Jun 5, 2019
@ghost
Copy link

ghost commented Jun 5, 2019

It should be trivial to remove this limit.

Hmm... ok, not so trivial, because of this: https://github.com/adventuregamestudio/ags/blob/release-3.5.0/Engine/ac/game.cpp#L2523

@sonneveld
Copy link
Member

Is it possible for the game to determine if there's enough space for a new save game? Or does it have to rely on an error message occurring on save?

@ghost
Copy link

ghost commented Jun 5, 2019

Is it possible for the game to determine if there's enough space for a new save game? Or does it have to rely on an error message occurring on save?

Do you mean a disk space, an available save slot, or available element in the internal array limited by MAXSAVEGAMES?

@sonneveld
Copy link
Member

sonneveld commented Jun 5, 2019

Do you mean a disk space, an available save slot, or available element in the internal array limited by MAXSAVEGAMES?

I guess all of the above. A IsSpaceAvailableForSave() or something. Or a TrySaveGameSlot() that returns an error message on failure. A lot of games I've played will warn you before saving that you need to delete games (even if it's only because they've reached the game limit), so a pre-check would be consistent with that.

I know some games already check if the save game listbox (after calling FillSaveGameList) has 50 items so they can show their own warning message. In fact it looks like it's recommended in the manual.

With available diskspace, I realise it's difficult to predict because save games will increase in size over time (e.g. 2meg initial room, 6 meg last room)

@sonneveld
Copy link
Member

sonneveld commented Jun 5, 2019

Also, some games use custom save slot numbers (like 101, or maybe even 21) for auto save. They can pick these numbers because the engine dialog only reserves slots 1-20.

@ghost
Copy link

ghost commented Jun 5, 2019

  • MAXSAVEGAMES is an arbitrary limit only used in default API functions which display list of saves. Probably intended to emulate old Sierra games.
  • AGS itself technically supports save slots from 0 to 999, with 999 usually reserved for automatic checkpoint.
  • I don't think it tests for available disk space before trying to save.

Also, some games use custom save slot numbers (like 101, or maybe even 21) for auto save. They can pick these numbers because the engine dialog only reserves slots 1-20.

Yes, I did not consider that at first. This could be a real problem.
Removing the list limit itself is not that hard, but this has to be somehow consistent with the script API.
Game developers must be able to limit what is shown in these automatic dialogs.

Also, there's no way to predict game expectations about reserved slots without practical experiment.

On a quick thought, what may be possible to do for now is:

  • remove array limit in code
  • enforce the limit of 20/50 saves by default
  • add a config option to override it (there's an "override" section in config for similar purposes).

Above would allow to have more saves in legacy games. This config option has to be annotated with a warning that this is not compatible with reserved save slots and should be used with care.

This is as much as I can suggest right now. But guess ideal solution is to revise savegame API by e.g. allowing developers to tell the range of slots shown in the listbox. Or introduce new slot system which has a separation between regular slots and special reserved slots.

@ghost
Copy link

ghost commented Jun 5, 2019

PS. Actually, the obvious workaround (which I used myself in some games, even in old Sierra games like QFG) is to move older saves to subdirectories and copy them back when necessary.

@ivan-mogilko ivan-mogilko removed this from the 3.5.0 milestone Jun 5, 2019
@sonneveld
Copy link
Member

  • add a config option to override it (there's an "override" section in config for similar purposes).

Instead of a config option, we could search scripts to see if they use SaveGameSlot or FillSaveGameList. If not used, then we could allow more saves in the save game dialog.

I don't think it tests for available disk space before trying to save.

Currently we check if at least 2 meg is available. Which isn't quite enough for larger games. Refer to https://github.com/adventuregamestudio/ags/blob/master/Engine/ac/game.cpp#L1096

@sonneveld
Copy link
Member

PS. It's on my list to investigate using zlib to compress the games, so instead of saves being 2-8 megs, they're only 64k to 700k.

@ghost
Copy link

ghost commented Jun 5, 2019

How does the save size and disk space is relevant to this particular request?

Instead of a config option, we could search scripts to see if they use SaveGameSlot or FillSaveGameList. If not used, then we could allow more saves in the save game dialog.

This would require to run through all the script modules and all the room scripts?
Also plugins may call these script functions too.

@sonneveld
Copy link
Member

How does the save size and disk space is relevant to this particular request?

It's another variable that can determine the max save game limit. Sorry, wasn't trying to derail discussion.

This would require to run through all the script modules and all the room scripts?
Also plugins may call these script functions too.

Ah I didn't consider plugins. I guess if plugins exist, you can't expand either. But I thought scripts only had the function imports they used? At least from the scripts I've looked at. It's how I was able to determine which functions were used over time.

@ivan-mogilko ivan-mogilko removed their assignment Jun 5, 2019
@ghost
Copy link

ghost commented Jun 5, 2019

we could search scripts to see if they use SaveGameSlot or FillSaveGameList. If not used, then we could allow more saves

I'm afraid this kind of detection also does not have much sense, because it is FillSaveGameList that has 50 slots limit, the one that is suggested for removal. Such detection would only affect SaveGameDialog() with 20 slots which is not used often in recent games.

@rofl0r

This comment was marked as abuse.

@morganwillcock
Copy link
Member

Some platforms don't have valid free space checks.
https://github.com/adventuregamestudio/ags/blob/ags3/Engine/platform/linux/acpllnx.cpp#L141-L144

@rofl0r

This comment was marked as abuse.

@morganwillcock
Copy link
Member

right. the only valid way to check for this is to simply try to write, and if it fails, one can look at errno to determine the cause, if one really wants to.

Yes I agree, because otherwise you also have to accomodate differences in how different filesystem report freespace as well as how you would establish that the location is writeable.

@ghost
Copy link

ghost commented Jun 5, 2019

So, to sum up, there are following relevant problems:

Legacy API.

  1. There's an internal array (50 slots) that stores "file indexes", a result of of the last call to FillSaveGameList script function. This array is a part of legacy API, it is exported to scripts as "savegameindex" global array. Array was used along with functions like ListBoxSaveGameList and contained save slot indexes corresponding to save names in list box. (This information may be found in old manuals)
    This array may be either replaced with std::vector or simply expanded to 1000 slots because right now AGS has 1000 save slots max (0 - 999). If it's turned into a vector, then it's manager should also be replaced into a new variant of StaticArray class which translates legacy memory access (won't go into details much here, but examples may be found in code).
    Of course if old games impose same limit in their own script, they won't be able to use more slots unless you hack these scripts.

  2. There's storage limit (20 slots) for built-in Save/Load dialogs called with SaveGameDialog() and RestoreGameDialog() functions, but this storage seem to be completely disconnected from anything else, and therefore may be easily changed to dynamic array or std::vector, hence allowing any number of save slots.


Contemporary API

  1. There's arbitrary limit (50 slots) when filling list box using ListBox.FillSaveGameList, but because ListBox itself is no longer limited by number of items it may contain that slot restriction may be trivially removed.

Logical problem

  1. The problems above are all technical, but even if they are solved there's still a logical problem. As @sonneveld noted earlier, some games may take this old 20/50 slots limit into account and use any above slots for special purposes: such as automatic checkpoints or even storing "game levels" as saves (I've seen this couple of times before).

This leads to two questions:

  • how to deal with this in new games? Apparently script API has to be adjusted to either allow to limit number of saves filled with the call to ListBox.FillSaveGameList, or changed further and separate "regular" slots and special slots which do not mix. Or maybe save slot system may be revamped into something different in the future versions (but that's an issue for another day).
  • how to deal with this when running old games, if you want engine to override their saves limit. What I suggested earlier is a config option that would tell engine to just ignore this constraint. Of course in particular games this may lead to unconventionally used slots also getting in the list, but I don't think this can be helped, player should just be aware and careful.

@sonneveld
Copy link
Member

sonneveld commented Jun 5, 2019

that would be prone to race conditions e.g. some other process writes to disk and when you call IsSpaceAvailableForSave() it would return true,

I agree there's an issue with race conditions, but that's assuming there are other processes writing to disk :) edit: I suppose for games on those platforms you can just set an arbitrary limit inside the game itself

@ivan-mogilko ivan-mogilko added context: game logic type: enhancement a suggestion or necessity to have something improved and removed ags3 related to ags3 (version with full backward compatibility) labels Oct 24, 2019
@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Oct 7, 2024

Fixed by #2541.

Among other things added a new config setting:

[override]
max_save=N

where N will be the topmost save used in standard save/restore dialogs.
This may be used when running existing games which use these standard dialogs.

In case this problem appears in another game which has its own custom save menu, there's a emergency solution, added some time earlier: save_game_key and restore_game_key settings as explained here:
https://github.com/adventuregamestudio/ags/blob/master/OPTIONS.md
these should bring up builtin save/restore dialogs with unlimited (or rather limited by 999) save capacity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: game logic res: in consideration type: enhancement a suggestion or necessity to have something improved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants