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

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

Open
5 tasks
rmtew opened this issue Aug 19, 2024 · 18 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rmtew
Copy link
Owner

rmtew commented Aug 19, 2024

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:

  • Ties the correctness of a compiled module to the Incursion executable that built it.
  • Ties the correctness of a saved game to the Incursion executable that saved it.

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:

  • Defines.h should select appropriate platform and architecture values when compiling.
  • Registry.cpp:fileHeader should have fields for the platform and architecture.
  • Modify deserialisation to check file header for correct platform and architecture.
  • Modify serialisation to write identified platform and architecture in file header.
  • Display clear, intuitive, useful messages to the user.
@HexDecimal
Copy link
Collaborator

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

@rmtew
Copy link
Owner Author

rmtew commented Aug 19, 2024

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

image

@rmtew
Copy link
Owner Author

rmtew commented Aug 19, 2024

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
Copy link
Owner Author

rmtew commented Aug 19, 2024

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

@rmtew
Copy link
Owner Author

rmtew commented Aug 19, 2024

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
Copy link
Owner Author

rmtew commented Aug 19, 2024

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
Copy link
Collaborator

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

@HexDecimal
Copy link
Collaborator

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
Copy link
Collaborator

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

@rmtew
Copy link
Owner Author

rmtew commented Aug 19, 2024

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
Copy link
Owner Author

rmtew commented Aug 19, 2024

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

@HexDecimal
Copy link
Collaborator

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

@rmtew rmtew changed the title Loading a module compiled for another architecture unhelpfully talks about corruptness Better handle loading files (modules, save games) from different platforms and architectures Aug 20, 2024
@rmtew rmtew added the good first issue Good for newcomers label Aug 20, 2024
@rmtew
Copy link
Owner Author

rmtew commented Aug 20, 2024

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 rmtew added the enhancement New feature or request label Aug 20, 2024
@rmtew
Copy link
Owner Author

rmtew commented Aug 20, 2024

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
Copy link
Collaborator

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
Copy link
Owner Author

rmtew commented Aug 20, 2024

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

@HexDecimal
Copy link
Collaborator

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
Copy link
Owner Author

rmtew commented Aug 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants