-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Memcard: Add tracking for when memory cards are busy writing #10372
Conversation
Attempted to cover all entry points for VM shutdowns/resets/states which could be possible danger points for memcards. Try to find ways to fool the system into allowing you to load a state, save a state, shutdown, reset, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There probably should be two variants of the warning - one from within Qt (like you've done here), and another that uses ImGuiFullscreen's confirm prompt, for if the shutdown is initiated from Big Picture. Locking the VM and exiting fullscreen still tends to be problematic on some platforms, and janky at best.
pcsx2-qt/MainWindow.cpp
Outdated
@@ -2730,6 +2784,22 @@ void MainWindow::doDiscChange(CDVD_SourceType source, const QString& path) | |||
{ | |||
const auto lock = pauseAndLockVM(); | |||
|
|||
// Check if memcard is busy, deny request if so | |||
if (!GSDumpReplayer::IsReplayingDump() && MemcardBusy::IsBusy()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be extracted out into a function - don't duplicate it 3 times.
pcsx2-qt/MainWindow.cpp
Outdated
VMLock lock(pauseAndLockVM()); | ||
|
||
QMessageBox msgbox(lock.getDialogParent()); | ||
msgbox.setIcon(QMessageBox::Warning); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to QMessageBox::question(...)
.
pcsx2/SIO/Sio.cpp
Outdated
// Decremented once per frame if nonzero, indicates how many more frames must pass before | ||
// memcards are considered "no longer being written to". Used as a way to detect if it is | ||
// unsafe to shutdown the VM due to memcard access. | ||
size_t MemcardBusy::currentTicks = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not accessed outside of these functions, so it can be static/file-local.
@stenzek Code quality issues should be addressed on the existing bits. Also added for big picture. I did however end up doing more duplication, but that is because I could not for the life of me figure out how to pass a function reference and then have it also survive down to the lambda callback. Since shutdown takes a bool param and the others do not I tried templating it and for whatever reason I could not get anything to compile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, untested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pcsx2-qt/MainWindow.cpp
Outdated
const QMessageBox::StandardButton res = QMessageBox::question( | ||
lock.getDialogParent(), | ||
tr("WARNING: Memory Card Busy"), | ||
tr("WARNING: Your memory card is still writing data. Shutting down now will IRREVERSIBLY DESTORY YOUR MEMORY CARD. It is strongly recommended to resume your game and let it finish writing to your memory card.\n\nDo you wish to shutdown anyways and IRREVERSIBLY DESTROY YOUR MEMORY CARD?") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tates aware of memcard busy status
f2055b1
to
5138162
Compare
Description of Changes
Adds a counter, set to 300 frames when a write operation is sent to a memory card. Every frame, the counter is decremented. On shutdown, the counter is cleared.
Also moved memory card auto eject countdown function out to Counters.cpp. It will run at the same frequency, it's just now it's not stuck in CDVD anymore, it's in Counters.cpp which makes more sense.
Rationale behind Changes
New MemcardBusy::IsBusy() function can be used by the frontend and/or emu thread to detect if memcard activity has happened within the last 300 frames. That can then be used to determine whether to continue with or abort shutdowns, restarts, disc swaps, savestates, etc.
I attempted to make the frontend changes but quickly realized I did not understand how anything worked. I am going to either need assistance or pass that off to someone who does.Suggested Testing Steps
MAKE NEW MEMCARDS. DO NOT PUT MEMCARDS YOU CARE ABOUT ANYWHERE NEAR THIS.
Boot a game, get it to a point where it will write something to the memcard. Purposely try to shut it down, reset it, load a savestate, while it is writing to the memcard. You should be blocked from completing your "desired" action.