fix: RetroAchievements memory regions built too late, disabling achievements - #801
Open
apommel wants to merge 1 commit into
Open
fix: RetroAchievements memory regions built too late, disabling achievements#801apommel wants to merge 1 commit into
apommel wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a regression from #775 where some achievements, notably from GBC, stopped unlocking. Closes #800.
#775 aimed to use the console detection from rcheevos to improve RetroAchievements behavior on minarch to not rely exclusively on emulator tags, which allows custom emulator cores from Pak and emulator cores supporting several consoles, to work properly.
However I misunderstood the rcheevos process and I thought it was safe to initialize the memory regions in
ra_game_loaded_callback(), once the console has been identified, but it appears that at this point rcheevos has probed every memory address and permanently disabled achievements with any read error. I did not notice the issue because there was a fallback inra_read_memory(), which allowed the first memory region to work when it mapped to system RAM, and as I tested on SNES games, where this is the case, it seemed to work as expected. On Game Boy, the RAM was not the first memory region and achievements unlocks were therefore not evaluatedTo fix this issue while maintaining the fix, I moved the regions build to the first memory read instead. RetroArch does a similar thing: libretro/RetroArch/cheevos/cheevos.c#L1457.
In addition, when I looked at RetroArch source code I saw that they mention some cores only expose memory after the first
retro_run(). To handle that, ra_read_memory() now reports memory as readable while the game is still loading, so rcheevos doesn't outright disable achievements in this case, and RA_doFrame() retries the init every second while holding off rc_client_do_frame(). This should be an improvement over the previous behavior, which never retried. I did not test this part however, because RetroArch mentions Mupen64 that needs that, but it does not work in minarch (it needs to get the GL context).I tested the fix on Dragon Warrior III on GBC, with the achievement identified in #800. I also re-tested SNES achievements, and an achievement in a custom Pak (Genesis Plus GX, GPGX) in QuackShot. I did test in both online and offline modes.
Sorry about the regression, I should have tested on more systems in my previous PR.
I used Claude Opus 5 to help with understanding the issue and creating the fix.
@frysee @clintonium-119