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

Patches for Resident Evil 2/3 audio issues #9308

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

smurf3tte
Copy link
Contributor

These games are erroneously zeroing buffers before they can be fully copied to ARAM by DMA. The responsible memset() calls are followed by a call to DVDRead() which issues dcbi instructions that effectively cancel the memset() on real hardware. Because Dolphin lacks dcache emulation, the effects of the memset() calls are observed, which causes missing audio.

In a comment on the original bug, phire noted that the issue can be corrected by simply nop'ing out the offending memset() calls. Because the games dynamically load different .rel executables based on the character and/or language, the addresses of these calls can vary.

To deal generally with the problem of dynamic code being loaded to fixed, known addresses, the patch engine is extended to support conditional patches which require a match against a known value. This sort of thing is already achievable with Action Replay codes, but their use depends on enabling cheats globally in Dolphin, which is not a prerequisite shared by patches.

Patches are included for every region, character, and language combination.

The end result is an approximation of the games' behavior on real hardware without the associated complexity of proper dcache emulation.

https://bugs.dolphin-emu.org/issues/9840

@smurf3tte
Copy link
Contributor Author

I added two new strings when updating the patch editor UI. Let me know if anything else is required to make then properly localizable.

  auto* conditional = new QCheckBox(tr("Conditional"));
  auto* comparand_label = new QLabel(tr("Comparand:"));

@JosJuice
Copy link
Member

JosJuice commented Dec 4, 2020

What you've already done (adding a call to tr) is all you should do.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Looks good to me other than one comment, though I haven't looked at the DolphinQt part in detail.

Source/Core/Core/PatchEngine.cpp Outdated Show resolved Hide resolved
@BhaaLseN
Copy link
Member

BhaaLseN commented Dec 5, 2020

You're welcome to combine those commits down into one, since the 2nd commit doesn't make much sense in the resulting history. We try to do the commits right during the PR so we can refer to them later if necessary (instead of squashing during the merge).
Also, for future reference, you might be easier off using git commit --amend (or a similarly named switch in your GUI tool of choice) instead. Do note that this action will require a force push afterwards.


To squash those commits into one before continuing; you can use the following commands (replace upstream with the name of the Dolphin remote and origin with the name of your Fork remote):

git fetch upstream
git reset --soft upstream/master
git commit -m "your commit message here"
git push -f origin HEAD

The first command makes sure you get the latest commits from master (from the Dolphin repository) to work on; the second picks all your changes, switches to that master state and puts the changes in the staging area. The third command then commits it so you have one commit total that has all the changes (which you can also replace with your commit method of choice, such as git gui or any other GUI tool); and the forth command pushes your current branch into your own remote (which should be your Fork) while overwriting all previous changes (to update this PR).

@smurf3tte
Copy link
Contributor Author

I can give it a shot, but I'm a bit perplexed... how is that preferable to a squash merge on PR completion?

@BhaaLseN
Copy link
Member

BhaaLseN commented Dec 5, 2020

It makes no difference for your PR, but other (larger) ones would make it more difficult in finding problems after the fact when all we have is a single squashed commit (without the detailed info of every single commit).
Feel free to drop by on IRC if you need any help.

@JosJuice
Copy link
Member

JosJuice commented Dec 5, 2020

I can give it a shot, but I'm a bit perplexed... how is that preferable to a squash merge on PR completion?

The squash merge doesn't result in a merge commit getting added.

@smurf3tte
Copy link
Contributor Author

Now enabled by default.

@smurf3tte smurf3tte force-pushed the re23_patch branch 2 times, most recently from 7d12be6 to 533c66b Compare December 15, 2020 19:46
@smurf3tte
Copy link
Contributor Author

The last update to this PR fixed a pretty nasty heap corruption bug in the existing code. All it takes to reproduce is to add patch entries until the internal std::vector<> is reallocated, then go back and edit any of the entries that were already there. I can put that fix in a separate PR if there is still any hesitation about this change.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Code and approach LGTM, but I haven't tested the patches since I don't have any of these games.

@JMC47 If you haven't tested the patches, please do so. In particular, if Resident Evil 2 has some way to return to the main menu (even if it's only when you beat the game), please test that the patch continues working correctly after that.

These games are erroneously zeroing buffers before they can be fully copied to ARAM by DMA. The responsible memset() calls are followed by a call to DVDRead() which issues dcbi instructions that effectively cancel the memset() on real hardware. Because Dolphin lacks dcache emulation, the effects of the memset() calls are observed, which causes missing audio.

In a comment on the original bug, phire noted that the issue can be corrected by simply nop'ing out the offending memset() calls. Because the games dynamically load different .rel executables based on the character and/or language, the addresses of these calls can vary.

To deal generally with the problem of code being dynamically loaded to fixed, known addresses, the patch engine is extended to support conditional patches which require a match against a known value. This sort of thing is already achievable with Action Replay/Gecko codes, but their use depends on enabling cheats globally in Dolphin, which is not a prerequisite shared by patches.

Patches are included for every region, character, and language combination. They are enabled by default.

The end result is an approximation of the games' behavior on real hardware without the associated complexity of proper dcache emulation.

https://bugs.dolphin-emu.org/issues/9840
This code was storing references to patch entries which could move around in memory if a patch was erased from the middle of a vector or if the vector itself was reallocated. Instead, NewPatchDialog maintains a separate copy of the patch entries which are committed back to the patch if the user accepts the changes.
@JMC47
Copy link
Contributor

JMC47 commented Jan 5, 2021

Tested Resident Evil 2 NTSC/PAL/NTSC-J and swapped modes multiple times.
Tested Resident Evil 3 NTSC/PAL.

Note that in single core, Resident Evil 3 hangs after the first area, but that is unrelated to these codes. LGTM.

@leoetlino leoetlino added QA done The PR has been checked to work properly. and removed needs testing labels Jan 5, 2021
@JMC47
Copy link
Contributor

JMC47 commented Jan 5, 2021

Oh yeah, forgot to note - if these codes are used in older versions of Dolphins, they will make the game crash. Not sure if there's anything that can be done about it but is worth mentioning.

@leoetlino leoetlino merged commit 4cdcbb6 into dolphin-emu:master Jan 6, 2021
@smurf3tte
Copy link
Contributor Author

smurf3tte commented Apr 8, 2021

I just tested 5.0-13963 win10 x64 and sound cut off is there again. RE2 didn't test 3, should be same

I'm not hearing any issues on that build. A few questions:

  1. In the game list in Dolphin, if you right click the game, then Properties, then the Patches tab, is "Fix audio issues" checked?
  2. Are you hearing issues at the very start of the game, or do they only occur later?
  3. Is it possible you are experiencing a different audio issue? Does your audio sound like the RE2 example in this progress report?

@JosJuice
Copy link
Member

JosJuice commented Apr 8, 2021

Dolphin 5.0 wouldn't be appropriate to use for a comparison, since it contains the ARAM DMA timing hack which avoids the audio issue in these two games.

@smurf3tte
Copy link
Contributor Author

Good point. I noticed the progress report actually included a sample of the original bug, so I just linked that for comparison.

@smurf3tte
Copy link
Contributor Author

No worries! Glad that's all it was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA done The PR has been checked to work properly.
6 participants