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

Various GPGX settings break savestates despite not being syncsettings #1167

Closed
NarryG opened this issue Mar 31, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@NarryG
Copy link
Contributor

commented Mar 31, 2018

Various settings in the GPGX core break savestates despite them not being flagged as SyncSettings

See the end of this message for details on invoking 
just-in-time (JIT) debugging instead of this dialog box.

************** Exception Text **************
System.InvalidOperationException: Memory consistency check failed.  Is this savestate from different SyncSettings?

The ones that appear to affect it are:
Audio Filter
Low Pass Range
Three band low cutoff
Three band high cutoff
Three band low gain
Three band mid gain
Three band high gain
BackdropColor

"Use custom backdrop color" doesn't affect the loading of savestates, but it appears to be saved inside the state regardless (turn it on, savestate, turn it off, loadstate, it'll be marked as "disabled" while drawing the custom bg color)

@vadosnaprimer

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

Which emu version?

@NarryG

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2018

Latest dev build. Seems like it has existed for a while though. Guessing it has been like this since the core was waterboxed

@vadosnaprimer

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

In that case it can't be "fixed", because it's how the whole waterbox concept works. It makes a snapshot of all the relevant memory the core occupies, as opposed to specific vars the core feels like putting to a savestate. This not only fixes non-determinism in some badly written savestates, but adds savestates to cores that don't have such ability to begin with!

tl;dr: This is by design.

@NarryG

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2018

It can be fixed as I fixed it in my local build. I just didn't submit it as a pull request as I wasn't sure about the whole Use Custom Backdrop Color issue (it doesn't break loading but still appears to be saved in the state regardless which seems contrary to the waterbox concept). These settings that break compatibility are all specifically exposed at core creation in ISettable through a function GetNativeSettings().

Moving the settings and that method into the SyncSetting block fixes the issue. As far as I can tell, these settings should have been marked as SyncSettings when the waterbox took place but weren't which is why I said it's existed since the waterbox occurred

@NarryG

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2018

@vadosnaprimer I've submitted #1170 which fixes the core issue here. I didn't touch "Use Custom Background Color" as I'm guessing that the color is baked into the savestate through a change in memory rather than an override in the core? Unsure. Either way, that setting doesn't break savestate consistency the way the others do.

@vadosnaprimer

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

Ah, alright. Maybe it should be done for more wbx cores.

@nattthebear

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

This isn't the first time this sort of thing has come up. I've fixed it in a few places in other cores as well. The ideal solution is to have the waterbox no longer track the non-sync savestate variable, but that can be difficult to accomplish.

@nattthebear

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

I'm willing to accept this change. If someone comes in with a specific use case where they need these settings to be not in syncsettings, then I'll look at making some changes in the native core, but I don't think anyone cares that much as it's just a minor annoyance.

@zeromus

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2018

Performing the analysis required to positively determine that a setting is free to be a non-sync setting is just the kind of troublesome analysis we would like to avoid. In fact, even reasoning that out as the original programmer can be trouble-prone. Doing this as-needed sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.