FS#1859 - Automatic rail type upgrade for trains - WIP - need help to integrate into official code

Attached to Project: OpenTTD
Opened by Garthy (Garthy) - Monday, 17 March 2008, 04:11 GMT
Last edited by Remko Bijker (Rubidium) - Wednesday, 09 September 2009, 21:12 GMT
Type Work in progress
Category Vehicles
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 2
Private No


I have written code to allow trains to be automatically upgraded to a new
rail type. This can be done on a one-off basis for an individual train, or
via the conversion tool for all trains in a depot. This code also sets the
groundwork for a mass-conversion tool between rail types (eg. rail to
monorail, monorail to maglev).

I am looking to get in touch with someone who is able to look over the
code (I've only been using the OpenTTD code for a couple of days), let me
know what needs fixing, help with an issue or two, and then integrate it
into the official release.

The code is currently written against release r12366.

Further details follow. If you're one of the main devs, these are the same
as in the original email I sent you, nothing new here. Anyway:

With the changes I've made, there are two new things you can do:

- Select a depot with a new rail type, select "Clone Train", then a train
on an incompatible rail type. A new train will be built at the station
using a best-fit selection of replacement locos and wagons.
- Using the rail conversion tool, select a depot. This will convert every
train inside and the depot to the new rail type, using the best-fit
selection mentioned above. This means that if you drag the tool across
the whole map while all trains are in their depots, *everything* is
upgraded at once.

Please note the code uses vehicle cloning rather than MaybeReplaceVehicle
and ReplaceVehicle (I tried, couldn't get it to work), so things such as
service history and custom train names are not preserved (yet- could be
added in the future). I also haven't figured out the patch system yet, so
I've just keyed if off a single global variable at the moment. I might need
a hand there. It is also full of tracewrites at the moment, so I will need
to remove these. Please bear in mind that I've only been using the source
for OpenTTD for a couple of days now, so it's still pretty new to me.
The code probably has bugs.

Possible future additions would be to add a dropdown to the train manage
list to convert the whole map, and integration with the vehicle upgrade
tool to allow players to explicitly select conversions. I'll just stick to
getting this first bit in for now though. :)

I'm now wondering what the best way is to get this code to you, what I need
to do to clean it up for inclusion, et cetera. Patches versus the source? A
tgz of the changed files?

I'm also happy to spend time cleaning up the code, adding additional
commenting, changing things as needed, and writing small bits of
documentation for the changes as well.

When you can, please drop me an email and let me know what I need to do to
get my changes included in the official code.

Thanks in advance! :)


PS. I'm based in Australia, so please bear the different timezone in mind
- it might be the middle of the night when I get your reply. :)
This task depends upon

Closed by  Remko Bijker (Rubidium)
Wednesday, 09 September 2009, 21:12 GMT
Reason for closing:  Out of date
Comment by Peter Nelson (peter1138) - Monday, 17 March 2008, 07:59 GMT
You might like to include a patch.
Comment by Garthy (Garthy) - Monday, 17 March 2008, 09:01 GMT
Certainly. I would recommend that this patch only be used for once-off testing at this stage, it's not 100% ready. Full details about what is wrong with it in the original post.

The file supplied is the output of:

diff -rup orig/OTTD-source-nightly-r12366 work/OTTD-source-nightly-r12366 > railtypeupgrade.diff

If the patch is best supplied in another form, please let me know and I'll see what I can do.
Comment by Arthur Pallas (athanasios) - Tuesday, 18 March 2008, 00:22 GMT
Does it preserve group train belongs to?
Comment by Garthy (Garthy) - Tuesday, 18 March 2008, 08:06 GMT
Hi Arthur,

Whilst I didn't add explicit support for it, and haven't ever used groups, I tried it out and it actually does preserve them. Train cloning seems to preserve group, and since my code uses train cloning to create a new train, the group seems to be preserved.

So yes, purely by accident. ;)
Comment by Garthy (Garthy) - Wednesday, 19 March 2008, 08:42 GMT
Okay, plan B: Here's the same patch with the tracewrites and incomplete openttd patch support code removed. It should be suitable to apply against the source to give you support for train rail type upgrades.

If you're here from a web search, and are looking to upgrade your trains from one rail type to another (eg. monorail to maglev), and want this feature before it is officially in the code, feel free to download the patch and apply against your code. Just be aware of the caveats I mention in my original post!

Devs: Please feel free to drop me a line if you'd like me to work with you to get this patch into the codebase. :)
Comment by Garthy (Garthy) - Wednesday, 19 March 2008, 08:46 GMT
Same patch tidied up a bit...
Comment by Zdeněk Sojka (SmatZ) - Monday, 24 March 2008, 18:19 GMT
I am afraid this can cause different test and exec runs very easily (see DC_EXEC). What happens if you can convert only some trains you have in the depot? (because of money, because of no available engine replacement). I didn't read the code in very detail, but I was able to make it assert after a while (several times converting between different rail types)
Also have a look at
Comment by Garthy (Garthy) - Wednesday, 26 March 2008, 09:40 GMT
Hi Zdenek,

Thanks for checking out my code.

Bearing in mind that I am new to the OpenTTD codebase, I should point out that I'm not too strong on the function of DC_EXEC. The impression I got is that most operations are handled by a run without DC_EXEC first, and then a run with. I had thought that on a run without DC_EXEC with failure conditions (eg. not corresponding wagon) it will fail and not proceed on the DC_EXEC run? If I am incorrect- do you have a set of circumstances that cause it to fail, so I can make it fail here as well?

Any tips on changing money levels (up and down) to allow me to experiment with this as well? I should be able to figure it out, I'm just wondering if there is an easy way.

Re assert failures, may I ask which version you applied from, what caused it, and where the assert failed? The patch removes two assertions, because they no longer hold true (one, for example, is a check that the train lengths match- something that isn't always true post-patch). Perhaps something went wrong there?

Please feel free to send me details and/or savefiles directly if you like- my email address can be found in this image: If I can recreate the problem, I might be able to fix it.

Thanks for the link to the coding style document- I have been guessing the style for now, an official doc will help me clean it up.
Comment by Zdeněk Sojka (SmatZ) - Monday, 07 April 2008, 21:20 GMT

this is the assert I get:
src/command.cpp:628: bool DoCommandP(TileIndex, uint32, uint32, void (*)(bool, TileIndex, uint32, uint32), uint32, bool): Assertion `res.GetCost() == res2.GetCost() && CmdFailed(res) == CmdFailed(res2)' failed.

I think I was converting two depots at once, but maybe it was caused by something different. Maybe one depot could be converted while the second couldn't (the was train partially outside the depot).

When you need money, use the cheat - Ctrl+Alt+C
Comment by Garthy (Garthy) - Friday, 11 April 2008, 01:57 GMT
It is unlikely that I'll be able to find the error with just the assert line, given my limited understanding of the code thus far. I fear I'll need a fair bit more information- such as version, a savefile, how to reproduce- to have much of a chance of finding the problem.

"Cheats" to reduce money are probably more helpful in testing than ones that increase it, especially in the context of "What happens if you can convert only some trains ... because of money".


It's been a few weeks now since I sent in the original code and I'm still not sure of a path to get this code into the main codebase, or if it is even useful or wanted. Enormously frustrating. As such, I think it best if I leave things be and move on at this point.

I hope that the code I've sent in proves useful in some way, even if just as inspiration or similar. The depot-finding code and wagon suggestion code included might be useful in some way.

Had this or similar code made it in, I imagine the direction I would have moved in next would have been:

- Add the ability to specify rail-type upgrades to the "Manage Trains" dialog in some way. Then use the wagon suggestion code as a fallback for when the player has not specified something explicitly.

- Add another option after "Manage Trains" with a name such as "Complete Rail Upgrade" to perform a complete rail-type upgrade of the whole map to the latest available type. Block it with a "Are you sure?"-type dialog to prevent misclicks.

I hope these ideas also prove useful in some way.

Anyway, many thanks for making OpenTTD available. I've had a *lot* of fun playing this game- much more than the original TTD. Best of luck with development!