dolphin
dolphin copied to clipboard
dolphin-emu
Fix SYSCONF movie settings
The movie config layer is not active for recording, only playback. Thus, recording ends up stuck with default SYSCONF settings. The fix somewhat hacky, but seems to work.
This fixes https://bugs.dolphin-emu.org/issues/12822
I'm not convinced your first change actually does anything? It already returns a nonconst pointer and I see no place that could possibly produce a copy.
Also you probably need to explain the second one in more detail, how exactly does that fix the issue? e: Is that even safe? The generated loader (and layer) stores a pointer to the passed header, which would go out of scope at the end of BeginRecordingInput().
I'm not convinced your first change actually does anything? It already returns a nonconst pointer and I see no place that could possibly produce a copy.
Yep, the explanation seems a bit dubious to me. A const iterator (an iterator that is const) and a const_iterator are not the same thing, and the same overload of begin/end is being called regardless of the constness of the iterator variable in which you store the result of find_if.
I'm not convinced your first change actually does anything? It already returns a nonconst pointer and I see no place that could possibly produce a copy.
It's UB. You aren't allowed to modify the object from a const_iterator. I'm just saying the behavior I observed from this (I assume this is some "protection" against modification).
Also you probably need to explain the second one in more detail, how exactly does that fix the issue? e: Is that even safe? The generated loader (and layer) stores a pointer to the passed header, which would go out of scope at the end of
BeginRecordingInput().
The header is only read once on construction from what I saw. I could change it to just use tmpHeader (or maybe just make this instance static there).
It fixes the issue as it now stores the relevant sysconf settings to the movie layer, allowing it to pass the later predicate used for setting sysconf: https://github.com/dolphin-emu/dolphin/blob/128fa8aec3738ab76075836780f7724a02d87f42/Source/Core/Core/BootManager.cpp#L162
I'm not convinced your first change actually does anything? It already returns a nonconst pointer and I see no place that could possibly produce a copy.
Yep, the explanation seems a bit dubious to me. A const iterator (an iterator that is const) and a const_iterator are not the same thing, and the same overload of begin/end is being called regardless of the constness of the iterator variable in which you store the result of find_if.
The type is const std::vector<Entry>::const_iterator. This can plainly be seen in a debugger. You are not allowed to modify the object from a const_iterator.
The type is const std::vector::const_iterator. This can plainly be seen in a debugger. You are not allowed to modify the object from a const_iterator.
But there's no reason why find_if would be returning a const_iterator. Besides, if it was a const_iterator, evaluating &*iterator would result in a const SysConf::Entry*, which would cause a compilation error since the return type of the function is declared as SysConf::Entry*.
For what it's worth, the inline type display in VS2022 shows it as iterator and not const_iterator for me.
That seems weird, my setup shows it as const iterator when it's const auto, not iterator.
From what I can see to, it doesn't evaluate as const SysConf::Entry*, rather it evaluates to SysConf::Entry* and of course doesn't cause any errors for compiling.
It says ::iterator right there in the second line, what are you talking about??
It says
::iteratorright there in the second line, what are you talking about??
Yes, when it's auto it's iterator, as the above function (that this PR modifies to be just auto rather than const auto) has. The below function is const auto, which has const_iterator.
Again, that doesn't make any sense. Whether the member function is const qualified is what determines whether the const overload of begin/end gets called (and whether you get const_iterator or not). Const qualifying the variable does absolutely nothing and definitely does not change an iterator into a const_iterator. Note the underscore; those are different types.
Ok, seems to go to iterator adding back the const auto. Weird, not sure what I was seeing then. Apologies then, I'll just revert that change back.
Regarding the now remaining change: Doesn't this make it so that the "placeholder" header you're creating will override the user's settings?
The "placeholder" header only contains settings the DTM would normally have and is filled with current settings at the time of starting the movie (with the ConfigLoaders::SaveToDTM(&header);). It should be similar to playing back a movie, although the settings would just be taken from the current active user settings rather than an actual file.
Ah, so SaveToDTM doesn't actually save to a DTM file, it saves to the header you passed in. Yes, that makes sense then.
Personally I'm not happy with more global state, and I also don't really understand why this works -- which also means I can't tell if this breaks anything.
fwiw since I think our TAS setup logic needs a big overhaul anyway (like, in terms of letting the user configure what settings they want to start with) and since you're part of the TASing community (I think so, yeah?) if you're sure this works we can merge for now, probably...
It works because the SYSCONF settings when recording a movie try to take them from the movie config layer specifically, and uses "default" settings otherwise (not sure how this is decided exactly, but moot anyways). This is intended so SYSCONF is set only with movie settings and other settings not exposed in the DTM go to predictable defaults rather than whatever is set otherwise.
This movie config layer is only set when playing back a movie, not recording one, so this ends up breaking when recording a movie. Language is hit hard since "default" here means "English" even for e.g. Japanese games, which can just cause the game to immediately crash. Other SYSCONF settings are also affected but probably don't have the same impact language has here?
This PR simply actually adds the movie config layer when recording a movie (also proceeds to remove it once the movie is done playing/recording, as it shouldn't be needed anymore).
I'm still not 100% happy here but since this is well documented as a hack and very tiny, sure, whatever.