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

HW/Memmap: Refuse to load savestate if memory settings are different. #10591

Merged
merged 1 commit into from May 8, 2022

Conversation

AdmiralCurtiss
Copy link
Contributor

This fixes ugly error messages (and potentially crashes, the state read logic doesn't seem to check if it reads past the end of the buffer...) when you try to load a GameCube savestate with MMU enabled when MMU is off and vice-versa.

@AdmiralCurtiss
Copy link
Contributor Author

These kinda errors:
stateload

@shuffle2
Copy link
Contributor

fwiw if you(/everyone) rebase your PRs when you submit them, much more likely that the windows buildbot will finish quickly

@AdmiralCurtiss
Copy link
Contributor Author

This is based on current master!

Though you're not wrong, I have also observed the Windows buildbot (and even my local builds) often recompiling lots of files for no obvious reason. I wonder if our VS projects are misconfigured? (We really should switch to CMake on Windows one of these days.)

@shuffle2
Copy link
Contributor

shuffle2 commented Apr 18, 2022

It happens because windows buildbots dont have a compilation cache in place (other bots use ccache). So the only cache effect is if the build can reuse objects in the build directory from the previous build. the msbuild files are intentionally created such that Base.props holds all settings, which is great in the normal case - but as soon as Base.props is changed, it causes all objects to be recompiled. Therefor if you have the builders jumping between commits with different Base.props it causes it to rebuild everything each time.

Making a cache work on windows (buildbot, at least) is on my list of things to do, but it turns out all existing solutions are not very good.

btw, another reason the build has been slower recently is that the DolphinTool drags in almost all of Core stuff (because of the Config system, apparently), and a lot of time is spent in the linking stage. In the future I will probably submit a PR to merge DolphinTool functionality into main dolphin binary because it really isn't lightweight or a standalone thing

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.

Untested but LGTM

@AdmiralCurtiss
Copy link
Contributor Author

Made the user-visible message a bit less cryptic, no changes otherwise.

// If we're loading a savestate and any of the above differs between the savestate and the current
// state, cancel the load. This is technically possible to support but would require a bunch of
// reinitialization of things that depend on these.
if (current_ram_size != state_ram_size || current_l1_cache_size != state_l1_cache_size ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be easier to read this way:

// I created intermediate variables for clarity
const auto current_settings = std::tie(current_ram_size, current_l1_cache_size, current_have_fake_vmem, current_fake_vmem_size, current_have_exram, current_exram_size);
const auto state_settings = std::tie(state_ram_size, state_l1_cache_size, state_have_fake_vmem, state_fake_vmem_size, state_have_exram, state_exram_size);
if (!(current_settings == state_settings))  // != for tuple is removed? in C++20

Copy link
Member

@lioncash lioncash Apr 19, 2022

Choose a reason for hiding this comment

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

tuple's operator!= still exists in C++20, it's synthesized from the use of the <=> comparison operator in tuple's interface (which makes all of the manual comparison implementations obsolete. If they actually removed operator!=, that would break a ton of code

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. That was the part I didn't understand correctly, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that's slightly more readable? Don't really mind either way.

@JosJuice JosJuice merged commit 299d5c0 into dolphin-emu:master May 8, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants