added multi-softpatching support (#9947) + show an OSD message on successful softpatch load

[eadmaster]

Description

Fixes #9947

If an ips/bps/ups patch is found, will replace the last char in the filename with a counter and loop until another patch is found (in any format).

Example matching filenames:

Contra (Japan).nes
Contra (Japan).ips # applied first
Contra (Japan).ips1 # applied second
Contra (Japan).ups2 # applied third
...

EDIT: notice that if one filename breaks the loop, the following patches won't be applied too. e.g.:

Contra (Japan).nes
Contra (Japan).ips # applied
Contra (Japan) (Translation).ips1 # partial name match, not applied
Contra (Japan).ups3 # not applied either
...

EDIT2: updated to refrect the new extensions format.

[keithbowes]

It needs to be possible to disable the OSD somehow, like a notification_show_softpatch_load setting (and an option in the Notification Visibility section of the UI).

[eadmaster]

It needs to be possible to disable the OSD somehow, like a notification_show_softpatch_load setting (and an option in the Notification Visibility section of the UI).

do all the OSD messages have an option to disable them?
I don't see one for the savestates and this softpatch notification is not very obtrusive since it shows only once when new content is loaded.

[eadmaster]

UPDATE: i still have another fix to push for this to limit the number of patches to 9, do not merge it yet!

[eadmaster]

ok, should be good to merge now...

[Sanaki]

Just tested this out. The functionality works like a dream, except that the patch number is double incrementing. You can load up to five total right now, in order: ips, ip2, ip4, ip6, ip8. Any odd numbers are skipped over.

[INFO] [CONTENT LOAD]: Loading content file: /usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).sfc.
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ips", attempting to patch ...
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ip2", attempting to patch ...
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ip4", attempting to patch ...
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ip6", attempting to patch ...
[INFO] [CONTENT LOAD]: CRC32: 0x45abffba .

[lgtm-com]

This pull request introduces 1 alert when merging 4802449 into 88b2359 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[bslenul]

While messing with this commit yesterday I've just replaced

for (i = 1; i < 10; i++)

with

for (i = 1; i < 10;)

and I kept the i++; inside the loop, so the increment works properly and the if( i == 1 ) that comes after still works if no patch is found.

[Sanaki]

Yeah, that's the better solution. Shows what happens when I touch C.

[eadmaster]

Yeah, that's the better solution. Shows what happens when I touch C.

what if i change the following if( i == 1 ) to if( i == 2 ) instead ?

[Sanaki]

That seems to work fine too.

[jdgleaver]

@eadmaster This is very useful functionality! Thanks for working on this :)

There are a few small issues, however:

• This is unsupported when using many toolchains:

   const int OSD_MSG_MAX_LEN = FILENAME_MAX + strlen(msg_hash_to_str(MSG_PATCH_LOADED));
   char osd_msg[OSD_MSG_MAX_LEN]; 

The size of osd_msg must be a 'real' constant, or this won't compile on several platforms

• I really dislike the practice of modifying function arguments that are meant to be const - I would much prefer that you modify copies of the patch file paths

• I think replacing the last character of the patch file extension kinda strips the patch file of its identity. .ips is clear - .ip with a number could refer to an entirely different file type. I would prefer the index to be appended after the last file extension character.

• In your for (i = 2; i < 11; i++) loop, you increment up to i = 10. If i is 10, then '0'+i will not produce a valid character

• Small naming/wording semantics: I think a patch should be 'applied' rather than 'loaded', in the same way that cheats are 'applied'

• We 100% need an option to hide patch notifications. When applying more than a couple, the OSD log spam is unbearable!

I have prepared a diff that fixes all these issues: soft_patch.zip

Could you please apply this on-top of your PR, and then check that you're happy with the changes? If so, I'd then request that you rebase and squash your commits - and then we can go ahead and merge this :)

[Sanaki]

Tested the patch out, works beautifully. Definitely appreciate the ability to hide the OSD notifications. Verified it now supports 10 patches (ips, ips1, ips2, and so on). Verified it will skip a non-matching bps in the middle of the chain and continue patching in order after that.

[jdgleaver]

@Sanaki Nice! Thank you very much for testing this :)

[eadmaster]

I think replacing the last character of the patch file extension kinda strips the patch file of its identity. .ips is clear - .ip with a number could refer to an entirely different file type. I would prefer the index to be appended after the last file extension character.

ZSNES used these extensions for multipatch support, but i guess the choice was forced by 8+3 DOS filesystem limitations. Still, i think 3 chars filename extensions is more portable...

I have prepared a diff that fixes all these issues: soft_patch.zip

Will do that asaic

[jdgleaver]

Ah, DOS 8.3 filenames! Yes, I hadn't thought of that...

Hmm... Well maybe, at least until we get the RetroArch DOS port up and running (it's not really usable at present), we can stick with the .ips1 naming? No currently supported platform will have problems with that :)

If we need to support DOS 8.3, then we should probably also support all the manifold naming schemes listed here: snes9xgit/snes9x#217 (comment) - but that represents quite a significant performance overhead....

[eadmaster]

we can stick with the .ips1 naming? No currently supported platform will have problems with that :)

At this point i prefer the .XXX.ips extension used by snes9x, which keeps the regular .ips extension.

[Sanaki]

If it supports up to 999 patches, at that point the popup would probably need to be modified to just say "Applied 327 patches" or whatever. Otherwise you run the potential of rolling notifications for literal hours.

[eadmaster]

update: I've added support for *.ipX and *.XXX.ips filenames with additional loops.

When combined this gives up to 8*3 patches support.

@Sanaki: @jdgleaver: i've noticed you started the patch index counter at 1, while in my original code it was 2 ( .ips and .ips1 have an ambiguous ordering for me, while it's obvious .ips2 comes second).

[Sanaki]

I merely reported the behavior shown by jdgleaver's patched code. That said, this iteration seems to need a touch more work. Just starting it up with the same four base patches I had before, this was the result:

[INFO] [CONTENT LOAD]: Loading content file: /usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).sfc.
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ips", attempting to patch ...
[INFO] Found IPS file in "/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ips3", attempting to patch ...
/usr/local/games/ROMs/MSU-1/Final Fantasy V/Final Fantasy V (Japan).ip2.001.ips
[INFO] [CONTENT LOAD]: CRC32: 0xd3824c42 .

Regardless of whether that naming scheme is still correct, that behavior is not.

[eadmaster]

I merely reported the behavior shown by jdgleaver's patched code.

Sorry, i've made confusion with the original author of the patch :-)

That said, this iteration seems to need a touch more work. Just starting it up with the same four base patches I had before, this was the result:

@Sanaki: shoud be fixed now...

[Sanaki]

Works now (ips, ips2, ips3, ips4), but I still get the odd .ip2.001.ips line in my output. Not sure what that one's about, or if it has any actual effect.

[eadmaster]

Works now (ips, ips2, ips3, ips4), but I still get the odd .ip2.001.ips line in my output. Not sure what that one's about, or if it has any actual effect.

yeah, sorry... forgot to restore the ips extension before the final loop...

[eadmaster]

Now i'm thinking it should be better to replace all the direct string manipulations with snprintf() calls for better readability.

In the meanwhile, if you have other improvements or fixes you should be able to send PRs here.

An improvement i'd like to see myself is partial filenames support like in the example above, which should be possible using retro_opendir+readdir(). But i guess this should be left for another PR...

[inactive123]

Merge conflict.

Also, I'd measure performance first of direct string manipulation vs. snprintf. If it's just a few trivial sections where we use direct string manipulation, that is probably worth it for the better performance.

[jdgleaver]

I don't like the way this PR is going...

With my diff applied, this was a simple solution that was ready to merge. Now the performance overheads are escalating, and *.XXX.ips format support is half-baked, since users will expect *.999.ips to work, and it doesn't. Moreover, there is a confusing inconsistency in the indexing - *.XXX.ips starts from 1, while the other formats start from 2.

If you want to support multiple formats like this, then serious thought needs to go into the efficiency of the whole process. path_is_valid() is slow on many platforms - and now we're calling it at least 9 times after the first patch is found. Nor is this cleanly extensible if we want to support additional naming schemes (and as soon as you bring snprintf() into the mix, it gets even worse). And as Sanaki points out, if we're supporting arbitrary files then there must be only one notification with the total number of patches applied.

I guess my point is that we either have a simple extension to the existing code (which is what was originally proposed), or the whole thing needs to be rewritten properly, and rigorously, with a new library for finding/applying patches and options for users to disable any functionality that unduly increases performance overheads.

[eadmaster]

... *.XXX.ips format support is half-baked, since users will expect *.999.ips to work, and it doesn't. Moreover, there is a confusing inconsistency in the indexing - *.XXX.ips starts from 1, while the other formats start from 2.

This can easily be fixed.

path_is_valid() is slow on many platforms - and now we're calling it at least 9 times after the first patch is found.

Why 9 times? It will break immediately after a patch is not found, so only 3 times if there are no other patches...

Nor is this cleanly extensible if we want to support additional naming schemes (and as soon as you bring snprintf() into the mix, it gets even worse).

Actually snprintf() shoud make the code more readable with less direct string manipulations, and so should be easier to add more naming schemes later.

And as Sanaki points out, if we're supporting arbitrary files then there must be only one notification with the total number of patches applied.

Ok, this definitively makes sense. Individual notifications are useful only with partial filenames matching.

EDIT: nevermind, i'm reverting my latest changes...

[Sanaki]

Build error on x86_64 Linux:

CC tasks/task_patch.c
distcc[12447] ERROR: compile tasks/task_patch.c on localhost failed
tasks/task_patch.c: In function ‘patch_content’:
tasks/task_patch.c:839:45: error: lvalue required as left operand of assignment
  839 |          name_ips_indexed[name_ips_len] - 1 = index_char;
      |                                             ^
tasks/task_patch.c:840:45: error: lvalue required as left operand of assignment
  840 |          name_bps_indexed[name_bps_len] - 1 = index_char;
      |                                             ^
tasks/task_patch.c:841:45: error: lvalue required as left operand of assignment
  841 |          name_ups_indexed[name_ups_len] - 1 = index_char;
      |                                             ^
Makefile:204: recipe for target 'obj-unix/release/tasks/task_patch.o' failed
make: *** [obj-unix/release/tasks/task_patch.o] Error 1

[eadmaster]

the more i use git the more i am hating it...

[eadmaster]

UPDATE: rebased the whole thing, pls check if i've skipped something...

@jdgleaver: maybe using a static buffer for the filenames is faster than using malloc?

[jdgleaver]

I'm struggling with a PR of my own right now, but I'll check this over properly as soon as I'm done. I can see a couple of very trivial things right away, though, so I'll leave some comments in the code.

@jdgleaver: maybe using a static buffer for the filenames is faster than using malloc?

It would be faster, but we have to be careful about allocating lots of large static arrays. These are problematic on platforms with limited stack space... (i.e. it can sometimes break the console ports entirely)

[jdgleaver]

@eadmaster The rebase has failed again....

[eadmaster]

@eadmaster The rebase has failed again....

Since this PR is touching various files currently it is likely to happen again and again. I think we should rendezvous at a specific time and date for the next rebase+merge.

[jdgleaver]

@eadmaster That's not how it works...

These problems are only occurring because you are not working in a feature branch. I already told you how to do this: libretro/gambatte-libretro#167 (comment) (start reading from For future reference, it's good practice to work in a feature branch....)

I'm not sure if the PR as it stands is even fixable now...

[eadmaster]

Do you think it may help if i move the new code into a branch and send another PR? (This one will be closed obviusly)

[jdgleaver]

Yes, that would be great - it should solve these merge issues quite effectively :)

[eadmaster]

New PR sent.

[i30817]

There are roms that have filenames with '.' before the extension. Will this break on those and consider the second to last '.' a 'patch marker'?

I'm also not surprised by the problems making you go back to hardpatches. RA is full of assumptions that the filename 'uniquely identifies' roms, even on surprising places or where that assumption makes even less sense because they strip the filename of the path and 'just assume' a user has no duplicate named rom on another path; mostly because checksumming larger roms is terribly slow and emu devs don't consider hardlinks/duplicates to be something they have to take into account and if they do they consider it a advantage (ie: to move the ROM and keep saves working).

RA really really needs a 'amortized checksum only' model as a option, but is unlikely to get it, since even zip is terrible for it, not having a internal data total checksum - because some 'roms' have more than one file - (well, one could be made by iterating over the checksums of the files in a defined order and checksumming that but that would break as soon as the user added a text file to the zip).

[eadmaster]

There are roms that have filenames with '.' before the extension. Will this break on those and consider the second to last '.' a 'patch marker'?

It won't break on those filenames with a "." inside.

I'm also not surprised by the problems making you go back to hardpatches. ...

After some experiments, i resorted to have multiple patched copies of the same base ROM vs a single base ROM + multiple patches:
Less files = less cluttering to manage.

In the end, i think it still doesn't hurt having multiple options to choose. Maybe soft-patching is a bit easier
to use than hard-patching, since you do not need a tool to actually apply the patch.

As for the checksums, there is a method to read the crc32 of the loaded content which could be used to have a stronger association with the saves. I still think the Mednafen solution is pretty solid and easy to implement, since it does not require any db, but should be optional/keep the current filename scheme as default. Btw, this should be moved to a separate issue...

[i30817]

The reason CRC is not used is that it's not amortized all the time (ie: outside zips) and even inside zips, RA has no clue which singular file is 'uniquely identifying' so it's just as likely to crc the .cue file of a iso* something which is not unique at all, and speaking of that, crc32 large file (not amortized) takes too long.

This would be fine if the method was done only for data which you're sure is amortized correctly (ie: a internal sha1 checksum of a chd, which checksums all of the real data), but it has failure points otherwise.

* note the uniquely identifying file of a iso dump can vary too, it's not always track 1. For instance dreamcast dumps don't store the executable on track 1 i think, which means that if you hack a dreamcast dump, RA could happily identify the 'game' as the original game if it tries to checksum 'track 1' of all isos (something which happens with nearly all hacks today anyway for the other reason that RA is using serial numbers to scan, which almost never change in hacks).

The database has other problems too. My current favorite is Adventure Game Studio games, in the scummvm database, where some games are using a configuration file as a checksum, a configuration file that the user already changed if they have any kind of interest in configuring their games. I didn't check the DOS database either, but i know there is at least one single file game there that changes the executable file (and only file) to 'save games' instead of creating separate files (Hook a old adventure game from Ocean).

[Sanaki]

Please continue this conversation in an issue if you want to propose changes. This isn't the place for it.

[andiandi13]

@eadmaster It'd be great if patches could be detected as long as it ends with .ips

That'd be a workaround to name our patches and recognize them.

e.g.

Castlevania - Harmony of Dissonance (USA).gba
Castlevania - Harmony of Dissonance (USA).ips-no-blue_outlines
Castlevania - Harmony of Dissonance (USA).ips1-music-overhaul

Every character after ips ip1 would be ignored

[Sanaki]

While much more relevant to the topic at hand, please open an issue with a reference to this PR if you'd like to discuss further improvements to it. The PR is already merged and bug-free, so conversation about future changes doesn't fit well here and won't be seen by any others who might be interested in the topic.

Also feel free to tag me if you do, I'm always interested in further improvements to the softpatching system.

[andiandi13]

@Sanaki I will thank you for the advice