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

Move a SetEnableAlert call to UICommon #11088

Merged
merged 2 commits into from Sep 29, 2022

Conversation

JosJuice
Copy link
Member

This fixes a problem where changing the Use Panic Handlers setting on Android wouldn't take effect until the app was restarted.

This fixes a problem where changing the Use Panic Handlers setting on
Android wouldn't take effect until the app was restarted.
Because of the previous commit, this is needed to stop DolphinQt from
forgetting that the user pressed ignore whenever any part of the config
is changed.

This commit also changes the behavior a bit on DolphinQt: "Ignore for
this session" now applies to the current emulation session instead of
the current Dolphin launch. This matches how it already worked on
Android, and is in my opinion better because it means the user won't
lose out on important panic alerts in a game becase they played another
game first that had repeated panic alerts that they wanted to ignore.

For Android, this commit isn't necessary, but it makes the code cleaner.
@@ -108,6 +110,12 @@ static void InitCustomPaths()
#endif
}

static void RefreshConfig()
{
Common::SetEnableAlert(Config::Get(Config::MAIN_USE_PANIC_HANDLERS));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this doesn't work like you intended it. If you have Use Panic Handlers on, then click 'ignore for this session' and then change any setting, it would re-enable the panic handlers. We probably need to split the current session boolean from the setting boolean here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what the second commit fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I see, you do this via the config layering system, I missed that. Makes sense.

@lioncash lioncash merged commit e3e6c3d into dolphin-emu:master Sep 29, 2022
11 checks passed
@JosJuice JosJuice deleted the uicommon-set-enable-alert branch September 29, 2022 16:10
@AdmiralCurtiss
Copy link
Contributor

I think this ends up deadlocking itself in Single Core because the call to Config::SetCurrent(Config::MAIN_USE_PANIC_HANDLERS, false); triggers a OnConfigChanged() which calls RunAsCPUThread() (in FreeLook) which calls PauseAndLock() which waits for the CPU thread to be available but the CPU thread is busy waiting for the GUI thread to return from the PanicAlert...

@AdmiralCurtiss
Copy link
Contributor

Ah, hold on, this is more nefarious. I was testing https://github.com/dolphin-emu/dolphin/pull/11119/files#diff-354a123faea2cf2907f8934821753bd6541a31399140f21e440767e64f662a51R403 this PanicAlert but that actually happens on the Vulkan CommandBufferManager SubmitThread, not the CPU thread.

@AdmiralCurtiss
Copy link
Contributor

Okay yeah, this is actually a deadlock between the CPU thread calling WaitForWorkerThreadIdle() (in Renderer::Swap()) and the waited-for worker thread calling PauseAndLock() (from the SetCurrent()). I hate panic alerts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants