dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

GraphicsSettings: EFBAccessEnable=false by default

Open dreamsyntax opened this issue 1 year ago • 3 comments

Makes "Graphics -> Hacks -> Skip EFB Access from CPU enabled by default.

Due to some drivers resulting in GPU stalls when EFB access occurs in games where EFB is not even used. Most games that require this setting set to 'true' (thus being disabled) already have this defined in their game inis.

image

dreamsyntax avatar Oct 20 '24 00:10 dreamsyntax

I am in favor of this change. Most forks of Dolphin already do this anyway and we know how much users love those.

JMC47 avatar Oct 20 '24 00:10 JMC47

I'm in favour of this change, however, by just flipping the bool in the code, this will only be applied to fresh Dolphin installs, existing users will have this turned off.

In the past we handled that by changing the INI key, perhaps we should do that too? It would also be a perfect opportunity to make the true/false value match the checked/unchecked status in the GUI...

mbc07 avatar Oct 20 '24 00:10 mbc07

I mean, I don't mind it not overriding old defaults. Not saying it has to be this way, but it is also the simplest way to go about it.

JMC47 avatar Oct 20 '24 00:10 JMC47

I'm in favour of this change, however, by just flipping the bool in the code, this will only be applied to fresh Dolphin installs, existing users will have this turned off.

If the user has never touched the setting, the new default will apply. Note that this was different in the past before we switched to the layered config system.

JosJuice avatar Oct 20 '24 06:10 JosJuice

You must also update the default value in the Android GUI: https://github.com/dolphin-emu/dolphin/blob/da530a2eb9d217821a0afcfb0478b62a2672112f/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/features/settings/model/BooleanSetting.kt#L792

JosJuice avatar Oct 20 '24 06:10 JosJuice

You must also update the default value in the Android GUI:

https://github.com/dolphin-emu/dolphin/blob/da530a2eb9d217821a0afcfb0478b62a2672112f/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/features/settings/model/BooleanSetting.kt#L792

Good catch, thanks.

dreamsyntax avatar Oct 20 '24 06:10 dreamsyntax

Any further thoughts on this? I think this is a slam dunk swap.

JMC47 avatar Oct 21 '24 19:10 JMC47

If we want this to be on by default, should we maybe also update the description to say "If unsure, leave this checked"?

Personally I'm leaning towards not renaming the INI key, especially to not break existing game configs.

JosJuice avatar Oct 22 '24 18:10 JosJuice

maybe also update the description to say "If unsure, leave this checked"?

Good catch, the tooltip should be updated

I'm leaning towards not renaming the INI key, especially to not break existing game configs.

Exceptionally in this case, that's desired, IMO

mbc07 avatar Oct 22 '24 18:10 mbc07

...update the description to say "If unsure, leave this checked"?

Done; Also installed Android and checked there were no such tooltips etc needing updating.

dreamsyntax avatar Oct 22 '24 19:10 dreamsyntax

There are 4 .ini files that specify EFBAccessEnable = false:

  • GT6 (Terminator 3: The Redemption)
  • GXB (SSX3)
  • RTH (Tony Hawk's Downhill Jam)
  • SNC (SONIC COLOURS)

Most of the commits that added those settings were many years ago, so I don't know if they're specifically needed anymore.

Dentomologist avatar Oct 22 '24 22:10 Dentomologist

There are 4 .ini files that specify EFBAccessEnable = false:

* GT6 (Terminator 3: The Redemption)

* GXB (SSX3)

* RTH (Tony Hawk's Downhill Jam)

* SNC (SONIC COLOURS)

Most of the commits that added those settings were many years ago, so I don't know if they're specifically needed anymore.

I'm not sure why they would be set to =false unless they were also bringing up the GPU hanging issue. It doesn't really make sense for that to be in the game inis.

Do we want to remove them in this PR, now that this is the default anyway?

dreamsyntax avatar Oct 23 '24 08:10 dreamsyntax

Sure, why not remove them.

I checked the history of the Sonic Colors INI, and we've had EFB access disabled for that game ever since the game INI system was added in 2009, without any explanation as far as I was able to find.

JosJuice avatar Oct 23 '24 16:10 JosJuice

Personally I'm against renaming the INI key, for what it's worth, because it would introduce a break for any user INIs that have forced the value one way or another. For sys INIs we could account for that, but not for user ones.

AdmiralCurtiss avatar Oct 23 '24 17:10 AdmiralCurtiss

Sure, why not remove them.

Does it make sense to delete a Sys\GameSettings ini entirely if there were no other changes?

Example: GXB.ini

# GXBE69, GXBP69 - SSX3

[Core]
# Values set here will override the main Dolphin settings.

[OnFrame]
# Add memory patches to be applied every frame here.

[ActionReplay]
# Add action replay cheats here.

[Video_Hacks]
EFBAccessEnable = False

Or should I just remove the Video_Hacks section entirely, but keeping the empty ini? I checked and other inis omit the section if not used.

dreamsyntax avatar Oct 23 '24 18:10 dreamsyntax

INIs like this, that only contain sections and comments without any actual settings, should be deleted.

JosJuice avatar Oct 23 '24 19:10 JosJuice

Pushed, wasn't sure if it made sense to have that in the same commit; Let me know if it should be squashed.

dreamsyntax avatar Oct 24 '24 01:10 dreamsyntax

Let's try this out.

JMC47 avatar Oct 28 '24 22:10 JMC47