The International Simutrans Forum

 

Author Topic: Incorporating changes from Standard  (Read 1152 times)

0 Members and 1 Guest are viewing this topic.

Offline ACarlotti

  • *
  • Posts: 223
Incorporating changes from Standard
« on: May 06, 2018, 12:08:01 PM »
This is firstly a question, and then secondly an offer to try doing something about it (as time permits).

The question: What is the current process for incorporating bug fixes and features from Standard? As far as I can tell we've just been cherry-picking changes whenever we notice something we want, but are making no thorough attempt to include all relevant fixes/changes. This has become apparent from discussion/investigation relating to a tunnel/catenary graphics bug. From source control, I can see that there was last an explicit merge from Standard in February 2014, but my understanding is that it was subsequently decided that merges were not the way forward. So what's been happening since then?

The proposal (subject to discussion) is that someone (possibly me) attempts to go through the Standard revision history methodically up to point at which this was last done (potentially up to 1354 revisions and counting), to check for and incorporate any relevant revisions.

Offline Matthew

  • *
  • Posts: 98
  • Languages: EN, some ZH, DE & SQ
Re: Incorporating changes from Standard
« Reply #1 on: May 06, 2018, 02:37:30 PM »
I realize that your question is primarily directed at James as project lead, but it's great that you're considering contributing to the community in this way.

 :thumbsup:

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #2 on: May 06, 2018, 04:23:10 PM »
Initially, Extended ("Experimental" as then it was known) was just a patch with a few extra features from Standard: the changes from Standard were merged automatically. This automatic merging continued for a few years, but became unmanageable around 2012/2013 when Extended diverged too far from Standard for this to work effectively, with the amount of time and effort necessary to resolve automatic merge conflicts greatly exceeding the amount of time and effort necessary simply to merge changes manually.

Since then, changes have been merged manually, although some more complex changes (such as GUI theme support and additions to the scripting support) have not been merged owing to the amount of work that that involves.

If you would be able to assist with merging additional changes from Standard into Extended (I have not had chance to do any in the last few months), that would be extremely helpful, and it would be even more helpful if you were able, for example, to go back and merge in full GUI theme support and make this work with all of the Extended GUI.

Thank you very much for your offer to help with this: it is much appreciated.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #3 on: May 10, 2018, 07:42:41 PM »
I have now made a small start on this (about 8 standard revisions so far). Before going too much further, I would like to check that my approach is acceptable. Namely, my current workflow is to cherry pick any comments from Standard that are relevant and haven't yet been included in Extended. This creates a commit history that looks like this:

Code: [Select]
commit 2ad79548da4183d3d1f27b25852d7429635a7f06 (HEAD -> merge-from-standard)
Author:     Dwachs
AuthorDate: Wed Feb 19 19:19:36 2014 +0000
Commit:     Andrew Carlotti
CommitDate: Thu May 10 20:32:29 2018 +0100

    FIX problem with scrollbar offset in line selector of depot window - take two
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7074 8aca7d54-2c30-db11-9de9-000461428c89

commit 4731fa931d52f2e8c8662884589874fd11d36d3e
Author:     Dwachs
AuthorDate: Sun Feb 16 12:12:19 2014 +0000
Commit:     Andrew Carlotti
CommitDate: Thu May 10 20:26:21 2018 +0100

    FIX problem with scrollbar offset in line selector of depot window
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7072 8aca7d54-2c30-db11-9de9-000461428c89

commit aa44f5e8e6f065021c377bcf89f9b622e0c81888
Author:     Dwachs
AuthorDate: Sat Feb 15 15:58:41 2014 +0000
Commit:     Andrew Carlotti
CommitDate: Thu May 10 20:19:01 2018 +0100

    squelch compiler warning
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7070 8aca7d54-2c30-db11-9de9-000461428c89

commit 66ab44890619c391d9d6d23e52a7e20690786021
Author:     Dwachs
AuthorDate: Sat Feb 15 15:57:10 2014 +0000
Commit:     Andrew Carlotti
CommitDate: Thu May 10 20:17:31 2018 +0100

    (eipi) documentation in hausbauer
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7069 8aca7d54-2c30-db11-9de9-000461428c89

commit 7eddff2d9ae8573c012aa9d2c5017b41c14d50f7
Author:     Dwachs
AuthorDate: Sat Feb 15 15:38:31 2014 +0000
Commit:     Andrew Carlotti
CommitDate: Thu May 10 18:07:50 2018 +0100

    delete wrong comment
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7068 8aca7d54-2c30-db11-9de9-000461428c89

commit 5d705385301ea49ec448a53f23691d0295c6c2e0 (upstream/master, upstream/HEAD, master)
Author:     James E. Petts
AuthorDate: Tue May 8 23:21:50 2018 +0100
Commit:     James E. Petts

Does anyone (especially James) thing this is the wrong way of doing things, or that I could be doing things better (e.g. adding text to the original commit messages)?

Offline O01eg

  • *
  • Posts: 79
  • Languages: EN, RU
Re: Incorporating changes from Standard
« Reply #4 on: May 10, 2018, 08:04:34 PM »
Long ago I tried to merge them but without tests and knowledge of the code its impossible to solve conflicts.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #5 on: May 11, 2018, 11:00:43 PM »
Splendid, that is very helpful: thank you. I cannot see these commits on your branch; I assume that you have not pushed them yet?

When I merge commits from Standard, what I usually do is to make some standardised alterations and additions to the text of the commit. So, for example, if the original commit message was,

Quote
FIX problem with scrollbar offset in line selector of depot window - take two

The amended version would be

Quote
FIX: Problem with scrollbar offset in line selector of depot window - take two (Dwachs, from Standard)

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #6 on: May 11, 2018, 11:54:39 PM »
I am aware of that (and have previously made commits like that myself). If you would like, I can add that info. However, when using a cherry-pick workflow (as opposed to manually copying a patch), all that information is automatically available - the git-svn-id line included in the commit message implies that it's from Standard, and the original author is preserved as the author of the commit (along with the author date; only the commiter and commit date reflect that I am applying these changes now). When there are no conflicts, the commit is made immediately without me ever touching the commit message.

Another thing that I have encountered are places where I have made non-trivial changes to resolve conflicts. In these cases I have been adding my own comments to the original commit messages, as shown below. (This also shows what "git log" returns without specifying "--format==fuller", or similar.)
Code: [Select]
commit 715550271750076e106f91b62302418ae4c5d087 (HEAD -> merge-from-standard)
Author: Dwachs <dwachs@gmx.net>
Date:   Sat Feb 22 14:53:28 2014 +0000

    sqapi: export lines
   
    ACarlotti: Commented out new functions using WAYTOLL
   
    git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7081 8aca7d54-2c30-db11-9de9-000461428c89
I have considered moving the added line(s) to below the git-svn-id, but I think it would make little difference either way.

You assume correctly that I have not pushed them yet; I saw no value in doing so at this stage, and having them only stored locally means I can go back and directly amend the commits I have already made without all the issues that can arise once those commits are public. My plan at the moment is to check almost all commits to see that they build, and to check that the game seems to run fine at regular intervals, including before pushing to Github. Once I've made significant progress (e.g. ported a major feature), then I'll start asking people to tell me what I've broken.

So, given the above information, would you still like the commit messages amended as you state above? I think there is still a distinction to be made between bulk import of Standard features (as I'm doing now), compared to manual transfer of patches (as has otherwise been happening in the past four years). I don't intend to remove the git-svn-id line, which includes the Standard release number, so the commit messages would already look different to those for manually transferred patches. A further (minor) disadvantage is that manually editting all the commit messages would mean a small increase in workload.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #7 on: May 12, 2018, 10:25:11 AM »
Ahh, now I understand: my apologies, I did not realise that you were using an automated workflow. If you are using an automated/cherry pick workflow, then you need not add this information manually. The additional comments on your modifications also seem to make sense.

Thank you very much for this: this is most helpful.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #8 on: May 15, 2018, 01:02:17 PM »
I've now pushed the first set of changes. These incorporate changes in r7068 (Feb 15th 2014) thru r7127 (Mar 31st 2014), with changes incorporated from 42 of these 60 revisions (the other revisions covered changes that were either already fully ported to Extended, or applied to code that was replaced by Standard). There are also a few additional fixes that I have made to related code while merging.

The major new feature so far is the ability to filter the transport network display in the minimap by player, goods type and vehicle type. I imagine this will be particularly useful in the server game.

James: I think it is probably best that you merge each set of changes as I upload and post about them; that way they will be less likely to break things horribly all at once, and it will be easier to fix any bugs I accidentally introduce as they arise. Unless I think I *have* broken something, in which case I'll ask for a bit more testing first.

Finally, a code comment (by James) that demonstrates perfectly why this is long overdue.
"It is not clear why this is necessary in Extended but not in Standard, ..." (Answer: Because an equivalent fix in the same location was already in Standard - and had been for almost three years)

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #9 on: May 15, 2018, 01:12:04 PM »
Thank you - that is exceedingly helpful. I will incorporate and test this when I get home this evening.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #10 on: May 16, 2018, 12:02:48 AM »
I have now had a chance to look at this, but I am getting "unresolved external symbol" errors when I try to compile. Specifically, I get the following two errors:

Code: [Select]
>------ Build started: Project: Simutrans-Extended, Configuration: Debug (open GL) x64 ------
1>git : Not a git repository warning GitNR1: Git output not valid! Check if the folder is actually versioned. A revision file already exists and its revision number won't be updated. Make sure the revision number is correct or you won't be able to play online with this build.
1>  libbz2.lib(bzlib.obj) : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
1>LINK : warning LNK4075: ignoring '/INCREMENTAL' due to '/LTCG' specification
1>export_objs.obj : error LNK2001: unresolved external symbol "void __cdecl export_line(struct SQVM *)" (?export_line@@YAXPEAUSQVM@@@Z)
1>script.obj : error LNK2001: unresolved external symbol "void __cdecl export_include(struct SQVM *,char const *)" (?export_include@@YAXPEAUSQVM@@PEBD@Z)
1>.\simutrans\Simutrans-Extended-debug.exe : fatal error LNK1120: 2 unresolved externals

Am I missing a new library/include somewhere?

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #11 on: May 16, 2018, 11:36:16 AM »
I forgot to add the new files to the Extended .vcxproj. If this happens in future, I recommend checking the diff for the Standard .vcxproj to see if I've missed anything. In any case, I've corrected this on Github; hopefully that will fix it now.

Incidentally, the history of these two files (api_line.cc and api_include.cc) is a bit strange. You copied them to Extended at some point, I think believing you'd accidentally failed to commit them, when what had actually happened is that the changes that introduced them in Standard hadn't been transferred to Extended. So at the moment I've actually reverted them to an earlier version, consistent with the rest of the scripting stuff.

Another thing I forgot to mention: It is possible that I've broken any scripts that try to use Standard's waytolls. I suspect scripts are probably somewhat broken in Extended anyway, although I've never actually tried using them.

Offline O01eg

  • *
  • Posts: 79
  • Languages: EN, RU
Re: Incorporating changes from Standard
« Reply #12 on: May 16, 2018, 04:21:23 PM »
It would be useful to use AppVeyor CI and Travis CI to test commits and PRs.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #13 on: May 16, 2018, 09:58:42 PM »
Thank you very much for that: now incorporated. That is extremely helpful.

Scripting has not been maintained or developed so far in Extended, and was confirmed to be broken some time ago, so do not worry about breaking any scripts that there might be as I believe that there are none.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #14 on: May 30, 2018, 01:06:33 AM »
I have uploaded another batch of changes, incorporating revisions 7129 to 7161. The main things to note:
1. This includes the fix to allow compiling in gcc 8.
2. It might now be possible to load standard savegames with save version 120.0; I have only tested the latest standard version, which doesn't load yet. The relevant change in save format will be included in extended saves as well when the save version is incremented above 13.5 (no need to do that now though).
3. There are some changes involving way_height_clearance, including reverting a reversion of pak_height_conversion_factor->way_height_clearance (the original revert was apparently because elevated roads were built at the wrong height, but I cannot reproduce this). There is a small possibility of regressions in this code, though the only change I'm aware of is that it is no longer possible to build an elevated road along a road crossed by a low bridge.
4. I have discovered a number of bugs relating to way_height_clearance, many of which are also present in Standard. Since they don't appear to be particularly urgent, I intend to fix them after merging the remaining Standard changes (or at least all of the ones that touch relevant code).

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #15 on: May 30, 2018, 10:09:19 PM »
Excellent, thank you very much: now incorporated.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #16 on: July 14, 2018, 06:37:34 AM »
I've pushed some more changes; I don't think there's anything particularly significant in this batch.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #17 on: July 14, 2018, 10:17:40 AM »
Splendid, thank you: now incorporated.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #18 on: August 14, 2018, 05:40:10 PM »
A longer bath of changes this time, covering a period from May 7 to Oct 12 2014.

The main things of note are:
There were changes relating to the pakselector in Standard that I only partly ported because Bernd Gabriel rewrote the selector for Extended back in 2013. Since I don't think there are any differences to the functionality requirements, this seems like an example of a change that (in my opinion) shouldn't have been made only in Extended (cf. Code Quality thread).

I have reverted an accidental overwrite of readme.txt (in the repository root) with a file relating to help texts. I have also now added an initial section relating to Simutrans Extended.

I have also added a commit applying the translation of komp->comp to the OTRP code.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #19 on: August 14, 2018, 07:24:34 PM »
Thank you very much for this - reviewing the code, I see that this is a very substantial amount of work. I have now committed this, and a further commit removing redundant instances of CACHE_TRANSIT so that accidentally defining this does not cause incorrect behaviour.

These changes should appear in to-morrow's nightly build.

Offline ACarlotti

  • *
  • Posts: 223
Re: Incorporating changes from Standard
« Reply #20 on: September 17, 2018, 06:52:34 AM »
I've now uploaded another set of changes. These include (among other things) a lot corrections to comments; a bug fix for (I think) handling changing goods connections when changing a station auxillary building; and random commit to remove some accidentally duplicated code that I spotted.

Progress is now up to r7395 from December 3 2014.

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 17586
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: Incorporating changes from Standard
« Reply #21 on: September 18, 2018, 08:17:39 PM »
Thank you very much - now incorporated.