libretro / gpsp

gpSP for libretro.
GNU General Public License v2.0
52 stars 52 forks source link

Fix save_type in game overrides (gba_over.h / game_config.txt) #220

Closed andymcca closed 1 year ago

andymcca commented 1 year ago

The 'save_type' field in gba_over.h / game_config.txt doesn't currently work, due to no code being in place to process the field.

It should provide an override in the case where the backup method (e.g. Flash, SRAM or EEPROM) is incorrectly detected by gpsp and thus prevents the game from being able to save (mgba has similar functionality). This also causes the copy protection to trigger in some games, such as the Dragon Ball series.

This PR adds code to process save_type values against games in gba_over.h.

save_type can now have 6 possible values - no override, flash, sram, eeprom(1k), eeprom(8k) or backup disabled . gpsp should do the rest in determining the necessary size for the sram and flash values. The backup disabled option is required for games like Top Gun Combat Zone, which don't use saving, and employ copy protection based on if the game detects any flash/sram/eeprom at all.

Have also added a few game entries to gba_over.h which were used in testing.

anzz1 commented 1 year ago

Code looks good, however I don't have enough experience with the emulator to actually review.

I do have a suggestion though, regarding the variable naming.

BACKUP_DISABLED sounds awful lot like BACKUP_NONE , and "none" here seems to actually mean "undefined" aka "not known", and not that there is none, except on gba_memory.c : load_backup(), where "none" seems to mean "empty", which is different from before as in "exists, but contains nothing". There also already exists variable called FLASH_DEVICE_UNDEFINED for the flash, further suggesting that "undefined" and "none" should mean different things. In any case it's all very confusing, and in the current form it just adds to the confusion.

So the names should be changed to describe what they actually mean. A suggestion for the naming scheme which would make sense in my opinion:

a) When there is no override defined, initial value is BACKUP_UNDEFINED , which then gets changed to the proper guessed type. b) If there is no battery save either by determination or override, that would be then BACKUP_NONE

I'm not sure if changing some of the logic is needed or is it enough just to rename

But in any case you shouldn't have both "NONE" and "DISABLED". Also "EMPTY" should mean different thing than either of those, as "glass is empty" is different than "there is no glass". NONE/DISABLED = no battery save. EMPTY = has battery save, but contains nothing yet.

andymcca commented 1 year ago

@anzz1 thanks for the suggestions. Actually, David is working on his own implementation to fix based on this PR, with some better code similar to what you are saying above which will hopefully also eliminate any confusion.

davidgfnet commented 1 year ago

Let's close this. If we missed any game let us know and we will add it. Not many games need to force save-type, only the ones that had checks for cartridge piracy. Thanks!