Topic: Incorporating changes from Standard

[ACarlotti]

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.

[Matthew]

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.

 

[jamespetts]

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.

[ACarlotti]

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

[O01eg]

Long ago I tried to merge them but without tests and knowledge of the code its impossible to solve conflicts.

[jamespetts]

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,

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

The amended version would be

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

[ACarlotti]

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.

[jamespetts]

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.

[ACarlotti]

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)

[jamespetts]

Thank you - that is exceedingly helpful. I will incorporate and test this when I get home this evening.

[jamespetts]

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?

[ACarlotti]

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.

[O01eg]

It would be useful to use AppVeyor CI and Travis CI to test commits and PRs.

[jamespetts]

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.

[ACarlotti]

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

[jamespetts]

Excellent, thank you very much: now incorporated.

[ACarlotti]

I've pushed some more changes; I don't think there's anything particularly significant in this batch.

[jamespetts]

Splendid, thank you: now incorporated.

[ACarlotti]

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.

[jamespetts]

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.

[ACarlotti]

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.

[jamespetts]

Thank you very much - now incorporated.

[Ranran]

I threw a pull request for fixing this bug from standard (by prissi). (´・ω・｀)

[jamespetts]

Splendid, thank you: now incorporated.

[Phystam]

I started to merge standard code, following A. Carlotti-san's work.

There was too many conflicts due to translation from German to English, including the file or directory names. (e.g. descriptor is known as "besch")
I solved them and pushed to my repository (merge-from-standard branch), and send a pull-request. This pull-request contains changes from Dec. 4 2014 to Dec. 7 (from r7395 to r7408).
If this way is correct, I will continue to work this.

[jamespetts]

Thank you very much for that. I have now incorporated these. I did have to comment out one part of the code relating to the scripting API (setting text labels) as this would not compile in Visual Studio. This is not a high priority issue as the scripting API is not currently supported, and I do not know what has caused this specific issue, but it would be helpful if you could look into this at some point.

[ACarlotti]

Phystam: You have just duplicated some work that I have already done but hadn't yet uploaded. I think it would be difficult to coordinate multiple people trying to systematically port changes at the same time.

In any case, this makes it clear that an update on my work is needed. I haven't made much further progress on incorporating changes since September, for two different reasons. Firstly, I was a little busier than previously, so had less time available (although not so little that I couldn't have continued incorporating changes). However, the second issue that I am reluctant to add major changes to the code wholesale while we are still trying to debug the current desync issue. This is partly because I perceived that such changes would receive less testing at present, and partly because I think there is a risk of masking the desync-causing bug by making other changes to the codebase. Perhaps neither of these concerns is particularly justifiable - if that is the case then I will probably begin merging new changes soon.

Anyway, while I appreciate your effort in trying to incorporate changes yourself, I don't feel that it is particularly helpful, given that my workflow typically consists of merging several weeks of changes of the course of a few weeks, and then uploading them all as a batch. (I also avoid pushing WIP to Github so that I can safely amend commits as required when I inevitably make mistakes).

[Phystam]

jamespetts-san,
Thank you so much Can we skip merging changes related with sq api?

A. Carlotti-san,
Yes, I also think that changing code while debugging desync issues is a little bit dangerous. But we can work separately, using another branch. If you push WIP to your github branch, it will be our benefit -- we can check your important progress.
And when I follow your workflow of incorporating, can we accept and follow our works each other?
Anyway sorry for duplicated works --- my apologies.

I have already merged changes from Dec. 8  to Feb. 9 2015 (r7525). From previous pull-request, almost all commits have already merged previously, so there were few commits to merge.

[ACarlotti]

A few observations:
Some of your commit messages consist mostly of a long list of merge conflicts. I don't know why those are present - by default I thought those commented lines were ignored in generating the commit messages. In any case, I think they are unhelpful and shouldn't be there, so it's probably worth checking your configuration to see why they are appearing.
In many cases rebasing or amending past commits is bad. However, there are also times when it is very useful. In this cases, rather than making a new commit to revert your accidental creation of simwerkz.cc/.h, I think it would have been better to amend your past commits to remove the original commit from the branch. Since you spotted your mistake after only half an hour, and almost certainly noone else will have downloaded your past commits by that point, then the tidier option is probably better.
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.

I've now gone through and applied my more accurate changes on top of yours. However, this essentially required doing a couple of hours of work that wasn't previously necessary, and means that the commit history will now be somewhat more confusing. To avoid me having to repeat this work on more of the Standard changes, could I ask that you(/James) don't merge any more of your changes for now, at least until I've had a chance to sort out the above changes I'd already ported, and upload them (by which I mostly mean rebase onto the new branch point and check it all still works).

[ACarlotti]

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, given that you claim to have got through changes from Dec 8 to Feb 9 relatively quickly, I think that you have been far less thorough than I am. As well as porting any functional changes and bug fixes, I am also trying to minimise the differences to Standard in general. Furthermore, I don't trust James to have merged changes correctly (unfortunately) - I have found many minor errors where he hasn't done a merge correctly.

As an example (from the next really large commit, which I am part way through porting): Did you notice that simwerkz-dialogs.h was effectively copied instead of renamed? And that's one of the biggest mistakes present. (I'll upload the fix once I've done the rest of that commit, and probably the next few. Most of these sorts of changes won't affect functionality at all so almost certainly won't intefere with the desync).

So, for consistency and soundness of mind, I would like to work through all these changes myself (at least for the moment).

January 2015 had a load of translations, which means lots of big changes which don't merge easily (and so are more likely to have errors/inconsistencies). So I think getting through that month will be quite slow, but things will be quicker afterwards because there will be fewer translation conflicts.

[jamespetts]

Thank you very much for your work on this. It is difficult keeping on top of a large number of different coding issues when this is essentially a hobby, so my apologies for the manual merging in the past not being always to the highest standard. Co-ordinating different people's contributions is also a difficult task leading to surprisingly complex decisions about what would be preferable in many cases, and any system or protocol that can assist reliably in the long-term (i.e. consistently over the course of many years, withstanding unpredictable changes in which people take responsibility for which aspects of development) of which anyone can think would be most helpful.

Can I check - did you want me to merge your latest changes on this branch now or await further work on attempting to fix the loss of synchronisation bug?

Thank you again for your work on this.

[ACarlotti]

Given that I don't think there are any major or particularly relevant changes, and fills in gaps in what Phystam has ported, then I think it may as well go in now.

[jamespetts]

Splendid, thank you.

Do you think it sensible to hold off further merging until the loss of synchronisation issue has been identified?

[ACarlotti]

I think that the next chunk of work to do is mostly tidying up translation commits, which should have virtually no impact on functionality. But then after that it would perhaps be sensible to wait again - it depends on exactly what is coming up.

[jamespetts]

Splendid, thank you for clarifying.