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

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.

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.

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:

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

git-svn-id: svn://tron.homeunix.org/simutrans/simutrans/trunk@7081 8aca7d54-2c30-db11-9de9-000461428c89I 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.

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.

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)

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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

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.

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?

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.