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: Fix getSavegameFile for almost all engines #3430

Merged
merged 1 commit into from Nov 10, 2021

Conversation

@orgads
Copy link
Contributor

@orgads orgads commented Oct 20, 2021

Use kSimpleSavesNames correctly, add where needed, remove where needed.

Trac #12977

if (saveGameIdx == kSavegameFilePattern) {
// Pattern requested
const char *pattern = hasFeature(kSavesUseExtendedFormat) ? "%s.###" : "%s.s##";
Copy link
Contributor Author

@orgads orgads Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original usage of kSavesUseExtendedFormat looks like a mistake, at least according to the comment that describes kSimpleSavesNames

Loading

@criezy
Copy link
Member

@criezy criezy commented Oct 20, 2021

There was some discussion on Discord, but it is better to have it here. So let me copy the relevant part...

I think that the use of kSavesUseExtendedFormat to switch between <target>.### and <target>.s## is indeed dubious. But I don't think that kSimpleSavesNames was intended to be used that way either.

I am planning to spend more time this weekend to look at the code and the history and try to make more sense from it. But from the kSimpleSavesNames documentation it seems to me that it was intended as a way to indicate to the cloud savegame synchronization code that the savegame files can be identified with a simple pattern. Currently it hardcodes this pattern to _target + ".###" in SaveLoadChooserDialog::listSaves(), but it could possibly (and maybe should) use _metaEngine->getSavegameFile(kSavegameFilePattern, _target) instead. And the kSimpleSavesNames would then mean that MetaEngine:: getSavegameFile can be used to get a pattern to identify the savegames files (but not necessarily that the pattern is exactly <target>.###).

In such a case we would need something else to differentiate between <target>.### and <target>.s## in getSavegameFile. This could be yet another flag, or deciding that any engine that uses <target>.s## (or anything else other than <target>.###) need to reimplement getSavegameFile.

Loading

engines/sword1/metaengine.cpp Outdated Show resolved Hide resolved
Loading
@orgads
Copy link
Contributor Author

@orgads orgads commented Oct 21, 2021

@criezy Not sure I understand your point. If the savegame doesn't match this pattern, what's the point of setting the kSimpleSavesNames feature? And if it does, why not add it?

Loading

@criezy
Copy link
Member

@criezy criezy commented Oct 24, 2021

The changes in this pull request update the various hasFeature() implementations (to properly return true if the engine savegame file patter is <pattern>.###, and return false if it is something else) is a good one that should fix missing background cloud sync'ing of savegames for some engines.

My remark is only related to the implementation of getSavegameFile(). The change that you are proposing here should work, but I think it might be cleaner to only handle the most common case (<target>.###) in the default implementation, and reimplement it in engines that use any other pattern.

My issue with it is that it is using kSimpleSavesNames for a different purpose than what it was introduced for. That flag was introduced with the cloud code, as that code has a special handling of some savegame files (to allow sync in background) and it needed a way to identify engines that use such savegames. Currently that means engines whose savegames match the <target>.### pattern, but that could change in the future. If we for example update the cloud code so that it can also handle the <target>.s## for background sync'ing, then we will need to add the kSimpleSavesNames feature to those engines as well. And with your proposed implementation that would cause getSavegameFile() to return an incorrect pattern/name.

Loading

engines/dreamweb/metaengine.cpp Outdated Show resolved Hide resolved
Loading
engines/mohawk/metaengine.cpp Show resolved Hide resolved
Loading
engines/ngi/metaengine.cpp Show resolved Hide resolved
Loading
engines/parallaction/metaengine.cpp Show resolved Hide resolved
Loading
engines/pegasus/metaengine.cpp Outdated Show resolved Hide resolved
Loading
engines/sword1/metaengine.cpp Outdated Show resolved Hide resolved
Loading
Copy link
Member

@criezy criezy left a comment

Thank you for this work!
I think a few of the changes are incorrect though and will need changes though. See the various comments I added for details.

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

@criezy it looks like most (all?) the changes were made. Would you give it another round of review?

Loading

@orgads
Copy link
Contributor Author

@orgads orgads commented Nov 1, 2021

@sev- he already reviewed it. I still have some changes to do.

Loading

@orgads
Copy link
Contributor Author

@orgads orgads commented Nov 3, 2021

My issue with it is that it is using kSimpleSavesNames for a different purpose than what it was introduced for. That flag was introduced with the cloud code, as that code has a special handling of some savegame files (to allow sync in background) and it needed a way to identify engines that use such savegames. Currently that means engines whose savegames match the <target>.### pattern, but that could change in the future. If we for example update the cloud code so that it can also handle the <target>.s## for background sync'ing, then we will need to add the kSimpleSavesNames feature to those engines as well. And with your proposed implementation that would cause getSavegameFile() to return an incorrect pattern/name.

I see your point, but I think that instead of reimplementing getSavegameFile so many times, I'd rather add another flag.

But since this flag will be equivalent to kSimpleSavesNames, I suggest adding a comment next to the existing flag (that it serves 2 purposes), and in case the cloud maintainer will decide to cover more cases, we can then split this flag.

What do you say?

Loading

@criezy
Copy link
Member

@criezy criezy commented Nov 7, 2021

But since this flag will be equivalent to kSimpleSavesNames, I suggest adding a comment next to the existing flag (that it serves 2 purposes), and in case the cloud maintainer will decide to cover more cases, we can then split this flag.

That seems fine.
Any future changes to the cloud code are hypothetical and may never happen. So we can do that additional work if and when that happens. Flagging that with a comment is sufficient for now.

Loading

engines/pegasus/pegasus.cpp Outdated Show resolved Hide resolved
Loading
engines/pegasus/metaengine.cpp Outdated Show resolved Hide resolved
Loading
engines/tinsel/metaengine.cpp Outdated Show resolved Hide resolved
Loading
@criezy
Copy link
Member

@criezy criezy commented Nov 7, 2021

I think we need a couple of changes for the pegasus engine, but other than that it looks good and is almost ready to be merged.

Loading

Use kSimpleSavesNames correctly, add where needed, remove where needed.

Trac #12977
@orgads
Copy link
Contributor Author

@orgads orgads commented Nov 10, 2021

All done, hopefully :)

Loading

@orgads orgads requested a review from criezy Nov 10, 2021
criezy
criezy approved these changes Nov 10, 2021
Copy link
Member

@criezy criezy left a comment

Thank you. It look good to me now.

Loading

@criezy
Copy link
Member

@criezy criezy commented Nov 10, 2021

I will merge this now.

But I also have a question: other than the tinsel engine, are you aware of any other engine where getSavegameFile might still be incorrect or as far as you know they should all be correct now? I didn't see any myself, but you probably looked at this more carefully than I did.

Loading

@criezy criezy merged commit 78ef620 into scummvm:master Nov 10, 2021
5 of 8 checks passed
Loading
@orgads
Copy link
Contributor Author

@orgads orgads commented Nov 11, 2021

myst3 also has a TODO.

Loading

@orgads orgads deleted the simple-saves branch Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants