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: Savegame code simplifications #2037

Merged
merged 11 commits into from Feb 5, 2020
Merged

Conversation

@dreammaster
Copy link
Member

dreammaster commented Feb 3, 2020

This pull request aims to simply provide some simplification and improvements for the savegame code, particularly going forward for new engines added in the future. It consists of:

  1. Showing the save/load dialogs due to in-game actions
    A common feature of a lot of the existing games is that they need to trigger showing the save/restore dialogs from in-game buttons, menus, etc., in addition to from the GMM. As such, engines have gotten into the habit of continuously repeating code copied from the GMM to instantiate the dialog, get the result, and call saveGameState/loadGameState. Two new methods are added to the Engine class - loadGameDialog and saveGameDialog. I've also converted several of the engines to use the new methods rather than their own code. I wasn't pedantic about it.. some engines I wasn't familiar with, or had extra code directly in their save/load methods. It could perhaps be a future task for GSOC applicants to identify other existing engines that could be similarly simplified.

  2. Simplified integration of the new extended savegame headers for future engines
    Recently adding support for the new extended savegame format to Nuvie and Pentagram, I found it a bit cumbersome to properly integrate. I had to get help to find out the method to call to add the extra info to the savegame, and afterwards I only coincidentally noticed a problem with total play time resetting because I wasn't calling setTotalPlayTime in the load code.

So, to combat this, I've added several new methods to the base Engine class, and changed the implementation of loadGameState and saveGameState. First of all, this shouldn't affect any of the existing engines, since they all override loadGameState and saveGameState, so they'll remain unaffected. When not overridden, the assumption is that new engines will be using the new extended savegame code. The loadGameState and saveGameState methods now contain the code for:

  • Opening up the savefiles
  • Calling new methods readGameStream/writeGameStream which new engines would now override instead of loadGameState/saveGameState. These methods take in the already opened savefile streams, so there's no need for the engines to provide their own code anymore.
  • Finally, either appending the extended savegame info for saving, or setting total play time for loading.
  1. Autosaves
    Many games have logic for when a save is done, such as displaying a "saved" message of some sort. To facilitate this, the saveGameStream method has an isAutoSave parameter to indicate if the game was being saved due to autosave kicking in, or via a player action. New engines will thus be able to do whatever UI notification they want in the method, and have it be done for saves done both via the GMM, or if the game itself manually called the saveGameDialog method.

Speaking of autosaves, I think this is something that should really be considered for being moved into the core as well. My latest commit added autosave support for Griffon, but it occurred to me that that's somewhat inefficient. Currently, not even a quarter of the engines call the "shouldPerformAutoSave" method. I was thinking maybe we could do the checks in the events managers - it could access the global engine pointer to call the method as well as canSaveGameStateCurrently, and maintain the last autosave time. That way, we could do away with engines having to do it themselves. Thoughts?

dreammaster added 9 commits Feb 2, 2020
This is primarily future-proofing. Many games either show a message
or do some other UI action like closing an open game menu, and
obviously that should only be done when a savegame created by the
user is done, rather than for regular autosaves. By making this a
flag, when engines override saveGameStream, they'll be able to
tell if it's an autosave, and only do UI changes if not
if (slotNum != -1)
return loadGameState(slotNum).getCode() == Common::kNoError;

return false;
Comment on lines +704 to +707

This comment has been minimized.

Copy link
@bgK

bgK Feb 3, 2020

Member

I like showing a GUI error message when loading / saving failed.

Suggested change
if (slotNum != -1)
return loadGameState(slotNum).getCode() == Common::kNoError;
return false;
if (slotNum < 0)
return false;
Common::Error loadError = loadGameState(slotNum);
if (loadError.getCode() != Common::kNoError) {
GUI::MessageDialog dialog(loadError.getDesc());
dialog.runModal();
return false;
}
return true;
}

bool Engine::canSaveGameStateCurrently() {
// Do not allow saving by default
return false;
}

bool Engine::loadGameDialog() {
if (!canLoadGameStateCurrently())

This comment has been minimized.

Copy link
@bgK

bgK Feb 3, 2020

Member

I'm not sure where we are going with this check. It seems to me it can create situations where the player clicks on the load button and nothing happens because loading is not allowed currently. So maybe this should be an assertion to force the caller handle the case or maybe this should display a GUI error message telling the player loading is not possible at the moment.

This comment has been minimized.

Copy link
@sev-

sev- Feb 3, 2020

Member

Or an OSD message?

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 3, 2020

Author Member

Good point both of you; that'd make more sense to users that simply ignoring the call.

@sev-
sev- approved these changes Feb 3, 2020
Copy link
Member

sev- left a comment

I love the change and implementation.

@@ -631,8 +634,25 @@ void Engine::flipMute() {
}

Common::Error Engine::loadGameState(int slot) {
// Do nothing by default
return Common::kNoError;
Common::InSaveFile *saveFile = _saveFileMan->openForLoading(getSaveStateName(slot));

This comment has been minimized.

Copy link
@sev-

sev- Feb 3, 2020

Member

It seems to me, the indentation here is two spaces instead of a tab.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 3, 2020

Author Member

I'll double check the indentation

}

bool Engine::canSaveGameStateCurrently() {
// Do not allow saving by default
return false;
}

bool Engine::loadGameDialog() {
if (!canLoadGameStateCurrently())

This comment has been minimized.

Copy link
@sev-

sev- Feb 3, 2020

Member

Or an OSD message?

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 3, 2020

I really like the implementation too. Nice!

@henke37

This comment has been minimized.

Copy link
Contributor

henke37 commented Feb 3, 2020

This for sure is a real issue. We have some missing code in pink that I could easily implement with these new features.

@bluegr
bluegr approved these changes Feb 3, 2020
@sev-

This comment has been minimized.

Copy link
Member

sev- commented on engines/engine.cpp in 7526a2e Feb 4, 2020

Did you consider using less intrusive OSD here?

This comment has been minimized.

Copy link
Member Author

dreammaster replied Feb 4, 2020

Did you have something in mind? I'm afraid I haven't used the GUI code much.. I tried using GUIErrorMessage as well, but that resets the resolution back to 320x200 and erases the screen.

This comment has been minimized.

Copy link
Member

sev- replied Feb 4, 2020

Yes, I meant to call here:

displayMessageOnOSD(_("Loading game is currently unavailable"));

That should be it. Then a non-blocking message will appear and fade away on the screen.

@dreammaster dreammaster merged commit 2d09358 into scummvm:master Feb 5, 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

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