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

Disable ICache emulation for some games #8937

Merged
merged 1 commit into from Apr 6, 2021

Conversation

Pokechu22
Copy link
Contributor

@Pokechu22 Pokechu22 commented Jul 5, 2020

Specifically, 'Scooby-Doo! Mystery Mayhem' (Bug 9561) and 'Ed, Edd n Eddy: The Mis-Edventures'.

For Scooby-Doo!, I tested by loading the credits under the extras menu. Prior to this change, it would fail to load; now, it loads successfully (even on exiting and re-entering the credits). I haven't tested much actual gameplay. I was also unable to confirm that there was ever an issue with loading save files; saving and loading at the first camera works fine in 5.0-12247.

For Ed, Edd n Eddy, the fix is only meaningful when combined with #8935; see #8935 (comment). (EDIT: It also is only necessary when MMU is disabled; the icache issue doesn't come up with MMU enabled.)

There may be other games that need this fix (for instance, I see Bug 10177 for Scooby Doo! Unmasked). I don't own that game, so I don't know if it's a similar issue.

@Pokechu22
Copy link
Contributor Author

The new configuration value was modeled based on existing code for AccurateNaNs. I didn't add an equivalent of the netplay-related line, though:

StartUp.bAccurateNaNs = netplay_settings.m_AccurateNaNs;

I don't know if I need to do something like that (for one, both of the affected games are singleplayer).

@JosJuice
Copy link
Member

JosJuice commented Jul 5, 2020

It would be good to do so for completeness (if someone decides to use the setting with a multiplayer game in the future).

@Pokechu22
Copy link
Contributor Author

Done, though I'm not sure if there's some kind of protocol version constant I should update.

@hthh
Copy link

hthh commented Jul 6, 2020

Superficially this looks like a hack that decreases accuracy to get a game working, but reading the explanation in the issue (that "this is related to JIT caching causing the emulated icache to not get invalidated properly") it probably makes sense... Maybe that explanation could be added as a comment or to the commit message? Would it make sense to ignore this flag on (uncached) Interpreter so that that mode doesn't become less accurate?

@Pokechu22
Copy link
Contributor Author

It's definitely a hack, but I think it's overall more accurate for these games (icache definitely shouldn't be triggering here at least). A more true fix would be to update icache when entering JIT blocks, but I looked into doing that in the past and didn't see a reasonable way of implementing it.

How about "The JIT cache causes problems with emulatied icache invalidation in this game resulting in areas failing to load" as a comment in the game config?

Would it make sense to ignore this flag on (uncached) Interpreter so that that mode doesn't become less accurate?

That would probably be a good idea accuracy-wise, but I'm not sure if it's a good idea to add more checks here. I feel like ReadInstruction is pretty hot code.

@JosJuice
Copy link
Member

JosJuice commented Jul 6, 2020

Done, though I'm not sure if there's some kind of protocol version constant I should update.

No. The Dolphin version fetched from git acts as the protocol version.

@orbea
Copy link
Contributor

orbea commented Jul 6, 2020

It's definitely a hack, but I think it's overall more accurate for these games (icache definitely shouldn't be triggering here at least). A more true fix would be to update icache when entering JIT blocks, but I looked into doing that in the past and didn't see a reasonable way of implementing it.

While I understand the desire to get games working, a hack likely could make it that a more correct fix is never implemented.

@JMC47
Copy link
Contributor

JMC47 commented Jul 6, 2020

Even if proper icache emulation were ever finished, it'd likely be a big enough performance penalty that we'd need a hack like this anyway.

@orbea
Copy link
Contributor

orbea commented Jul 6, 2020

At least proper icache emulation would exist then.

@JMC47
Copy link
Contributor

JMC47 commented Jul 6, 2020

I'm saying in the case of icache emulation, they may be mutually exclusive. This hack doesn't fix all cases, as the GameCube BIOS still needs proper icache emulation for disc swaps to work.

@hthh
Copy link

hthh commented Jul 6, 2020

Yeah, I'm not a fan of hacks, but I'm in favour of this change. It doesn't decrease accuracy, just adjusts behavior that's already inaccurate for performance reasons. Proper (or at least proper enough for these games) icache emulation already exists in the interpreter, so we're not losing out in that respect.

That comment sounds good to me.

(And although I'd imagine another well-predicted branch in ReadInstruction wouldn't have a measurable performance impact, I guess it's probably good to leave it disabled in interpreter anyway, just to keep the name simple and have it do what it says.)

@JMC47
Copy link
Contributor

JMC47 commented Sep 7, 2020

I'd rather enable MMU emulation than disable icache emulation for Ed, Edd, and Eddy. Unfortunately, I don't think that's an option for Android, so we may have to add the hack regardless.

@JMC47
Copy link
Contributor

JMC47 commented Sep 14, 2020

Can this be rebased so that the disc timing fixes are in for testing further into Ed, Edd, and Eddy?

@JMC47
Copy link
Contributor

JMC47 commented Sep 14, 2020

@SirMangler made a rebased test build for someone playing Ed, Edd, and Eddy and it got some further testing.

Apparently enabling MMU is a really large performance issue for this game even on non-Android and there are more crashes with the icache later in the game. I'm now in favor of the icache hack for Ed, Edd and Eddy.

@JMC47
Copy link
Contributor

JMC47 commented Mar 3, 2021

So, here's my thoughts on this hack in general. I relate it most closely to the dcache hack we employ for the Disney Infinity and Disney's Cars 2 malicious anti-emulation code. While these games aren't doing anything malicious, it appears they have the same bug that is manifesting in a similar way.

Similarly to the dcache lowmem hack, this is a very small footprint in the code and is able to handle multiple games. It also is immediately descriptive of what it is doing when you look in the INI based on the option name.

While it's always a bit annoying to have to use a hack, it's likely dolphin never sees icache emulation accurate enough to fix all of these issues while maintaining decent performance. And if it does, the fact this is one localized setting makes it extremely easy to see what games would be fixed and compare performance in them.

Note that this isn't a catch all for icache issues, and does not fix other games with more complicated icache issues, such as 1080 Snowboarding.

@Miksel12
Copy link
Contributor

Happy Feet on the Wii is also fixed by disabling ICache: https://bugs.dolphin-emu.org/issues/12415

@Pokechu22
Copy link
Contributor Author

It probably would be worth checking if Scooby Doo: Unmasked is also fixed by this (https://bugs.dolphin-emu.org/issues/10177), but I don't plan on buying a second Scooby Doo game I'll never play just to check.

@JMC47
Copy link
Contributor

JMC47 commented Mar 24, 2021

I'll check on it.

@JMC47
Copy link
Contributor

JMC47 commented Apr 6, 2021

I beat the cookie factory in the issue report without experiencing a crash. Though, I kind of skipped some of the talking in the tutorial because this game has some interesting collision detection and I found it fun to see how far I could get it. I do not think Scooby Doo Unmasked crashes anymore.

Specifically, 'Scooby-Doo! Mystery Mayhem', 'Scooby-Doo! Unmasked', 'Ed, Edd n Eddy: The Mis-Edventures', and the Wii version of 'Happy Feet'.

The JIT cache causes problems with emulated icache invalidation in these games, resulting in areas failing to load.
@Pokechu22
Copy link
Contributor Author

I've added this hack to the GameINIs for Scooby-Doo! Unmasked and the Wii version of Happy Feet.

@leoetlino leoetlino merged commit 2c537e3 into dolphin-emu:master Apr 6, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants