Fix source of netplay desyncs

[N7Alpha]

- The cur_x/cur_y coordinates are clipped anyway in later code so they don't really need to be initialized to min_x/min_y
- The rational for using unsigned char is to avoid endian issues
- cur_x/cur_y need to be an int in the internal representation because they need to represent out-of-bounds potentially (for clipping purposes)

I found this issue because I was fuzz testing input for netplay code I was writing. The culprit was pretty straightforward. Some of the tracked input state by the core wasn't added to the savestate so this clearly would lead to desyncs. Specifically the worst offender was the A & B turbo buttons. With default settings there was a 2 in 3 chance a desync could happen. I moved that data into the serialize state, and haven't seen a desync since.

Kind of an unfortunate side-effect of fixing this is it makes savestates incompatible with the standalone nestopia emulator since it used to literally just call the procedure the emulator gave to create the savestate. I'm sure there might be unused/useless fields in the savestate you could try to pack the "tracked input state" data into if you wanted to so you didn't break compatibility. This doesn't really matter though so I never actually looked that far into it though.

[hizzlekizzle]

FWIW, I don't think the loss of savestate compatibility with the standalone program is a major issue.

[N7Alpha]

FWIW, I don't think the loss of savestate compatibility with the standalone program is a major issue.

Cool yeah that’s what I was thinking. I was just kind of surprised when I saw the code I thought there would be more synchronization of state instead of it just grabbing the savestate directly. It surprised me so much I thought it might have been intentional and I didn’t want to break it.

[carmiker]

The idea is correct, but the implementation could be improved.

The state format does not need to be changed, as there is already support for adding extended data. This can be seen in most of the mapper code. Just create a new section of the state for the libretro port using an ASCII ID of "LRO" perhaps. This way the states will remain compatible with other ports of the emulator, and stay in line with how the emulator was originally designed to function. There are many examples contained here, although you may need to do some black magic to instantiate the state saver/loader in libretro.cpp: https://github.com/libretro/nestopia/blob/master/source/core/board/NstBoardKaiser.cpp

Secondly, the way input polling is done in the libretro port is far less than ideal. Nestopia was designed to poll input via callbacks. The fact that you can access the internal input state and manipulate it is pure luck. The way it is done right now in the libretro port is to poll once per frame, testing what type of emulated input device is plugged in, and resetting the internal state, then populating it again using the frontend's input state data. The correct way to handle input is to create callbacks for each emulated input device type, and then associate each callback with its corresponding device. Games will strobe the input pins whenever they so choose, not necessarily once at the beginning/end of a frame -- and in order to handle some of the more exotic input devices, there is no choice but to use the callback method, because there are strobe segments which will expect different data to be returned to the core depending on the segment. By overhauling the way input is done in the libretro port, you may alleviate some of the issues, and would certainly pave the way to support more devices.

The correct reference is the win32 port. The Linux Nestopia UE port also does this like libretro, which is incorrect. The answers may be found here: https://github.com/0ldsk00l/nestopia/blob/master/source/win32/NstManagerInput.cpp

In summary, you need to "Connect" a controller to a port, which should have a callback associated with it. The emulator will strobe the pins when the game instructs it to. Any special information, such as strobe segment, will be passed in the callback. Implementing input in this manner should make the libretro port more readable, and also speed things up and potentially reduce input latency. For states, you can just add a new segment in the existing Nestopia state data format instead of appending raw data to the end, though you may find that by changing the way input is done, you don't need to save all of this libretro-specific data.

[N7Alpha]

The state format does not need to be changed, as there is already support for adding extended data. This can be seen in most of the mapper code. Just create a new section of the state for the libretro port using an ASCII ID of "LRO" perhaps. This way the states will remain compatible with other ports of the emulator, and stay in line with how the emulator was originally designed to function. There are many examples contained here, although you may need to do some black magic to instantiate the state saver/loader in libretro.cpp: https://github.com/libretro/nestopia/blob/master/source/core/board/NstBoardKaiser.cpp

If like you say this could be added as a new data segment to the savestate then I think that is worthwhile. So it sounds like I need to basically make a mock class that has the state I want to serialize and inherits from Board or some other class? If you could show me a minimal example with one field and the relevant entry points I need to call in nestopia's code I'd have a better idea of what to do. Basically I don't know what this means to do some black magic to instantiate the state saver/loader. Alternatively if you are a registered maintainer of this repo you could just commit the whole refactor as you see fit directly to my branch too.

Secondly, the way input polling is done in the libretro port is far less than ideal. Nestopia was designed to poll input via callbacks. The fact that you can access the internal input state and manipulate it is pure luck. The way it is done right now in the libretro port is to poll once per frame, testing what type of emulated input device is plugged in, and resetting the internal state, then populating it again using the frontend's input state data. The correct way to handle input is to create callbacks for each emulated input device type, and then associate each callback with its corresponding device. Games will strobe the input pins whenever they so choose, not necessarily once at the beginning/end of a frame -- and in order to handle some of the more exotic input devices, there is no choice but to use the callback method, because there are strobe segments which will expect different data to be returned to the core depending on the segment. By overhauling the way input is done in the libretro port, you may alleviate some of the issues, and would certainly pave the way to support more devices.

The correct reference is the win32 port. The Linux Nestopia UE port also does this like libretro, which is incorrect. The answers may be found here: https://github.com/0ldsk00l/nestopia/blob/master/source/win32/NstManagerInput.cpp

In summary, you need to "Connect" a controller to a port, which should have a callback associated with it. The emulator will strobe the pins when the game instructs it to. Any special information, such as strobe segment, will be passed in the callback. Implementing input in this manner should make the libretro port more readable, and also speed things up and potentially reduce input latency. For states, you can just add a new segment in the existing Nestopia state data format instead of appending raw data to the end, though you may find that by changing the way input is done, you don't need to save all of this libretro-specific data.

This sounds interesting and worthwhile, but this seems out of scope for this PR. Maybe move this to a new issue? Also I'd list the problematic games to test that depend on the callbacks working the way they do so the implementer would have a benchmark to work with.

[hizzlekizzle]

@carmiker had mentioned to me separately that the Arkanoid paddle controller would be affected by the change.

[carmiker]

I started writing some example code, but upon hex editing the uncompressed state file I discovered that RetroArch already encapsulates the state inside its own header, meaning it is already incompatible with the other downstream branches. Nobody has cared thus far, so I think this can indeed be considered a non-issue. The code can probably be merged as it stands now. However, I would still recommend rewriting the input to use the callback system, and perhaps some of the variables being added to the state for libretro will no longer be required.

@N7Alpha if you do want to do it using the Nestopia "chunk" system then here is the diff of my proof of concept -- note that the loading portion is not included because I stopped when I realized RA encapsulates the state:

diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp
index 47154ef..6387c9d 100644
--- a/libretro/libretro.cpp
+++ b/libretro/libretro.cpp
@@ -19,6 +19,7 @@
 #include "../source/core/api/NstApiFds.hpp"

 #include "../source/core/NstMachine.hpp"
+#include "../source/core/NstState.hpp"

 #include "nstdatabase.hpp"

@@ -1646,7 +1647,7 @@ size_t retro_serialize_size(void)
    std::stringstream ss;
    if (machine->SaveState(ss, Api::Machine::NO_COMPRESSION))
       return 0;
-   return ss.str().size();
+   return ss.str().size() + 3 + 8; // 3 extra bytes plus 8 for the "LRO" chunk
 }

 bool retro_serialize(void *data, size_t size)
@@ -1659,6 +1660,19 @@ bool retro_serialize(void *data, size_t size)
    if (state.size() > size)
       return false;

+   ss.str("");
+   ss.clear();
+
+   Core::State::Saver saver(&ss, false, false, false);
+   saver.Begin(Core::AsciiId<'L','R','O'>::V);
+   // Save 3 extra bytes of libretro-specific data
+   saver.Write8(0xff);
+   saver.Write8(0xfe);
+   saver.Write8(0xfd);
+   saver.End();
+
+   state += ss.str();
+
    std::copy(state.begin(), state.end(), reinterpret_cast<char*>(data));
    return true;
 }

[N7Alpha]

Thanks for providing some example code @carmiker. I looked into using the method you described and after about a half-hour I was still pretty confused where to start. I’d say just merge it as is since it sounds like this code will be removed or refactored once #81 is finished based on your description

[carmiker]

@N7Alpha if you have the chance to test the master branch again for netplay desyncs, we will know if switching to callbacks fixed the issue. If not, we can adjust your strategy to the latest code.

[N7Alpha]

@carmiker Cool thanks for working on this I was afraid it was forgotten about. Unfortunately I'm still getting desyncs. And it's exhibiting the same behavior as before where there the desyncs depend on the tstate variable having a 2/3 chance of not being the same when you load in a savestate. I just went a head and rebased the changes and fixed the conflicts. It looks like github didn't like that which I had kind of thought it was a security hole but they've let me do it before. In any case it seems it now doesn't desync again. I have not tested arkanoid or zapper roms yet though

In an effort to not spam the PR's you can just apply this commit to master and that's all.