HostFileSystem: Set all NAND folders to be saved in save states

[Lobsterzelda]

This change makes all files and folders in NAND be recursively saved/loaded in save states. More specifically, when a movie is being created/played back, the full contents of the Dolphin Emulator/WiiSession folder will be saved/restored recursively.

[CasualPokePlayer]

Suggested change

	std::string Path = BuildFilename("/").host_path;
	std::string Path = BuildFilename(Core::WantsDeterminism() ? "/" : "/tmp").host_path;

This could be done to only do a full state when recording a movie (where this would be most desired and least problematic).

[JosJuice]

While the "handle /tmp" comment should be changed, I don't think your suggested replacement is good. The user directory is only called "Dolphin Emulator" on Windows, and even then, only if you're not using portable.txt. I also think listing the paths at all is overly much detail (though this is more subjective, and I'm willing to be overruled).

[Lobsterzelda]

When the user is playing/recording a movie, the path that "/" corresponds to is "Dolphin Emulator/WiiSession". Otherwise, the path that "/" corresponds to is "Dolphin Emulator/Wii".

[Lobsterzelda]

This could be done to only do a full state when recording a movie (where this would be most desired and least problematic).

Updated my PR to include your suggested changes.

[mbc07]

Um, do files outside /tmp (maybe the game's save folder and accompanying DLC, if applicable?) has any influence on movie playback? I'm asking because this PR is proposing storing up to 512 MB of additional data on a save state, which could make saving/loading them in the worst scenario (full NAND) exponentially longer than it already is...

[CasualPokePlayer]

Um, do files outside /tmp (maybe the game's save folder and accompanying DLC, if applicable?) has any influence on movie playback?

Yes https://bugs.dolphin-emu.org/issues/7425

Blank NAND for a TAS should not be an issue (most of the time), casual use however should probably just keep to the tmp folder to prevent giant states.

[mbc07]

The issue suggests that only the game's save data might cause desyncs. Storing the game's save in addition to /tmp still is a significantly less overhead than storing the entire NAND unconditionally, as is currently proposed, and I don't think that's a good idea. This definitely should be made optional, or restricted to movie recording only...

[JosJuice]

Storing the game's save in addition to /tmp still is a significantly less overhead than storing the entire NAND unconditionally, as is currently proposed, and I don't think that's a good idea.

When you're TASing, the temporary NAND only contains /tmp, the game's save, and some tiny files.

This definitely should be made optional, or restricted to movie recording only...

Absolutely. I think it would make most sense to do it for movies only, as is already the case with memory cards:

dolphin/Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp

Line 499 in dd3db94

bool storeContents = (Movie::IsMovieActive());

[AdmiralCurtiss]

Yeah, don't do this unconditionally, you don't want to revert the entire NAND of anyone loading an old savestate. This should be restricted to movie-only, like the memory card code.

You may also want to be careful here when mixing modes, ie loading a state made when movie was active in movie-inactive mode and vice-versa.

[JMC47]

I'd be afraid of this getting triggered on netplay too.

[Lobsterzelda]

I'd be afraid of this getting triggered on netplay too.

So, do you want the full NAND to only be saved when a TAS is being recorded or played back?

[JosJuice]

I think you should make it like the memory card code I linked above: The full NAND should be savestated if a movie was active at the time the state was saved.

[Lobsterzelda]

I think you should make it like the memory card code I linked above: The full NAND should be savestated if a movie was active at the time the state was saved.

The downside of that is that if you load a savestate made in a movie by accident when you're outside of record mode, then all of your NAND save files will be deleted.

[JosJuice]

Yes, that's a valid point. But what would be the correct way to handle it? Read everything from the savestate, but only store the files that are in /tmp?

[mbc07]

I would add the game save file to this...

[JosJuice]

I would add the game save file to this...

To the files restored when saving during a movie and loading not during a movie?

[mbc07]

Yep, (saving/restoring the game save file unconditionally) but on a second thought, I figured this can cause issues. Like the loading of an older save state could end overwriting a newer in-game save...

[Lobsterzelda]

Here is the current logic/pseudocode for the savestates that I implemented, which I think is probably the best way to go about doing this. I'll describe the order that things are saved first, and then I'll describe how they're loaded:

SAVE ORDER:

1. p.do(Movie::IsMovieActive());
2. DoStateWrite(p, "/tmp");
3. p.do(nandSaveStateSize) // nandSaveStateSize is how large the information saved below is, or 0 if it isn't saved.
4. if ( Movie::IsMovie::Active()) then DoStateWrite(p, "/")

---

LOAD ORDER:

1. bool saveStateMadeDuringMovie = p.do();
2. DoStateRead(p, "/tmp");
3. if (saveStateMadeDuringMovie != Movie::IsMovieActive()) then p.doExternal(nandSaveStateSize); // skips past the save state copy of the rest of NAND. This value will be 0 if the rest of NAND wasn't saved to begin with - in which case it will just read the value for nandSaveStateSize and nothing else.
4. Else if (Movie::IsMovieActive()) then DoStateRead(p, "/");

[AdmiralCurtiss]

I really don't like how you have an extra implementation of practically the same code now that needs to be kept in sync... Can you not reserve 8 bytes or whatever in the output stream and then fill those in later?

[AdmiralCurtiss]

Wait, what the heck are you even doing with the nandSaveStateOffset? Are you expecting the first call in measure mode to set this global variable and the second call to use it??? That's extremely ugly.

[Lobsterzelda]

I really don't like how you have an extra implementation of practically the same code now that needs to be kept in sync... Can you not reserve 8 bytes or whatever in the output stream and then fill those in later?

Which part are you referring to? Are you talking about how I have a DoStateMeasure() and a DoStateWrite() method?

[Lobsterzelda]

Wait, what the heck are you even doing with the nandSaveStateOffset? Are you expecting the first call in measure mode to set this global variable and the second call to use it??? That's extremely ugly.

That is correct. It seems like the best option - since otherwise, there's no way to skip over this data when loading a savestate in the opposite movie playback mode (i.e. loading a savestate made during a movie recording while outside of a movie).

[AdmiralCurtiss]

Which part are you referring to? Are you talking about how I have a DoStateMeasure() and a DoStateWrite() method?

Yes, exactly. I feel like this could just as well be written as something like (this might need an extra function or two in PointerWrap):

if (not read mode) {
  u64 length = 0;
  auto length_pos = p.GetPosition();
  p.Do(length); // reserve bytes for size
  auto start_pos = p.GetPosition();
  // write all file data here
  auto end_pos = p.GetPosition();
  length = end_pos - start_pos;
  p.SetPosition(length_pos);
  p.Do(length);
  p.SetPosition(end_pos);
}

Maybe this can even be done with DoExternal() without adding any position get/set stuff, actually?

[Lobsterzelda]

Which part are you referring to? Are you talking about how I have a DoStateMeasure() and a DoStateWrite() method?

Yes, exactly. I feel like this could just as well be written as something like (this might need an extra function or two in PointerWrap):

if (not read mode) {
  u64 length = 0;
  auto length_pos = p.GetPosition();
  p.Do(length); // reserve bytes for size
  auto start_pos = p.GetPosition();
  // write all file data here
  auto end_pos = p.GetPosition();
  length = end_pos - start_pos;
  p.SetPosition(length_pos);
  p.Do(length);
  p.SetPosition(end_pos);
}

Maybe this can even be done with DoExternal() without adding any position get/set stuff, actually?

To me, jumping back and forth in the PointerWrap seems a lot more sloppy than just having a global variable. Come to think of it, I don't think there's any savestate code which writes some values, jumps back to an earlier point, and then jumps forwards.

However, DoExternal() allocates space for an int, and then jumps forward however much space you specified - followed by returning a pointer to where the pointer was immediately after allocating space for the int. Based on that, I could do something like this:

if ( NOT in_read_mode ) {
u8* startPointer = p.doExternal(0);
//Write/Measure file NAND here.
u8* endPointer = p.doExternal(0);
if ( in_write_mode) {
*(startPointer - sizeof(u32)) = (u32) (endPointer - startPointer - sizeof(u32));
}
}

However, this also looks really sloppy/bad. Adding 2 functions like the following would probably be better, with the first function being called at the start of HostFileSystem::DoState(), and the 2nd one being called at the end of the function to get the length:

u8* PointerWrap::getPositionAndDoInt(u32& val) {
u8* prevPointer = m_curr_ptr;
Do(val);
return prevPointer;
}

u32 PointerWrap::getOffsetFromPreviousPosition(u8* prevPointer) {
return (u32) m_curr_ptr - prevPointer - sizeof(u32);
}

The user would then be responsible for writing the value of this int to where the pointer was at the appropriate time, like this:

u8* prevPointer = p.getPositionAndDoInt(nandSize);
... //write/measure NAND.
nandSize = p.getOffsetFromPreviousPosition(prevPointer);
((u32) prevPointer) = nandSize;

[Lobsterzelda]

Could also make everything be internal to PointerWrap to keep everything cleaner by making the 2nd function be like this:

u32 PointerWrap::doOffsetAtPreviousPosition(u8* prevPointer) {
*((u32*)prevPointer) = (u32) (m_curr_ptr - prevPointer - sizeof(u32));
}

[AdmiralCurtiss]

Okay, sorry I was a bit snippy about this yesterday.

Global variables are a bad idea because they introduce invisible state. They make it a lot harder to reason about the actual output of any given function call (since every global variable is effectively an extra parameter to the function that is not obvious at the call site), and they make multithreading a lot more complicated if not outright impossible. Consider what would happen with your nandSaveStateOffset variable here if two separate threads called the function at the same time.

In this specific case you're also introducing a dependency that a Measure call of DoState() has to happen right before the Write call, or otherwise the Write call does the wrong thing. Just because that currently is always the case doesn't mean that that's a thing you should rely on.

Anyway, yes, the variant with grabbing a pointer and filling it in later is a lot better, though you should memcpy into the memory instead of type punning.

[Lobsterzelda]

I updated my code to reflect the suggestions in this thread. I think the PR should be better now.

[AdmiralCurtiss]

And lastly: Our variables are snake_case, not camelCase. Please adjust all of them.

[AdmiralCurtiss]

The logic seems mostly good to me now, stylistically I have a few problems still.

[AdmiralCurtiss]

Yes, this looks good to me now. Untested though.

[Lobsterzelda]

Yes, this looks good to me now. Untested though.

For what it's worth, I tested it in the following cases:

1. Making a savestate while recording a movie, and then loading the savestate while still in recording mode.
2. Making a savestate while not recording a movie, and then loading the savestate while still not recording.
3. Making a savestate while recording a movie, and then loading the savestate while not in recording mode.
4. Making a savestate while not recording a movie, and then loading the svaestate while in recording mode.

Each of these cases worked as expected. Additionally, I verified that the contents of the "/tmp" directory are always saved/loaded no matter what. However, the contents of the "/" directory (recursively) are only loaded when the savestate was made while recording a movie and the savestate was loaded while still in recording mode.

[AdmiralCurtiss]

Hm, I'm not sure this is your fault, but there's still something odd happening here. If you:

• Make state in Movie mode.
• Exit movie mode and go to normal emulation.
• Load that state.
• Load that state again.

It overwrites the global NAND with the savestate one.

I think what's happening is that the first load enables movie mode, but doesn't re-point the configured NAND. Not ideal...

[AdmiralCurtiss]

Hm the more I think about this, this is extremely fishy; basically all checks for active movie while savestating are invalid because it can re-set this value at the end of the state load. I wonder if this is related to our desync problem! Basically, if you start emulation and load a movie state exactly once before saving, you end up with inconsistent data, because the first load didn't eg. load the memory card data correctly.

Sigh.

[Lobsterzelda]

Hm the more I think about this, this is extremely fishy; basically all checks for active movie while savestating are invalid because it can re-set this value at the end of the state load

Sigh.

I wonder if I could fix this by just checking what the path of the NAND folder currently is, and seeing if it's equal to the permanent NAND folder location or the temp location. Idk where/to what extent the user can modify the default values of "Wii" and "WiiSession" for permanent and temp NAND respectively, though...

[Lobsterzelda]

Well, the solution I came up with is pretty ugly, but I think that this should solve all of the issues mentioned above in this thread - test out this new code to see if it solves the issue of NAND being overwritten.

[AdmiralCurtiss]

Yeah, the WiiRootIsTemporary() is a good check here, if a bit of a sledgehammer. We can think about how we solve the more fundamental issue of switching in and out of recording mode on state load in another PR.

[AdmiralCurtiss]

We might need to return to this later once we have cleared up the underlying issues, but for now this is fine.