Add a recovery reader for the v90 flight log

[Gliese852]

Although the flight log is now a single list, it still has the same three types of entries. Therefore it can still be shown without changing the GUI in the start menu, where 3 sections (entry types) are shown separately, depending on the combobox.

It is now possible to determine the log entry type only by the object type, which becomes available after full deserialization. But I would not want to do this, including reusing the actual code from the game, since it may change over time. Information about the object type is contained in the lua_class field, which is lost when the save tree is unpickled. In order to save this information, and at the same time not change anything radically in the unpickled tree, it was decided to put all type names into one separate subtable in the save tree, type_names, the keys in which will be the tables corresponding to the unpickled objects.

[impaktor]

Pinging in @JonBooth78

[impaktor]

Will this hardcoding of the version need to be updated or changed at some point? Or it's only there while we want to have compatibility with versions prior to 90?
(Sorry, I'm too lazy to read the code)

[Gliese852]

This number is here so that the following function is called for saves v90 and newer. Immediately before you can see the function for v89, it will be called when restoring saves v89 and older. This list can grow on both sides to support older and newer save versions.

[JonBooth78]

I'm concerned about a couple of things here. Firstly, I'm not a fan of renaming the text field of an entry to entry. IMHO the entry encompasses all the data, including location, time etc. The text is just one of those pieces of data - having an entry within an entry and so perhaps accessing it like entry.entry feels off. Fairly minor thing though.

[JonBooth78]

My second concern here is we're moving to a world where we maintain 2 versions of the flight log code - both serialization and UI, one for in-game and one for restoration.

This is very far from ideal as it essentially will make it twice as hard to evolve that part of the game and makes it brittle and hard to maintain. So I'd actually like look at consolidating it all together.

I've not had time to dig into this far enough but I think this means that loading of flight-logs should always goes through the 'restore' path somehow, or that we have a common interface for deserialization that they both conform to in order to provide the POD that needs interpreting.

Similarly we should provide a 'flightlog' UI element that can be embedded elsewhere and is configurable.

All of this is possible and I think we should consider as the architectural end-goal.

None of this stops the usefulness of where we are now; I love the restore function and I'm happy for this kind of change until we (perhaps I - I'm happy to add it to my list) have time to address that architectural issue.

[Gliese852]

@JonBooth78 regarding the entry field, I renamed it because that's what you called it, in the old flight log, the entries were arrays.

pioneer/data/modules/FlightLog/FlightLogEntries.lua

Line 100 in 2619555

self.entry = entry

Regarding the two versions of the UI and deserialization, I agree with you. I would like to note that the problem of GUI reuse most likely relates not to recovery, but to the new game start menu. There are examples of GUI reuse - SystemOverviewWidget is used in the 'Location' tab.

The problem of reusing deserialization seems to me on the one hand more complex - I think that in order to fully reuse this, we need to run the full process of loading the game, but we can hardly do this for the old save, but that's the essence of recovery.
On the other hand, deserialization within a game is usually trivial:

pioneer/data/modules/FlightLog/FlightLog.lua

Line 440 in 2619555

loaded_data = data

The code you wrote to support previous version is not actually needed (just this could be transferred to the recovery area, but the recovery is already there, so you can just delete it).

But overall, this is a very interesting topic to think about, and there is definitely room for improvement.

[Web-eWorks]

@Gliese852

In order to save this information, and at the same time not change anything radically in the unpickled tree, it was decided to put all type names into one separate subtable in the save tree, type_names, the keys in which will be the tables corresponding to the unpickled objects.

Note: you could also create one "dummy" metatable per unique lua_class name and provide a Helpers.GetLuaClass(obj) which retrieves the object's lua_class by indexing into the table mapping metatables to strings (or just store the string name in the metatable directly).

regarding the entry field, I renamed it because that's what you called it, in the old flight log, the entries were arrays.

I would strongly recommend this field either remain text or be named something like note, as that's what it is - it's a string containing the player's notes for that entry in the logbook. You have the overall "form entry" in the "logbook" and then inside that form entry there's a text field for the pilot/captain to record any special observations.

[Gliese852]

@Web-eWorks

I would strongly recommend this field either remain text or be named something like note

Perhaps I wasn't specific enough, the name entry is now in the 20240203, and in v90 saves. In v89 it wasn't called anything at all. In this sense, it cannot remain text since it was never called that.
Now, in order not to call this entry, we need to change the main FlightLog code, and this will be savegamebump.

[Gliese852]

@Web-eWorks Implemented what was suggested in fixup commit (about metatables).

[JonBooth78]

Darn, it seems like my change from entry -> text also got eaten by my poor rebase and so we went with entry, this wasn't what was intended :-(

I think I should look at re-tidying this up.

Re: the versioning code; it's actually very handy to have in the flight log as then one can change this without having to recover or version bump the entire thing :-)

The whole FlightLog.InsertCustomEntry set of functions in FlightLog seem off, however, that's due to the dichotomy of the loading/saving code, which I think needs a good look over.

[Web-eWorks]

Looks sane to me, sorry about the delay in reviewing. I'd suggest this PR also remove the Version == 1 migration code present in the FlightLog itself, since that should now be entirely handled by the savegame recovery code (as saves with the previous version of flight log data are no longer compatible).

[Gliese852]

Forced a fixup commit with a small amendment and removed the in-game migration code.