rmtew / incursion-roguelike

The legendary computer game Incursion: Hall of the Goblin King!
https://incursion-roguelike.net/
Other
5 stars 1 forks source link

Better handle loading files (modules, save games) from different platforms and architectures #17

Open rmtew opened 4 weeks ago

rmtew commented 4 weeks ago

We need to give a clean intuitive user experience when a user loads a module or game which is not compatible. Serialised files are already fixed to a given release version and deserialised ones are checked to match. The Incursion serialisation mechanic however writes C++ objects to disk for both module loading/saving and game loading/saving. This memory layout is architecture specific which:

A bad file version gets a displayed "File Version Mismatch" error. A bad platform or architecture should display text along the lines of "File is for another operating system" and so on. Ideally we would revisit the whole mechanic of how these messages are displayed to the user and exceptions passed, and show the expected value and the actual value that is not usable.

This is a non-binding guess at the work involved:

HexDecimal commented 4 weeks ago

I thought the modules would be platform independent after they were compiled. What's the current error message?

rmtew commented 4 weeks ago

It might be fixable. I might have caused it or it might be some minor thing Julian never thought about.

image

rmtew commented 4 weeks ago

I'm guessing this isn't portable..

        /* ...reattach its virtual table pointer by calling
              a kludged, overridden new() operator in Object
              that "allocates" the space we've allocated, and
              then goes to a constructor that does nothing. */
rmtew commented 4 weeks ago

I take that back. I have no idea. I've stared at the serialisation code.

rmtew commented 4 weeks ago

Incursion seems to dump things from memory by the looks of it. So when it looks to see if the manually written Type in the serialisation stream matches the following object's embedded Type, it looks for the architecture-specific Type offset. And Type comes after the virtual table pointer.

image

image

So it probably is the virtual table pointer.

rmtew commented 4 weeks ago

This is what I get for the type size of the module object that is loaded for x64 and win32 respectively. This is more than just the offset of Type.

image

HexDecimal commented 4 weeks ago

I just ran into this myself while working on #4.

HexDecimal commented 4 weeks ago

Do saves still work across platforms? Because I assume those will break when moving between x86 and x64 if it's a simple memory dump. Might need to be more careful with pointers.

HexDecimal commented 4 weeks ago

At the very least I might need to modify the CI to bundle the mod based on the correct platforms.

rmtew commented 4 weeks ago

The module compilation as you did it is per-debug arch IIRC, so IIRC it should already be.

I think we should serialise the architecture in files we save, whether games or modules. they use the same mechanic. At the moment we detect corruption purely because someone decided that checking the deserialised type means something.

rmtew commented 4 weeks ago

Oh, I see, unless you're building debug for release, you can't be compiling the module for release builds.

HexDecimal commented 4 weeks ago

It currently shares the x86 mod, so the x64 build breaks. I've checked and confirmed this.

rmtew commented 4 weeks ago

Thanks for fixing CI. I've clarified the description and broken it down into some hopefully simple tasks. I don't think this is critical as the two signatures should ensure that corruption is detected. It's just a nice to have that no-one is asking for.

rmtew commented 4 weeks ago

Another point to consider is tying builds to commits for the purpose of compatibility. We wouldn't want to be in the situation where users compile their own builds from random commits for a given version and give the save files to us. Again this is in no way likely or critical, just a possible ideal.

HexDecimal commented 4 weeks ago

The issues with serialization are extensive. It's something I probably could've worked on if I didn't have to work with the compiled module file simultaneously.

rmtew commented 4 weeks ago

Those would be code maintainability issues though, not user visible problems, right?

HexDecimal commented 4 weeks ago

Not being able to open serialized data saved on a different platform is a visible problem. The current implementation is inherently unstable, making any minor change break backwards compatibility. For now users will have to assume that their saves are not forward compatible until further notice.

rmtew commented 4 weeks ago

I originally leant towards forward compatibility. But players didn't care about it at all. It was something that would have required a huge amount of work and complicated development, so I quickly aligned with the players on this. Not having to deal with this or preserve it makes development a lot simpler.