Incorporating changes from Standard

[neroden]

OK. I'm up to 7501.

There was a missing comment change from r7495 which I caught, proving that there is value to going through this.

While looking into 7502, I stumbled across some *complicated* issues and so I'm going to be working on some other code cleanups and bugfixes before getting back to the merge from standard.

The first of these is a bug fix for a bug I was encountering in bridgewater-brunel.  I have tested this and the bug is, in fact, fixed. 

This is on the priority-loading-bug branch (please merge, as noted in the patch requests forum).  That is branched from the merge-from-standard branch so if you merge the priority-loading-bug branch you'll be up to date on my merge-from-standard progress too.

[jamespetts]

It seems that my understanding of the standard save version was incorrect.
check it again please. In my tests some older builds successfully launched from the generated xml.
While the merge order from standard is inconsistent, it is better to align with the implemented extended revision.

Thank you very much for that - the settings.xml problem appears to have been resolved. However, I notice now that querying game servers no longer works: even if I query the Bridgewater-Brunel server manually in the network dialogue, I get an error message "server did not respond". I do not get this error when querying with the master branch - if I check "show mismatched", it correctly shows the current game state on the Bridgewater-Brunel server. Choosing from the list gets a "server version too old" error message in place of the query data, which is potentially correct, but "server did not respond" is not correct.

I am still not sure, however, whether this is an actual problem with the server querying code or whether this is just an bug giving rise to the wrong error message being shown and the underlying issue is simply that the server querying data format has changed.

[jamespetts]

Further testing shows that this is definitely a bug, I am afraid: starting a new server locally with this branch and then trying to query it with another instance of the same build produces the error (shown in the query window) "send of NWC_GAMEINFO failed". 

Force connecting to the server (net:127.0.0.1 in the "load" dialogue) works, so the problem is specific to querying.

[neroden]

Yes, indeed - looking at my config.default, builddir and progdir are indeed the same. In config.template, they are also both the same, but set to a variable:

Code Select Expand

BUILDDIR = $(shell pwd)
...
PROGDIR  = $(shell pwd)


Huh.  When I changed it in config.default for myself I didn't realize that I hadn't ported the changes to config.template...

...OK, patch incoming.  On branch config-template-update.  Give it a shot...

[jamespetts]

Huh.  When I changed it in config.default for myself I didn't realize that I hadn't ported the changes to config.template...

...OK, patch incoming.  On branch config-template-update.  Give it a shot...

Thank you - now incorporated.

[jamespetts]

Ranran - thank you for your latest pull request on the std-r10996 branch. I have tested this, but, unfortunately, this does not solve the problem described above relating to querying the state of network servers: the original behaviour persists.

Additionally, there is now a problem relating to scrolling in the load and save game windows, where moving the mouse pointer up and down over the windows will scroll the list, whereas the mouse wheel does not work. This causes it to be nearly impossible to select a file name with the mouse. I suspect that this is related to code intended to make Simutrans workable for touch screens.

[jamespetts]

As a note: the testing methodology that I had been using to verify that the network server listing was working was erroneous. Having re-tested, I have confirmed that the merging up to r10996 is now working, and have merged in Ranran's changes up to this revision in both master and ex-15 branches.

Ranran reports that this does contain the fix from Standard for some subset of the losses of synchronisation, most particularly, those associated with the failure to unpause on connexion.

I have incremented the minor version from 22 to 23 with this work, and the newly merged version will be available in to-morrow's nightly build.

[neroden]

OK, I'm up to r7505 -- there's a comment fix which was missed earlier.  Now on the r7505 branch, ready to pull.

EDIT: OK, I'm up to r7509 -- large comment changes in standard's simuconf.tab.  I think there's no reason for us to have different comments in this case.  Now on the r7509 branch, ready to pull.

EDIT: OK, r7512 was not merged previously.  This changes files in the Windows NSIS installer for installing paksets which are probably not functioning right in Extended right now.  The version currently in Extended is just the old version from Standard, so I figure adopting the patch from Standard removes differences and doesn't make anything any worse.  This is on the r7512 branch (cumulative with the other two branches and ready to pull).

It's looking like the 7500s are going to be slow.

[jamespetts]

OK, I'm up to r7505 -- there's a comment fix which was missed earlier.  Now on the r7505 branch, ready to pull.

EDIT: OK, I'm up to r7509 -- large comment changes in standard's simuconf.tab.  I think there's no reason for us to have different comments in this case.  Now on the r7509 branch, ready to pull.

EDIT: OK, r7512 was not merged previously.  This changes files in the Windows NSIS installer for installing paksets which are probably not functioning right in Extended right now.  The version currently in Extended is just the old version from Standard, so I figure adopting the patch from Standard removes differences and doesn't make anything any worse.  This is on the r7512 branch (cumulative with the other two branches and ready to pull).

Excellent, thank you for that: now incorporated.

I should note that I have made some of the simuconf.tab headings more user-friendly, as "####program stuff####" is very difficult for anyone to make sense of. "#### Base settings ####" (etc.) is much clearer.

[neroden]

OK, now we're getting into the tough stuff.  r7515 is part of DrSuperGood's city growth calculation changes, and OMG, parts of this are in Extended and parts of it aren't.  I'm starting by nibbling around the edges and getting the easy stuff (format changes, comments, etc.)  The next problem I've hit is the save file format.

Here's the patch which went into Standard (which has since been modified in inconsequential ways):

Code Select Expand

@@ -1206,6 +1219,24 @@ void stadt_t::rdwr(loadsave_t* file)
    file->rdwr_long( stadtinfo_options);
  }

+ if (file->get_version() > 120000) {
+  // 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 );
+  }
+ }
+ else if(file->is_loading()) {
+  // Initalize differential statistics assuming a differential of 0.
+  sint64 const (&h)[MAX_CITY_HISTORY] = city_history_month
[0][o];
+  city_growth_factor_previous
[0][o].demand = h[HIST_PAS_GENERATED];
+  city_growth_factor_previous
[0][o].supplied = h[HIST_PAS_TRANSPORTED];
+  city_growth_factor_previous[1].demand = h[HIST_MAIL_GENERATED];
+  city_growth_factor_previous[1].supplied = h[HIST_MAIL_TRANSPORTED];
+  city_growth_factor_previous[2].demand = h[HIST_GOODS_NEEDED];
+  city_growth_factor_previous[2].supplied = h[HIST_GOODS_RECIEVED];
+ }
+
  if(file->get_version()>99014  &&  file->get_version()<99016) {
    sint32 dummy = 0;
    file->rdwr_long(dummy);
....Right.  So here's the code in Extended:

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);
    }
  }

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.  So the first clause triggers every time, and we never save the differential statistics and we never read them. 

The second clause here is logically incorrect: if file->get_extended_version() == 0 and standard_version is >= 120.1, we should be reading the differential statistics which are in the Standard file (except, we aren't using them, so I guess resetting to zero is fine?).

Correct me if I'm wrong here, but this is what I'm seeing.  I'm going to try to first adopt all the "little" changes in r7515 (which is a very big patch), and then I'm going to come back and attempt to actually implement DrSuperGood's city growth changes.  Although we will want to do city growth differently in some ways I think the version from Standard is just as good a basis for doing this as anything else, probably.  But this is going to require bumping the version numbers -- so I'm going to want some advice on how to handle that properly.

[neroden]

Related question.  Standard, in stadt_t::step:

Code Select Expand

// update history (might be changed do to construction/destroying of houses)
city_history_month
[0][o][HIST_CITIZENS] = get_einwohner(); // total number
 city_history_year
[0][o][HIST_CITIZENS] = get_einwohner();

 city_history_month
[0][o][HIST_GROWTH] = city_history_month
[0][o][HIST_CITIZENS]-city_history_month[1][HIST_CITIZENS]; // growth
 city_history_year
[0][o][HIST_GROWTH] = city_history_year
[0][o][HIST_CITIZENS]-city_history_year[1][HIST_CITIZENS];

 city_history_month
[0][o][HIST_BUILDING] = buildings.get_count();
 city_history_year
[0][o][HIST_BUILDING] = buildings.get_count();

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?

[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)