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

Core: Add option to not report state change to SetState (bugfix) #12299

Merged
merged 2 commits into from Nov 22, 2023

Conversation

TryTwo
Copy link
Contributor

@TryTwo TryTwo commented Nov 16, 2023

bug: https://bugs.dolphin-emu.org/issues/13277

Frame advance was reporting the incorrect emulation state to prevent flickering UI updates, which was breaking debugger widgets and crashing dolphin. Replaced with the option to not report a state change on SetState().

Applied the new option in CodeDiff, because it's a similar quick, temporary state switch.

Breakpoints now work correctly, and playing from a breakpoint will finish the frame advance and then pause.

@iwubcode
Copy link
Contributor

What is causing the crash when CallOnStateChangedCallbacks is called? Why does not sending the callback fix things?

@TryTwo
Copy link
Contributor Author

TryTwo commented Nov 16, 2023

You probably understand most of it.
if (system.GetCPU().IsStepping() || s_frame_step) exists to stop the UI from flickering (fast pause -> play -> pause UI updates) on frame advance, but forces the incorrect state to be reported (forces Running to be seen as Paused during frame advanced). This was causing a crash in the debugger after, I think, recent guard safety updates (#11554)..

Since having an incorrect state be reported seemed to be the erroneous thing, I removed it and confirmed it fixed the crash. Then I checked the purpose of that and it was for the UI flicker. So, I tried to optionally prevent the UI update call on frame advance, and it stopped the flicker without causing any obvious issues. Also, preventing it reduces unnecessary update calls in the debugger, which is good.

So to answer your question, CallOnStateChangedCallbacks does not cause a crash, it was the if (system.GetCPU().IsStepping() || s_frame_step) crashing and the CallOnStateChangedCallbackschange change was to preserve what was intended with fixing flicker.

Another component of the crash could be UpdateDisasmDialog being unnecessarily called, but fixing the state fixes things.

@JosJuice
Copy link
Member

Seems like a reasonable approach, but I would like a comment about why we have the functionality to skip calling the state changed callbacks.

This was implemented to prevent UI flickering due to  the state rapidly switching between pause/play.  Recently, it has been causing issues with debugger windows, which update during frame advance.
Some state changes are meant to be near instantanoues, before switching to something else. By reporting ithe instant switch, the UI will flicker between states (pause/play button) and the debugger will unnecessarily update. Skipping the callback avoids these issues.
@TryTwo
Copy link
Contributor Author

TryTwo commented Nov 16, 2023

Split the commits. Added a comment, hopefully it's clear. Also, restored the callback for the last CodeDiff state that will persist at the end of the function.. It wasn't causing any issues, but for extra safety.

@iwubcode
Copy link
Contributor

Just wondering if it it'd be possible to have a flag in the debugger to ignore a state callback, instead of changing the SetState definition for only this one piece of functionality

@TryTwo
Copy link
Contributor Author

TryTwo commented Nov 16, 2023

I'm happy to try any other ideas, but not sure how that'd work. Toolbar, MemoryViewWidget, CodeWidget, and CodeViewWidget are the most affected.

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.

Personally I'm okay with the current state of the PR. Avoiding flicker is not something that's exclusive to the various debugger widgets – it also affects things like the Play/Pause button, and disabling/enabling checkboxes in config windows. I haven't been able to identify anything in the UI that should be switching back and forth rapidly when using frame advance, and the state changed callbacks are only used by the frontend.

One comment about the code: The way CodeDiffDialog.cpp is setting the state to paused looks like it has a time-of-check to time-of-use bug. Using CPUThreadGuard would be better. But let's leave that out of this PR, as it's a separate problem.

@AdmiralCurtiss AdmiralCurtiss merged commit 937cb8e into dolphin-emu:master Nov 22, 2023
11 checks passed
@TryTwo TryTwo deleted the PR_bugfix_frame_advance branch November 23, 2023 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants