ISettable<...> PutSyncSettings exists

[Morilli]

On discord, a problem came up related to sync settings and how changing them via the settings dialog caused an exception due to those changed sync settings being used even though the core has not been restarted yet.

The problem here is that setting sync settings immediately sets them in the running core instance via PutSyncSettings. The core is then both expected to return those sync settings from GetSyncSettings (so it NEEDS to store them) but also not change its active sync settings because, well, they're sync settings and changing them in the middle of running the emulator could cause anything from desync to outright crashes, as seen above.

Now I'm left wondering: Is there ever a valid scenario where this function needs to exist? If a core is always expected to not use the passed in sync settings because otherwise it might crash, what's the point in passing sync settings to the core in the first place? It is not intuitive to me that this structure is expected and I expect that my wip branch for mupen would also fail due to this design as it's using sync settings outside of the ctor.

The documentation for this function helpfully notes that sync settings should never be changed while recording, but does that mean there's an expectation for trying to change them while NOT recording?

BizHawk/src/BizHawk.Emulation.Common/Interfaces/Services/ISettable.cs

Line 50 in 152b3ad

/// changes the movie-sync relevant settings. THIS SHOULD NEVER BE CALLED WHILE RECORDING

Also, I feel like every sensible implementation will require a reboot for sync settings change, so I don't know how useful the return value for that function is.

[vadosnaprimer]

I agree, anything that can change on the fly will just be kept in non-sync settings, which usually don't need a reboot. But if a sync setting does need a reboot, it doesn't make sense to live-update that setting before we've rebooted.

A workaround is to cache initial sync settings during init and then only use that for any case when the core needs to read them. But then why is live syncsettings even there? It's there for the cure to decide things based on it. I don't think it has to be disallowed for the core to ever read syncsettings after init.

[nattthebear]

I think the API makes sense and is easy to fulfill the post-conditions for. If your core can accept a sync settings change on the fly, then do that and return PutSettingsDirtyBits.None. (Many cores, notably waterbox, can't do this, but it's possible and I think a larger portion of our cores could do this in the past.) If your core can't accept a sync settings change on the fly, then don't apply the update and return PutSettingsDirtyBits.RebootCore.

If anything, GetSyncSettings is the more confusing method, because if the core has refused to apply the update now it has two sets of sync settings, but the API doesn't distinguish between those. I believe cores are supposed to always return the latest thing they were passed; if they're in a non-applied reboot-needed case, they maintain the current actual settings as an extra shadow object that's no longer accessible publicly. But I'm not certain everything actually does that.