Incorporating changes from Standard

[ceeac]

The terraformer changes are now finished: #609 for the master branch and #610 for the ex-15 branch.

[neroden]

That's great news.  I see that the cherry-picked-commits.txt file runs from 9700 to 10802, plus three stray cherry-picks from earlier.

A. Carlotti definitely got everything up to #7429, with Phystam working up to 7753 or 8125, and it looks like Ranran started around there.  I see that James merged Ranran's merge-from-8434 branch.  Are we sure we have everything from 8435 to 9700? 

That's where the discussion in this thread got confusing and lost in questions of compilation so I'm a bit confused.

Regarding the source reorganization script.... well.... I kind of want to make sure we have all the patches we want from standard which went in *before* the source reorganization *before* we apply the source reorganization script, which is why I'm asking about this.

[jamespetts]

Ceeac - thank you for this. Now incorporated.

[RESTRICTED ACCOUNT]

Does someone have a summary of the current state of incorporation of changes from standard?
Is the documentation/cherry-picked-commits.txt file relevant to this question?

The reason I wrote out the cherry picked commits is to make it clear that I'm only picking up specific commits because I skipped commits that were not UI related. Because I just followed ACarlotti's recommendation and proceeded with the merge from the standard to expedite the work of implementing the GUI.

There is a concern that changing the folder structure while missing important commits will make it difficult to obtain those commits in the future.


A bunch of big unfetched commits that I can recall are:

(1) moisture patch

(2) Support for the new dat description method of makeobj
dat file for standard may not be reused as it is for extended. Specifically, simplification of image specification description.

(3) multi-tile citybuilding

Of particular importance is the repair of the broken multi-tile citybuilding code.
Extended's multi-tile citybuilding code was written by THLeaderH. But standard is not. This only makes it difficult to move between standard and extended and reduces maintainability.
And prissi points out that the Extended multi-tile citybuilding code is broken. But unfortunately extended doesn't have a normal pakset with multi-tile citybuilding so it's not easy to test it.
It should also be noted that he introduced a number of bugs in the overtaking feature that he has neglected to fix most of the time. The irresponsible Japanese who wrote the code won't help us anyway. And so years have passed.


Please refer to the following.
https://forum.simutrans.com/index.php/topic,17857.0.html
https://forum.simutrans.com/index.php/topic,17814.0.html




Some relevant thread are below:
https://forum.simutrans.com/index.php/topic,21166.msg201034.html#msg201034

https://forum.simutrans.com/index.php/topic,21029.msg196285.html#msg196285

https://forum.simutrans.com/index.php?msg=184192

https://forum.simutrans.com/index.php?msg=196974

A. Carlotti definitely got everything up to #7429

I've completed some missing trivial commits, but I think his work is mostly accurate.

with Phystam working up to 7753 or 8125

I find Phystam's work is not much, but there were a lot of errors. It's similar to what A.Carlotti pointed out about his work before he left. Note that A.Carlotti mentioned commits before r7753, but I'm talking about after r7753.

There's are several instances where, for no reason I can tell, you didn't incorporate a change accurately, resulting in a completely unnecessary difference from the standard codebase. There are some changes that you didn't transfer for some reason.

There were some examples of missing changes (sets of rows).

Code Select Expand

A
B
A'
B' Sometimes he missed A'B' with a change like that. (I mean he removed A'B' from the change. Looking for it by looking at the history is a very troublesome task.)

Many errors were missing "_rgb" in ABC_rgb. And many typos (upper and lower case mistakes). A number of similar variable names were mixed up. He missed a lot of it being changed to xxx_rgb.

I got the impression he was manually typing and transcribing changes because there were many weird typos. (Such typos don't happen with automerge or copy and paste.)

I've fixed the ones I noticed, but for example the crash bug when switching players was caused by incorrect merging and was overlooked until recently. Note that I am not saying this to condemn him, but to clarify the possibility that such errors may already be lurking if someone takes over the work, to clarify the cause of the error, and to prevent its recurrence.

I've pushed changes up to the beginning of January (including some bits that Phystam missed). Getting much further than that will take a little while because there are a lot of big commits in there.

Phystam also missed a lot of basic changes since r7753.


Unfortunately, I also have doubts about the accuracy of my work.


Extended currently still has some old style GUIs.
(1) Player list
(2) Schedule list

Since these are extended with many functions, they need to be dealt with individually.
As we merged from standard, the old dialogs became more and more anomalous.
Also, some GUI parts may have bugs in extended due to placement not implemented in standard. For example combobox. But I think it's a bug in standard.

[jamespetts]

Ranran - thank you for that summary: that is most helpful. I agree that the priority is fixing the multi-tile city building code (i.e. incorporating the fixes from Standard).

[neroden]

So, I may have a chance this winter to work on cleaning this up and properly incorporating the commits from 7429 to 10444, which should be done before the source reorganization.

The problem I have is this.  I want to do this work properly using Git (specifically, actually using "git merge" and "git cherry-pick"). 

But how do I tell what SVN commit number matches to what Git commit?  Is there an easy way to do this?

[ceeac]

But how do I tell what SVN commit number matches to what Git commit?  Is there an easy way to do this?

Code Select Expand

$ git log -n1 master | grep git-svn-id | grep -Poe "(?<=@)[0-9]+"
10984


[jamespetts]

So, I may have a chance this winter to work on cleaning this up and properly incorporating the commits from 7429 to 10444, which should be done before the source reorganization.

The problem I have is this.  I want to do this work properly using Git (specifically, actually using "git merge" and "git cherry-pick"). 

But how do I tell what SVN commit number matches to what Git commit?  Is there an easy way to do this?

That would be splendid - I shall look forward to your work on this.

[prissi]

For linux/max, you need to change the makefile/cmake to include libfontconfig. Maybe you missed an essential patch. The windows functions are since windows NT, so since 1995 included ...

[prissi]

Seems like it is compiled with the wide char functions as default. I will add code to standard. Essentially, add a W to NONCLIENMETRICSW and to SystemParameterInfosW and change faceName to a string wsFaceName and remove the next definition.

[prissi]

THe following code compiles also as C++20:

Code Select Expand

    NONCLIENTMETRICSW ncm;
    ncm.cbSize = sizeof(NONCLIENTMETRICSW);
    SystemParametersInfoW(SPI_GETNONCLIENTMETRICS, sizeof(NONCLIENTMETRICSW), &ncm, 0);
    std::wstring wsFaceName = ncm.lfMessageFont.lfFaceName;

    LPCWSTR fontRegistryPath = L"Software\\Microsoft\\Windows NT\\CurrentVersion\\Fonts";
    HKEY hKey;
    LONG result;

[jamespetts]

Thank you for your work on this. I have tested the first two pull requests on a Windows system - I have not had a chance to test this on Linux yet.

The first thing that I notice is an unusual artefacting along coast lines thus (visible in Pak128.Britain-Ex's demo.sve):



This artefacting changes as the map is scrolled or zoomed.

Also, because these changes go beyond the UI, this will need more thorough testing: I will need to test whether this compiles and runs under Linux and whether a server and client can maintain synchronisation.

Additionally, the level of changes in these branches mean that I will need to be able to be confident that this will merge cleanly with the Ex-15 branch before incorporating it. Can I ask whether you have tested Ex-15 compatibility?

Edit: For clarity, I should note that the artefacting appears when merging the first of the pull requests and is unaffected by the second.

[jamespetts]

Excellent, thank you for your work with this. A brief test shows that I am able to compile std-2403 in Linux and run this, and that the artefacting is no longer present on coastlines. I have not had time to run a full test with this, but one issue that I found with this is that it seems as though it is no longer possible to remove trees with the demolish/remove tool. I have not tried this in Windows yet. I should be grateful if you could look into this. Thank you.

Edit: On further testing, this problem (inability to remove trees) also appears in the master branch.

[jamespetts]

Thank you very much for that. I have now tested further. I can confirm that the tree deletion problem is fixed and that the server and client stay in synchronisation (at least, as much as they do in the current master branch - the current server game does not stay in sync and this will need extensive investigation at some point; but having a saved game where this can be reproduced will be helpful).

However, I am having trouble compiling the ex-15 version: attempting to compile it in Visual Studio (branch ex15-2405-std) produces the following compile errors:

Code Select Expand

Severity Code Description Project File Line Suppression State
Error C2065 'ips': undeclared identifier Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error (active) E0020 identifier "ips" is undefined Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2672 'begin': no matching overloaded function found Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2893 Failed to specialize function template 'unknown-type std::begin(_Container &)' Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2784 'const _Elem *std::begin(std::initializer_list<_Elem>) noexcept': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unknown-type' Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2672 'end': no matching overloaded function found Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2893 Failed to specialize function template 'unknown-type std::end(_Container &)' Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2784 'const _Elem *std::end(std::initializer_list<_Elem>) noexcept': could not deduce template argument for 'std::initializer_list<_Elem>' from 'unknown-type' Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C3536 '<begin>$L0': cannot be used before it is initialized Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C3536 '<end>$L0': cannot be used before it is initialized Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2100 illegal indirection Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434
Error C2440 'initializing': cannot convert from 'int' to 'const std::string &' Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 434


[jamespetts]

Thank you for confirming. I believe I have fixed it.

Thank you: I can confirm that the compile error has now been fixed.

I have now been able to test this, but I am not able to get the network mode working in the ex-15 build. When attempting to connect to net:127.0.0.1 when running a local server, I get an in-game error message, "An operation was attempted on something that is not a socket". This only occurs on the ex-15 build. I presume that this is related to the network changes merged from Standard.

[jamespetts]

Thank you for looking into this. I did type the error message exactly - I strongly suspect that the text comes not from the Simutrans code, but rather from code in a library used for networking by Simutrans.

However, I do not think that the omission of the changes to which you refer was intentional, so it may well be that the most efficient way of dealing with the problem is simply to update ex-15 to the same network code as on the master branch, as there are no intentional differences in network code between the branches to the best of my recollection.

[jamespetts]

Unfortunately there is a bigger difference between the ex15 branch and the master branch than expected.
For some reason there is a big difference in the script folder that is not used by extended, which makes it impossible to resolve by just copying the network folder.

I suspect there are many commits on the ex15 branch that aren't being retrieved anyway.

EDIT:
https://github.com/Ranran-the-JuicyPork/simutrans-ex-fix/commit/d68b3ac56de289df8cbc3a6c318c4ccdc89b802b#diff-0c1bbc98bed862c026a15039cc84518c299dc81d49a270be62be98a83c3b8a2b

This commit makes a big difference between the two, so we need to resolve the conflict.
Would it be best to incorporate this commit into master?

Yes, that might be sensible, as this appears to be merging from Standard rather than any ex-15 specific features. Thank you for investigating this.

[jamespetts]

I re-imported a number of commits related to the changes regarding the aforementioned script from the ex15 branch and tried to resolve the conflicts. Please test again as many changes have been made to the 2403-std branch.

Thank you for that. Unfortunately, I am now getting a compile error in Visual Studio:

Code Select Expand

Severity Code Description Project File Line Suppression State
Error C1083 Cannot open include file: 'miniupnpc/miniupnpc.h': No such file or directory Simutrans-Extended C:\Users\James E. Petts\Documents\Development\Simutrans\simutrans-extended-sources\network\network.cc 954 

Accordingly, I have not been able to test further.

[jamespetts]

I think I have fixed it.

Thank you for that. Unfortunately, I now get a merge conflict when merging this into the latest master branch as a result of the player ranking dialogue changes.

[neroden]

Code Select Expand

$ git log -n1 master | grep git-svn-id | grep -Poe "(?<=@)[0-9]+"
10984


Yeah, that doesn't quite do what I want.... let's see.  I wrote a short program called "ssvn".  Then I can run `ssvn 7430` to get the appropriate git commit number.

Code Select Expand

#! /usr/bin/perl 
#
# Program name ssvn
# Given an SVN commit number, return the GIT commit number
# Used for Simutrans.  Branch name fixed as "standard-master"
my ($svn_num) = @ARGV;
$branch = "standard-master"; 
#print "SVN num is $svn_num\n";
$grep_str = "trunk\@$svn_num"; 
#print "Grep_str is $grep_str\n"; 
$commit_comment = `git log $branch --grep \"$grep_str\"`;
#print "Commit string is $commit_string]n";
$commit_comment =~ /commit (\w*)/;
$commit_id = $1;
print $1;

This script takes an SVN commit number and gives me the GIT commit ID.

So, reminding myself of what the unpleasant maintenance job is: I need to go through 7430 through 10444, skipping the commits in cherry-picked-commits, and check each one for merging.

Code Select Expand

git show `ssvn 7430`

OK.  So I started the tedious scutwork.  So far I've looked at three commits (7429, 7430, 7431) and verified that they're fully integrated.

Now, I have James's master branch on jp-master.  Then I ran

Code Select Expand

git checkout jp-master
git checkout -b merge-from-standard
to create a new merge-from-standard branch derived from James's.

Now here's the clever bit.  We're really quite sure we've gotten everything up to 7431 now.

Code Select Expand

git merge --strategy ours `ssvn 7431`
This records a git merge so that git understands that we've successfully merged everything up to 7431.  "--strategy ours" means we actually ignore all the commits on Standard up to that point... because we already checked by hand that we merged them.

Then I just added info to cherry-picked-commits.txt.

I've pushed this to github as the "merge-from-standard" branch and I request that it be merged.  I'll try to do more of this.  For a while it's going to be a lot of "yeah, we already got that commit", but I'm going to go through them one at a time.

[neroden]

There's so much with Extended which needs to have new script hooks in order to function at all anyway, due to its different behavior, that I'm hardly worrying about scripting.  At the moment there are serious desyncing issues, as well as some long-standing nasty bugs which appear even in single-player, so I don't even want to think about scripting.

My priority is getting the commits which were sloppily ported by whatshisname checked over and verified; then getting the rest of the pre-reorganization commits ported/verified; cleaning up the git merge history; and then doing the mass reorg/renaming. Then it'll be possible to do an actual "diff" from standard to extended, without the side effects of the reorg, which will make a lot of things clearer.

[RESTRICTED ACCOUNT]

The most troublesome problem is that the multi-tile city building and overtaking codes are different between standard and extended, the extended one is broken (prissi and THleaderH who coded it say so), the person who did the work knowingly left it alone, and the amount of changes is huge. Therefore, it is difficult to repair.
I think that if the work progresses until 2018, it will hit that roadblock.
Since our time is limited, we had no choice but to take a detour.

[neroden]

And a bunch of that is before the reorg, right.

Multi-tile city building... well, we don't have any such buildings except the town hall in extended.  I can probably get that straightened out if we get to that point.  We've got some rather nice improvements on single-tile city building in extended -- the build-in-similar-style parameter -- which we'll have to retain, which will create some interesting logic coding questions, but I don't expect this to be super difficult.

Overtaking code... has a bunch of pretty obvious buggy behavior in extended.  But we also have the one way and parallel stopping code, which I really like even though it's also buggy. We'll have to rework it.  Annoying.  I've never looked into this code at all.

If I can clear out a bunch of the little stuff it'll mean there's less diffs to look at though.

If I can get the divergences down some I *still* want to stop rotating the world.  This is going to be a rather substantial detour, even though 80% of the code is done -- the best strategy I can think of is to open up a branch; just stop rotating the world and put rotation strictly into a "view" thing; and then deal with the graphical fallout (which is much less serious IMO than the developers of standard think it is).  But I'd like to do this so it's adaptable to standard and that requires getting the reorg done.

[neroden]

So I'm expecting this to be incredibly boring for a while.  I'm still not up to the "beginning of January" which A Carlotti said he'd worked on (past 7429, but the only easy way to find this is to go through the commits one at a time, since I'm up to Dec 27 2014), so I shouldn't find any uncommitted stuff until then.  Since Phystam did (incomplete) work up to 7753 and (even less complete) work up to 8125, I'm expecting to find not very much for another several hundred commits.  But I do have to go through them all one at a time, so here goes.

Update: now done through 7442, and yes, they were all integrated already.  This was not easy to prove because the code has subsequently changed in some of the files, but I've proven it now.  Now we're into "January" so potentially past what A Carlotti said he'd worked on.  I have to check more carefully, and the next one is a bulk renaming/translation patch.  Yuck.  But straightforward.  I'll get to it.

OK.  7443 verified.

7444 verified.

7445 and I have my first actual difference: stupid whitespace and punctuation differences.  I have conformed them to the situation in standard as of right now in order to minimize future diffs, even though the whitespace and punctuation choices in standard are arguably bad.  Minimizing diffs is more important IMO.

[jamespetts]

Thank you both for your ongoing work on this. First of all, I have merged Neroden's work: thank you for that. Secondly, I have attempted to merge the 2403-std work from Ranran, but unfortunately, I am getting compile errors (I am away and so using my Linux computer at the moment, so the following is an output from GCC):

===> CXX dataobj/tabfile.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, route_t::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, route_t::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;
      |                                                       ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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 | ion_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 | 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: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 | _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 | UIRRELVM 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: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:55: simutrans/dataobj/scenario.o] Error 1
make: *** Waiting for unfinished jobs....

This is after resetting to Ranran's 2403-std most recent commit. After I merge the latest master, I get these compile errors:

===> CXX dataobj/translator.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, route_t::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, route_t::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;
      |                                                       ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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 | ion_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 | 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: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 | _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 | UIRRELVM 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: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:55: simutrans/dataobj/scenario.o] Error 1
make: *** Waiting for unfinished jobs....

Edit: Interestingly, I get the same errors trying to compile the latest ex-15 branch on Linux, too.

As to merging more generally - I am very appreciative of both of your work on this. It is a challenging project, and one which I lack the resources to deal with for a long time yet.

The multi-tiled building issue is of some importance, as Pak128.Britain-Ex is definitely intended to have these buildings in the future (the town hall does not count as a city building, as the code treats it as a category unto itself). The idea is that, after the marathon that is the ex-15 work will come work on town growth. Multi-tile city buildings are intended to be integral to the town growth work and better town balancing. Ex-15 has been especially challenging because so many large and difficult things have to be done at the same time because they all affect balance and a lot of the things cannot work (from a balance perspective) without the other things. Because the work is so difficult, it has been especially vulnerable to personal circumstances that limit the amount of time that I have to work on Simutrans-Extended, of which I have had a number in the last 18 months or so (although I did quite a lot of work on this in late 2022 and early 2023).

All this does unfortunately mean that any non-trivial patches (and especially the merging from standard work, since it can affect code in so many areas) have to be tested in both the master and ex-15 branches until the merge with ex-15 should take place, but this is better than getting to the point of restarting work on ex-15 and finding it impossible due to merge conflicts or - worse - the code not working in unexpected ways because of conflicts with the merged code.

Before returning more substantially to the ex-15 branch, I do want to focus on bugs for a while (and my building works are still causing me some disruption, but not as much as at the beginning of the year), especially the loss of synchronisation bugs which are, unfortunately, the most difficult of all sorts of bugs to fix. We thus also need to be careful that we do not introduce any new loss of synchronisation bugs, either in master or ex-15, when merging from Standard, which entails testing for this explicitly with each batch of merges, which, in turn, means that accepting these types of pull requests will be slower than in respect of more minor changes (e.g. individual bug fixes or purely UI changes) because it requires me to have sufficient clear time to carry out a full suite of tests. This should not discourage either of you, however, as this is extremely valuable and worthwhile work - just do not be surprised if there are more delays incorporating these than other changes.

In any event, thank you both again for your splendid work on this.

[neroden]

James -- that looks like Ranran did not account properly for my new ocean travel route code in extended and may have broken it by accident.  That only generates warnings though... the errors are from the scripting code.  We'll have to merge that more properly.  I'll work on that as I go through in order but it's going to take a while to even get to it.

[neroden]

Yes, this looks like the method `get_cost` had a signature change on it in standard.  The overrides and callers didn't get corrected when the signature change was merged from standard.  Going through and correcting all of them seems like the right fix for that problem.

[neroden]

I am currently slogging my way through the github logs to figure out which standard SVN commits were already cherry-picked by Ranran or someone else onto Extended, filling in the back part of the cherry-picked-commits file. 

I'm still going to recheck everything up to 8125 by hand because we don't trust Phystam's work.  But a lot of the time, I'm trying to verify whether a change was merged when the code it affects has subsequently been completely removed or severely altered, so figuring out what *later* patches have already been successfully merged is really helpful when checking the earlier ones.

The new merge-from-standard branch has carefully manually been updated by a few more commits (cleaning up the git history so that future merges won't try to remerge the same commits).  The only changes so far other than git history are still whitespace, documentation, and comments, so it should be safe to merge for both master and ex-15.

[jamespetts]

Thank you both for this. I have incorporated Neroden's commits, but I am now getting merge conflicts when I attempt to merge the latest master into 2403-std:

Code Select Expand

Auto-merging boden/wege/weg.cc
Auto-merging documentation/cherry-picked-commits.txt
CONFLICT (content): Merge conflict in documentation/cherry-picked-commits.txt
Auto-merging gui/components/gui_chart.cc
Auto-merging gui/components/gui_combobox.cc
Auto-merging gui/components/gui_convoiinfo.cc
Auto-merging gui/convoi_frame.cc
Auto-merging gui/convoi_frame.h
Auto-merging gui/money_frame.cc
Auto-merging player/finance.h
Auto-merging player/simplay.cc
Auto-merging player/simplay.h
Auto-merging script/api/api_player.cc
CONFLICT (content): Merge conflict in script/api/api_player.cc
Auto-merging script/dynamic_string.cc
Auto-merging simconvoi.cc
Auto-merging simhalt.cc
Auto-merging simline.cc
Auto-merging simtool.cc
Auto-merging simutrans/text/ja.tab
Auto-merging simworld.cc
Auto-merging vehicle/vehicle.cc
Automatic merge failed; fix conflicts and then commit the result.

Attempting to merge the latest master into ex-15 also gives merge conflicts:

Code Select Expand

Auto-merging boden/wege/weg.cc
Auto-merging documentation/cherry-picked-commits.txt
CONFLICT (content): Merge conflict in documentation/cherry-picked-commits.txt
Auto-merging gui/components/gui_chart.cc
Auto-merging gui/components/gui_combobox.cc
Auto-merging gui/components/gui_convoiinfo.cc
Auto-merging gui/convoi_frame.cc
Auto-merging gui/convoi_frame.h
Auto-merging gui/money_frame.cc
Auto-merging player/finance.h
Auto-merging player/simplay.cc
Auto-merging player/simplay.h
Auto-merging script/api/api_player.cc
CONFLICT (content): Merge conflict in script/api/api_player.cc
Auto-merging script/dynamic_string.cc
Auto-merging simconvoi.cc
Auto-merging simhalt.cc
Auto-merging simline.cc
Auto-merging simtool.cc
Auto-merging simutrans/text/ja.tab
Auto-merging simworld.cc
Auto-merging vehicle/vehicle.cc
Automatic merge failed; fix conflicts and then commit the result.


[neroden]

In the case of merging in the merge-from-standard branch, you're likely to want to prefer whatever you already had on your other branch.  Doing a standard "merge" can dredge up old code by accident, which is why I've had to hand-check code, add the few bits which didn't get copied over, and then use `git merge --strategy ours` to tell git that we've already incorporated the rest.

My current procedure is:

Code Select Expand

git show `ssvn 1234`
Read through the affected files to see if each of the patches was already applied, one frickin' line at a time.
Do searches if it's a mass name replace to make sure there are no instances of the old name that I missed
Check later-applied patches if the code seems to be editing code which has been deleted, to figure out whether that code was deleted intentionally
Apply any outstanding fixes (all comments and whitespace so far)

Code Select Expand

git merge --strategy ours `ssvn 1234`
git diff jamespetts/master merge-from-standard
Check for anything which changed but doesn't look like it should have changed, and fix that.

There really isn't a better way to deal with the mass renames.  I'm hoping to get past those after a while; there are a lot.  Phystam did get most of them.

If you'd like I can do the ex-15 merges myself (on a merge-ex-15-from-standard branch), since it's going to be a similar procedure.
---

In the meantime, I have discovered that the entire `dataobj/records.h` and `dataobj/records.cc` files were ripped out of extended at some point *before* A Carlotti started his work.  This means that the various bulk-rename fixes to them, of course, have not been applied either. 

The speed records are kind of a nice thing to have and we probably want to restore the feature at one point.  Checking them every time a convoy moves is probably a ridiculous waste of computer time, so I did *not* put that code back in.  But I went to the effort to figure out how to get the record tracking code back together, with all the general renaming and upgrading patches which have been applied since then (there are a LOT of them). 

My thought for experimental is that when a convoy arrives at a stop, and we *record its travel time*, we should instantly calculate the station to station speed (which is already being calculated and displayed for other purposes), and update the speed records then.  So we'd be reporting actual travel speed records, not momentary "it was going real fast downhill" speed records.  I am not ready to do this but I figured getting the records code back together was worthwhile.  This is on the `merge-records-from-standard` branch, branched from the tag `merge-records-from-standard-branchpoint` if you want a clean diff.

I don't see the need to merge this at this time (because it's currently dead code!) but it's something to keep in mind.  I mostly did it because I'm verifying that the bulk-rename type commits have been fully incorporated, and of course, they haven't been applied to the files which have been removed (aargh).  So that I can sign off on those commits as being fully incoprorated I made this branch to make sure they were incorporated in these two files too.

---

[jamespetts]

Thank you both for the evidently immense amount of work that you are putting into this - it is very much appreciated. I have managed to merge the latest 2403-std commits now (the only remaining merge conflict was in cherry-picked-commits.txt, which I resolved manually - I hope that I have resolved this correctly). I am still getting the compile errors with GCC however, as Ranran predicted:

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, route_t::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, route_t::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/translator.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: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 |                 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: 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: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:55: simutrans/dataobj/scenario.o] Error 1
make: *** Waiting for unfinished jobs....

In relation to the records, these had long ceased to have any particular function in Simutrans-Extended since the "speed bonus" was abolished in favour of distance based pricing, classes and speed based competition (and had, in fact, long since ceased to affect the "speed bonus" in any event even in Standard, since people worked out that the whole system of calibrating the speed bonuses based on actual speeds could not be balanced properly if people put the Concorde supersonic airliner into the game), and was, as Ranran pointed out, not working properly and just spamming annoying messages. Having no function, I don't think that anybody wanted to go the effort of fixing it, so it was removed.

If anyone would like to restore it just for the sake of having some interesting information, there is no reason not to reinstate it for that purpose. I agree that average speeds between stops might be a good way of doing this, although one drawback is that the average speed is a combination of the actual vehicle speed and the directness of the route, so I am not sure how usefully comparable that these records would really be with each other, so this may need some further thought. If it were to be reinstated, I wonder whether it might usefully have some place in the new ranking system perhaps?

Edit: Interestingly, modifying config.default to c++14 from c++11 has reduced but not eliminated the compile errors. I now get only:

===> CXX script/api/api_obj_desc.cc
In file included from script/api/api_obj_desc.cc:29:
script/api/../../bauer/wegbauer.h:66:16: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
   66 |         static const bool is_active_waytype(const waytype_t wtyp);
      |                ^~~~~
script/api/api_obj_desc.cc: In function 'const vector_tpl<const way_obj_desc_t*>& get_available_wayobjs(waytype_t)':
script/api/api_obj_desc.cc:281:16: warning: unused variable 'time' [-Wunused-variable]
  281 |         uint16 time = welt->get_timeline_year_month();
      |                ^~~~
script/api/api_obj_desc.cc:277:74: warning: unused parameter 'wt' [-Wunused-parameter]
  277 | const vector_tpl<const way_obj_desc_t*>& get_available_wayobjs(waytype_t wt)
      |                                                                ~~~~~~~~~~^~
===> CXX script/api/api_obj_desc_base.cc
In file included from script/api/api_obj_desc_base.cc:14:
script/api/../../bauer/wegbauer.h:66:16: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
   66 |         static const bool is_active_waytype(const waytype_t wtyp);
      |                ^~~~~
===> CXX script/api/api_pathfinding.cc
In file included from script/api/api_pathfinding.cc:15:
script/api/../../bauer/wegbauer.h:66:16: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
   66 |         static const bool is_active_waytype(const waytype_t wtyp);
      |                ^~~~~
===> CXX script/api/api_player.cc
script/api/api_player.cc: In function 'void export_player(HSQUIRRELVM, bool)':
script/api/api_player.cc:255:75: error: 'freevariable3' was not declared in this scope
  255 |         register_method_fv(vm, &get_player_stat, "get_convoys",           freevariable3<sint32,sint32,bool>(ATV_CONVOIS, TT_ALL, true), true);
      |                                                                           ^~~~~~~~~~~~~
script/api/api_player.cc:255:95: error: expected primary-expression before ',' token
  255 |       register_method_fv(vm, &get_player_stat, "get_convoys",           freevariable3<sint32,sint32,bool>(ATV_CONVOIS, TT_ALL, true), true);
      |                                                                                             ^
script/api/api_player.cc:255:102: error: expected primary-expression before ',' token
  255 | egister_method_fv(vm, &get_player_stat, "get_convoys",           freevariable3<sint32,sint32,bool>(ATV_CONVOIS, TT_ALL, true), true);
      |                                                                                             ^
script/api/api_player.cc:255:103: error: expected primary-expression before 'bool'
  255 | gister_method_fv(vm, &get_player_stat, "get_convoys",           freevariable3<sint32,sint32,bool>(ATV_CONVOIS, TT_ALL, true), true);
      |       

Are we maybe missing some Squirrel related .h files, perhaps?

[neroden]

If anyone would like to restore it just for the sake of having some interesting information, there is no reason not to reinstate it for that purpose. I agree that average speeds between stops might be a good way of doing this, although one drawback is that the average speed is a combination of the actual vehicle speed and the directness of the route, so I am not sure how usefully comparable that these records would really be with each other, so this may need some further thought. If it were to be reinstated, I wonder whether it might usefully have some place in the new ranking system perhaps?

Yeah, I think that would be cool.  "Player with the fastest train routes", etc. 

The code I would be reinstating basically just has a class for keeping track of top speeds in each mode and nothing much more.  The fact that the speed is a combination of the actual vehicle speed and the route directness is... desirable, in my opionion.   The only catch is I need to actually find the code where the journey time tables are updated so I can put in the calls and see if I can make this work.  I have found plenty of code which *reads* it but I don't remember where it gets *written*.

[neroden]

OK.  In the long run, that should be "freevariable" everywhere it says "freevariable3".  And it DOES require C++14.

The relevant commit is r10424.

....but we don't want to be trying to port that right now.  It should be working as it is.

script/api/api_player.cc includes script/api_function.h, which includes script/api_param.h, which includes the definition of the freevariable3 template.  It's there in merge-from-standard.  (And I'm just checking whether merge-from-standard compiles -- I expect it to).

So check whether you accidentally deleted one of the includes, or the definition of freevariable3.

[neroden]

As for the world record, if I remember correctly it was removed by Freddy after ACarlotti started working on it. It announced a meaningless world record every time I loaded the game without being recorded. It only spammed messages and only kept temporary records, but did not carry over the records. I rather agree with him. At least it had to be recorded correctly. No wonder it was considered worthless at the time.
However, things may change if the chat function is improved.

It's not too complicated a feature set -- I'll keep it on a branch and see if I can fix it up

[neroden]

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