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

STARK: Implement the save & load menu #1422

Merged
merged 17 commits into from Jun 4, 2018

Conversation

Projects
None yet
3 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented May 29, 2018

This PR is corresponding to implement the save & load menu of The Longest Journey.

@DouglasLiuGamer DouglasLiuGamer requested a review from bgK May 29, 2018

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-saveload branch 2 times, most recently from cb1dc6b to 5559fee May 29, 2018

@@ -336,7 +336,8 @@ Common::Error StarkEngine::loadGameState(int slot) {

bool StarkEngine::canSaveGameStateCurrently() {
// Disallow saving when there is no level loaded or when a script is running
return _global->getLevel() && _userInterface->isInteractive();
// or when the save & load menu is currently displayed
return _global->getLevel() && _userInterface->isInteractive() && !_userInterface->isInSaveLoadMenuScreen();
}

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 31, 2018

Author Collaborator

I used to try an implementation that allows saving through GMM while the save & load menu is displayed, but it is kind of complex since the menu needs to be notified when a save slot is updated and update the thumbnail of the corresponding widget. Maybe just disable the saving is a better approach?

This comment has been minimized.

@bgK

bgK May 31, 2018

Member

Disabling saving from the GMM when in the saveload menu to avoid too much complication sounds good to me.

@@ -113,7 +131,16 @@ void SaveMenuScreen::open() {
}

void SaveMenuScreen::onSlotSelected(int slot) {
g_engine->saveGameState(slot, "TestSave");
int chapter = StarkGlobal->getCurrentChapter();

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 31, 2018

Author Collaborator

What does the returned value of getCurrentChapter() really mean? I thought it was the index of the chapter, but calling this function in Chapter 1 gives me value 10.

_textDesc(gfx),
_textTime(gfx),
_isMouseHovered(false) {
// Load the corresponding save slot data
Common::String filename = StarkEngine::formatSaveName(ConfMan.getActiveDomainName().c_str(), _slot);
Common::InSaveFile *save = g_system->getSavefileManager()->openForLoading(filename);
if (save) {

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer May 31, 2018

Author Collaborator

I feel like putting a delete save; at the end is needed, but it will crash the game. The StarkEngine::loadGameState() also doesn't delete the save, why is that?

This comment has been minimized.

@dafioram

dafioram May 31, 2018

Contributor

The constructor for StateReadStream() is two arguments, the file and a flag (DisposeAfterUse::YES and DisposeAfterUse::NO) the flag indicates if the input file should be deleted or not after use.

In your case you used it with 1 argument which looks like that defaults to the 2 argument case with the flag set to dispose. I am not exactly sure where that is happening since the constructor to SubReadStream defaults the dispose to NO (see common/substeam.h). StateReadStream is a SeekableSubReadStream which is also a SubReadStream.

So you don't need to delete save since that is already being done by the destructor of StateReadStream, but just to be clear you may want to explicitly set the 2nd argument to be DisposeAfterUse::YES in the constructor of StateReadStream.

Alternately, you can set it to NO and delete the save manually.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented May 31, 2018

Beware
I change the SaveMetaData class to include the second part of the saving time, which may invalidate old save data. New save data may also have problems loading to old ResidualVM.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented May 31, 2018

I change the SaveMetaData class to include the second part of the saving time, which may invalidate old save data. New save data may also have problems loading to old ResidualVM.

Hi, Please increment the save version number and load the seconds for saves that have at least the new version number. This way old saves remain compatible.

static const uint kSaveVersion = 9;

metadata.saveHour, metadata.saveMinute, metadata.saveSecond,
metadata.saveMonth, metadata.saveDay, metadata.saveYear % 100));
_textTime.setColor(_textColor);
_textTime.setFont(FontProvider::FontType::kCustomFont, 3);

This comment has been minimized.

@dafioram

dafioram May 31, 2018

Contributor

I think you want FontProvider::kCustomFont instead of FontProvider::FontType::kCustomFont.

@bgK
Copy link
Member

bgK left a comment

What does the returned value of getCurrentChapter() really mean? I thought it was the index of the chapter, but calling this function in Chapter 1 gives me value 10.

It's more a progress marker than exactly the chapter number. To obtain the actual chapter number, you need to divide that value by 10.

@@ -336,7 +336,8 @@ Common::Error StarkEngine::loadGameState(int slot) {

bool StarkEngine::canSaveGameStateCurrently() {
// Disallow saving when there is no level loaded or when a script is running
return _global->getLevel() && _userInterface->isInteractive();
// or when the save & load menu is currently displayed
return _global->getLevel() && _userInterface->isInteractive() && !_userInterface->isInSaveLoadMenuScreen();
}

This comment has been minimized.

@bgK

bgK May 31, 2018

Member

Disabling saving from the GMM when in the saveload menu to avoid too much complication sounds good to me.

if (chapter == 0) {
desc = "Prologue";
} else {
desc = Common::String::format("Chapter %d", chapter);

This comment has been minimized.

@bgK

bgK May 31, 2018

Member

Ultimately, chapter names will have to come from chapters.ini so they are localized. But I guess having them hardcoded is fine for the first version. The original engine uses the string before the colon in chapters.ini.

// Obtain the thumbnail
Graphics::Surface *thumb = metadata.readGameScreenThumbnail(&stream);
_texture->update(thumb);
delete thumb;

This comment has been minimized.

@bgK

bgK May 31, 2018

Member

Graphics::Surface is very stupid. ->free() needs to be called as well.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 2, 2018

@bgK Currently SaveDataWidget will load and update its thumbnail and text only when it is created and clicked on the save menu. Obviously it is unrealistic to update the content on every frame. But the GMM provides saving and deleting operations, which will invalidate the current menu screen's content. Implementing the updating on the change from GMM is going to be a complex task, and it feels like unnecessary for such a strange behaviour. I mean, who on earth will open two save & load menus just for fun?

So, is it okay to just block both saving and loading from GMM when the game is in save & load menu?

p.s. Currently, the saving is already blocked. Deleting a save data through GMM while the load menu is displayed will make the corresponding widget, if it's on the current page, unresponsive.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 2, 2018

A new service for retrieving the chapter title and subtitle from chapters.ini is created. This would also be helpful when implementing the conversation log, I guess.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 2, 2018

So, is it okay to just block both saving and loading from GMM when the game is in save & load menu?

Sure, ok.


Common::String line, title, subtitle;

// Assume that the formats of all chapters.ini are the same

This comment has been minimized.

@bgK

bgK Jun 2, 2018

Member

Did you notice Common::INIFile?

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 2, 2018

Author Collaborator

Oops, didn't see that. I'll take a look.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 2, 2018

The current mechanism of capturing the screenshot will also capture the cursor. I have spent some time trying to get rid of it, but the best I could get will also remove the model from the thumbnail. Any suggestion?

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 3, 2018

The current mechanism of capturing the screenshot will also capture the cursor. I have spent some time trying to get rid of it, but the best I could get will also remove the model from the thumbnail. Any suggestion?

A simple way would be to re-render the game window only just before capturing the screenshot. (And maybe re-render the whole screen afterwards if that causes glitches)

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 3, 2018

A simple way would be to re-render the game window only just before capturing the screenshot.

Already tried this before. Simply re-render the game window (or even the game screen) will cause only the background rendered, without the models. Calling updateScreen() after will also render the cursor.

Still can't figure out what causes this.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 3, 2018

Already tried this before. Simply re-render the game window (or even the game screen) will cause only the background rendered, without the models.

Oh, right. The depth buffer needs to be cleared for the depths tests to work correctly. Calling StarkGfx->clearScreen should do it.

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-saveload branch from bfeb0b4 to 3b4270e Jun 3, 2018

}

// Obtain the thumbnail
Graphics::Surface *thumb = metadata.readGameScreenThumbnail(&stream);

This comment has been minimized.

@bgK

bgK Jun 3, 2018

Member

The thumbnail is only available since save metadata version 9. Most of my saves are older than that and show garbage instead of the thumbnail.

// Freeze the screen for a while to let the user notice the change
widget->loadSaveDataElements();
render();
g_system->updateScreen();

This comment has been minimized.

@bgK

bgK Jun 3, 2018

Member

Use GfxDriver->flipBuffer() instead not to break the Driver abstraction.

lineSurface.free();

// Set the position
_thumbPos.x = 41 + (_slot % 3) * (_thumbWidth + 39);

This comment has been minimized.

@bgK

bgK Jun 3, 2018

Member

There could be constants for the number of slots per lines, the number of slot lines and the number of slots per page to make the code easier to understand.

}

void SaveLoadMenuScreen::changePage(int page) {
assert(page >= 0 && page <= 10);

This comment has been minimized.

@bgK

bgK Jun 3, 2018

Member

Please add a constant for the maximum number of pages. We may want to increase that number as the ResidualVM save and load dialogs have a lot more slots.

@@ -94,7 +94,7 @@ void DiaryIndexScreen::open() {
_widgets.push_back(new StaticLocationWidget(
"Back",
CLICK_HANDLER(DiaryIndexScreen, backHandler),
MOVE_HANDLER(DiaryIndexScreen, widgetTextColorHandler)));
nullptr));

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 3, 2018

Author Collaborator

Just found out that this text will not change to blue in the original game.

_texture->update(thumb);
thumb->free();
delete thumb;
}

This comment has been minimized.

@DouglasLiuGamer

DouglasLiuGamer Jun 3, 2018

Author Collaborator

I don't have save data older than version 9. Could you please check this code for me?

This comment has been minimized.

@bgK

bgK Jun 3, 2018

Member

It's working, thanks!

This is my collection of old saved games in case you need it: https://filenurse.com/download/361b5da097c32696eff053a7b026ad7b.html

A lot of them were made back when some entity save handlers were missing, so they don't load all the state, but oh well...

@bgK bgK merged commit 88fd362 into residualvm:master Jun 4, 2018

@DouglasLiuGamer DouglasLiuGamer deleted the DouglasLiuGamer:branch-saveload branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.