ekeeke / Genesis-Plus-GX

An enhanced port of Genesis Plus - accurate & portable Sega 8/16 bit emulator
Other
697 stars 198 forks source link

State Load/Save Refactor #548

Closed SergioMartin86 closed 6 months ago

SergioMartin86 commented 6 months ago

This PR aims to do the following:

Secondary goals:

Motivation:

What I observed:

For casual play, the current state load/save functions are mostly fine. However, for TASing we require that a savestate stores the full state of the system and, when loaded, it recovers it fully without side-effect or inaccuracies. Unfortunately, I have detected that the current version in upstream desyncs. To prove this, I created GPGX Compare, a bespoke tool that runs pre-recorded movies using GPGX in two possible modes:

To determine whether a desync has happened I use two elements:

What I did:

Moved all global variables into the state.h and state.c files and created alternative implementation of the save and load state procedures to faithfully capture the entire state of the system (except for ROM, pointers, and read-only allocations).

Tests performed:

I made a selection of several games and some playaround movies. I call the upstream version Base, and this PR's vesion New. I test the Simple and Rerecord cycles for both.

Base Simple anotherWorld base simple SHA1: 0x39D83AEEBDB2389D6F8DAFD7128A4ABF

Base Rerecord anotherWorld base rerecord SHA1: 0x306EA9F2FBE7F0F85FB6A290B518E175 (desync)

New Simple anotherWorld new simple SHA1: 0x39D83AEEBDB2389D6F8DAFD7128A4ABF

New Rerecord anotherWorld new rerecord SHA1: 0x39D83AEEBDB2389D6F8DAFD7128A4ABF

Base Simple ecco base simple SHA1: 0x795EA2F64D49FC895C690ABCAF8F46D8

Base Rerecord ecco base rerecord (slight offset) SHA1: 0x795EA2F64D49FC895C690ABCAF8F46D8

New Simple ecco new simple SHA1: 0x795EA2F64D49FC895C690ABCAF8F46D8

New Rerecord ecco new rerecord SHA1: 0x795EA2F64D49FC895C690ABCAF8F46D8

Base Simple princeOfPersia base simple SHA1: 0xA0C392C59E6B0A5AAAD0804DAEB434CC

Base Rerecord princeOfPersia base rerecord SHA1: 0x3D60CD0D23DC02865048B14EFD1E2AF9 (desync)

New Simple princeOfPersia new simple SHA1: 0xA0C392C59E6B0A5AAAD0804DAEB434CC

New Rerecord princeOfPersia new rerecord SHA1: 0xA0C392C59E6B0A5AAAD0804DAEB434CC

Base Simple virtuaRacing base simple SHA1: 0xCC1D15C62A0771567AE6BAD596D0BE74

Base Rerecord virtuaRacing base rerecord SHA1: 0x34CB91F58EFA1697A3565BFE2B9091E7 (desync)

New Simple image SHA1: 0xCC1D15C62A0771567AE6BAD596D0BE74

New Rerecord virtuaRacing new rerecord SHA1: 0xCC1D15C62A0771567AE6BAD596D0BE74

Pending Work

Thanks for considering this PR and hope it contributes to improve the core

ekeeke commented 6 months ago

Sorry, I understand there have been a lot of work into this pull request but I'm not going to merge this for several reasons.

First is that I don't like the idea of totally revamping the whole codebase and design to move every context load/save methods and global data into a single code module. I think a better approach would have been to identify the root causes of the desync issues you have observed with TAS recordings and eventually fix them individually (most likely by adding missing data to the saved context as indeed the current context is designed for loading/saving state files from an emulator frontend which interrupts emulation every 'frame' and therefore don't need to save context which is reinitialized or have known fixed state at the start of a frame).

Secondly, even if I understand this is still work in progress, it seems that, at this stage, this would have significant impact on existing features that are considered important by many users (like loading/saving state files while playing game or runahead and netplay features in retroarch) while the only benefits (again, from what I understand) are for Bizhawk TAS users, which already uses a heavily modified (and afaik quite outdated) version of GPGX core. Even if you plan to integrate this in your own TAS-dedicated emulator, those are changes that do not really benefit to traditional emulator users (i.e gamers), which are the main target of this emulator. That does not mean you are not encouraged to develop your own emulator, based on GPGX core, that aims at more specific features like TASing or debugging but I do not see good reasons to merge heavy design changes made solely because of these features into main core repository,.

Lastly, I think the type and enums refactoring is not really needed in the context of this pull request and I generally don't agree with purely cosmetical changes that mostly depend on random developers preference or coding habits.Surely it is not perfect or validated by traditional coding standards but it works and is what I, as main (or more exactly sole) developer of this emulator core, am used to. If there are compilation warnings or errors that you think should be fixed, this should be made in a specific pull request or idsue report, so it is considered specifically.

Thank you for reading ;-)

SergioMartin86 commented 6 months ago

Gotcha, thanks for taking a look!