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

Automatic ROM Patching #75

Merged
merged 2 commits into from Jul 14, 2022
Merged

Automatic ROM Patching #75

merged 2 commits into from Jul 14, 2022

Conversation

spitzeqc
Copy link
Contributor

@spitzeqc spitzeqc commented Jul 1, 2022

Added support for automatic ROM patching for IPS and UPS patches

ROMs can now have patches applied if ".ips" or ".ups" is present. Currently this is achieved by running "patchRom()" within the fixRomPadding function. This is done to allow for patches to be applied before mirroring. Currently autodetecting of saves does not work if the rom gets patched as detectSaveType is run after loadGbaRom, and by then the hash has changed. This could be fixed by either adjusting the logic of oafInitAndRun and loadGbaRom to load rom into memory, detect save type, patch, and (if needed) mirror, or by adding patched rom data to the save database (I believe the latter is a better option as it would also work for pre-patched roms, and it requires less code to be rewritten).

Future revisions/improvements could add patch options in game-specific settings, and possibly optimizing the patch function as it is a bit on the slow side currently (~15 seconds to apply the mother3 translation patch)

@profi200
Copy link
Owner

profi200 commented Jul 2, 2022

Honestly in the current state i can't merge this. We need to bring down the patch time significantly. 15 seconds is a lot. I see you are using calloc() for very small buffers in many places. This can be changed to stack buffers. And i see you do fRead() calls with very small size. I would cache the patch data for fast access (let's say 512 bytes of cache).

And the save type detection is also a problem. We could assume by default the patched ROM uses the same save type as the original but not sure how reliable this is.

@spitzeqc
Copy link
Contributor Author

spitzeqc commented Jul 3, 2022

I have gotten ups patch time down to ~2 seconds for mother3 translation, and smaller ups patches seem to load almost instantly.

As for the save detection issue, I'm not sure what the best course of action should be. Moving the patch function to after the save type is detected works (current method of latest push), but I fear any patches applied to classic nes roms (and other roms that get mirrored) may not patch properly. To be fair, my quick search for classic nes patches only brings up emulation/anti-piracy patches so this may not be a real issue in practice. But if there are patches for mirrored roms, the logic would need to be changed (assuming the mirroring messes up patch offsets) unless an alternative solution is found.

EDIT:
further testing has reveled that the latest push is not functioning properly ATM, so dont merge

EDIT 2:
it appears the rom does patch properly. Perhaps ram did not fully clear before patching, causing issues? (rom does not take all 32MB)

@profi200
Copy link
Owner

profi200 commented Jul 4, 2022

What is not functioning properly?

@spitzeqc
Copy link
Contributor Author

spitzeqc commented Jul 4, 2022

Patching Tomato Adventure with a ups translation patch (converted from bps) created distortion on the title screen. However patching was functioning properly before, and after powering off and waiting a minute or so it worked properly again which is why i believe it is ram related. I have not been able to reproduce the issue. Further investigation may be needed, but at present patching appears to be functioning properly

Copy link
Owner

@profi200 profi200 left a comment

Choose a reason for hiding this comment

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

I also don't know how i feel about the corruption you mentioned. It sounds to me like some edge case while parsing the patch file. I don't know these patch file formats at all and would like to make sure they are parsed correctly and corrupted patch files don't crash.

source/arm11/open_agb_firm.c Outdated Show resolved Hide resolved
source/arm11/open_agb_firm.c Outdated Show resolved Hide resolved
source/arm11/open_agb_firm.c Outdated Show resolved Hide resolved
source/arm11/patch.c Outdated Show resolved Hide resolved
source/arm11/patch.c Outdated Show resolved Hide resolved
source/arm11/patch.c Outdated Show resolved Hide resolved
source/arm11/patch.c Outdated Show resolved Hide resolved
@spitzeqc
Copy link
Contributor Author

spitzeqc commented Jul 9, 2022

I have begun testing with classic nes series crack patches (these were the only ones I could find). So far, the Dr. Mario, Metroid, and Icd Climbers patches match their prepatched counterparts without adjusting fixRomPadding()'s logic (though neither autopatched or prepatched versions run properly, I'm getting "Game Pak Error" for both). More testing is needed to confirm however, as these patches may only need to patch data that does not get moved around.

It may be a good idea to distribute a prerelease version (with debug output) with the latest commits to see if patching is truly working for classic nes games, along with gathering a larger patch sample size

@profi200
Copy link
Owner

The reason it's showing this error is because there is something wrong with the save type. These specific games are working using the save type database. Possibly the AP patches patch these to SRAM saving or some other nonsense.

Copy link
Owner

@profi200 profi200 left a comment

Choose a reason for hiding this comment

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

I will merge this for now but i might change or rewrite parts of it to solve the remaining issues and improve speed further.

edit:
Actually i can't merge due to conflicts. Maybe your fork is outdated?

@profi200
Copy link
Owner

There are still conflicts. It looks like you are trying to PR commits which already got merged (the backlight stuff). No idea how this happened.

@spitzeqc
Copy link
Contributor Author

Ok, I think I have fixed the issue with the backlight merge stuff

@profi200 profi200 merged commit 834c0ff into profi200:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants