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

Raw Memory Card Path handling unification and consistency fixes. #10587

Merged
merged 4 commits into from Jun 14, 2022

Conversation

AdmiralCurtiss
Copy link
Contributor

I've been aware for quite some time now that our handling of raw memory card paths is quite confusing and inconsistent. This should unify that into something sensible, hopefully breaking as few existing configurations as possible.

For details read the linked issue report, but in summary: Previously the code would behave sensibly if the path in the INI ended in .{region}.raw, and misbehave in various ways if it didn't. In particular, if the configured path contained no region specifier, the behavior depended on whether the file existed or not, which effectively picks a different file on the first run compared to subsequent ones. Changing the path in the GUI did not verify whether the path contained a region specifier, which could easily get you into a confusing situation.

Now the code will always pick a path containing a region specifier, regardless of how it's set in the INI or whether the file exists. Changing the path in the GUI now tells you to rename the card if it does not contain a region specifier.

(This is a slightly rewritten and partial #10379, which got massively stalled by people not agreeing about the form of GUI changes. As such, this contains no GUI changes except for a possible new error message.)

@AdmiralCurtiss
Copy link
Contributor Author

Someone review/test this please!

@JMC47
Copy link
Contributor

JMC47 commented Jun 5, 2022

This is definitely a good change, sometimes it's hard to know which Memory Card a game is using when not using GCI folders, which is why I usually recommend people use GCI folders when possible.

I've had to deal with a lot of support requests where users literally do not know where their memory card that actually has their savefiles is.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Other than my small nits, this LGTM. Untested, JMC said they'd do that!

@JMC47
Copy link
Contributor

JMC47 commented Jun 14, 2022

Works for me, tested and made sure my old AGP memory cards work, it would not let me use illegally named memory cards either.

@JMC47 JMC47 merged commit 9315ac7 into dolphin-emu:master Jun 14, 2022
10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the memcard-path-unify branch June 14, 2022 00:19
@Techjar
Copy link
Contributor

Techjar commented Jun 15, 2022

This broke mid-emulation memcard swaps.

const std::string old_eu_path = Config::GetMemcardPath(slot, DiscIO::Region::PAL);
if (eu_path != old_eu_path)
{
// ChangeDevice unplugs the device for 1 second, which means that games should notice that
// the path has changed and thus the memory card contents have changed
ExpansionInterface::ChangeDevice(slot, ExpansionInterface::EXIDeviceType::MemoryCard);
}
Copy link
Contributor

@Techjar Techjar Jun 15, 2022

Choose a reason for hiding this comment

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

This whole part seems... unfinished? Why is it only checking PAL? Explains swapping being broken, as I'm using USA memcards, unless I'm misunderstanding something here. Also if I'm not mistaken, this GetMemcardPath is being called too late, as you set the memcard path just above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it only checking PAL?

FWIW, I asked that question above and they put a comment, it's likely the same reason:

// The EU path is compared here, but it doesn't actually matter which one we compare since they
// follow a known pattern, so if the EU path matches the other match too and vice-versa.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. It was in a comment a bit above this block of code so I missed it. Seems I was right on the second point though.

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