Incorporating changes from Standard

[neroden]

Meanwhile, the trivial changes are on the r7515-firstpass branch, along with some updates to cherry-picked-commits regarding the next few commits (some of which are already in, some of which are just as hard)

[neroden]

BTW, this is a possible cause of start-of-month desyncs since the save-restore on this (which is not happening in extended) is supposed to be necessary to avoid desyncs.
----

....So I'm really not clear on the versioning stuff here.  It's gotten very confusing.

I believe get_extended_version is returning ***14*** on the master branch and ***15*** in the ex-15 branch, correct?  So the check that it's less than 25 is always true?

(Master branch: EX_VERSION 14, EX_VERSION_MINOR 23, EX_SAVE_MINOR 65.  Ex-15 branch: EX_VERSION_MAJOR 15, EX_VERSION_MINOR 0, EX_SAVE_MINOR 0).

It looks like the regular "minor" version isn't used for anything (except confusing the player) any more, and only the SAVE_MINOR is?  Is this correct?  SAVE_MINOR has a dot prepended to it and becomes EXTENDED_REVISION_NR ?

I know I need to update the version number (in simversion.h) and I know I need to flag the load/save behaviour off the version number here, but I'm not sure what version numbers to use.

For the new version which does save and restore these, I think we want:
 -- EX_VERSION_14 / EX_SAVE_MINOR 65 for the master branch
-- EX_VERSION_15 / EX_SAVE_MINOR 1 for the ex-15 branch

And then I think the check should look something like this:

Code Select Expand

  // differential history
  if ( 
file->is_version_less(120, 1) || (file->get_extended_version()
> 0 && file->is_ex_version_less(14,65)) || file->is_ex_version_equal(15,0)  ) {
    // These versions did not save the factor statistics in the save file
    if (file->is_loading()) {
      // Initialize differential statistics assuming a differential of 0.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }
  }
  else if (file->get_extended_version() == 0) {
    if (file->is_loading()) {
      // Initialize differential statistics assuming a differential of 0.
      // Initializes the fourth statistic which isn't present in Standard.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }
    // load/save differential statistics.
    for (uint32 i = 0; i < 3; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
  else {
    // load/save differential statistics.
    for (uint32 i = 0; i < GROWTH_FACTOR_NUMBER; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
Does this look right?  I'm not comfortable with the versioning code.

Once it's verified that this looks right for the versioning code, then I'll go through and prepare separate patches for both master and ex-15 for the "content" code of this section (as well as the version code). 

I think there was a literal error in Extended here: I doubt it was supposed to be

Code Select Expand

file->get_extended_version() < 25This was probably supposed to be get_extended_revision or something similar.  Hard to tell.

When the correct versioning stuff is verified and after the patches for master and ex-15 are ready, tested, and accepted, I will move on to the later 7500s. 

I'm already looking at the multi-tile building code (since it happens to be in many of the same files) and this should not be a problem (remember I've worked on the code in this area quite a lot before), but I am going to keep working on the merges from standard in order from oldest to newest, to avoid trouble.

[neroden]

Seeing no comments after a week, I will try to prepare a patch assuming that I got the versioning stuff right.

[jamespetts]

Thank you very much for your work on this, and apologies for not having been able to reply earlier - I have been dealing with a very substantial matter at work and have had time to deal with little else for about a week and a half.

First of all, to answer the question in relation to this code:

Code Select Expand

// differential history
 
 if (  file->is_version_less(120, 1) ||
(file->get_extended_version() > 0 &&
file->get_extended_version() < 25)) {
    if (file->is_loading()) {
      // Initalize differential statistics assuming a differential of 0.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }
  }
  else if (file->get_extended_version() == 0) {
    if (file->is_loading()) {
      // Initalize differential statistics assuming a differential of 0.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }

    // load/save differential statistics.
    for (uint32 i = 0; i < 3; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
  else {
    // load/save differential statistics.
    for (uint32 i = 0; i < GROWTH_FACTOR_NUMBER; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
The reference to

Code Select Expand

file->get_extended_version() < 25
looks to be an error as the extended version has never got anywhere near 25. This is the major extended version, and I suspect that what was intended was that this number be 15, I assume the idea being that this would work with version 15.x onwards. That may have been coded before it was realised that that major version would take some time to be the version on the master branch.

To fix this, we need to increment the minor version and then modify the code accordingly. This will break previous games saved with the 15.x branch, but that should not be a major problem as nobody is expecting to be able to use that branch yet. Incrementing the Extended minor version is necessary as we will break previous saved games saved with the master 14.x branch (which is a problem as this is what people are playing with now), as those games will have been saved without those data.

Correct me if I'm wrong -- If I'm not very much mistaken, file->get_extended_version() is still 23 for a brand new save file from extended.

file->get_extended_version() will currently return 14 on the master branch, and 15 on the ex-15 branch, as you correctly deduce in your later post.

In summary: the major version number also affects the saved game format and is displayed. The minor version number does not affect the saved format but is displayed. The save minor version number affects the saved game format but is not displayed.

In the past, there was no separate save minor version, and all increments to the save format were displayed (but no changes, even if significant, that did not affect the save format were displayed). Then, many years ago, another coder (I forget who) thought that it would be sensible to separate these two things and came up with the above system which we have used ever since.

Your deduction that the following is the correct fix is correct:

Code Select Expand

  // differential history
  if ( 
file->is_version_less(120, 1) || (file->get_extended_version()
> 0 && file->is_ex_version_less(14,65)) || file->is_ex_version_equal(15,0)  ) {
    // These versions did not save the factor statistics in the save file
    if (file->is_loading()) {
      // Initialize differential statistics assuming a differential of 0.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }
  }
  else if (file->get_extended_version() == 0) {
    if (file->is_loading()) {
      // Initialize differential statistics assuming a differential of 0.
      // Initializes the fourth statistic which isn't present in Standard.
      city_growth_get_factors(city_growth_factor_previous, 0);
    }
    // load/save differential statistics.
    for (uint32 i = 0; i < 3; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
  else {
    // load/save differential statistics.
    for (uint32 i = 0; i < GROWTH_FACTOR_NUMBER; i++) {
      file->rdwr_longlong(city_growth_factor_previous[i].demand);
      file->rdwr_longlong(city_growth_factor_previous[i].supplied);
    }
  }
the possible exception being that we do not necessarily want to increment the 15.x version number from 0 to 1 since the file format for 15.x is not fixed and nobody should have been saving games with this expecting it to work in future versions of the 15.x/ex-15 branch yet.

Extended has removed the reset of total city population in the history here (the growth and building counts are still reset).  It's quite hard to track down when a *deletion* was done using git blame, so I'm not sure when this was changed and why.  Anyone remember?

I am afraid that I cannot recall this - my apologies.

Meanwhile, the trivial changes are on the r7515-firstpass branch, along with some updates to cherry-picked-commits regarding the next few commits (some of which are already in, some of which are just as hard)

Now incorporated - thank you.

I'm already looking at the multi-tile building code (since it happens to be in many of the same files) and this should not be a problem (remember I've worked on the code in this area quite a lot before), but I am going to keep working on the merges from standard in order from oldest to newest, to avoid trouble.

Excellent - that will be very helpful indeed.

[jamespetts]

Update: I have now implemented what I believe to be the correct fix to the versioning bug identified. Thank you for pointing this out.

[neroden]

Thanks for the reply about the version numbering.  If we don't have to maintain load-save compatibility on the 15.x branch, that makes the version logic simpler.  Opinions?  Are people running long-standing 15.x games or always starting a new one fresh?

[neroden]

Update: I have now implemented what I believe to be the correct fix to the versioning bug identified. Thank you for pointing this out.

Aaaah.... wait... let me check your work please before committing things like this!  There's some serious subtlety here and I may have to bump the save version number again if you didn't get all the bits of the change.

By making this commit *while I am in the middle of working on the same patch and the same code* you have made my job *harder*.  Please hold off for at least a few days before committing changes for a patch I am actively in the middle of working on! You have just added somewhere between 15 minutes and an hour of work for me to do.  If you've got a proposed fix to something I'm in the middle of working on, please post it for review rather than just committing immediately.

[neroden]

If you want to be helpful, give me the IDs for the relevant commit or commites you made.  I'm already comparing

(a) Standard before the patch
(b) Standard after the patch
(c) Extended before the half-assed previous patches
(d) Extended after the half-assed previous patches
And this will just add an (e) to my comparison list

[jamespetts]

My apologies: for reference, my versioning changes were in this commit. Merging your work was in this commit.

In the future, should I assume that any commit(s) in respect of which you have not issued a pull request are not ready for incorporation into the master branch? If we can establish that convention, then this should avoid the risk of our respective work clashing in the future.

[neroden]

My apologies: for reference, my versioning changes were in this commit. Merging your work was in this commit.

In the future, should I assume that any commit(s) in respect of which you have not issued a pull request are not ready for incorporation into the master branch? If we can establish that convention, then this should avoid the risk of our respective work clashing in the future.

If I don't issue a pull request through Github and I don't say "the following is ready to go, please merge it" here, yes, then it's not ready for incorporation

The firstpass stuff was ready to merge, so that's OK.

I should be able to handle the stuff with the version number now that I know what your commit was

Sorry I haven't been able to work on programming on Simutrans for a while.  (I had to deal with medical appointments for family, very stressful since offices are not practicing infection control, not wearing respirator masks, not using air filters, and we can't afford to have anyone infected with Covid by medical offices, which has happened twice before)