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

RFC: Make HE games use the target name in all save files #366

Merged
merged 2 commits into from Jan 17, 2014

Conversation

@clone2727
Copy link
Contributor

commented Aug 3, 2013

As is our current policy, this updates all HE save games to make sure the target name is always part of the save file name now. Older saves will be checked for loading still, so compatibility is kept.

In cases with games that have a save file name (before the extension) that match the game id, it will be replaced by the target name. In other cases, the target name will just be prepended in front of the full save file name.

This also fixes Backyard Football and Backyard Soccer which previously were broken because the save names were incorrectly made into the same name for multiple files.

I've tested a lot of games, but I wasn't able to test all of them.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

The real problem with Backyard Sports titles and the map editors in some of the arcade games is the lack of support for working with directories in our save system. This seems like too much of a hack for that issue, or is it mean to resolve other problems too?

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2013

As far as I know, the only thing in the Backyard sports titles that uses directories (minus Moonbase) is for the internet save files, which we're obviously not supporting at this time. The issue with Backyard Football and Soccer is not because of directories, but rather because we renamed "coach.sg?" to "soccer.sg?" and conflicted with other "soccer.sg?" saves.

This isn't just a hack to solve that though; this makes all the saves be prefixed with the target which is what should be done for all saves in ScummVM.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

Backyard Basketball uses separate directories for coaches, Spy Fox in Cheese Chase uses directories when saving custom levels, and FunShop titles make use of directories when saving too. Plus several Backyard Sports and Blue's Clues games create a single separate saved game directory, to prevent any file conflicts for saved games.

So I would suggest only altering file path conversion code to fix Backyard Football and Soccer issue. Rather than altering the filename of all saved games, passed through the file conversion code. Maybe add separate section to handle all Backyard Sports titles, since their saved game methods are quite different to the adventure games.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2013

In Basketball, I don't see any separate directories being used. It just creates a .plr file. Blue's Birthday also doesn't used directories, and instead creates .bca. I don't have any FunShop game, Cheese Chase, or the other Blues Clues games.

Like I said, this ensures that all the save files are prefixed with the target name which is supposed to be done to all our save files, so the conversion code is still necessary for all saved games.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

If you search over dumped scripts for 'createDirectory' you will see exactly where directories are used for saved games. Later games start using a default saved games directory of 'c:\hegames\gametitle', instead of 'c:\hegames', around HE99 onwards.

We have not prefixed saved games for HE games in the past, as there has been no need, nor do I see a reason to start at this point. The only known conflicts, are due to lack of support for handling directories in our saved game system.

@fuzzie

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

Adding directory support to our saved game system would be non-trivial (and some backends would end up falling back on a "add directory name to the start of the filename" approach anyway..).

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

Yes, but it will eventually be required, at least in the games mentioned above. I expect directory support in the save game system might have to be compile time optional, if not possible of specific platforms.

The later HE games wouldn't be possible on many portable platforms, due to the color depth and memory requirements anyway.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

Regarding directory support: could we keep the flat directory structure of our savegames, and write the directory they are supposed to be at in the savegame header?

i.e. we could have savegame A with directory "foo" in its header, and savegame B with directory "foo/bar". Thus, when the game asks for saved games, we could return only the ones with "foo" in that field, and return the "foo/bar" ones when it's asking for, say, custom levels

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 4, 2013

Another idea would be to append an extra bit to the names of the savegames that are supposed to be stored in another directory. E.g. The backyard basketball coach files could be named "basketball-coach.000", Chase custom levels "chase-level.000" and so on

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2013

@Kirben But every saved game for a particular game is stored in the same directory? ie. anything for "Pajama Sam 3" would be in "c:\hegames\pajama3"? So wouldn't prefixing them work for this?

And we should start prefixing saved gamed in ScummVM because it's really our policy for all saved games.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2013

I updated after LordHoto's config -> INI rename.

I also changed it to remove the gameid/target swapping since it wasn't that nice to chop up the original saved game name (it also failed for things like freddi5). Now the target is just prepended in front of any save name unconditionally (sans directory names).

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2013

@Kirben I only see the games creating the result of the "SaveGamePath" config option in o72_createDirectory (which we overwrite to a default directory anyway). Can you please tell me where something else is used? I'd really like to get this finished and pushed into the master.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

@bluegr I don't see how the first idea would work, since it still isn't clear what filename to use, if the same filename is used in multiple paths.
@bluegr The second idea would lead to longer convoluted paths like 'chase-000-chase-001.map' or 'moonbase-user-User001.map' or 'puttputtsfunshop-Customizer-putt.osa'.

@clone2727: The specific games using directories have already been mentioned above, there are still five titles, if I was mistaken about basketball. chase uses separate directories (i.e. 000/chase001.map 000/chase001.obj) for the new custom levels, the three funshop titles make use of directories (i.e. Customizer/luther.osa Paintpad/free.osa) for new custom files, and moonbase uses user directory (i.e. user/User001.map user/User001.thm user/User001.wiz) for new custom levels. While other later HE Games only use a separate saved game directory, to prevent any file conflicts.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2013

@Kirben Of those games, I only have moonbase. I don't see it creating a "user" directory using the o72_createDirectory opcode, just that it uses a "user" directory in the SaveGamePath. It creates a test file early on after the game starts up called "user\test" without any directory creation.

In any case, this directory issue would still be separate from the issues this pull request fixes. This one still does exactly what it says on the tin: it makes the target name prepended to all saved games (as per http://wiki.scummvm.org/index.php/HOWTO-Engines#File_name_conventions) and it fixes Backyard Soccer and Backyard Football.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

I do agree with @clone2727: HE games create their own save files without a game prefix, which makes it hard to figure out which save belongs to what game. e.g. SPY Fox in Cheese Chase creates a file called "names.cc", which is a list of all custom level names.

But looking at the save game itself makes it hard to understand which game it belongs to, plus, save name clashes between games are very likely to occur - which is why we got the file name conventions in the first place.

So, IMHO this pull request is a step towards the right direction, as it makes HE games create uniform save game names, it makes it easy to figure out which save games belong to what game, plus it addresses save name clashes across different games

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

Also, I did hack in a solution for Cheese Chase, by adding this block inside convertFilePath(), after all the other path processing code:

if (dst[0] == '*' && Common::isDigit(dst[2]) && Common::isDigit(dst[3]) && Common::isDigit(dst[4])) { // Custom levels (Cheese Chase)
    char customFile[40];
    memcpy(customFile, dst + 6, len - 6);
    customFile[len - 6] = 0;
    snprintf((char *)dst, dstSize, "%s-%s.%c%c%c", _targetName.c_str(), customFile, dst[2], dst[3], dst[4]);
}

Effectively, this changes files like "*\010\chase001.obj" to "ase-us-chase001.obj.010". Not optimal, but at least the player can create custom levels this way.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

The directory issues are relevant to these changes, as separate directories would be a cleaner solution, if directory support is added to the save game system in the future. We could simply use a sub folder based on target name, similar to the original games, with no further filename changes required.

Prefixing filenames or other methods should only be used, if that is definitely not going to happen in the future.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

I don't get this: why are HE saves not following the same naming scheme as all other engines? Prefixing saved games is what all other engines do, and it avoids confusion. So again, I fail to understand why HE games have to be the exception to the rule.

To clarify: in cases where games save multiple files in a directory, then yes, a directory scheme based on the target would make sense, to avoid filling up the main save folder with loads of files. But for single files, such as single saved games, this doesn't make sense, IMHO

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 19, 2013

HE games were an expansion of the SCUMM engine, and added long before those naming conventions for new game engines were even created. I think HE Games were the first games to use additional save games files, and it is rather late to be making changes to those saved game filenames now, unless it can't be avoided.

chase adds 30 files every time a new set of custom levels are created. While the arcade and sports titles can add up to 3 files, each time a new coach/player is added to that game.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2013

@Kirben If this hypothetical directory-based system were to happen, this is still a prerequisite for that to happen. Please, bring it up on -devel if you want to go that route, since there hasn't been a discussion there on that.

In addition, it's never late to fix problems. The saves should not stay as-is.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 20, 2013

@clone2727 Except your solution creates another problem, since it uses a completely different set of saved game filenames, depending on the ScummVM version. There is no notice to users about this change, so users will wonder where saved games went, if they have to switch back to older ScummVM version.

Save game versions in SCUMM change over time, but the GUI makes it clear they aren't compatible with older version. Save game prefixes have changed in the past too, but they have always been auto upgraded.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 20, 2013

As a reminder, there were other engines in the past with their own save game naming and structure, such as drascula and sword1.

These have been changed to follow the ScummVM naming standards, and their savegame index files have been removed. These engines have GUI dialogs on startup which upgrade all the older saved games to the new format.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2013

@Kirben No, that's not true. This code supports all of the older saves. I made sure that backwards compatibility was kept. Only new saves will be output with the new name. We have never claimed to have older code be compatible with newer code.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 20, 2013

@clone2727 Your missed the point, any new saved games created after this patch, can't be used on older ScummVM versions. If a user has to go back to ScummVM 1.6.0, due to a bug or older port, the newer saved games will not be found, due to the filename changes.

@bluegr

This comment has been minimized.

Copy link
Member

commented Aug 20, 2013

@Kirben : But we never claimed that older ScummVM versions are compatible with the savegames of newer ScummVM versions - only the opposite, i.e. newer ScummVM versions are compatible with older ones.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2013

@Kirben No, my point still holds. We shouldn't worry about anyone who goes back to use an older version. This isn't the first time we've changed file names for an engine either, as @bluegr has said.

@Kirben

This comment has been minimized.

Copy link
Member

commented Aug 27, 2013

Looks like directory support in our save game system isn't a viable option for the future, and I don't know of any other cleaner method for filenames of saved games of HE games. So go ahead and commit these changes, when you are ready.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2013

I'll do some cleanup this week before merging the branch.

@sev-

This comment has been minimized.

Copy link
Member

commented Sep 12, 2013

Looking forward to the updated pull request.

This makes HE follow the ScummVM convention of using the target name everywhere. It also fixes having more than one team in both soccer and football.

Loading old saves will still work and they will be tried if the newer save names are not found.
@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2013

Sorry that took so long, but I got around to updating the branch today. There was previously a bug with reporting custom save file names that should be fixed.

I tested some more games to make sure they still work (and they do), but I would love if a few others could test before a merge.

@clone2727

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2014

Well, I guess no one else tested with the branch. I'll merge in a couple days unless there's any objections.

clone2727 added a commit that referenced this pull request Jan 17, 2014
RFC: Make HE games use the target name in all save files
@clone2727 clone2727 merged commit 9d9ced0 into scummvm:master Jan 17, 2014
@clone2727 clone2727 deleted the clone2727:he-saves-target-name branch Jan 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.