Very WIP: Refactor flight log recover

[JonBooth78]

So far this is proof of concept; imagining what it might look like to try and consolidate flight log related code, both serialization (standard case and to recover) but also rendering.

It's very rough and work in progress but I think shows this can be done and is worth exploring.

It's not ready for a detailed review, however, roughly it:

• renames flight-log.lua in the new game code to flight-log-tab.lua. I believe it's confusing to have multiple files roughly called flight log.
• passes through the recover code into the flight log deseralization code to unify these; this requires passing some optional helpers
• separates the code to draw flight logs out from the 06-flightlog.lua code into it's own place. This should probably sit in the pygui tree though rather than the more generic modules tree to be honest.
• Uses the above drawing code to draw the flightlog in the new game tab by providing an option to not show all the interactive features, such as editing text or removing custom log entries. This needs a bit more work, to show the options, but actually seems to work well; also needs testing at different screen resolutions.
  ** This also currently has issues working out how to format system paths and I think needs tidying up; perhaps by creating these and caching them at the time the flight log entries are created, rather than when they are drawn. I don't know if this will cause issues if the language is changed or not if we cache strings from the localization system.
  *** These issues occur due to difficulty in accessing the Galaxy object. Is the galaxy invariant within a version of the game, if so, then I wonder why we need an active game to access it? Perhaps it would be good to allow for multiple galaxies, I'm not sure so this is just idle speculation.

[Web-eWorks]

Obviously this is very raw and WIP code (and I think I see the botched rebase reintroduced some very old code there with the TotalDefaultElements - I remember that getting rewritten before merge and am sad to see it back 😐) so I have no qualms about suggesting huge rewrites / refactors to achieve a more ergonomic API.

I think it's well worth taking a significant amount of time to find an optimal solution here, as this code is most likely going to be the example from which most module-level serialization+recovery functionality is copied going forward.

[Web-eWorks]

I mentioned this on IRC as well, but since we've done a savebump to v90, saves containing the version 1 flight log data structures are incompatible and thus this entire if-branch is redundant. Instead, the save recovery function for the flight log data will be used.

[JonBooth78]

So I think this and the below comment are kind of what I'm getting at.

There are two options I can see:

1. The flight-log code owns the flight log serialization, including recovery, which can be generalized as any system own's it's own code and specific recovery logic
2. The flight-log code only owns loading the latest** version of the code and recovery is separated into the new-game menu (or some other module), which handles recovery for everything.

I was also asking the same question of the UI rendering :-)

I'll comment more below as this ties into the next comment

[Web-eWorks]

I am beginning to strongly think that the recovery reader concept should be extended to instead just return a Lua table to be passed directly to the unserialize function set in Serializer:Register("FlightLog", ...). This would allow chaining migration functions (v89 rewrites the table to the v90 format, etc.), consolidates all of the post-processing in one logical location, and only has the thorny problem of needing to reify / unpickle class-like tables (e.g. converting flight log entries into actual FlightLog objects).

This reification step could take place at the end of the recovery process, so the recovery functions manipulate the "raw values" as they are present in the save file, and the output of the recovery step is treated as though it's a "fresh" save needing to have all lua_class tagged tables converted into class instances.

To demonstrate the hypothetical, this could be written as (making several generous assumptions about the capabilities of the system):

-- Takes a source module name, an optional destination module name, and an array of migration steps
-- Helpers.ProcessModule("FlightLog", { ...
{
  version = 89,
  fnc = function(data, error)
    local out = {}
    for _, v in ipairs(data.System) do
      -- Copied from the unserialization code above
      local systempath = pathFixup ~= nil and pathFixup( v[1] ) or v[1]
      local data = { systemp = systempath, arrtime = v[2], deptime = nil, entry = v[4] }

      if not --[[ some validation ]] then error("Cannot create entry") end -- Proper flow control to skip the entry, not going to type it all out

      -- tags the new table with a lua_class metatable - the table will be unserialized into a class instance at the end of the recovery process
      -- (e.g. class is deleted in v91, so a new recovery step rewrites/amends the table to work with the replacement class)
      local val = Helpers.TagClass(data, "FlightLogEntry.System")

      table.insert(out, val)
    end

    -- Passed directly to the unserialize method for the FlightLog module
    return out
  end
},
-- ...

Hopefully this is clear enough that you can pick up the general concept - the idea here is to write the compatibility code once (and locate it with all of the other code concerned with save recovery), and ensure the actual module only has to deal with the "current" data model. This borrows strongly from (my understanding of) the way database migration is handled in web applications, if you have any experience with that.

[JonBooth78]

In no particular order:

1. Yes I'm familiar with db migration scripts, have written a fair few to do bi-directional db migration. I'm not sure (and so asking) that this paradigm is actually the most effective here. It's a weakly held opinion right now that, maybe it's not.

I think that db migration scripts are great - for databases. Here we are working with a formal database (typically SQL based), where the data has to be organized in specific ways for the queries and such that run on top of it to be run. The query language is formal and specific and quite limited in what it can do; however, hugely quick and powerful. It also serves to create a really good abstraction and barrier between the data and the logic layers and makes us think really hard about our data layout.

However, we don't actually have db here, nor any of those limitations and I don't think we need any of the strengths a db provides. Otherwise, I'd assume we'd be using something like SQLite for our gave-save format.

That means that we can write code to handle various versions in our modules. It's my opinion, that this is the correct place to do so; flight-log feature is the place that holds all the logic around the flight log. Rather than having it spread across various different modules.

2. Having said that I do like the concept of the recovery code, essentially doing a migration to the 'current' format and then the serialization being the simple form - just load the serialized lua objects. However, I'd want to explore the idea that even if we do that, the code to do that recovery migration, would live with the flight-log code and not with the generic recovery code.

The reason is that I feel like game recovery should be a utility that we offer to our modules/features rather than a feature of it's own.

3. I added a ** to the comment above where I said

The flight-log code only owns loading the latest** version of the code

The other issue I see with the current structure, is that any-time we want to add or modify any of the format of the way we want to serialize any part of the game logic, we need to save bump everything, potentially having to write or document recovery steps across many different, disparate subsystems that are not involved. This has already lead me to the perception that it creates brittle and inflexible code, given the suggestion that if I were to want to rename the entry field of a flight log element, it would require an entire new game-save version.

I'm not very attached to that because, as a developer, I both want the freedom and flexibility to make changes easily and also as a player-developer, I want to keep being able to use my saves. That's why I added the serialization/update logic in the first place to flight log, using the pre-added version field of it's specific data. I did so because I wanted to be able to test with and keep playing my save games. I'm pretty sure that our users would want our developers to also have this freedom too.

So I think there are also some questions around what requires a full save game bump and recovery. Some features (such as the flight-log) can be updated and have new save logic, in isolation, without requiring a recovery. Others, such as galaxy generation cannot (even though in the case of the galaxies, it may well be that the serialization code and structure hasn't changed at all, just that some system paths now are either invalid or point at different places than they did).

Maybe for each system that registers in lua with the Serializer it also registers it's own migration scripts. Thereby, when loading a save game, if (and I'll keep using flightlog as an example), the flightlog's internal version has bumped from 2 to 3, then the migration script gets called as part of a normal 'load' and a full-blown recovery is only needed when fundamental systems (galaxy, equip-v2 etc) have changes. That way, if I want to change some minor implementation detail of the flightlog, I can, without causing any issues elsewhere.

It would kind of end up with a 'minor' and 'major' versioning system where minor changes and versions are specific to modules.

This concept would, of course need buy in to the suggestion that the version migration scripts live with the module at question, rather than together in a recovery module although actually the same thing could be done in a centralized recovery module too.

Again, I want to stress, I love recovery and the work that's being done; I just want to ask questions to make sure I understand the long-term goals and direction.

To be clear - the purpose of this WIP PR is not to try and say 'fait-accompli', the codes written we should do it this way, it's because I often find it easier to discuss/describe code ideas with a roughed out POC. So I'm happy enough if we decide against going this way; the value is in the conversation.

[Web-eWorks]

Thank you for taking the time to write this out, because I find this perspective to mirror my own now that I've attempted to prototype the architecture I suggested above. Replying in no particular order:

To be clear - the purpose of this WIP PR is not to try and say 'fait-accompli', the codes written we should do it this way, it's because I often find it easier to discuss/describe code ideas with a roughed out POC. So I'm happy enough if we decide against going this way; the value is in the conversation.

Absolutely - I'd ask for my own opinions / statements to be taken with a grain of salt as well, as I have no intention of "drawing a line in the sand" regarding the best way to implement the functionality under discussion. I had originally proposed the code snippet above as a (seemingly) clearer and more ergonomic way of expressing the proposed feature, but upon further introspection and prototyping it's become clear it's not as useful as I was first thinking.

I'm not sure (and so asking) that this paradigm is actually the most effective here. It's a weakly held opinion right now that, maybe it's not.
[...]
However, we don't actually have db here, nor any of those limitations and I don't think we need any of the strengths a db provides. Otherwise, I'd assume we'd be using something like SQLite for our gave-save format.

Upon digging further into it, I'd agree with this opinion. I was originally conflating two different responsibilities, that of recovering truly incompatible save files and that of migrating "almost compatible" save data as systems are evolved but remain backwards-compatible.

In fact, after doing some prototyping, one of the biggest strengths of our save format in my opinion is that we can directly serialize objects themselves into the savefile and transparently load them again with the rest of the game - trying to treat the save as akin to a traditional database conflicts with that advantage and creates intractable problems (as the "database" in our model implicitly depends on the rest of the application runtime environment).

That means that we can write code to handle various versions in our modules. It's my opinion, that this is the correct place to do so; flight-log feature is the place that holds all the logic around the flight log. Rather than having it spread across various different modules.

I would generally agree with this, although the current architecture (with regards to UI mostly) in my mind is not quite fully "pluggable" in the way that would be optimal for the FlightLog to be a truly self-contained module with no code dependencies anywhere else. I'd really like to get there however, as that would directly affect the quality of life for mods which don't replace or override existing files.

The other issue I see with the current structure, is that any-time we want to add or modify any of the format of the way we want to serialize any part of the game logic, we need to save bump everything, potentially having to write or document recovery steps across many different, disparate subsystems that are not involved. This has already lead me to the perception that it creates brittle and inflexible code, given the suggestion that if I were to want to rename the entry field of a flight log element, it would require an entire new game-save version.

I had originally considered the idea of a monotonically-increasing "minor save version", which is used as a global tag incremented every time a new migration is necessitated (to identify the format permutation of all disparate modules involved in the save) but not preventing the save from being loaded the way the primary save version number does. This could also be implemented with each module serializing its own version number with little "real" tradeoff in terms of functionality or ergonomics.

The above is definitely an accurate criticism, because the existing save code is already either brittle and inflexible in the way you described, or completely ad-hoc and reinventing the wheel at each callsite. It's become clear that we need a much more intentional way of handling this which can be re-used without a huge amount of duplicated effort.

So I think there are also some questions around what requires a full save game bump and recovery. Some features (such as the flight-log) can be updated and have new save logic, in isolation, without requiring a recovery.

Something that complicates this is when the feature in question is being used in multiple places, e.g. the flight log needs to be used when loading a game save, but also needs to be able to have data extracted when recovering a save as it's used as new game parameter. I think we're both on the same page regarding the need for a clear way to architecturally structure that cats-cradle of dependencies.

I did so because I wanted to be able to test with and keep playing my save games. I'm pretty sure that our users would want our developers to also have this freedom too.

I would personally like every Lua "module" in the game to support this level of flexibility. In my mind, the only thing that should necessitate a recovery/savebump is a major change to the data-structure of the game (e.g. replacing the entire equipment system... e.g. #5734) or changes to the determinism of the galaxy layout which completely invalidate the "saved state" of the game.

Maybe for each system that registers in lua with the Serializer it also registers it's own migration scripts. Thereby, when loading a save game, if (and I'll keep using flightlog as an example), the flightlog's internal version has bumped from 2 to 3, then the migration script gets called as part of a normal 'load' and a full-blown recovery is only needed when fundamental systems (galaxy, equip-v2 etc) have changes.

This is the approach we have as an ad-hoc solution and I think the correct way forwards. While I had originally liked the idea of separating migration from the "module" code itself (one centralized location to apply transforms to the savefile data), these migrations are much more domain-specific and would work fine in module code.

The only concern I have here is that there is still some duplicated work as recovery code and "live" migrations are two similar (albeit different) processes - if carefully structured it might be possible to write the migration so that it can be applied to both recovered saves as well as regularly-loaded save files.

Again, I want to stress, I love recovery and the work that's being done; I just want to ask questions to make sure I understand the long-term goals and direction.

I have a saying - the "first version" is always the most difficult, has the worst results, and takes the longest. A lot of the long-term goals and direction for the feature (and improvements to the serialization code in general) are still being figured out and posing these questions really helps to narrow down the "search space" and produce good designs.

Feel free to ask if I've not explained something clearly or completely addressed a question - it's nighttime and the English part of the brain sometimes gets turned off to let the code part of the brain do its job. 😄

[JonBooth78]

Wow, thanks for consuming and replying so thoroughly to my somewhat ad-hoc thoughts.

To do the conversation and your efforts justice there is lots here for me to consider and I seem to have a lot on my plate at the moment. So I'll take it offline rather than say the first thing that comes to mind and try and come back to you later in the week.

However, I think on the surface I agree with what you said. I'm also really only thinking of this though the lens of flight log and not other modules and I suspect flight is somewhat an outlier.

Also, I'm personally less upset about missions and so forth getting dumped and reset on a recovery than I am for the other core elements, such as equipment, ship, cargo, cash, reputation , and I guess the log 😀

I feel like we can explain that away as some kind of weird interdimensional slip that occurs, maybe due to exposure to FTL radiation, beaming you through the 'Pioneerverse'. Also, it's not a game ending level of frustration compared to having to regain some of that other stuff..

[JonBooth78]

Sorry, I've been hugely distracted; a lot going on over here.

I'm not sure I have much more to add other than wonder what next steps are... Do we try and work out what a perfect system would look like and then take baby steps towards it? Do we just know the direction and allow that to dictate our future thinking?

I'm also about to reply to Gliese852 below but I've been thinking about how we've got lots of different modules, all of which do and have to do some fairly common jobs and so could share common structure and tools but all too often are different and therefore somewhat ad-hoc. Admittedly this is a very raw opinion from a surface look at them, I might feel a bit more educated as I try and go through them and refactor them to use the new EquipV2 goodies that I'm aware I volunteered to do. However, this is a good example; I'd like to think of the game engine and core providing commonly needed services to the gameplay modules to make the modder/gameplay developer's job easier; they can concentrate on the gameplay, ideally using these tools provided. Ideally, I'd like to distill out a 'blueprint' or simple structure to get all the gameplay elements to follow and/or one day, as a crazy thought, extend the editor so non-developers can put together dialogue and options and script story based missions and the like...

Anyway, I'm always thinking a hundred steps ahead of the challenge today... The thing I wanted to say relating to game recovery is that I feel it's a utility service that can be provided in the nicest, most intuitive way possible to the modules, ideally in a way that it can be used with similar patterns everywhere - however, I don't really conceive of it as a module itself. Hope that makes sense, I'm about to say the same thing below :-D

[Gliese852]

I would like to point out that now the game code does not know and is in no way dependent on the recovery code, I would be sorry if this changed, it seems this is the reverse process of decoupling.
I would also like to point out that game parameters in the general case can merge and separate. (Let's say cargo somehow leaks into the equipment), and can also gain and lose dependencies on each other. That is, you most likely cannot isolate the recovery of your individual module from other modules.
Also, as a result of some UI decisions, the flight log may move from a separate tab.

flight-log feature is the place that holds all the logic around the flight log. Rather than having it spread across various different modules.

Here I can argue that all the recovery logic is now in one place, and you are proposing to spread it across different modules.

[JonBooth78]

I would like to point out that now the game code does not know and is in no way dependent on the recovery code, I would be sorry if this changed, it seems this is the reverse process of decoupling. I would also like to point out that game parameters in the general case can merge and separate. (Let's say cargo somehow leaks into the equipment), and can also gain and lose dependencies on each other. That is, you most likely cannot isolate the recovery of your individual module from other modules. Also, as a result of some UI decisions, the flight log may move from a separate tab.

flight-log feature is the place that holds all the logic around the flight log. Rather than having it spread across various different modules.

Here I can argue that all the recovery logic is now in one place, and you are proposing to spread it across different modules.

Yeah, so here is the pivotal point of this discussion, I think. Where are the lines of responsibility that we want to draw? I know I've looked at the design as-is, where we've done a great job of decoupling all the game code from the recovery code and am suggesting a different concept, so apologies for that.

Off the top of my head, the advantages of separating all the recovery code from the game code are:

• Game code is easier to write, it doesn't have to worry at all about how recovery works, making it more accessible to new developers as the recovery code and concepts are fairly involved
• The recovery code all sits together - so someone can become expert in the recovery code and share knowledge throughout it,

Disadvantages are:

• Either the same person has to make changes to both game code and recovery code, in disparate areas (which make it less likely to happen) or someone else has to come along later and work out the changes made to the game code and then provide some kind of migration code in the recovery code. Neither of these concepts really seem efficient especially if they get separated and done at different times and potentially by different developers. Their is an advantage here too I guess, in that more people look at these changes, so over time they're likely to end up higher quality.
• The recovery code and migration scripts are, I think, likely to either duplicate logic and/or be harder to engineer when separated from the game code for which they relate
• With the current system, there is a limitation that any change to the game-code serialization format, anywhere, will require a save-bump; so we can't do minor migrations that are transparent to the use.

There are probably more. So perhaps we need to come to a shared consensus on what the pros/cons of each approach are and then see if we can agree on their relative priorities, which would help inform a decision.

I also have a bit of a philosophical standpoint on this architecture that I mentioned above; I see game recovery more as a utility that is provided to modules than a module or feature of its own. Which, of course if this becomes the prevailing opinion presents it's own challenges on how do we provide that utility to the module code in such a way as it's robust, safe and intuitive to use and encourages good practices there?

Again, I'd like to stress, I think the game recovery is great and I'm in no way trying to take away from the work or achievement made. My opinions are weakly held and I'll not be offended in any way if nothing comes of this. I'd like to share the thoughts as I'm interested in these kinds of architectural discussions and want to contribute to the future success of the project.