Vega-strike-engine-source: 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 :ear: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]

When I ported priv:wcu to py3 (on the 0.5.x VS engine), I encountered this bug and developed a simple engine workaround for this that noisily drops savegame flight group items that cannot be parsed; it basically involves having the engine check if it's trying to allocate a negative amount of memory for the relevant strings (which will obviously fail).

That said, I feel like the most obvious solution is to

1) Have the engine flat out refuse to load non-utf8 savegames and

2) Noisily tell the user (via a GUI text box if necessary) to run the py3 conversion script on the savegame.

This would mean that we follow the principle of least surprise and ensure that users manually run the converter on their (old & wonky) save games.

If the savegame format is ever changed again, we can use the same approach.

[ermo]

In relation to the above, I just checked and can see that my workaround was dropped at some point.

It starts at engine/src/savegame.cpp @ line 502:

(original)

        if ( !select_data || select_data_filter.count( mag_num ) ) {
            vecfloat = &missiondata->m[mag_num];
            vecfloat->clear();
            vecfloat->reserve(md_i_size);
            skip = false;
        }

(my workaround -- need to be updated to current logging etc. standards)

        // In rare instances (seen with wonky save game file),
        // md_i_size can be < 0, which will cause the reserve call to fail hard
        // Add a debug message to explain the issue and point out that the save
        // game file is broken.
        if ( md_i_size >= 0 && ( !select_data || select_data_filter.count( mag_num ) ) ) {
             vecstring = &missionstringdata->m[mag_num];
             vecstring->clear();
             vecstring->reserve(md_i_size);
             skip = false;
        } else {
            // debugging attempt -- show why the allocation would fail
            vs_dprintf(1, " SaveGame::ReadMissionStringData: vecstring->reserve(md_i_size = %d) will fail, bailing out (i = %d)\n", md_i_size, i);
        }

Note that my workaround doesn't really catch all the combinations, but in practice it was enough to avoid crashing on wonky saves.

It should probably be rewritten if the goal is to implement my above suggestion.

[BenjamenMeyer]

@ermo let's integrate that back in. I wonder if we can post a message to the user in-game too so they don't have to crawl through the logs to find out that it failed; but that's certainly a start.

[ermo]

@ermo let's integrate that back in. I wonder if we can post a message to the user in-game too so they don't have to crawl through the logs to find out that it failed; but that's certainly a start.

Be my guest; it should probably be done by one of the legit C++ guys here? @royfalk @stephengtuggy

I guess the check really ought to be made into a guard like if ( md_i_size < 0) { don't_try_to_allocate, write debug string and throw an exception }

FWIW, it seems to me that it would make sense to chain this together with the action to load a savegame; e.g. if there's a wonky string which can't be decoded, throw an exception with a message which can be displayed in the Load/Save GUI?

[BenjamenMeyer]

@ermo It may have gotten saved in ReadMissionStringData(), but probably needs to be in ReadMissionData() too.

[BenjamenMeyer]

Adding that over to ReadMissiongData didn't resolve it.
So there's two sides to this issue:

• the engine loads the asset data and saved game unscrupulously in C++
• the engine then passes the string over to Python, which then tries to translate it to Unicode and fails

Now the translation to Unicode can fail for several reasons:

• it's not valid data
• the assumed unicode format is different from the one used (UTF-8, UTF-16, UTF-32, Big Endian vs Little Endian, etc).

BTW - Windows uses UTF-16 (probably UTF-16LE); while Linux typically standardizes on UTF-32.

One thing I'm not seeing in the engine is the C Python functions (Py_*). Integrating these might help with catching these kinds of things on the C/C++ side as it would occur prior to the jump to Python. With Py2 since string data was byte data this kind of oversight could probably be overlooked, but with Py3 having Unicode strings (OS Dependent for which type) it may not be possible to overlook it any longer. Closer integration with Python in this respect may also help us in terms of performance too, but it'll be a major work to do, something we can't do with 0.8.0 unfortunately.

In this respect, we might need to move this to 0.9.x, but we probably have to resolve it no matter what in case someone pulls a saved game across platforms.

[BenjamenMeyer]

Okay - I managed to get the game to load with the following delta:

Assets-Production$ git diff 
diff --git a/modules/dynamic_battle.py b/modules/dynamic_battle.py
index 6015ae9e..6911872b 100644
--- a/modules/dynamic_battle.py
+++ b/modules/dynamic_battle.py
@@ -349,7 +349,12 @@ def LookForTrouble (faction):
         if faction in faction_ships.fighterProductionRate:
             AddFighterTo("Alpha",faction,True)
         return 0
-    i = Director.getSaveString(fg_util.ccp,key,lftiter)
+    try:
+        i = Director.getSaveString(fg_util.ccp,key,lftiter)
+    except UnicodeDecodeError as ex:
+        print("Error decoding {0}".format(ex))
+        return 0
+
     lftiter+=1
     sys = fg_util.FGSystem (i,faction)
     citizen=VS.isCitizen(faction)
diff --git a/modules/fg_util.py b/modules/fg_util.py
index c58fc3c1..3f712d49 100644
--- a/modules/fg_util.py
+++ b/modules/fg_util.py
@@ -1,6 +1,9 @@
+# -*- coding: utf-8 -*-
 #WARNING THIS FILE HAS A MIRROR FILE IN C++ FOR SIMILAR ACCESS...
 #SO THAT C++ CAN LEARN ABOUT THE DYNAMIC UNIVERSE SHIPS
 #
+import sys
+
 import Director
 import VS
 import vsrandom
@@ -169,7 +172,13 @@ def ReadStringList (cp,key):
     siz = Director.getSaveStringLength (cp,key)
     tup =[]
     for i in range (siz):
-        tup.append(Director.getSaveString(cp,key,i))
+        try:
+            s = Director.getSaveString(cp,key,i)
+        except UnicodeDecodeError as ex:
+            print("Error decoding {0}".format(ex))
+            continue
+
+        tup.append(s)
     return tup

 def AllFlightgroups (faction):
@@ -186,7 +195,11 @@ def RandomFlightgroup (faction):
     if (i==0):
         return ''
     import vsrandom
-    return Director.getSaveString(ccp,key,vsrandom.randrange(0,i))
+    try:
+        return Director.getSaveString(ccp,key,vsrandom.randrange(0,i))
+    except UnicodeDecodeError as ex:
+        print("Error decoding {0}".format(ex))
+        return ""

 def ListToPipe (tup):
     fina=''

The following errors were generated as a result:

Processing News
Error decoding 'utf-8' codec can't decode byte 0xf6 in position 3: invalid start byte
Error decoding 'utf-8' codec can't decode byte 0xfc in position 6: invalid start byte

The good news is that it didn't crash.

The bad news is that the saved game didn't quite load correctly; at a minimum the assets to display the ship did not load, or at least were not selected for visibility.

Yes, I'm aware that this isn't quite the right fix; however, it does at least identify the spots where we need to fix stuff up. The common link is on the engine side:

• savegame.cpp:getMissionStringData() - which just returns a vector of strings based on a string key.

Not sure where that data gets loaded (yet), but we could do a translation/sanitization in there somwehere.

[ermo]

@BenjamenMeyer

FWIW, it seems to me that you're pretty close to a working prototype; the only thing missing is a python-powered dialog with a suitable piece of text and bailing to the load/save interface when the user has clicked the single Understood button?

This is on the assumption that the first milestone is to just stop the game from crashing and subsequently informing the user that they are attempting to load an unsupported save game due to character set issues and that they should fix their save game with the included tool/instructions.

[BenjamenMeyer]

@ermo not quite...I've found something that prevents the crash; but it won't report what the string was that caused the error. We probably need a filter when reading the files. I'll have to dig in some more; but this might be quite a bit of work to get right.

[P-D-E]

@BenjamenMeyer Troublesome saves need total conversion (for standard cp1252 0.5.x [1]) and sanitization (unlucky bzbr case) anyway, would the player really need to know the faulty string?

I think avoiding the crash and reporting, as you suggested, is the best effort the py modules can make when served bad data; as you also said earlier, preventing such bad data should be a job for engine/src/cmd/script/director_generic.cpp

[1] Since my Linux saves use that encoding, I'm assuming it was the chosen one on all platforms, could any Windows/MacOS users check their 0.5.x saves and confirm or correct the assumption?

[BenjamenMeyer]

Does the player need to know? No; the player doesn't but it would be good to log it so we can get the information in bug reports.

In my case, I reproduced the error purely on Linux using the same version (same git hash), and compiling builds against both Py2 and Py3, then using the 0.5.x Assets. I haven't checked the encoding on the assets.

[ermo]

So, my take is:

1) Check the encoding of the savegame upon loading (doesn't matter if it's python or C++ -- whichever is easier to maintain gets my vote).
2) IF we detect an encoding problem (!= UTF-8), THEN stop/refuse to load AND notify the user that there is a problem.
3) Either fix the problem automagically (asking the user to accept the attempted fix as part of the notification) XOR Tell the user how to fix the problem themselves with a separate tool.

In the ideal world, we would be able to detect the code page automatically and just attempt to apply the fix.

In the real world, the user may need to try different code pages before things work (if the savegame is not UTF-8, on Windows CP-1252 is the norm, on Linux iso-8859-1 is the norm IIRC).

[BenjamenMeyer]

@ermo only issue I have with fixing it on the Python side is that it would have to be done in each game asset set - so PWCU would have to fix it too; but yes, I like your approach.

[P-D-E]

Does the player need to know? No; the player doesn't but it would be good to log it so we can get the information in bug reports.

True that, but byte and position are already in the exception message; maybe it could be useful to also report the values of key and i from e.g. getSaveString(cp,key,i) ?

[ermo]

Looks like https://docs.python.org/3/library/codecs.html allows us to (attempt to) decode various code pages (including cp-1252 and iso-8859-1) and convert to utf-8?

In addition, https://docs.python.org/3/howto/unicode.html appears to sport some useful examples.

[P-D-E]

@ermo
Yes it does, for instance I could rewrite the safe_read / byte_read pair in vssc with a modified byte_read registered as an error handler ( codecs.register_error('bzbr', byte_read) ), and the file opened as codecs.open(file_path, encoding=enc, errors='bzbr')

But for the VS modules failing above it's too late, as the getSaveString raises an exception and does not provide the string bytes; it would take a sort of getSaveBytes to get around that, then the modules could attempt to encode the data as utf-8 and handle the exceptions properly.

Considering there are quite a few getSaveString calls across the various py modules, I think it'd be cleaner to have getSaveString speaking utf-8 natively.

[BenjamenMeyer]

@P-D-E agreed, it needs to be handled on the C++ side; not sure getSaveString is the correct place as it's just an index into an array:

class MissionStringDat
{
public:
    typedef std::map< string, vector< std::string > >MSD;
    MSD m;
};
...
SaveGame::SaveGame( const std::string &pilot )
{
    callsign = pilot;
    ForceStarSystem   = string( "" );
    PlayerLocation.Set( FLT_MAX, FLT_MAX, FLT_MAX );
    missionstringdata = new MissionStringDat;
    missiondata = new MissionFloatDat;
}
...
const std::vector< string >& SaveGame::readMissionStringData( const std::string &magic_number ) const
{
    static const std::vector< string > empty;
    MissionStringDat::MSD::const_iterator it = missionstringdata->m.find( magic_number );
    return ( it == missionstringdata->m.end() ) ? empty : it->second;
}

std::vector< string >& SaveGame::getMissionStringData( const std::string &magic_number )
{
    // auto-creates a new value if the magic_number does not exist in the dataset
    return missionstringdata->m[magic_number];
}

Will continue looking around to see if there is some way to do the translation better.

Long term is may be a matter of inspecting the interfaces, etc for how this all works. If we don't actually need the string data in Python, then using something else may be a better solution...but again that's a longer task than 0.8.0. Will see what I can do for the immediate.

[P-D-E]

@BenjamenMeyer Indeed, getSaveString is where it fails with Py3 as it promises to return a string, but addressing the issue when the mission gets loaded is the cleanest way.
For now, I think your Py solution above is already much, much better than a game crash.

[stephengtuggy]

@BenjamenMeyer I like the direction you're heading here

[BenjamenMeyer]

I wonder if we provide a wide character string instead of an ASCII string if that would help the issue; remember we can't pass raw byte data to Python any more for string data like we could with Py2.

[BenjamenMeyer]

Okay - figured out where the strings get loaded; also found a function that can be removed as it's near duplicate of another function, with the only difference being the return value is dropped. Compilers will now optimize around that, so no need for the second version any more. (If need be, we can keep it around a simple wrapper; fewer bugs that way.)

I'm porting my string detection functionality into there and we can then properly capture the actual string that is causing the error in the saved game file on load and send it to our logs. Will see if I can get the load game to error out in the process too.

[BenjamenMeyer]

Made some progress; but not getting the logs I was expecting to get. Not sure why yet.
I can keep it from crashing; but noticed that the it fails to load under Py2 properly too when it gets the Unicode errors. Any how...more digging for another time.

[P-D-E]

Possibly interesting convo here: https://github.com/boostorg/python/pull/54

[BenjamenMeyer]

Yes technically we should use wstring or u8/16/32string for unicode data. But that's a bigger change than I'd like for 0.8.x as it'll have a lot of side effects and be a rather big change across thr codebase. That said we should make the change in the long run.

My goal for 0.8.x is the minimum we need to run without crashing on Py3. Starting with 0.9.x we'll start some more substantial changes with larger refactors.

[BenjamenMeyer]

argh...okay - so now I have something that will skip loading a bad file...but...it just loads a dummy/default profile instead and dumps the player out in space. There doesn't seem to be much in the way of error handling either.

So please vote:

• :-1: (1) Leave AS-IS for 0.8.x
• :+1: (2) Add try-catches in the Assets for 0.8.x so game doesn't crash; saved game partially loads but may not load correctly
• :confused: (3) Add a validation check to SaveGame::ParseSaveGame; game doesn't load the saved game at all and dumps the player into space; add a log message to inform the user (in the console) that the game didn't load due to a bad formatting.

3 is the closest I've gotten to detecting everything properly, but the user can only either play from an unknown state (possibly stranded) or quit the game entirely; but 2 leaves the user in the best state where they can get to a menu and select another game to load.

The correct fix will be delayed until we can get proper error handling in. This is about how to handle it for the 0.8.x release. Please use the associated emoji's above (:+1: , :-1:, :confused: ) to vote for what we should do for 0.8.x. After the vote I'll get a PR up.

@Loki1950 @P-D-E other ideas?

[ermo]

I'd prefer a combination of (2) and (3) tbqh.

The ideal scenario for 0.8.x is that the (broken) save isn't loaded and the user is presented with an asset dialog saying so and then is sent back to the Load GUI.

So:

1./ The python assets call into the C++ code and listens for a custom exception that the C++ code throws (Option 3 w/a slight addition).

And:

2./ Upon receiving said exception, the assets catch it, stop loading (=reset state), show a GUI dialog and when that is closed, return to the Load/Save GUI (Option 2, but with a slight addition).

This requires a little C++ code and a little asset code, but seems well worth the effort compared to a full blown string refactor?

[P-D-E]

Seconding @ermo on the GUI dialog;
if possible, a GUI dialog would inform the player in an easier way than just a console message, so they'd know for sure they need to take action (e.g. convert the save before playing it).

About the long term bigger effort solution, the engine might check the file as soon as the user chooses to load it rather than failing on the first non-utf8 string served (e.g. with something like this ) and then it could either (from minimum to maximum code effort):

• just inform the player
• fix the file by calling the external tool and load it
• fix the file internally and load it

The biggest effort might not be worth it as new players would never need it; maybe an intermediate solution could be to integrate the check/convert into vegasettings?

[stephengtuggy]

I think I agree with @ermo 's suggestion as well. I'm not familiar enough with the relevant code to know how feasible it is or isn't, but it seems reasonable.

[BenjamenMeyer]

@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.

Players that start with our 0.7.x assets for VS:UtCS should never run into; perhaps even 0.6.x. I'm not sure about PWCU; we'd have to search the code there.

@Loki1950 how tightly tied is a saved game to the universe data that is auto-generate? If we lose the universe data in ~/.vegastrike does that kill the integrity of the saved game data too?

[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 :rocket: emoji.

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

If you disagree with most or all of the above, give a :-1: 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:

• [x] Use case 1: Command-line specified
• [x] 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