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

ENGINES: Standardization of autosaves #2050

Merged
merged 28 commits into from Feb 16, 2020
Merged

Conversation

@dreammaster
Copy link
Member

dreammaster commented Feb 8, 2020

This pull request is intended to help deal with the fact that the bulk of engines don't implement autosaves, by making it part of the standard base Engine class. There are several provisios, which I'll discuss below:

  • First, and foremost, the new autosave functionality relies on an engine implementing savegames via the saveGameState method. This means that even with this, engines like DreamWeb and Tinsel which don't allow saving via the GMM won't benefit from this.

  • As a requirement for autosaves to work with the existing engines, I had to standardize saveGameState to always have the extra isAutosave parameter, whereas before in my previous pull request for extended save improvements, there was a separate overloaded version of saveGameState to the existing one. This will slightly affect any existing engines that implement saveGameState and aren't yet merged into master, such as the Blazing Dragons engine. Luckily it'll be easy to simply add in the extra needed parameter to the method, even if unused.

  • At it's core, the changes work by adding a new "handleAutosave" method to the base engine class, which the DefaultEventManager's pollEvent method now calls. This seemed the best place, and the method was already accessing the engine via g_engine anyway (for EVENT_MAINMENU handling), so it fits in well. Now, as engines regularly call pollEvent, when the autosave point is reached, it will trigger an autosave, so long as the engine's canSaveGameStateCurrently method returns true.

  • To avoid clashes, I removed existing autosave code from the engines, with the exception of scumm.. it's autosaving handling seems somewhat more complicated, so to be on the safe side I left it intact, and instead flag to the Engine that it's autosaves should be turned off. I've added a new method canSaveAutosaveCurrently, which by default simply calls canSaveGameStateCurrently.

  • I also added a method getAutosaveSlot for engines to specify which slot is for autosaves. One engine had it in slot 99, and another in slot 999. Griffon, for example, I put the autosave in slot 4, since the engine (based on the in-game save dialog) was previously limited to 4 slots (0-3). Most of the existing engines already supporting autosaves, however, use slot 0, which is the default.

  • Some of the engines, already with autosaves in mind, map in-game saves to start at slot 1 so there's no chance of overlap. But since some don't, I've added some logic to mark slot 0 as write protected.. which some engines were also doing already. I also did a minor change to the save load chooser to call a new overloaded listSaves method in the Meta Engine that takes in a 'saveMode' parameter. When in save mode, if autosaves are turned on and there isn't yet a save in the autosave slot, then it will show a dummy 'Autosave' entry that is marked as read only (slots can't be marked as write protected without having an entry for that slot). This won't show in Load mode to avoid confusion. However, obviously, there may still be an outstanding problem if an in-game/origional save dialog puts the first save as slot 0.. potentially they could end up saving a game quickly that later gets overwritten by autosaves.

For the above, one thing I'd like to do is use the querySaveMetaInfos method of the meta engine to generically get the basic information of any existing save, and only save the autosave if the slot is empty, or the description of an existing save matches "Autosave". This would prevent existing saves accidentally getting overwritten. However, I'm not sure how to cleanly get a reference to the meta engine in the engine's saveAutosaveIfEnabled method. The SaveLoadChooser has a lot of code for finding plugins based on engine ids, and getting the meta engine from there.. I'm wondering if there's any existing methods accessible to the engine that simplifies/abstracts the process.

For that matter, I currently have a duplication of getAutosaveSlot in both the engine and meta engine base classes. If there's an easy standardized way for an engine to access the meta engine that launched it, I could do away with the duplication.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 8, 2020

Excellent work! I really like the revised autosave logic. And the general cleanup of the save code is great, too!

if (saveError.getCode() != Common::kNoError)
warning("Attempt to autosave has failed.");
bool MohawkEngine_Riven::canSaveAutosaveCurrently() {
return canSaveGameStateCurrently() && !_gameEnded;

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

_saveLoad->isAutoSaveAllowed() is no longer called?

The thing is in Myst and Riven, we check whether the save in the autosave slot can be overwritten by the autosave (maybe the user somehow put their own save in there). To do so, we open the save and check if it has the autosave flag set.
Reading the file is a somewhat costly operation, and we were careful to do it at most once per autosave interval. Now, in the worst case, it is done every frame and for each event in the queue. So maybe canSaveAutosaveCurrently should allow to tell the autosave system "no, autosaving is not possible currently, and don't try again immediately".

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 9, 2020

Author Member

Actually, that's not the case. The way I set things up in Engine is that it waits for the autosave interval to expire, and only then calls saveAutosaveIfEnabled. It's there that I check canAutosaveCurrently, and even if it returns false, I still reset the autosave interval.

But in retrospect, it does have the downside that if autosaves were, say, set for half a hour, if you just happen to have a dialog or cutscene open on the exact minute the autosave check triggers, it'd wait another half hour before it tried saving again. What I think I'll do is have that if it can't do the autosave, it'll reset the next interval to try in 5 minutes time, irrespective of what the normal autosave interval is.

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

Oh, right, I mixed things up with the previous behavior, sorry.

That call to _saveLoad->isAutoSaveAllowed() still needs to be restored though.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 10, 2020

Author Member

Sorry I forgot it. Fixed.


void Engine::saveAutosaveIfEnabled() {
if (_autosaveInterval != 0 && canSaveAutosaveCurrently())
saveGameState(getAutosaveSlot(), _("Autosave"), true);

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

Should we warn the player if autosaving failed? With an OSD message perhaps? At least there should be a warning message in the log/console.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 10, 2020

Author Member

Thanks for reminding me. I'd originally decided against showing a message when all I knew was the error message call, but with the OSD display, it'll give the player a non-blocking informative display.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Feb 9, 2020

The SaveLoadChooser has a lot of code for finding plugins based on engine ids, and getting the meta engine from there.. I'm wondering if there's any existing methods accessible to the engine that simplifies/abstracts the process.

I don't think there is a method to retrieve the MetaEngine from the engine. We can add one in the Engine class:

MetaEngine &Engine::getMetaEngine() const {
	const Plugin *plugin = EngineMan.findPlugin(ConfMan.get("engineid"));
	assert(plugin);
	return plugin->get<MetaEngine>();
}
@dreammaster

This comment has been minimized.

Copy link
Member Author

dreammaster commented Feb 10, 2020

I don't think there is a method to retrieve the MetaEngine from the engine. We can add one in the Engine class:

Thanks. I added it in, and used it to add code that avoids saving over a slot if it has a custom save.

@dreammaster dreammaster force-pushed the dreammaster:autosaves branch from e3c8982 to 7f90142 Feb 10, 2020
// First check for an existing savegame in the slot, and if present, if it's an autosave
SaveStateDescriptor desc = getMetaEngine().querySaveMetaInfos(
_targetName.c_str(), getAutosaveSlot());
saveFlag = desc.getSaveSlot() == -1 || desc.getDescription() == saveName;

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

Is testing on the save name really a good idea? What if the user changes the language of the GUI?

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 10, 2020

Author Member

There is that possibility, yes. But testing the name is the best solution I could come up. I think still allowing for the 'Autosave' to be translateable is better for foreign language users then forcing the text to always be English.

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

Have you considered adding a isAutoSave flag to SaveStateDescriptor? Engines that save the information that a save is an autosave could valorize it. For the other engines it could default to the description being Autosave. That way, newer engines can be more robust.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 10, 2020

Author Member

The trouble with that is all the different historical engines provide their own querySaveMetaInfos method implementations. So even if I added an isAutosave property to SaveStateDescriptor, I'd have to add the comparison to "Autosave" to every one of them to set it as necessary. Which is too much code duplication for my liking.

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

What about:

enum SaveType {
    kSaveTypeUndetermined,
    kSaveTypeRegular,
    kSaveTypeAutosave
};

SaveType _saveType { kSaveTypeUndetermined };

void setAutosave(bool autosave) {
    _saveType = autosave ? kSaveTypeAutosave : kSaveTypeRegular;
}

bool isAutosave() const {
    if (_saveType != kSaveTypeUndetermined) {
        return _saveType == kSaveTypeAutosave;
    } else {
        return _description == _("Autosave");
    }
}

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 11, 2020

Author Member

Now that I like. And an autosave property could also be fairly easily added to the extended savegame format to as well, which could set the property.

@dreammaster dreammaster force-pushed the dreammaster:autosaves branch 2 times, most recently from b59a748 to c48f1a6 Feb 10, 2020
@bgK
bgK approved these changes Feb 13, 2020
@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 13, 2020

Are there any pending tasks regarding this pull request? If not, it can be merged this Sunday, the 16th of February

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 16, 2020

@dreammaster: unfortunately, this has conflicts now. Could you please resolve them?

@dreammaster

This comment has been minimized.

Copy link
Member Author

dreammaster commented Feb 16, 2020

Sure. I'll look into it later today

dreammaster added 17 commits Feb 5, 2020
Unfortunately, this will only apply to new engines that
use the Engine::loadGameState method. Other existing engines call
loadGameState directly and provide their own implementations,
so there's nowhere convenient to add the call that'd work for
all of them
I haven't figured out a good way to an exit on save for all engines,
so I'm re-adding the save autosave on exit that Myst previously had,
to call the new common Engine autosave method
There's still some specific code remaining to keep the way the
existing autosave is handled the same, even though it now saves
it to slot 0, rather than with a .asd extension
The autosave code looks more complicated for the Scumm engine, and
since it's already present and working, it's simpler just to disable
the autosave code in the base engine for just this engine, letting
it keep taking care of it
This removes filename methods when it matched the Engine method.
Secondly, ensuring there was an overriden getSaveStateName method
for engines that didn't do the standard target.00x save filenames
@dreammaster dreammaster force-pushed the dreammaster:autosaves branch from d4366b0 to 8766e22 Feb 16, 2020
@dreammaster dreammaster merged commit c3c0d04 into scummvm:master Feb 16, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.