GUI: Options dialog complains about non-existing saves in autosave slot

[eriktorbjorn]

I disabled autosaving to test something. When I re-enabled it, ScummVM complained about non-autosaves using the autosave slot. In most cases, it was correct in doing so, but not always:

Apparently it mistakes the dummy savegame description generated for Future Wars as an existing savegame?

[macca8]

I’m not sure that this is an issue with the GUI (unless, of course, you’re referring to the text in the dialog), because the dialog is functioning as expected by recognizing an inappropriately named dummy autosave, then reporting it as a non-autosave saved game, which would otherwise block autosaving.

Have you tested Future Wars to confirm that this dummy autosave description also triggers the in-game warning message when attempting an autosave?

The underlying issue here is that this particular dummy autosave description doesn’t conform to the name-testing criteria as defined in hasAutosaveName(), which requires the case-sensitive “Autosave” string, or its translated equivalent, to be present in any valid autosave description, whether dummy or actual. For engines which don’t set their own, this defaults to “Autosave” in both cases.

Furthermore, since the 2.5.1 release, hasAutosaveName() has been updated to allow for partial matching of the “Autosave” string (for example, if trimmed by an engine that imposes a limit on savename sizes). As a result, the “Autosave” string must be positioned at the beginning of any description which includes additional characters in the resulting savename (as is the case here).

This “Empty autosave” description originates from the Cine engine, and can be found in two locations in engines/cine/metaengine.cpp: L151-154 (in listSaves()) & L214-216 (in querySaveMetaInfos()).

In my opinion, the least disruptive solution here is to change this description from “Empty autosave” to “Autosave empty” or similar (or simply “Autosave”, which is the default for most games), which should remove the entry from this dialog, and avoid displaying the in-game warning when autosaving.

Since there’s no evidence at this time that other engines are similarly affected, treating this as a one-off seems logical, so any further action should be unnecessary.

As I’m an end user, please feel free to follow up this suggestion if you consider it worthwhile. Thanks for your time.

[eriktorbjorn]

My problem, which I perhaps did not explain as well as I should have, is that I don't have any savesgames at all for Future Wars. It's complaining about files that do not exist.

If I try to load a savegame from the launcher, it does show "Empty autosave" but of course it can't be loaded. If I try, the game says "This backup doesn't exist ...". (Well, the CD version does. The floppy version draws it in black on a black background, so you don't actually see it.)

[macca8]

Thanks for your response. I understand your concerns are about non-existent saved games (dummy savenames) being listed in this dialog, because in its current form, the dialog doesn't relate to this situation. However, I'm curious as to what you consider is your ideal outcome.

Is it a change in the dialog's messaging?
Should these entries be generally excluded from the dialog, even though their presence indicates a problem with the savename (under current testing criteria)?
Perhaps the name-testing criteria should be adjusted to exclude savenames which don't have an existing saved game (ie. dummy savenames)?

If you believe that non-compliant dummy autosaves are likely to be a significant issue across multiple games (and thus appear in this dialog), then changing the current testing criteria to exclude dummy savenames is probably the way to go.
If not, it's hard to justify any significant changes to current testing criteria, particularly if a simple name change in the affected engine's source code can eliminate the problem without affecting other games.

By the way, did you try starting a new game of Future Wars, then wait for an autosave to confirm if it triggers the in-game warning?
I expect it would, but if not, then the solution to your issue may reside in the testing code within saveAutosaveIfEnabled().

[eriktorbjorn]

Yes, ScummVM will eventually complain that there is an autosave in the way, even though there isn't one:

Cine is the only game engine for which I own games that behaves this way, as far as I know.

[macca8]

Sorry for the delay in getting back to you, but thanks for your feedback! This clarifies the current situation nicely, confirming that it’s a bug with monitoring the dummy autosave.

Fortunately, I may have stumbled onto the optimal solution, which, unlike my suggestion in comment:3, applies globally to any engines with customised dummy autosaves, without needing to make any changes to those engines.

The clue is in PR’s ​3261 & ​3304, which created the regression by removing isAutosave() (i.e. the SaveType-test) from saveAutosaveIfEnabled() with PR 3261, replacing it by prioritising the name-test as the goto test for triggering the new in-game & Options warning dialogs.

The solution is to restore the SaveType-test for each dialog (by calling isAutosave()), except this time it should be called in addition to, and after, the name-test (i.e. after hasAutosaveName()).

The order probably doesn’t matter with the Options dialog because all tests have to be completed, but testing for the in-game dialog need only continue until one of the tests returns true (which requires tests to be completed in a logical order).

Following this change, hasAutosaveName() will continue to test the dummy autosave, but if this test returns false (i.e. if named other than the default "Autosave"), its status will be corrected by isAutosave() returning true (when autosaving is enabled), eliminating it from any possible inclusion in either dialog.

The overall effect of this is that the dummy autosave is ignored when autosaving, regardless of its name, as it should be (since it contains no game data that could be lost if overwritten). It has no adverse effect on user-initiated saves as they always return a SaveType-test result of false, regardless of save slot.

There’s only one change required for each dialog, and the location and proposed change for each are as follows:

in-game warning dialog - engines/engine.cpp/warnBeforeOverwritingAutosave(), change L569:

if (!desc.isValid() || desc.hasAutosaveName() || desc.isAutosave())

Options warning dialog - gui/options.cpp/updateAutosavePeriod(), change L2558:

if (desc.getSaveSlot() != -1 && !desc.getDescription().empty() && !desc.hasAutosaveName() && !desc.isAutosave())

It’s a pretty trivial fix by your standards, but I haven’t been able to test this, so if you agree with what I’ve proposed, and can find the time, please consider implementing it as a viable solution.

Hope this helps, and thanks for clarifying things.

[macca8]

A word of caution.

As it stands, it may be unsafe to reinstate the isAutosave() function as a supplementary test for an existing save in the Autosave slot, without causing an autosave to automatically overwrite any non-autosave saved game which may be occupying the slot.

This is due to a change introduced by PR 3261, which initializes the _saveType value based on the slot type (the Autosave slot or a regular slot - refer engines/savestate.cpp/initSaveType()​60e0a6db), without any regard to the type of save which may be occupying the slot (the primary function of the _saveType property).

Currently, this value is only being used to determine if the Autosave slot is empty (isAutosave() replaces the "slot == autosave slot" test or equivalent), in which case a dummy autosave is created if required. Fortunately, only a minimal number of engines were directly affected by this change (Cine/, SCI/, engines/- metaengine.cpp)

For isAutosave() to be used safely in its proper and intended context, I'd recommend that the _saveType initialization in initSaveType() be reverted (and appropriate adjustments made to the affected changes in commit 60e0a6db), and initSaveType() be given a more appropriate name to reflect its primary function of initializing the writeProtected & Deletable flags.

[macca8]

While the subject of this report targets the GUI, I believe the issue is restricted to the CINE engine.

The issue regarding the invalid _saveType calculation is resolved by ​PR4393, which includes reinstating the setAutosave(true) call that was previously removed from the CINE engine. This properly identifies the dummy autosave as an autosave, which resolves this issue.

However, I don't believe that the dummy autosave should have been tested in the first place, since there's no saved game file attached to the entry. Currently, the CINE engine creates the dummy autosave in both its listSaves() & querySaveMetaInfos() methods. Normal practice is to include it only in the listSaves() method.

IMHO, the correct procedure is to remove the dummy autosave call from its querySaveMetaInfos() method, but someone familiar with the CINE engine needs to confirm that this call hasn't been placed here for some other obscure reason.