Added MemoryInterface variables to savestates #11125
Conversation
Source/Core/Core/State.cpp
Outdated
| @@ -94,7 +94,7 @@ static size_t s_state_writes_in_queue; | |||
| static std::condition_variable s_state_write_queue_is_empty; | |||
|
|
|||
| // Don't forget to increase this after doing changes on the savestate system | |||
| constexpr u32 STATE_VERSION = 149; // Last changed in PR 10781 | |||
| constexpr u32 STATE_VERSION = 150; // Last changed in PR 11124 on October 6, 2022 | |||
There was a problem hiding this comment.
Specifying the date isin't particularly necessary given that the date is contained in the commit and the pr and so on, fwiw.
Also, wrong PR number btw.
There was a problem hiding this comment.
Yeah, I kind of got sniped there by Pokechu's PR, lol!
I just added the date because it was annoying me earlier when I wanted to figure out how long it had been since the savestate version was last modified by just looking at it in Visual Studio.
There was a problem hiding this comment.
Either way, I fixed it.
There was a problem hiding this comment.
I don't see why knowing the date would be useful. And anyway, you can already enable the Blame view in Visual Studio if you want to see the date the line was last changed.
There was a problem hiding this comment.
From the perspective of TASers trying to look into the Savestate code (and who don't have very much experience looking through the Dolphin code/using visual studio), it seems like it would be a helpful add-on. However, if this PR won't get merged with the date in it, then I can remove it.
There was a problem hiding this comment.
I think the issue with the date is that it reflects when the PR was made, not when it was merged, making it a bit less useful.
I would say the PR number itself is not that useful too, but it does have a benefit: two PRs that both change the version from 149 to 150 would not create a merge conflict after the first PR is merged, but with the PR number you will get a merge conflict, which is good because the second PR would need to instead change the number from 150 to 151. Including the PR number thus forces you to update the state version if another PR changed it and was merged.
You can use the blame view in GitHub's web UI too: https://github.com/dolphin-emu/dolphin/blame/master/Source/Core/Core/State.cpp#L96
There was a problem hiding this comment.
In that case, I'd better finish this PR up and get it merged tonight so that the date will be accurate in any event!
There was a problem hiding this comment.
Updated the PR to include initializer + shutdown method for MemoryInterface, and fixed the savestate version number (also removed the date).
|
Could you add in Init and Shutdown functions to MemoryInterface (and add calls to those?) The lack of any Init itself can theoretically cause non-determinism here. |
|
I'll add in those 2 functions later tonight. |
Done. |
Lobsterzelda
requested review from
Pokechu22,
JosJuice and
Zopolis4
and removed request for
Zopolis4,
Pokechu22 and
JosJuice
|
Please use |
Source/Core/Core/HW/HW.cpp
Outdated
| @@ -70,6 +72,7 @@ void Shutdown() | |||
| DSP::Shutdown(); | |||
| AddressSpace::Shutdown(); | |||
| Memory::Shutdown(); | |||
| MemoryInterface::Shutdown(); | |||
There was a problem hiding this comment.
The order is inconsistent between Init and Shutdown here. Please Shutdown in the reverse order as Init, so put this one between DSP and AddressSpace.
This PR adds MemoryInterface variables to save states. Hopefully, this will reduce the frequency of desyncs in Dolphin - although it remains to be seen what effect this will have.