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

Feature: store table headers for each chunk in savegame #9322

Merged
merged 8 commits into from Jul 2, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 30, 2021

Motivation / Problem

So there is a bit of a story here.

First off, the main reason I started this change, is because I wanted to move around some settings. But the order of settings that are stored in the savegame cannot be changed. That is really annoying, as there isn't really way around that. Sure, we can change the order, do a savegame version bump .. but that also leaves us with a huge list of the original order to make sure old games load. I wanted this fixed.

Next, we quickly figured out this was an universal issue with our savegame format. It allows for no modification of order of anything .. often leaving the code a bit weird and odd, because of what is stored on disk. This tightly-coupled information has made many mistakes in the past.

Lastly, for a while now we have somewhat the ambition to move to SQLite as savegame format. But getting there from where we were, is far from easy.
I am the type of person that doesn't like to go from A to Z immediately, but I do like that the changes we make are at least on the route towards Z. This PR not only proves SQLite should be possible, but it also prepares our SaveLoad code enough that SQLite would be possible.

Those three things combined gave me the motivation (hihi, same word, different context) to write this PR.

Description

To decouple what fields are on disk and what we are expected, I basically made the savegames self-descriptive. With that I mean there is enough information in the savegame to know how to read the savegame, without having to know the savegame version or any internals about how OpenTTD loads the game .. well, other than the format, ofc.

The idea is simple: all chunks (with a few exceptions, see below) are now CH_TABLE, indicating they are a table. A table has a header and a list of entries (shocker). In other words, the old CH_ARRAY prefixed with a header.

The header is very easy: a list of (type, key) pairs, telling in what order the keys are stored, and what type the field is. The list ends with a type=0 entry.
If there is a struct inside that list, the header of that struct is directly followed by the header you are reading (so, it is depth-first). This gives you full view of the entries in the chunk.

The type-field is a hard-coded list, and can have 1 flag: is this is a list or not. If it is a list, when reading the entry the field will be start with a gamma telling you how many items to expect.

So how does OpenTTD react on headers that are different than what he expects?

  • If the order of fields is different, it will adjust the order to match what is in the savegame. So you are free to move around settings, members, etc.
  • If a field type is changed, loading fails. The developer should have bumped the version and made the conversion code as we already do.
  • If a field exists in the savegame but you don't know it, it will be skipped reading.
  • If a field doesn't exist in the savegame but you expect it, it will be skipped reading.

These last two are rather important: if the adding or removing of a field influences the gameplay, you should also bump the savegame version. Often this will mean: only removing doesn't need a bump, adding most likely does. But this is up to the developer.

To keep backwards compatibility, in src/saveload/compat are now tables with the original order of fields. As this wasn't stored in the savegame, this table is used.

Finally, to top off this PR, I created https://github.com/TrueBrain/OpenTTD-savegame-reader . This is a very simply Python application that only knows the savegame format. It doesn't know anything about any chunks. And still it can read savegames made with this PR fine. The reader is meant as verification that the client indeed creates savegames that can easily be read by external tools.
As bonus, it allows us to export savegames as JSON.
And because I love weird shit, I also made it for the web: https://truebrain.github.io/OpenTTD-savegame-reader/
:D

Testing

I did some extensive testing with this:

I have a test-suite of ~500 savegames, ranging from TTDp savegame up to 1.11.2. A total of over 100 different savegame versions.
For every savegame I did:

  • Save the old savegame, call it "main".
  • Load the old savegame (call it "original"), run it for 256 ticks. Save the game, export as JSON.
  • Load the "main", run it for 256 ticks. Save the game, export as JSON.
  • Compare the JSON files. There should be no difference, in theory.

And except some minor totally unrelated bugs, this is in fact the case. That shows that at the very least everything that was loaded is saved and loaded correctly again.

Lastly, I did a lot of testing by saving a game with this PR, and then removing fields from the code. That all seems to work out fine .. I can even remove complete structs, and it will do the right thing.

Limitations

  • MAP-chunks (with the exception of MAPS) are still a CH_RIFF. I could not think of a sane and useful way to convert them to tables, so I left them alone.
  • The compat tables only mention the order of the fields, not the type the field was original in. This could be an addition to this PR, but honestly, not sure that really adds value.
  • AIPL and GSTR contain "junk" at the end of each entry. This is the blob of data saved/loaded by AI/GSes in Squirrel. In theory this could be put in a table too, but the free format it has makes it a bit difficult. To not make this PR an endless deep hole, I kinda scoped that out of this PR for now. I might come back to this decision.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain force-pushed the settings-rework branch 3 times, most recently from 6c0081a to cd3043f Compare May 30, 2021 20:00
@JGRennison
Copy link
Contributor

Types that differ between savegame and client result in a saveload error. The developer should have added glue code and bumped the savegame version for these instances.

Where do you envision this glue code going? There doesn't seem to be an obvious way to handle something like a particular field being made wider at a particular savegame version with this scheme.
(This is likely more an issue for other chunks, rather than settings).

@TrueBrain
Copy link
Member Author

Types that differ between savegame and client result in a saveload error. The developer should have added glue code and bumped the savegame version for these instances.

Where do you envision this glue code going? There doesn't seem to be an obvious way to handle something like a particular field being made wider at a particular savegame version with this scheme.
(This is likely more an issue for other chunks, rather than settings).

For now, I suggest we keep the current system in place. So if you want to change the type, you will have to bump the savegame version and add a line for the old and new version, with the correct conversion attached to it. And there is always AfterLoad to fix up any real weirdness we might run into :D

@TrueBrain TrueBrain force-pushed the settings-rework branch 4 times, most recently from 1a83aa3 to bf0c2c6 Compare June 3, 2021 12:41
@TrueBrain
Copy link
Member Author

Possibly the last commit should go in another PR, but I have been experimenting with adding a header for more chunks, mostly the CH_ARRAY ones.

Basically what I did, is add two new chunk-types: CH_TABLE and CH_SPARSE_TABLE, which are really similar to their ARRAY counter-part, but they have a header at the start.

Next, I made sure SlCompatTableHeader loads both situations correctly; it checks if CH_ARRAY or CH_TABLE is expected (based on savegame-bump) and either loads the compat-header-table or the table from the savegame. The rest is really similar to how I did settings.

I converted just a few, and not all yet, as it is very boring job, and I first like to get some opinions if this is something we would want.
Mainly, please take note of game_sl.cpp. It is a really odd chunk to start with, but basically it is two chunks in one: one small blob to indicate how many blobs are following. So basically:

<entry> <counter> <value1> <value2> .. <valueN> (with N = counter)

Putting this in a header is not really possible; I solved this by making the header <entry> <counter> <value>, and leave it to Save/Load to know the value is repeated. But that does mean if external tools are reading the savegame, they still need to know these quirks of the chunk, where for other chunks they can just read (and mostly understand) the savegame completely. So I am not that big of a fan of this. But possibly we can rework how this chunk is saved anyway, as it is a bit of a weird code either way.

I am sure I will find more of these places as I convert more, but first, let's talk if this is something we want :)

@TrueBrain TrueBrain marked this pull request as draft June 3, 2021 13:56
src/saveload/compat/settings_sl_compat.h Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Outdated Show resolved Hide resolved
src/saveload/saveload.cpp Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain force-pushed the settings-rework branch 3 times, most recently from 7fe2a31 to e6e9d02 Compare June 4, 2021 08:42
@TrueBrain TrueBrain changed the title Add: store settings as key->value pair Add: store table header for each chunk in savegame Jun 4, 2021
@TrueBrain TrueBrain force-pushed the settings-rework branch 13 times, most recently from e7c369a to 6483aa2 Compare June 7, 2021 09:36
@TrueBrain TrueBrain force-pushed the settings-rework branch 2 times, most recently from 33c6226 to cbaa374 Compare June 19, 2021 11:51
@rubidium42
Copy link
Contributor

To me this looks okay. There's only one thing that we should be aware of. Merging this means that the "variable" names of objecs that are saved in the savegame do become fixed, i.e. part of the savegame format. Changing the variable name means having a savegame bump, or at least noting that the variable is named differently from the name in the savegame. So that gets uglier in the future.
Here I'm thinking about imaginative names like name_1 and name_2 in company, and other inconsistencies. I know such changes are not part of this PR, so the question is whether someone is bothered enough by such names in the savegame to fix them before merging this PR, or whether no one can be bothered and we are happy to put such names in the savegame.

@TrueBrain
Copy link
Member Author

A very valid question. Two things that I think are worth mentioning in this context:

  1. currently SLE_NNN get their name from the field, and cannot be changed. This is a bit of a loss, as that would often be a solution when renaming fields. But the best solution for this would be to remove (or reduce) the macros and make it into C++ members where an optional parameter "name" can be entered to influence this. I don't really know how this should look, but that for sure is something we are going to need in the future.

  2. I suspect that in the future we need an "alias" list of sorts, to keep backwards compatible names working over multiple revisions. To take your example, if we rename name_1 into, say, name_company, we would add an alias for that to the table so old games still load correctly. This would allow us to easily rename fields without worrying too much about all this.

I considered adding both in this PR, but this would add unused code to our code-base, in the hope it fits a future use-case. That I always find a bit tricky, so I opted to not do this. Mainly as we haven't renamed fields like this in years .. and I don't see a reason this would change any time soon :D

Another thing I considered here, and this is what you mention, is that although the name becomes part of the savegame, it is still an "internal" name not meant for the player. It does make tooling much easier, but I think they can live with weird names (they have to assign meaning to it anyway). This is why I opted not to change any names or fields for this PR, as that would make this already insane deep rabbit hole even deeper :P And also a lot larger .. as we have a lot of weird names :D

So in my opinion, we should keep it for future-work to rename fields, and add the "alias" list in some form. But in all honesty I am not sure if/when this will be picked up by us .. so I fully accept if we think that is too much of a risk, and that we already should do it for this PR. But this is I think exactly what @rubidium42 is also mentioning here :) So this extra text purely for context :)

@ldpl
Copy link
Contributor

ldpl commented Jun 27, 2021

I thought about it for a while but I still fail to see a good reason for changing the format other than "it just happened".

Because when I think about tooling having an external schema/metadata would make much more sense imo as with minimal modifications that would allow parsing any savegame version and help other applications that do not work with savegames directly like server configuration system.

Lastly, for a while now we have somewhat the ambition to move to SQLite as savegame format. But getting there from where we were, is far from easy.
I am the type of person that doesn't like to go from A to Z immediately, but I do like that the changes we make are at least on the route towards Z. This PR not only proves SQLite should be possible, but it also prepares our SaveLoad code enough that SQLite would be possible.

TBH, to me this sounds like a very good reason not to merge this PR. As a PoC for something that's still untested and undecided yet will leave a technical debt for as long as OpenTTD supports old savegames, which is seemingly an eternity.

Next, we quickly figured out this was an universal issue with our savegame format. It allows for no modification of order of anything .. often leaving the code a bit weird and odd, because of what is stored on disk. This tightly-coupled information has made many mistakes in the past.

This reason is very hard to judge without specifics as, in my mind, mistakes are more about the code and documentation than the format itself and I fail to see how adding additional conversion layer and extra flexibility along with voiding all the previous changes as possible examples for further modifications will help to reduce mistakes.

Even if settings ordering is such a huge problem why not solve it in some other way? Like, making it explicit, for example. Or, even the same table-like format can be made with integer unique field id's instead of strings eliminating the whole aliasing issue entirely.

I mean, I can see some advantages in this table/json-like self-describing format but none of them seem to be even mentioned in this PR. For example:

  1. It's easier to work with "blindly". I.e. when there is insufficient documentation or when repairing/debugging a broken file.
  2. Good potential for forward compatibility.
  3. Good extensibility, so, with some additions, can be helpful for patches/patchpacks.

But, question is, how much of that is really a concern for the savegame format?

@TrueBrain
Copy link
Member Author

I thought about it for a while but I still fail to see a good reason for changing the format other than "it just happened".

You seem to be confusing some things and taking other things out of context. This in term seems to confuse some other people again :D I already gave this a spin on Discord, but let me try here too.

First of all, you seem to be hang up on SQLite. This is one of 3 (!) motivations, and the lesser of the three (which is obvious by the way I wrote the motivations). This PR moves towards that, but this is not a PoC, this is not "untested" nor is this adding to technical debt. If anything, pretty much the reverse (it removes technical debt). I am not sure why you are zooming in on this one part about SQLite, seemly ignoring the rest. Using SQLite as goal just shaped the way of the header, in the midst of the other choices available. Means to an end, not any end on its own. And most important of all, not part of this PR at all. Just because someone says: we are going north, doesn't mean we are going to the north-pole. It just means we take a direction to walk to, and we will see how the scenery looks when we get closer. Clearly this doesn't help for you, but many other people find it useful to know we didn't take a direction blindly and "just did what-ever", but had an idea in our mind when shaping this PR.

Next, you seem to be a bit mistaken by what this PR does. Primary, as is apparent from the history of this PR, its motivation, and its comments, this allows settings to be in any order, instead of the current hard-coded fixed order. From this grew its superset that applies this identical solution to all chunks.
And sure, there are tons of other ways to solve this problem .. but isn't that always the case? You fail to give any argument why this would be a bad approach, other than "I do not like it". So I am confused what you complaint is .. that we didn't pick any other solution? Wouldn't that lead to exactly the same argument, as we didn't pick this solution in that case?

Lastly, you finish with advantages of this PR, leaving me puzzled. From where I am standing, you are complaining for the shake of complaining, and you are not presenting any argument why this would be a bad idea. So I also don't really have a way to help you here, as "I don't like it" is a valid opinion, but hardly an argument one can have a conversation about.

So just to reiterate what the PR does / PR description says: this PR decouples code-order from savegame-order, so we can freely change order of fields in the code without breaking savegames or having the write compatibility code for this. This finally makes some things possible, most of all reworking the huge settings.ini we have, and splitting it up in much smaller chunks. This is after all, as the motivation states, the reason this PR exists in the first place. But similar problems/solutions apply to other parts of the code too. But that is future-work.

And could this have been done an other way? Yes, of course. But this is always the case, no matter what. But this is not an argument not to do this. Just merely an observation.

If there are arguments against this, I would love to hear them. But so far all I read is: "I don't like it". Which is a perfectly fine opinion; just not one I share :D

@ldpl
Copy link
Contributor

ldpl commented Jun 27, 2021

Isn't the disadvantage blatantly obvious? You get two formats to maintain instead of just one. So now every tool that needs old save games needs to parse both. Every tool that doesn't needs a rewrite anyway. Every change to the savegame needs to consider both formats. Every old patch is broken, everyone who wants to make a new one needs to learn how and there a no examples. And none of it can ever be undone because it's needed for backward compatibility. How's that not a HUGE technical debt?

Sure that will make some changes easier in the future but it also doubles the logic in some other places making a lot of other changes more complex. So now the question is, is it really worth it? And this is where it gets weird because you mention a lot of stuff that it's supposed to be good for but when I look at anything specific it seems that it doesn't really solve it well and there are much better and simpler solutions. Which makes me feel like this PR is a solution looking for a problem. And that's not the right approach as it mixes cause and effect and makes any constructive conversation impossible.

Why am I hang up on SQlite? Because this PR is a huge leap in SQLite direction. In my eyes SQlite is the only reason that explains every decision here perfectly. Yet you keep pretending that it's not really relevant and just some mythical lesser "direction". So I come to the conclusion that what's really happening here is that you're preparing the code for SQLite and everything else is just an excuse to sneakily justify this step.

You say it's good for tooling? Well, what tools did you consider? All I see that it's somewhat helpful for diffing and very simple viewers/renderers.

You say settings need to be reordered in ini file, why so? For all I see there is not much problem with existing system, if anything the fact that settings are already split in multiple files makes it only harder to deal with. And how important reordering is anyway, why are you willing to sacrifice the convenience of other changes like renaming for it? Also you removed the reason of separating private settings from the motivation so I didn't comment on that but from what I understand you could've done that anyway as they are not stored in the save file. And even if you want to reorder and split everything so badly why not just add an explicit order? Or some calculated order? It's a minor change and much easier to maintain than name and type and old order that you need anyway. Why does the savegame format itself has to be orderless to make code better? It could always use the same mechanism it uses for old savegames and get rid of all the logic doubling for even cleaner code. Also, it would be much better if instead of abstract words you showed some actual code that could benefit from having an extra orderless format.

And I finish with advantages of the format because that's what I could see as possible reasons for this change yet they're left completely out of scope of this pr. I could offer some suggestions on how to make it more useful for my patchpack, for example, but what's the point if that's not an official goal of this pr?

P.S. Basically, I do a lot of stuff that's affected by this pr: tooling, pathpack, prs and from what I see the marginal benefits it brings are grossly overwhelmed by the extra complexity it adds. That's why "I do not like it".

@TrueBrain
Copy link
Member Author

Why am I hang up on SQlite? Because this PR is a huge leap in SQLite direction. In my eyes SQlite is the only reason that explains every decision here perfectly. Yet you keep pretending that it's not really relevant and just some mythical lesser "direction". So I come to the conclusion that what's really happening here is that you're preparing the code for SQLite and everything else is just an excuse to sneakily justify this step.

Holy crap dude ... you have some nerve. I spend a lot of time on you, explaining this in several ways. And STILL you insist I lie to you and try to deceive you? This is a problem on your side, not mine. Feel free to assume everyone is out to get you, but in reality, what I said earlier is true, and nobody is pretending. But .. when someone says things like you said, in my experience, it is pointless to argue, as you already made up your mind. I can only repeat earlier statements: this is not true. Or to put it in modern words: you created an alternative fact. Feel free to bite in it, but in reality, it does not exist.

I think I said all I can say about this.

@ldpl
Copy link
Contributor

ldpl commented Jun 27, 2021

I'm not saying you're lying I'm just saying how it looks like to me because so far that's the only explanation that makes sense.

@TrueBrain
Copy link
Member Author

TrueBrain commented Jun 27, 2021

Isn't the disadvantage blatantly obvious? You get two formats to maintain instead of just one. So now every tool that needs old save games needs to parse both. Every tool that doesn't needs a rewrite anyway. Every change to the savegame needs to consider both formats. Every old patch is broken, everyone who wants to make a new one needs to learn how and there a no examples. And none of it can ever be undone because it's needed for backward compatibility. How's that not a HUGE technical debt?

Just to help fellow developers understand that this is a complete alternative reality that is being created here, lets digest:

  • With this PR there are no "two formats that need maintaining". This is a complete misunderstanding what is happening here. Yes, this PR creates 2 new chunk types, but this PR also removes the usage of 2 others. In a very pedantic sense you could state that are two formats, but in no sensible way it that is true. It is as much creating two formats as making any change to any chunk is creating two formats.
  • "every tool" after this PR only needs to add 1 more way to parse ALL chunks. Instead of the current constant updating for every savegame version out there. So in the long run, this is an absolute win for any tooling out there, and heavily simplifies the world. In other words: instead of tools needing to update every savegame version we have, they only have to update once more, and for-ever be compatible. [Edit: fun fact, if you don't want to use the header in your tooling, you can simply skip it (as it is length-prefixed) and keep on doing it the old way. If that is your thing, nothing stands in your way]
  • "every change to the savegame needs to consider both formats" is a weird statement. It again assumes there are two formats, which there are not. But yes, change to the saveload code (the core code) needs to support two more chunk-types. Hardly an issue, given the simplicity of those two chunks (they are basically two existing chunks with a header). saveload chunk descriptions did not change at all, and don't need to know anything about this. [Edit: in fact, the most complicated part about the saveload core stuff is the "compatibility" code, as it needs to reorder the list for older versions]
  • "old patch is broken" is a lie. Any change to chunks will just merge fine. New chunks still works. They might want to upgrade to the new system, but this is not enforced. Nothing changed in how saveload chunk descriptions are defined. Well, you no longer have to do SLE_NULL, so there is even a simplification! So worst case, a patch would have to remove those lines, if they would have added them. Hardly worth calling it "broken".
  • "needs to learn how" is a lie. There is nothing new to learn. Two extra lines need to be added in a chunk Save/Load handler, and this can be copy/pasted from anywhere. There is nothing "new" here.
  • "there a[re] no examples" is a lie. There are 50+ examples. As much examples as we had before this PR.
  • "none of it can ever be undone" is a weird statement. It is as much true as saying that the feature which allows towns to build tunnels cannot be undone. That is just being scared of the future. [Edit: I have been pointed out that this is a reference to keeping these savegames working. But that is really simple; if it would ever come to that (please no), it would be as simple as merging CH_TABLE back into CH_ARRAY with some minor code (about 4 lines of code) in the Load handler to skip the header (it is length-prefixed). So that is hardly difficult. So that makes this statement untrue]

I cannot state enough that you do not seem to understand what this PR does. And I run out of energy to explain .. all you currently do is confuse other developers by making them think things are happening that are not really happening. I really did my best trying to explain you things, but as you keep skipping the "ask questions" part, and keep jumping to the "I made up my own story here", it is REALLY difficult to explain this, otherwise pretty simple, PR.

I'm not saying you're lying I'm just saying how it looks like to me because so far that's the only explanation that makes sense.

Let me paraphrase what you write: "I am not saying you are lying, but I am thinking it". Yes. Great job dude. you made this a whole lot better.

Just because something doesn't compute in your worldview, doesn't mean we are trying to hide stuff and sneak things in here. If I would want that, I would have gone about this completely differently, and never even mention it. I mean .. wtf dude.

We are back to what we talked about before ... you saying weird stuff, to which we have to defend. Again I advise you to ask questions. Be curious. "how do you see this happening?", "have you considered this?", etc. That is a conversation worth having. This constant bashing isn't gaining anything to anyone .. and all you get in return is this shitty post of mine as I run out of energy to make it polite. And I am sorry about that, as I hate to be like this .. but I am also out of ideas here.

I could offer some suggestions on how to make it more useful for my patchpack, for example, but what's the point if that's not an official goal of this pr?

This is such a shitty thing to say. You seem to forget this is not a company building a game. We are Open Source. Most of us do shit because we enjoy doing it. So how does this remark contribute to that? It is just shitting over this PR for no good reason.

First, you bitch you want a motivation. I write a lengthy motivation and description; next you bitch you have an idea but don't want to share because it is not an "official" goal of this PR? Wth? How did you drift so far away? Please get back to shore, where we can all walk in the field of dandies and enjoy the sunset. There really is no need for this .. if you have a good idea, share it. We do not have "official goals". I wrote this PR because I had an issue related to this in another PR .. that happens with rabbit holes. And others suggested to broaden the scope a bit. I don't mind. I enjoyed writing this PR. So now this is here. And if you have a good idea that would make it even better, share that idea.

I really think you somewhere took a turn, and got in a bad vibe when it comes to this PR, and completely filter everything you read in relation to it in a very dark black colour. That is really not needed, and really not as constructive as you might think. Please, come back to shore .. please, let's walk in dandies ... please don't insult me and call me a liar. I would really really appreciate that. Thinking we have alternative motives is .. really not true, as we are very very open about our aims and goals.

@ldpl
Copy link
Contributor

ldpl commented Jun 28, 2021

First, you bitch you want a motivation. I write a lengthy motivation and description; next you bitch you have an idea but don't want to share because it is not an "official" goal of this PR? Wth? How did you drift so far away? Please get back to shore, where we can all walk in the field of dandies and enjoy the sunset. There really is no need for this .. if you have a good idea, share it. We do not have "official goals". I wrote this PR because I had an issue related to this in another PR .. that happens with rabbit holes. And others suggested to broaden the scope a bit. I don't mind. I enjoyed writing this PR. So now this is here. And if you have a good idea that would make it even better, share that idea.

You realise that when I tried to talk about that lengthy motivation you bitched that I'm doing it wrong and now you're suggesting I should instead talk about things that weren't even mentioned anywhere before?
Am I supposed to read your mind or something? You're the one suggesting the change, take some effort to explain the motivation behind it properly instead of endlessly blaming me for having an opinion.

If pathes/patchpacks are a part of this PR, add that to motivation. If tooling/forward compatibility is, add it. And if SQlite isn't why is it there?
Reordering settings and forward compatibility are goals of incomparable significance, the outlook of this PR changes entirely if it's primary goal is different.

With this PR there are no "two formats that need maintaining". This is a complete misunderstanding what is happening here. Yes, this PR creates 2 new chunk types, but this PR also removes the usage of 2 others.

This "is a lie". You change the core principle of how the data in the format is identified. Instead of it being done by order it's now done by string id/name. You constantly try to downplay the significance of it but to me that's the very definition of format change, adding sqlite after that is just a technicality. Everything else I talk about is just a consequence of this change.

fun fact, if you don't want to use the header in your tooling, you can simply skip it (as it is length-prefixed) and keep on doing it the old way.

This "is a lie". If that was the case why did you change the way the game parses it? You could just use the same old way simplifying the codebase. Where are any guarantees that field order can still be used for identifying the field? For old format that was it's very nature, for the new one it's the side effect of the implementation if anything.

In other words: instead of tools needing to update every savegame version we have, they only have to update once more, and for-ever be compatible.

This "is a lie". That is a very strong forward compatibility guarantee. But not only there is not a word about it in this PR but I don't even see how you could possibly keep it for any tool that actually works with data rather than just dumping it mindlessly like your viewer. For example, how is it supposed to be forever compatible if fields it works with are removed?

Here are some basic changes I could think of that could be done to the data:

  1. reorder settings
  2. add new setting
  3. remove setting
  4. rename setting
  5. extend setting type (like uint16->uint32)
    6-10) same as 1-5 but for non-setting fields

For most of them, there are some examples in the changelog for the old system. Where are examples or explanations for the new one? You seem to be trying to say at the same time that any change is done exactly as before and that the new system doesn't require a savegame version bump now. Those two statements contradict each other, so where's the truth? For all I see renaming is pretty much impossible now as aliasing or whatever is required for it isn't even there and if it's added it's likely going to affect other operations as well due to the necessity of avoiding name conflict between active fields and old aliases.

If tooling is the goal, how are tools supposed to be made to "forever be compatible" with these changes?
If patchpacks are the goal how are they supposed to be made to not be broken by these changes?

@TrueBrain
Copy link
Member Author

If pathes/patchpacks are a part of this PR, add that to motivation. If tooling/forward compatibility is, add it. And if SQlite isn't why is it there?
Reordering settings and forward compatibility are goals of incomparable significance, the outlook of this PR changes entirely if it's primary goal is different.

You assign way too much value to "the goal of this PR". That is not how OpenTTD works, or I think Open Source in general. We do stuff because we had an itch, we scratch that itch, that results in some code changes. You keep trying to force me to make up motivations why I did this PR, but I am not going to do that. I am not going to retroactively make up stuff .. that is not how I roll.

Why I wrote this PR, why it exists, all has been said and done. You clearly don't believe it to be true, that is your right. But there really is nothing more to say about this. Please stop trying to corner me in the hope I say something so you can go: "HA! Now I got you!". That is weird.

If you see other possibilities for this PR we hadn't considered, speak up. If you think other doors are closed, speak up. But don't keep saying: "it is not part of the motivation so I am not going to talk about it". That is not what the word "motivation" means.

With this PR there are no "two formats that need maintaining". This is a complete misunderstanding what is happening here. Yes, this PR creates 2 new chunk types, but this PR also removes the usage of 2 others.

(..) Instead of it being done by order it's now done by string id/name. (..)

I am really unsure where you got this from, as this really is not what is going on. At the risk of repeating myself: this PR adds a header to each chunk to indicate the order of fields. That is all. It is an extra step in the save/load process to store the order in the savegame, instead of only in the code. This decouples code-field-order from savegame-field-order.

fun fact, if you don't want to use the header in your tooling, you can simply skip it (as it is length-prefixed) and keep on doing it the old way.

This "is a lie". If that was the case why did you change the way the game parses it? You could just use the same old way simplifying the codebase. Where are any guarantees that field order can still be used for identifying the field? For old format that was it's very nature, for the new one it's the side effect of the implementation if anything.

Again I think you are misunderstanding this PR. I am pretty sure I know the code I wrote .. and I am very sure (said sarcastically) you can skip the header and still be able to read the savegame like you used to. This is a rather fundamental part of this PR. See the markdown document about the savegame format for more details. But to repeat myself: the only thing that changes on disk is that CH_TABLE has a header; otherwise it is identical to CH_ARRAY.

In other words: instead of tools needing to update every savegame version we have, they only have to update once more, and for-ever be compatible.

This "is a lie". That is a very strong forward compatibility guarantee. But not only there is not a word about it in this PR but I don't even see how you could possibly keep it for any tool that actually works with data rather than just dumping it mindlessly like your viewer. For example, how is it supposed to be forever compatible if fields it works with are removed?

This are two totally different things you throw together. So lets split them apart. No, this is not a lie, as that is what this PR actually does: add information to the savegame so the order of fields can be understood. This means that if we reorder fields, no tooling has to suffer.

The other thing you ask: but what if tooling tries to understand the records, more than just read. Interpretation of chunk-data is always up to the tool of course, as there is nothing we can otherwise hint to what its function is. What I am a bit puzzled about is what you see as an issue here. This was already the case, and will remain the case. If a field is removed, and the tool was putting a value to it, nothing is changing: before this PR they would have to adjust, and after this PR they would too. I must be missing something here, as this seems a partial question of a use-case you have in your mind. But as you talk a bit in riddles, I am left to guess what that use-case is.

So lets take an example: we remove the supplied fields from industries. No clue why, but that is what we do (in other words: this is a hypothetical!). A tool is showing all the supplies an industry is giving, live on a website for a server that is running. It saves the game every minute, and the website updates accordingly. So if that field is removed, the tool stops working. But that has little to do with this PR .. that is just .. if information is removed from the savegame, a tool cannot work with it anymore.

The only edge-case, which I pointed out a few posts back, is when we start renaming fields. That is the bit awkward part in general, and we have to find a decent solution if/when we do this. But as it is rather unusual / unlikely for fields to be renamed, this is not a concern we currently carry.

Maybe you misread what I write about "compatible". I am talking about how data is stored in the savegame, how you can read the information out of it. Not about interpretation of the data itself. That is, and most likely always will be, a problem of the tooling. But this PR doesn't change that in any way shape or form. That was an issue before this PR, and that will remain an issue after. Most likely even till the end of time.

Here are some basic changes I could think of that could be done to the data:

1. reorder settings

2. add new setting

3. remove setting

4. rename setting

5. extend setting type (like uint16->uint32)
   6-10) same as 1-5 but for non-setting fields

For most of them, there are some examples in the changelog for the old system. Where are examples or explanations for the new one? You seem to be trying to say at the same time that any change is done exactly as before and that the new system doesn't require a savegame version bump now. Those two statements contradict each other, so where's the truth? For all I see renaming is pretty much impossible now as aliasing or whatever is required for it isn't even there and if it's added it's likely going to affect other operations as well due to the necessity of avoiding name conflict between active fields and old aliases.

This is a lot of things mixed together, and you pick a single one out of your lists and say: this is not handled by this PR so this is wrong. But that is heavily misrepresenting this. And I think this is why you see a contradiction where there isn't one.

The description also mentions all of these cases, but lets go through it again, worded slightly different:

  1. reordering can be done freely without savegame version bump. You can just move the field. This wasn't the case before this PR, and reordering was nearly impossible. In fact, I think it has never been done in the history of OpenTTD.
  2. adding a new setting can be done freely without savegame version bump. One does have to be mindful: if an older version of OpenTTD loads the savegame, is something going wrong because he skips this field. In general, it is advised to still bump the savegame version to prevent that from happening.
  3. removing a setting can be done freely without a savegame version bump, and has no consequences.
  4. renaming, I wrote a reply earlier in this thread that this is the main thing that we would have to look at. But again, it is a rare event in general, so possibly nobody is ever going to look at it.
  5. extending settings type is done in the exact same way as it currently is. Nothing changed on this level with this PR, and you are absolutely free to change an uint16 to an uint32. The other way around, narrowing, does need love and attention. But again, this is unchanged behaviour.
    6..10) exactly the same as above.

As for "there are no examples" argument, I hope you agree that for 1, 2, and 3, examples are .. not useful. I mean, 1) is changing swapping two lines of code. 2) is adding a new line of code. 3) is removing a line of code. Hardly worth an example. This is why the description puts it in words.
4) we talked about, and 5) has been done tons of times. As this is unchanged behaviour, logical, no example is provided.
6..10 are identical in answers to 1..5.

If tooling is the goal, how are tools supposed to be made to "forever be compatible" with these changes?

You seem to misunderstand what I wrote: tools can read the savegame in full with this PR for ever to come. This in contrast to before this PR, where it is absolutely needed to know each chunk in order to digest the savegame safely. What the fields and values represent, that is of course another story. But that is not what I was referring to. Being able to always safely read the savegame is a result of this PR.

If patchpacks are the goal how are they supposed to be made to not be broken by these changes?

I have no clue what you try to ask here. This is very vague and could mean a lot of things. You would have to be a bit more specific.

Despite the tone of these questions, I am happy that you are asking questions. I can answer questions. I cannot answer assumptions .. as I would first need to understand what the assumption was you made. So keep asking questions, that is a good thing. Tnx for that!

@michicc
Copy link
Member

michicc commented Jun 28, 2021

  1. reordering can be done freely without savegame version bump. You can just move the field. This wasn't the case before this PR, and reordering was nearly impossible. In fact, I think it has never been done in the history of OpenTTD.

I'll skip most of the things here, but if reordering should be allowed without any savegame bump, any tool is required to use and parse the chunk header, somewhat contradictory to saying that tools can just skip the header and read it as before.

@TrueBrain
Copy link
Member Author

TrueBrain commented Jun 28, 2021

  1. reordering can be done freely without savegame version bump. You can just move the field. This wasn't the case before this PR, and reordering was nearly impossible. In fact, I think it has never been done in the history of OpenTTD.

I'll skip most of the things here, but if reordering should be allowed without any savegame bump, any tool is required to use and parse the chunk header, somewhat contradictory to saying that tools can just skip the header and read it as before.

Not what I meant, but fair point. What I meant to say: you can skip the header and do it the old-fashioned way, where you of course need to know the order of the fields to decypher the bytes. That was the case before this PR, and is the case after. So if you don't want to use the header, you still can.

Basically what I try to say: this PR really only adds a header to assist reading. By no way it is mandatory. But of course not using it does require you to know the order (and type) of each record another way.

Does that clear it up a bit?

Edit: on third read, you might mean that without a savegame bump you cannot know the order anymore. That is a fair point. So yeah, that was a silly statement of mine that you can skip the header .. after the first reorder, you can't practically do that anymore. Fair point :)

@michicc
Copy link
Member

michicc commented Jun 28, 2021

You can only know the order if you know the specific savegame revision. If it is not coupled with some version marker change, you can only know this by looking at the chunk header.

I'm not saying this is good, bad or whatever BTW, this is just a statement.

We won't be able to make it fully self-descriptive (looking at you
MAP-chunks), but anything else can. With this framework, we can
add headers for each chunk explaining how each chunk looks like
in detail.

They also will all be tables, making it a lot easier to read in
external tooling, and opening the way to consider a database
(like SQLite) to use as savegame format.

Lastly, with the headers in the savegame, you can freely add
fields without needing a savegame version bump; older versions
of OpenTTD will simply ignore the new field. This also means
we can remove all the SLE_CONDNULL, as they are irrelevant.

The next few commits will start using this framework.
When a header is added, the chunk changes from CH_ARRAY type to
CH_TABLE type.
We no longer need them. If you want to remove a field .. just
remove it! Because of the headers in the savegame, on loading,
it will do the right thing and skip the field.

Do remember to bump the savegame version, as otherwise older
clients can still load the game, but will reset the field you
have removed .. that might be unintentially.
IsSavegameVersionUntil() did a [0, N] check, not [0, N) as the
name suggests.

Until can be a confusing word, where people consider it to be
including the upperbound. Dictionary states it means "before",
excluding the upperbound. There are long debates about who is right.

So, simply remove away from this ambiguity, and call it "before"
and "before or at". This makes the world easier for everyone.
…eries::Load

num_liveries indirectly contained the same information, but this
makes reading these things pretty difficult. So use IsSavegameVersionBefore()
like everywhere else instead.
@TrueBrain TrueBrain merged commit 1d99121 into OpenTTD:master Jul 2, 2021
@TrueBrain TrueBrain deleted the settings-rework branch July 2, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants