Incorporating changes from Standard

[neroden]

OK.  On the merge-from-standard branch I'm up to 7478.

I found my first non-comment, non-whitespace missing code here... and it's a pair of asserts (plus a const which I added to match up with the code in standard).  Should be trivially correct; take a look at the diff and then merge it into master.  I think I'll call it a night.

[jamespetts]

Thank you all for your help with this. Modifying freevariable3 to freevariable solved the remaining compile error, and I have now been able to compile and test 2403-std and its equivalent for the ex-15 branch satisfactorily, so this is now merged in to master and ex-15. I have also merged in Neroden's latest work. Thank you both very much for your ongoing work on this. 

Ranran, whenever you are able to confirm the merging of the later versions of your merge from standard work into ex-15, I can make a start on testing that, too, unless you think it better to wait until Neroden catches up with where you are with this first?

[jamespetts]

I don't know how long it will take for neroden to catch up, but I think it's a big task. My goal was to incorporate new font functionality, it's a big change, and I'm sure many players are looking forward to it. and I think that's just around the corner.

Excellent, thank you for clarifying. Do let me know, in that case, when you have the next instalment of merges from Standard ready for testing on both the master and ex-15 branches.

[RESTRICTED ACCOUNT]

Moral: do the merges in order, it saves a lot of trouble later

I think this is idealistic, and also assumes that Extended is just a baby bird fed by Standard.
I don't think things are that simple.
For example, player rankings UI seem to be incorporated into standard. I completed the implementation with Extended first.
In other words, if you look at it from another perspective, you can say that the future commits of Standard were merged first.
I also report bugs to Standard when I find them that are clearly caused by Standard's code. And if Standard fixes it, will we and the players have to be patient until all the merge work is done?
It would be ideal to completely catch up with the standard and eliminate the differences so that changes can be incorporated immediately. I think your opinion is completely correct if that is achieved.
Of course, we should get them in order whenever possible.
However, it is clear that maintaining that status is not an easy task. Because the work of everyone involved in it is a hobby service activity. Even if you work until you completely catch up, the gap will start to widen again when you stop.

It is difficult to merge correctly without understanding both codes. And the content is wide-ranging.
Therefore, there are only a limited number of people who can complete all tasks perfectly.

A.Carlotti, who was working on it, recommended that we (especially for me and Ves) wait to work on the UI until we have incorporated the major UI improvements (aligned container feature) in Extended. It is clear that it is more efficient in the long run. But he was gone and that day never came.
And Phystam tried to take it over but quickly ran aground. In the end, the only way to take advantage of the new UI features was to proceed with the work myself.
That's the story so far.

[prissi]

I think extended would also benefit to run the file sorting script "reorganise-code-r10444.sh" which would make it much easier to track changes between both ... Apart from having things in more arranged positions.

Also, I think ranran made a very good job at improving extended's GUI also with the scaled containers. But there is little need to look at code twice, i.e. the event handling support for touch should benefit extended too.

[neroden]

I think extended would also benefit to run the file sorting script "reorganise-code-r10444.sh" which would make it much easier to track changes between both ... Apart from having things in more arranged positions.

We absolutely cannot do that until we've merged the changes from standard from BEFORE 10444 -- it would make things much more difficult.  That's why I'm working on merging the changes from before 10444.

[neroden]

Thank you all for your help with this. Modifying freevariable3 to freevariable solved the remaining compile error, and I have now been able to compile and test 2403-std and its equivalent for the ex-15 branch satisfactorily, so this is now merged in to master and ex-15. I have also merged in Neroden's latest work. Thank you both very much for your ongoing work on this.

It ain't compiling on Linux for me.  The fixes are not in.  (Edit: see below, it's a config/makefile issue)

Code Select Expand

===> CXX dataobj/route.cc
dataobj/route.cc: In member function 'route_t::route_result_t route_t::append_straight_route_mostly_ocean(karte_t*, koord3d, sint32, koord3d&, bool)':
dataobj/route.cc:159:14: warning: unused variable 'land_ended_flag' [-Wunused-variable]
  159 |        bool land_ended_flag = false;
      |              ^~~~~~~~~~~~~~~
dataobj/route.cc:161:15: warning: variable 'first_water_after_land' set but not used [-Wunused-but-set-variable]
  161 |        koord first_water_after_land = koord::invalid;
      |              ^~~~~~~~~~~~~~~~~~~~~~
dataobj/route.cc: In member function 'bool route_t::find_route(karte_t*, koord3d, test_driver_t*, uint32, uint8, uint32, sint32, uint32, uint32, bool, find_route_flags)':
dataobj/route.cc:671:49: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
  671 |                                ((!current_city && (straight_line_tiles < max_commuting_distance_road_tiles) ||
      |                                  ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dataobj/route.cc: In member function 'route_t::route_result_t route_t::calc_route(karte_t*, koord3d, koord3d, test_driver_t*, sint32, uint32, bool, sint32, sint64, uint32, koord3d, uint8, find_route_flags)':
dataobj/route.cc:1570:58: warning: enumerated and non-enumerated type in conditional expression [-Wextra]
1570 |                                ribi_t::ribi go_dir = wg ? wg->get_ribi_maske(): ribi_t::all;
      |                                                      ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
===> CXX dataobj/scenario.cc
In file included from dataobj/../script/script.h:12,
                from dataobj/scenario.cc:31:
dataobj/../script/api_function.h:381:136: error: 'std::index_sequence' has not been declared
  381 |            static SQInteger call_function_helper(HSQUIRRELVM vm, std::function<R(A1,As...)> const& func, bool act_as_member, std::index_sequence<is...>)
      |                                                                                                                                    ^~~~~~~~~~~~~~

dataobj/../script/api_function.h:381:150: error: expected ',' or '...' before '<' token
  381 |  static SQInteger call_function_helper(HSQUIRRELVM vm, std::function<R(A1,As...)> const& func, bool act_as_member, std::index_sequence<is...>)
      |                                                                                                                                      ^

dataobj/../script/api_function.h: In static member function 'static SQInteger script_api::call_std_function_t<R(A1, As ...)>::call_function(HSQUIRRELVM, const std::function<R(A1, As ...)>&, bool)':
dataobj/../script/api_function.h:396:83: error: 'index_sequence_for' is not a member of 'std'
  396 |                        return call_function_helper(vm, func, act_as_member, std::index_sequence_for<As...>{});
      |                                                                                  ^~~~~~~~~~~~~~~~~~
dataobj/../script/api_function.h:396:83: note: 'std::index_sequence_for' is only available from C++14 onwards
dataobj/../script/api_function.h:396:104: error: expected primary-expression before '...' token
  396 |                        return call_function_helper(vm, func, act_as_member, std::index_sequence_for<As...>{});
      |                                                                                                        ^~~
dataobj/../script/api_function.h: At global scope:
dataobj/../script/api_function.h:405:139: error: 'std::index_sequence' has not been declared
  405 |          static SQInteger call_function_helper(HSQUIRRELVM vm, std::function<void(A1,As...)> const& func, bool act_as_member, std::index_sequence<is...>)
      |                                                                                                                                    ^~~~~~~~~~~~~~

dataobj/../script/api_function.h:405:153: error: expected ',' or '...' before '<' token
  405 | atic SQInteger call_function_helper(HSQUIRRELVM vm, std::function<void(A1,As...)> const& func, bool act_as_member, std::index_sequence<is...>)
      |                                                                                                                                      ^

dataobj/../script/api_function.h: In static member function 'static SQInteger script_api::call_std_function_t<void(A1, As ...)>::call_function(HSQUIRRELVM, const std::function<void(A1, As ...)>&, bool)':
dataobj/../script/api_function.h:419:83: error: 'index_sequence_for' is not a member of 'std'
  419 |                        return call_function_helper(vm, func, act_as_member, std::index_sequence_for<As...>{});
      |                                                                                  ^~~~~~~~~~~~~~~~~~
dataobj/../script/api_function.h:419:83: note: 'std::index_sequence_for' is only available from C++14 onwards
dataobj/../script/api_function.h:419:104: error: expected primary-expression before '...' token
  419 |                        return call_function_helper(vm, func, act_as_member, std::index_sequence_for<As...>{});
      |                                                                                                        ^~~
dataobj/scenario.cc: In member function 'bool scenario_t::open_info_win(const char*) const':
dataobj/scenario.cc:749:44: warning: unused parameter 'tab' [-Wunused-parameter]
  749 | bool scenario_t::open_info_win(const char* tab) const
      |                                ~~~~~~~~~~~~^~~
make: *** [common.mk:56: build/default/dataobj/scenario.o] Error 1


[neroden]

Just to verify that it's actually using C++ 14, I had common.mk echo the build command:

Code Select Expand

g++ -std=c++14 -DHAS_64_BIT_SYSTEM -O -DNDEBUG -DMSG_LEVEL=3 -DUSE_UPNP -DUSE_FREETYPE -I/usr/include/freetype2 -I/usr/include/libpng16 -DUSE_FLUIDSYNTH_MIDI -DMULTI_THREAD -DREVISION=9e6fb3c -Wall -W -Wcast-qual -Wpointer-arith -Wcast-align -DUSE_C -fno-delete-null-pointer-checks -fno-strict-aliasing -std=c++11 -I/usr/include/SDL2 -I/usr/include/freetype2 -I/usr/include/miniupnpc -DUSE_ZSTD -I/usr/include/SDL2 -D_REENTRANT -DCOLOUR_DEPTH=16 -c -MMD -o build/default/dataobj/scenario.o dataobj/scenario.cc

This is your master branch, pulled just now.

Part of the problem is that "make clean" isn't doing its job.  (Patch incoming, on `master-compilation-fixes-20240527` branch.)  But after fixing that it's still not working.

Ha ha.  It's injecting c++11 AFTER c++14.  Local config.default problem.  Fixed now...

OK, I've also fixed config.template to discourage this sort of problem from happening in the future.  Also on master-compilation-fixes-20240527 branch.

[neroden]

OK, up to 7482 and I've automated more of my workflow.  Found a missing piece which is going to be obliterated by a patch which hasn't gone in yet, so made a note of that.  I'm expecting the 7000s to be the most tedious.  I've got about 637 to go before I get into Ranran's work, which I think I won't need to verify nearly as much.  I reviewed about 60 commits in 4 days, so it's going fairly fast, and I expect it to get easier as I get closer to the present day.  The bulk renames are much slower to review and verify than other patches though.

[RESTRICTED ACCOUNT]

I think it will be a very difficult task to properly merge the r8000 series and above. As already mentioned, the codes for multi-tile city buildings are significantly different between extended and standard, but standard increases the number of commits related to multi-tile city buildings. prissi knew that extended's was broken, so standard implemented it on its own. In any case, Acarlotti's suggestion was ignored, resulting in a large and meaningless code difference. It was hoped that he would at least fix it himself, but that didn't happen.　However, once implemented in extended, it was extremely difficult to completely remove it. At least the people who implement it and are familiar with the code don't do it, so it was an impossible task for me who didn't know the code at all.

Also, the climate patch is a major change from standard, and river generation rules etc. conflict with extended, so I think you need to be careful.

[neroden]

I think it will be a very difficult task to properly merge the r8000 series and above.  As already mentioned, the codes for multi-tile city buildings are significantly different between extended and standard, but standard increases the number of commits related to multi-tile city buildings. prissi knew that extended's was broken, so standard implemented it on its own. In any case, Acarlotti's suggestion was ignored, resulting in a large and meaningless code difference. It was hoped that he would at least fix it himself, but that didn't happen.　However, once implemented in extended, it was extremely difficult to completely remove it. At least the people who implement it and are familiar with the code don't do it, so it was an impossible task for me who didn't know the code at all.

Also, the climate patch is a major change from standard, and river generation rules etc. conflict with extended, so I think you need to be careful.

Thanks for the warning.  I suspect I can get it done but it may not be quick.  If I can track and verify which SVN commits are in and which aren't (frankly the worst offender here is James, who adapted a bunch of stuff from standard without recording which SVN commit it came from), and get as much of the bulk-rename stuff cleared up as I can, then I can narrow down the major feature changes to a coherent list of patches and start isolating it to individual files and subroutines.  At that point I can probably figure it out.

(I have worked on the city building code quite a lot in the past.  I'm sure it's changed but I have some memory of it.)

[neroden]

James -- I uncovered a unrelated bug while doing this work.  I'm working on a fix because there are several choices of how to fix it.  Don't commit yet...

...OK.  It's on the bugfix-20240527 branch (two commits).  After that's committed I'll get back to the merge verification work.

This fencepost error isn't in standard, but a *different* variant of the fencepost error is in standard, so I submitted a separate patch for that.  There's a subtle behavior difference between standard and extended here which I preserved, however -- out-of-bounds values reset to 0 in standard but to get_count() - 1 in extended.  Since this could have many implications I left this behavior the same.

[jamespetts]

Thank you all for this. Neroden - your fixes are now incorporated. Ranran - I will have a look at your work on this shortly.

[neroden]

James, can I get rid of Simutrans-Experimental-Server-BG.vcxproj?  It's the source of spurious diffs and it seems to be obsolete.

[jamespetts]

James, can I get rid of Simutrans-Experimental-Server-BG.vcxproj?  It's the source of spurious diffs and it seems to be obsolete.

Yes, this is now obsolete.

[jamespetts]

I noticed that r7151 is not merged yet.
https://github.com/aburch/simutrans/commit/02e0e6f38415c21d62e988e2ad5005693340a574

I don't think that we need that one in Extended, as Extended uses 64 bit integers for time.

[neroden]

FYI, 'make clean' is supposed to delete executable files.

[jamespetts]

FYI, 'make clean' is supposed to delete executable files.

The problem that I was having that led me to revert the make clean commit was this. The code is pulled into a ~/simutrans-extended directory. There is then a /simutrans subdirectory where the executable is created and all the files that the executable needs to run (fonts, paksets, themes, etc.) are located. When we run make clean with the modification that you had inserted, not only are the executables removed from this directory, but so are all the files that the executables need to run, such as the themes, fonts and the symbolic link that I had created to the pakset. Also, the ~/simutrans-extended/simutrans folder was then filled with code files, which should be in the ~/simutrans-extended folder.

[neroden]

Ahhh... do you have BUILDDIR and PROGDIR set to the same value?  I think what you want is to set PROGDIR to ~/simutrans-extended/simutrans and to make sure BUILDDIR is set to the new default, which is ~/simutrans-extended/build/default/ (a completely separate directory).  I've had those two set to different values for a while.

You might want to make a new config.default file, if that's the build method you're using.  If you're using a different method, I'll see if I can figure it out.

[neroden]

In the meantime... I'm up to 7494, which was a humungous German-to-English code translation patch.  I found a few areas of Extended which needed the same translations done (BG's airline code) and matched up some commented-out code from standard to be commented-out versions of the *current* code from standard.  Also deleted the Simutrans-Experimental-Server-BG.vcxproj file. Whitespace, comments, variable renames.  It compiles.

I'm really glad to be past this one because the future patches I have to review will not have nearly as much German code to decipher to see whether they were applied.

[jamespetts]

Ahhh... do you have BUILDDIR and PROGDIR set to the same value?  I think what you want is to set PROGDIR to ~/simutrans-extended/simutrans and to make sure BUILDDIR is set to the new default, which is ~/simutrans-extended/build/default/ (a completely separate directory).  I've had those two set to different values for a while.

You might want to make a new config.default file, if that's the build method you're using.  If you're using a different method, I'll see if I can figure it out.

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)


[jamespetts]

In the meantime... I'm up to 7494, which was a humungous German-to-English code translation patch.  I found a few areas of Extended which needed the same translations done (BG's airline code) and matched up some commented-out code from standard to be commented-out versions of the *current* code from standard.  Also deleted the Simutrans-Experimental-Server-BG.vcxproj file. Whitespace, comments, variable renames.  It compiles.

I'm really glad to be past this one because the future patches I have to review will not have nearly as much German code to decipher to see whether they were applied.

Excellent, thank you - now incorporated.

[jamespetts]

Ranran - I have just tried merging your r10996 branch with the master branch, and have pushed the results to my std-r10996 branch. Unfortunately, when running the compiled binary from my branch, I get the following error:

Code Select Expand

Parsed simuconf.tab for directory layout; multiuser = 1
FATAL ERROR: loadsave_t::rdwr_xml_number() - expected "<i16>", got "</en>"
Aborting program execution ...

For help with this error or to file a bug report please see the Simutrans forum at
https://forum.simutrans.com
dr_fatal_notify: ERROR: FATAL ERROR: loadsave_t::rdwr_xml_number() - expected "<i16>", got "</en>"
Aborting program execution ...

For help with this error or to file a bug report please see the Simutrans forum at
https://forum.simutrans.com

Aborted (core dumped)

This error is normally caused when a version of settings.xml is loaded from a later version of the code when running an earlier version of the code. In principle, it is easy to work around this by deleting settings.xml, but this error occurring in a merge from Standard suggests that something is wrong - also, everyone else will have to delete their settings.xml files to get this working again if this were incorporated, so there is some issue here that will need to be resolved before this can be merged.

Thank you for your work on this so far.

[jamespetts]

Thank you for confirming. I think I've made a fix for that. It would be helpful if you could check it again.

Thank you for that. However, on checking it again, I still get the same error.

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