Py3 engine segfaults when using a savefile from a py2 engine

[evertvorster]

Hey there. I am playing with the latest vegastrike engine from master.
It has been compiled with PYTHON3=ON, and using the system boost libs.

So far, it's been going well. Due to the "jumping around issue" I personally don't think it's related to the SPEC, I have been trying a python2 version, as well as older branches of the engine. During this time I also advanced a little in the game, doing Janus' missions.

Now I have reverted back to python3 and the latest master, and I get a segfault when the ship launches. When the engine is Python2, and the same savefile, there is no segfault.

I have attached the savefile to this report.

Evert_empty.tar.gz

Here is the interesting bit of the error message:
-----snip---------

Adding news
finding nonloaded quest
quest_tutorial
finding quest
quest_tutorial
nonpfact
removing quest
Processing News
Compiling python module bases/launch_music.py
Traceback (most recent call last):
File "/usr/share/vegastrike/modules/missions/privateer.py", line 34, in Execute
i.Execute()
File "/usr/share/vegastrike/modules/random_encounters.py", line 291, in Execute
dynamic_battle.UpdateCombatTurn()
File "/usr/share/vegastrike/modules/dynamic_battle.py", line 42, in UpdateCombatTurn
lookorsiege=LookForTrouble (fac)
File "/usr/share/vegastrike/modules/dynamic_battle.py", line 367, in LookForTrouble
randomMovement (i,faction)
File "/usr/share/vegastrike/modules/dynamic_battle.py", line 258, in randomMovement
fg_util.TransferFG( fg,fac,suggestednewsys);
File "/usr/share/vegastrike/modules/fg_util.py", line 425, in TransferFG
_RemoveFGFromSystem(fgname,faction,starsystem)
File "/usr/share/vegastrike/modules/fg_util.py", line 251, in _RemoveFGFromSystem
if Director.getSaveString(ccp,key,index) == fgname:
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe3 in position 2: invalid continuation byte
terminate called after throwing an instance of 'boost::python::error_already_set'
/usr/bin/vs: line 7: 7015 Aborted (core dumped) vegastrike -d/usr/share/vegastrike
[evert@Vorster vegastrike-engine-git]$

[nabaco]

@evertvorster - is this issue still relevant, or it was related to the old savefile?

[evertvorster]

@nabaco, I am now doing a test run.
This is on my unified units.csv plus a few assorted other fixes that are still waiting in the wings. It's fair to say that there are no more missing units errors.
As a test, I will start a new game on Python 2. Do the first two Janus Missions, ie: fly to Jacobs and Serenity, and save in each location. Then I'll fly to Everett, and save somewhere there too. I'll switch over to the Py3 engine, and load each of the savegames and see if they work.

More on this test in a few minutes

[evertvorster]

@nabaco, still an issue.
Of my test above, just a clean brand new game in a brand new universe that was created with Python2, crashes the engine when loaded into a Python 3 engine.

Attached is the console log of the crash. I'll keep the save files around if you have any other tests for me to try on them
2020-07-12.txt

[P-D-E]

Hi,
opening and old save of mine in a text editor, the reported character encoding is CP1252; I saved a copy of the file as UTF-8, and the Py3 engine successfully loaded it.

[stephengtuggy]

@P-D-E Hmm, very interesting. Thanks for the tip.

[BenjamenMeyer]

For reference, CP1252 is a DOS/Windows Code Page reference (Code Page 1252) which is how DOS/Windows did region/language support years back.

It kind of makes sense that the age of the saved game file might require an encoding update; though I wonder if we an fix that automatically...

[P-D-E]

The funny thing is my saved game comes from Linux, I expected it to be UTF-8 already, but the command file reports it as "ISO-8859 text, with very long lines", compatible with the text editor's report (Leafpad).

[BenjamenMeyer]

I'm going to look into this a little more, I will say that in all my Python work I always hated getting the following error:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe3 in position 2: invalid continuation byte

It can occur with both Py2 and Py3 when something doesn't convert to-from Unicode correctly.

Any how, I'm going to try to reproduce this using master/0.8.x through the following:

• build Py2 version
• build Py3 version
• load Py2 version; start campaign; save; quit
• load Py3 version; load game

If that works, then we'll need to do some more deep dive to find what kinds of data is getting into the saved game file that is causing the error.

[P-D-E]

By the way, here's one offending line from one of my old saves:
FG_Relation_Hässleholm_Squadron
There are quite a few squadron names with non-ascii characters causing this issue.

[BenjamenMeyer]

@P-D-E cool - yeah, that's probably what's doing it. I might have to create some kind of tool to try to detect stuff; fortunately it should be an easy python script - read, parse, drop....

[P-D-E]

Assets-Production/universe/fgnames is the source of those names, but they've been purged of those characters from 0.6.x on (e.g. Hssleholm).
Maybe the old 0.5.1 ISO-8859 files could be converted to UTF-8 to preserve the correct names if the engine supports them.

[BenjamenMeyer]

@P-D-E thanks; it'll certainly be a migration effort for supporting older installs, and folks that are still using the 0.5.x Windows/Mac stuff.

[P-D-E]

@BenjamenMeyer I tested the conversion with a short Python script, should it become a command-line tool available to people with old savefiles?

[BenjamenMeyer]

@P-D-E that would be awesome! Probably something we should provide by default with the engine.

[P-D-E]

@BenjamenMeyer Here's a prototype:
https://github.com/P-D-E/vssc

[BenjamenMeyer]

@P-D-E looks good; if we cna get a verification then it'd be great to be able to bundle that into the main VS Engine as a utility tool
@evertvorster do you have any Py2 saved game files left over that exhibit this issue? If so, could you try @P-D-E 's utility to verify a fix?

[evertvorster]

Real life is getting in the way of coding at the moment, and that is probably a good thing!

My thought on this... should the engine not detect that it's a Py2 savegame and convert it automatically? Once it's saved, it's a Py3 savegame. :)

[BenjamenMeyer]

@evertvorster if possible, certainly. First is reliably reproducing and identifying the cause so we can fix and know we're know breaking things.

Oh, and yeah - it's a good thing that Real Life is getting in the way since that pays the bill ;) so when you can it'd be appreciated!

[BenjamenMeyer]

Reproduction:

• get a copy of the 0.5.3 assets out of github (checkout the 0.5.3 tag in for the Assets Production)
• load using the engine built against Py2, start a campaign, save the game
• load using the engine built against Py3, load the saved game
  engine crashes

I'll have to dig into this later.

I did check with the current release (0.7.0) and there is no issue there. So this is really about bringing old games forward from our vast group of users from years past. While many have likely lost the game files, there is still a good chance many have not, especially since we're still promoting 0.5.x on Windows and Mac for the time being.

Also checked that running the engine with Py2 and loading the same file doesn't crash the engine. So this is very much explicitly a Py2 vs Py3 thing.

[BenjamenMeyer]

Digging into this a little by following the stack trace...which unfortunately isn't terribly helpful since...

• the data was loaded successfully by the engine which doesn't seem to care about the formatting
• the call in the stack is just returning a string from the engine over to the Python side

Unfortunately I don't think there's a good way to solve this based on my little review thus far of savegame.cpp. Doing proper serialize/deserialize of data on load would probably enable us to centralize some of the functionality and resolve this nicely, but that's going to be some major work. Right now, applying a tool outside the game may be the simplest way to get users moved forward. If we can integrate a proper change into the engine in the future to support this that'd be great - but I doubt we'll be able to do that for 0.8.x.

Of course, I'm just getting into this so I'm all 👂s for better solutions

[evertvorster]

@BenjamenMeyer, it comes down to what the goal is.

If it's to have the simplest user experience, then there needs to be some way for the game engine to detect which Python was used to save a game, and then run a conversion utility internally.

If, on the other hand, the goal is to have a simple easily maintainable code base, then adding this complexity is a bad thing, as Python 2 is gone. Having an external conversion utility would then be the way to go, as a nod to all the long time users of the software.

However, it's your time, and you can spend it working on bit you really like. :)

[BenjamenMeyer]

@evertvorster agreed.

The main goal is to help users with old save game files not lose what they had as they move into the newer installs as IMHO that's a bad user experience. However, I'm not sure it's worth the effort to have it in-game:

• it's tied to the assets installation
• it's linked to file names
• game assets will be updated by using a newer set of asset files
• I'm not sure how moving from one assets version to another gets handled right now
• the likelihood of such is small right now due to the hiatus in development

So I'm thinking having the external tools will help mitigate things for the time being; though it'd be great to be able to provide some utilities with the game for users to detect such things, and possibly fix them if possible. I'm guessing this would mean:

• searching the directory structures for directory file names which don't convert properly and renaming them
• searching file for the directories and files found to update the files

Ultimately we want more easily maintainable code. If in the long run/process we can move this into the engine then great; if not then it's probably not a huge deal.

[P-D-E]

I'm with @BenjamenMeyer on this; the conversion is only needed once at most, and to me it's more like a post-install step.

Besides, I've discovered a nasty legacy from 0.5, the bzbr.txt file has some Spanish diacritics in an encoding I couldn't identify; I looped through all the supported encodings and none of them mapped those characters correctly.

Now, it's only 3 names out of 640, so the chances they slipped into a savefile are low (none were in my savefiles), but a bad user experience is to avoid at all costs.

So I had to add a fallback byte-mode handling to the tool, in case the cp1252 text read fails; this is the exact kind of thing I'd rather not burden an engine with, if possible.

That said, I think it'd be nicer if the assets shipped the squadron names with all the diacritics back; the conversion can be done manually with an editor like Leafpad, or even with the vssc tool itself, bar the bzbr.txt file.

[ermo]

@ermo Yes, I'd love that too; I'm just not sure we can get there in the near term as that would require error handling capabilities. I think that is certainly the long term goal. Right now I'm trying to identify a near-term solution that's acceptable.

In searching through this the functionality that loads a saved game doesn't seem to be able to return errors, and I haven't seen much in the way of actual error handling along it's path either. I could be completely wrong and if so great (all pointers welcomed). I did try setting a BadFormat error in the function, but just ended up as I described in 3 above.

A small change that doesn't require exceptions could be to change the signature for SaveGame::ParseSaveGame to return bool instead of void? If it hits an unparseable bit of text, it exits with false, otherwise it exits with true?

I wonder if it's possible (directly or indirectly) to pass said boolean on to the asset-level python code perhaps?

If so, you could guard it with an if-statement and execute the fallback in an else-clause when attempting to load a game from the python assets?

These are the places in the code where ack says that ParseSaveGame is mentioned:

[ermo@columbus Vega-Strike-Engine-Source]$ ack ParseSaveGame
engine/src/gfx/cockpit_generic.cpp
762:                    savegame->ParseSaveGame( savegamefile,

engine/src/main.cpp
676:            _Universe->AccessCockpit( k )->savegame->ParseSaveGame( savegamefile,

engine/src/savegame.cpp
905:void SaveGame::ParseSaveGame( const string &filename_p,

engine/src/savegame.h
144:    void ParseSaveGame( const std::string &filename, std::string &ForceStarSystem, const std::string &originalstarsystem,

engine/src/universe_util_generic.cpp
895:    savegame.ParseSaveGame( filename, system, "", pos, updatepos, creds, Ships,

[Loki1950]

The saved game is very tightly tied to the universe data that is generated with the first run of the game.

[BenjamenMeyer]

Okay I'm a bit closer now... @ermo I did add the return value...unfortunately that leaves it in an even worse state for the moment as the game just hangs on the load screen. I did trace it to being loaded from the call in cockpit_generic.cpp; but now I need to figure out how to revert to the previous screen.

@Loki1950 @P-D-E any pointers on doing the screen reversion would be appreciated; it's not very clear to me at the moment. But I'll keep poking around. I'll probably put this up as a draft PR so you all can start seeing the changes, etc too. UPDATE: See #453 for the Draft PR.

[ermo]

It may be worth trying to summarise the current thinking on how to handle the migration to Py3 and its "strings are UTF-8 by default" requirements:

Short-term goal:

• Ensure that old savegames in non-UTF-8 format do not crash the game
  • If a non-UTF-8 save-game is encountered during load, abort and show the user a message in the GUI on how to remedy it
• To achieve the above, ensure we have a single code-path for checking for non-UTF-8 text and ensure that Python and C++ code both use this single code-path, in whatever way is most convenient and least disruptive

The short term goal appears to be achievable if we adopt @P-D-E 's C++ PR here (where the Python side imports the exported C++ check via the VS module).

@royfalk suggested an alternative approach, where Boost C++ conversion functions are used. This alternative implementation could still be exposed using the same function signature as @P-D-E 's C++ PR, which means that the underlying C++ implementation (be it the custom C++ code or a suitable amalgamam of Boost functions) need not be set in stone.

Longer term goal:

• Switch to Unicode string types internally. This is expected to be a refactor worthy of a milestone in and of itself.
  • If possible, leverage this switch to make the conversion of older save-games transparent via built-in Boost/C++ Standard Library Unicode conversion functions.
• Design a reasonable Exception hierarchy (useful for catching conversion errors?) and make use of it across the C++ code base

For us to move forward, a consensus needs to be built that we agree that the short-term goal can be met with @P-D-E 's PRs (possibly looking into using Boost functions as an alternative as suggested by @royfalk ) and that, while the proposed solution is not necessarily a long-term one, it will mitigate the immediate blocker for Py3 migration and thus, on reflection, be worth the possible future churn in terms of refactoring.

If you agree with my description and proposed short-term goal + solution and are ready to punt on the longer-term goals, respond with a 🚀 emoji.

If you agree in principle, but think this needs more discussion, give a 👍 and make a comment describing your preferred 0.8.x solution.

If you disagree with most or all of the above, give a 👎 and make a comment describing your preferred 0.8.x solution.

[royfalk]

I don't know enough about this side of the game to voice my opinion. I'd note that adding an exception hierarchy or switching all strings to unicode to the code base will be hard to do.

[BenjamenMeyer]

We need to create a list of all the ways that a game can be loaded and tested. Thus far I'm aware of:

• Use case 1: Command-line specified
• Use case 2: Start Vega Strike, select Load Game, select game, click Load
• Use case 3: From Concourse go to a terminal, select Load/Save, select game, click Load

In testing #471 I found it failed use case 2 but use case 3 was resolved. I haven't tried use case 1 yet. We need to verify all of these to ensure this is properly solved so users don't end up in a bad state.

[P-D-E]

Use case 2 depends on the Python call to the C++ check, and that one is in Assets; the test will fail with assets older than pull 62.

[BenjamenMeyer]

@P-D-E thanks; verified. I'm not sure how to kick off the 3rd use case. Doing:

$ vegastrike-engine -d/path/to/assets /path/to/saved/game/file

Doesn't seem to work, it dumps out with a crash about not finding the mission, and nothing from the command-line options doesn't seem to align...looking closely at main.cpp it seems I might need to craft a special mission file some how (see https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/engine/src/main.cpp#L645)

[P-D-E]

By the time it gets there, the mission object has already been populated. Try inspecting what happens in engine/src/cmd/script/mission.cpp where Mission::ConstructMission is the one logging that panic exit.
Check what value configfile has, and then since top = domf.LoadXML(configfile); is probably failing, peek at that too.

[BenjamenMeyer]

@P-D-E i'm guessing VS doesn't directly support loading a saved game from the command-line, but as a side-effect of loading specific missions for testing purposes; more investigation certainly needed. We can probably get by for 0.8.x as that is probably more of a dev tool.

[P-D-E]

@BenjamenMeyer That's surely the case of the unit converter, which lets users see their models in action via mission/modelview.mission. If the command line option is only used for single mission files and not for entire saved games, then there's no real Use case 1, that'd be good news!
Update: I've just tried ./vegastrike-engine -d../data/ modelview.mission and it works as expected.

[BenjamenMeyer]

@P-D-E thanks. Closing this out