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

ActionReplay: Fix implementation of memory-copy zero codes #6105

Merged
merged 2 commits into from Oct 11, 2017

Conversation

6 participants
@CyberShadow
Contributor

CyberShadow commented Oct 6, 2017

This fixes the implementation of the non-standard size-3 zero codes to conform to kenobi's specification.

static bool ZeroCode_MemoryCopy(const u32 val_last, const ARAddr& addr, const u32 data)
{
const u32 addr_dest = val_last | 0x06000000;
const u32 addr_dest = val_last & ~0x06000000;

This comment has been minimized.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

This bug must have been due to a misinterpretation of the spec. The 0x06000000 mask needs to be applied by the user, not the implementation (which needs to remove it).

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

This bug must have been due to a misinterpretation of the spec. The 0x06000000 mask needs to be applied by the user, not the implementation (which needs to remove it).

This comment has been minimized.

@BhaaLseN

BhaaLseN Oct 6, 2017

Member

Feel free to leave this as a comment above the code snippet (explaining why the mask is here, not that the code is removing the mask which we can see from the code itself).
Right now it is either guesswork or knowing that such a spec exists; but someone with neither cannot understand (or reason about) why this is happening here.

@BhaaLseN

BhaaLseN Oct 6, 2017

Member

Feel free to leave this as a comment above the code snippet (explaining why the mask is here, not that the code is removing the mask which we can see from the code itself).
Right now it is either guesswork or knowing that such a spec exists; but someone with neither cannot understand (or reason about) why this is happening here.

This comment has been minimized.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

Yep, I added a comment above explaining and linking to the spec. A comment here would just be redundant, since it's implied that the bits removed here are not part of the address because they are part of the control bits which activate this command. There is a similar operation in the fill-and-slide code implementation.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

Yep, I added a comment above explaining and linking to the spec. A comment here would just be redundant, since it's implied that the bits removed here are not part of the address because they are part of the control bits which activate this command. There is a similar operation in the fill-and-slide code implementation.

Show outdated Hide outdated Source/Core/Core/ActionReplay.cpp
@@ -598,13 +600,13 @@ static bool ZeroCode_MemoryCopy(const u32 val_last, const ARAddr& addr, const u3
LogInfo("Src Address: %08x", addr_src);
LogInfo("Size: %08x", num_bytes);
if ((data & ~0x7FFF) == 0x0000)
if ((data & 0xFF0000) == 0)

This comment has been minimized.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

This, too, was due to a misinterpretation of the spec. Furthermore, this check made the "Memory Copy With Pointers Support" branch completely unreachable.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

This, too, was due to a misinterpretation of the spec. Furthermore, this check made the "Memory Copy With Pointers Support" branch completely unreachable.

ActionReplay: Fix implementation of memory-copy zero codes
This fixes the implementation of the non-standard size-3 zero codes to
conform to kenobi's specification.
{
PowerPC::HostWrite_U8(PowerPC::HostRead_U8(addr_src + i), addr_dest + i);
LogInfo("Wrote %08x to address %08x", PowerPC::HostRead_U8(addr_src + i), addr_dest + i);

This comment has been minimized.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

As it happens, other than the misguided hard-coded 138 count mentioned above, the "Memory Copy With Pointers Support" was actually identical to the other branch.

@CyberShadow

CyberShadow Oct 6, 2017

Contributor

As it happens, other than the misguided hard-coded 138 count mentioned above, the "Memory Copy With Pointers Support" was actually identical to the other branch.

@CyberShadow

This comment has been minimized.

Show comment
Hide comment
@CyberShadow

CyberShadow Oct 6, 2017

Contributor

Thanks for approving the CI.

I would like to play with this for a bit and make sure it works as expected, so perhaps hold off merging for now.

Contributor

CyberShadow commented Oct 6, 2017

Thanks for approving the CI.

I would like to play with this for a bit and make sure it works as expected, so perhaps hold off merging for now.

ActionReplay: Fix logging in Subtype_AddCode
The addition was being repeated (for logging) after it was committed
to memory, thus causing bogus values to appear in the log.
@CyberShadow

This comment has been minimized.

Show comment
Hide comment
@CyberShadow

CyberShadow Oct 6, 2017

Contributor

OK, seems to work nicely!

My use case is to allow selecting an item directly, bypassing the item wheel, with one keyboard keypress in LoZ:TP. The code shouldn't allow selecting items the player doesn't have, so it copies stuff from the player's inventory (hence the need for indirect read). I also took advantage of indirect write to add a shift button which selects items into the Y-slot instead of the X-slot. My implementation is here: https://gist.github.com/CyberShadow/02da7df8322400840f8de83d95d6f2ed. I'll put the code on the wiki once this is merged.

Contributor

CyberShadow commented Oct 6, 2017

OK, seems to work nicely!

My use case is to allow selecting an item directly, bypassing the item wheel, with one keyboard keypress in LoZ:TP. The code shouldn't allow selecting items the player doesn't have, so it copies stuff from the player's inventory (hence the need for indirect read). I also took advantage of indirect write to add a shift button which selects items into the Y-slot instead of the X-slot. My implementation is here: https://gist.github.com/CyberShadow/02da7df8322400840f8de83d95d6f2ed. I'll put the code on the wiki once this is merged.

@mbc07

This comment has been minimized.

Show comment
Hide comment
@mbc07

mbc07 Oct 6, 2017

Contributor

I'll put the code on the wiki once this is merged.

FYI, on the wiki we only accept AR codes to remove FPS cap, specific widescreen codes (4:3 => 16:9 or vice-versa) and codes that overcome emulation bugs. For anything else you should post in the forums instead...

Contributor

mbc07 commented Oct 6, 2017

I'll put the code on the wiki once this is merged.

FYI, on the wiki we only accept AR codes to remove FPS cap, specific widescreen codes (4:3 => 16:9 or vice-versa) and codes that overcome emulation bugs. For anything else you should post in the forums instead...

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Oct 6, 2017

Contributor

Do we allow codes to be added directly to INIs still?

Contributor

JMC47 commented Oct 6, 2017

Do we allow codes to be added directly to INIs still?

@gamemasterplc

This comment has been minimized.

Show comment
Hide comment
@gamemasterplc

gamemasterplc Oct 6, 2017

Contributor

yes, and we have before

Contributor

gamemasterplc commented Oct 6, 2017

yes, and we have before

@CyberShadow

This comment has been minimized.

Show comment
Hide comment
@CyberShadow

CyberShadow Oct 6, 2017

Contributor

FYI, on the wiki we only accept AR codes to remove FPS cap, specific widescreen codes (4:3 => 16:9 or vice-versa) and codes that overcome emulation bugs.

Thanks for the heads up. My reasoning for putting it on the wiki is that the code is insufficient by itself, it needs to be accompanied by a non-trivial controller profile.

As a user, I would find AR codes that are specific to emulation in general useful on the wiki, i.e. things that don't make sense on a real device. That would be a superset of the criteria you mentioned.

Contributor

CyberShadow commented Oct 6, 2017

FYI, on the wiki we only accept AR codes to remove FPS cap, specific widescreen codes (4:3 => 16:9 or vice-versa) and codes that overcome emulation bugs.

Thanks for the heads up. My reasoning for putting it on the wiki is that the code is insufficient by itself, it needs to be accompanied by a non-trivial controller profile.

As a user, I would find AR codes that are specific to emulation in general useful on the wiki, i.e. things that don't make sense on a real device. That would be a superset of the criteria you mentioned.

@leoetlino leoetlino merged commit 1c14881 into dolphin-emu:master Oct 11, 2017

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment