[v2.1.0] Template based train replacement [OpenTTD 1.9.3]

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

User avatar
DC-1
Engineer
Engineer
Posts: 88
Joined: 13 Mar 2013 13:53

Re: [v0.4c] Template based train replacement r24093

Post by DC-1 »

Another quickfix in orders_cmd.cpp near line 1660:

Code: Select all

        /* Only check every 20 days, so that we don't flood the message log */
-       if (v->owner == _local_company && v->day_counter % 20 == 0) {
+       if (v->owner == _local_company && v->day_counter % 20 == 0 && !HasBit(v->subtype, GVSF_VIRTUAL) ) {
Knatte
Engineer
Engineer
Posts: 6
Joined: 06 Aug 2006 10:59
Location: Sweden

Re: [v0.4c] Template based train replacement r24093

Post by Knatte »

Have found a bugs that have to do with this patch but unsure if last version is used (using YAPP http://www.tt-forums.net/viewtopic.php?t=65525)

If a train is forced in to a depot and that Group have a replacement in Place it is stopped and not started again, happens to both old(wrong template) and new(right template)
does not happen if set to maintence.

Other that I love the patch and it is even more important then any other patch I have used :) because I often upgrade my lines to longer trains then faster and stronger locomotives is available. :)
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.4c] Template based train replacement r24093

Post by ffpp »

Thanks for the bug pointers, I will integrate them (soon :mrgreen:)

I put the patch under less-manual version control, it is available as git repo now, see first post for the URL. The repo also contains a directory with the current patch and older versions of it.

I merged the current trunk version into the patch, so from that side there shouldn't be any compatibility problems and will continue to keep this up-to-date.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27156

Post by ffpp »

I added fixes for both reported bugs by DC-1.

1. The game would crash when the template gui was called from the station gui. This was because the group-gui and station-gui apparently use the same dropdown list for display of their items but the station-gui had no handler if the template-gui item was selected, which caused the game to crash.
I added the necessary call to create the template main window also from the station gui.

2. When vehicle orders are being checked, virtual vehicles (i.e. template trains) are no longer considered during those checks.
I don't know if this caused any problems (didn't notice any) but the check was unnecessary since template trains don't carry any orders.

I will also update the OP for the hotfixed version. Thanks for the reports @DC-1 !
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27171 + binarie

Post by ffpp »

I fixed the project files, they still contained the old filename prefixes and thus couldn't be used for compilation.

I also added linux + windows binaries. This should make it easier for some people to use/test the patch.
The windows build was done using Visual Studio, but I will try switching to the cross compilation method soon.
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27171 + binarie

Post by coherently_olive »

First of all, absolutely love this patch! A dream come true for everyone who hates tedious clickery.

Secondly, I found something in CmdTemplateReplaceVehicle (tbtr_templace_vehicle_func.cpp):

Code: Select all

	TileIndex tile = incoming->tile;
	TemplateVehicle *tv = GetTemplateVehicleByGroupID(incoming->group_id);
	EngineID eid = tv->engine_type;

	CommandCost buy(EXPENSES_NEW_VEHICLES);
	CommandCost move_cost(EXPENSES_NEW_VEHICLES);
	CommandCost tmp_result(EXPENSES_NEW_VEHICLES);

	/* first some tests on necessity and sanity */
	if ( !tv )
		return buy;
The sanity check on tv is performed after it has already been accessed. This crashed my game when I forced a vehicle which was getting old and was scheduled for auto-renewal into a depot. Moving the command cost definitions and the sanity check up, before tv->engineType fixed the crash, but the core issue still seems to be that this patch conflicts with auto-renewal, since the trains in question still do not get renewed. This is only my first, wild guess and I'm not all too familiar with the codebase, so please take the following with a grain of salt:
I think that auto-renewal uses the same system as the default auto-replacement feature. Now if a train is in a group that has a template assigned, this patch suppresses the auto-replacement. Thus trains do not get auto-renewed. For some odd reason, however, the train's group ID does not seem to be valid when it is passed to CmdTemplateReplaceVehicle, since incoming->group_id seemed to return 0 here, but only for trains which are scheduled for auto-renewal. My first guess was that the vehicles get auto-renewed, which would invalidate the reference in _vehicles_to_templatereplace. But then again, the train would be actually auto-renewed once I fixed the crash if this were the case, which it doesn't.

Edit: After the fix above, it actually does auto-renew the train, but only if it's forced into a depot. It does not auto-renew it when it's just stopping by for maintenance.
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27171 + binarie

Post by coherently_olive »

coherently_olive wrote: My first guess was that the vehicles get auto-renewed, which would invalidate the reference in _vehicles_to_templatereplace.
This was partially confirmed by the fact that switching the order of the loop that template-replaces vehicles with the loop that auto-replaces vehicles in CallVehicleTicks (vehicle.cpp) has the same effect as moving the null pointer check up as explained in the quoted post. The reason for the train being in _vehicles_to_autoreplace (despite this patch taking care of only adding vehicles to either autoreplace or templatereplace) was that it was put there by VehicleEnterDepot (vehicle.cpp). One of the passages that put it there is definitely from the timetable separation patch that I'm using, but the other might not be. In either case, I think that this patch should be able to handle inconsistent states of the autoreplace and templatereplace maps.
In my local repo, I therefore changed VehicleEnteredDepotThisTick to always add the vehicle to _vehicles_to_autoreplace. Then I switched the order of the loops for template- and auto-replacement such that template-replacement is now executed before auto-replacement is. If CmdTemplateReplaceVehicle().getCost().getMoney() != 0 in the template replace loop, then the train is removed from _vehicles_to_autoreplace. It seems that removal from the map has linear-time costs, but as the maps only contain vehicles that entered a depot that very tick, this shouldn't be all too bad. Although long-term testing is still withstanding to catch all possible combinations of template-/ auto-replacement states, this dirty fix should do the trick for now.

Edit: Someone with experience may check if template-replacement does in turn invalidate the pointer to the train in _vehicles_to_autoreplace. If this is the case, then my fix will break the game if a train is template replaced with 0 costs (e.g. because all the vehicles were already sitting in the depot?).

I attached patches for both the null pointer sanity check and the replacement map consistency. Please note that the line numbers might be off.
Attachments
tbtr-null-check.patch
(1.2 KiB) Downloaded 215 times
tbtr-inconsistent-maps.patch
(2.97 KiB) Downloaded 205 times
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27171 + binarie

Post by ffpp »

Thanks for the patches and thorough observations on the problem coherently_olive!
I added the first patch as-is for now, this is quite a clear case.

The second problem indeed requires some testing.
Do you still have the savegame that crashed, which was saved shortly before the auto-renewal ? (If I can even load it, that is)
Otherwise I would probably try to forge a newgrf to get a train that has a low lifetime and would need to get renewed shortly after buying. Then I could test the different combinations of auto-renewal, auto-replacement and template replacement.
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27171 + binarie

Post by coherently_olive »

I should still have the savegame, but I run multiple patches, some of which modify the data contained in the savegame. If you want, I can give you a patch combining all the changes and building on openttd trunk, but I think it'd be much less hassle (for both of us :wink: ) if you just started a game, bought a short-lived vehicle and let it run in circles on fast-forward. Maybe even simply modify the game so that trains already start out at their max age for testing purposes? Anyway, I still have not looked up whether the inconsistency between the map was introduced by any other patches I applied or if it's there by default. I don't know in how far you'd even want to support other patches interfering with your own if the former is the case.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27171 + binaries

Post by ffpp »

updated by merging trunk 27507
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27527 + binaries

Post by coherently_olive »

I would like to (shamelessly) post a link to a repository here containing my minimalistic patchpack.
Please note that I don't want to hijack this patch in any way, it's just that I've done a fair bit of modifications to the original TBTR for which I'm not willing to extract and submit individual patches. The bitbucket repository in the link below contains a branch called 'tbtr-patched', which is a clean version of just openTTD's trunk merged with TBTR, i.e. it is not influenced by any other patches that are contained in the master branch. In that branch, I added the following features:
  • Merged OpenTTD trunk r27669
  • Fixed a crash that occured when a train was scheduled to be both auto-replaced and template-replaced. Now, if it is scheduled for both, then it will be template-replaced and only if that fails it will be auto-replaced instead. Might still crash if a train was successfully template replaced with 0 costs (i.e. because all required engines / wagons were already sitting in the depot) -> This is basically the patch that I submitted somewhere in this thread
  • Added a feature that will only template-replace trains that would otherwise be auto-replaced because they're too old, for when users want to keep amortized costs low for trains they just bought. The "old" template replacement style is now called "forced replacement", the new style is called "replace when old".
  • Added the ability to show / hide vehicles in the vehicle list used for editing or creating template trains
  • Fixed the engine name offset that would hide the name behind icons in trainsets where an engine has multiple icons next to each other (e.g. engine type symbols and flags in 2cc)
  • Made the group list in the template window behave like the group list in the group window. This means that the template window now shows group hierarchically and with correct indentation. KeldorKatarn's group collapse functions have also been added to the template group list, meaning that users can collapse / expand the groups via buttons.
  • Added a feature that allows users to recursively issue template replacement orders to a group and all of its children in the group hierarchy. Recursive changes are only applied to children that are currently visible (because the parent is expanded). Recursively removing template replacement orders is _not_ included.
  • Added savegame versioning to allow loading vanilla savegames
I hope it's understandable that I forked your repo instead of submitting individual patches. Here's the link: https://bitbucket.org/ploewner/openttd- ... patchpack/
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27527 + binaries

Post by ffpp »

coherently_olive wrote:I would like to (shamelessly) post a link to a repository here containing my minimalistic patchpack.
Please note that I don't want to hijack this patch in any way, it's just that I've done a fair bit of modifications to the original TBTR for which I'm not willing to extract and submit individual patches. The bitbucket repository in the link below contains a branch called 'tbtr-patched', which is a clean version of just openTTD's trunk merged with TBTR, i.e. it is not influenced by any other patches that are contained in the master branch. In that branch, I added the following features:
  • Merged OpenTTD trunk r27669
  • Fixed a crash that occured when a train was scheduled to be both auto-replaced and template-replaced. Now, if it is scheduled for both, then it will be template-replaced and only if that fails it will be auto-replaced instead. Might still crash if a train was successfully template replaced with 0 costs (i.e. because all required engines / wagons were already sitting in the depot) -> This is basically the patch that I submitted somewhere in this thread
  • Added a feature that will only template-replace trains that would otherwise be auto-replaced because they're too old, for when users want to keep amortized costs low for trains they just bought. The "old" template replacement style is now called "forced replacement", the new style is called "replace when old".
  • Added the ability to show / hide vehicles in the vehicle list used for editing or creating template trains
  • Fixed the engine name offset that would hide the name behind icons in trainsets where an engine has multiple icons next to each other (e.g. engine type symbols and flags in 2cc)
  • Made the group list in the template window behave like the group list in the group window. This means that the template window now shows group hierarchically and with correct indentation. KeldorKatarn's group collapse functions have also been added to the template group list, meaning that users can collapse / expand the groups via buttons.
  • Added a feature that allows users to recursively issue template replacement orders to a group and all of its children in the group hierarchy. Recursive changes are only applied to children that are currently visible (because the parent is expanded). Recursively removing template replacement orders is _not_ included.
  • Added savegame versioning to allow loading vanilla savegames
I hope it's understandable that I forked your repo instead of submitting individual patches. Here's the link: https://bitbucket.org/ploewner/openttd- ... patchpack/

Hi!
That's fine. I think the list of your changes sounds interesting. I will test your patched version in-game.
Would it be o.k. with you for me integrate (a list of) your changes into my patch ? Listing you as a collaborator, of course.
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27527 + binaries

Post by coherently_olive »

ffpp wrote:Would it be o.k. with you for me integrate (a list of) your changes into my patch ? Listing you as a collaborator, of course.
Sure, having anything of it merged upstream would be great. Most of the group list stuff just consisted of copy-and-pasting OpenTTD trunk's group list widgets and their callback handlers to TBTR as well as merging KeldorKatarn's patch. I don't know how much that counts for a contribution, but you're free to list me anyway if you want.
[+] Spoiler
Also, please ignore the commit comment where I may or may not have called the way TBTR checks whether a replacement has been successful "hacky". I didn't mean it, especially now that I'm not totally sure any more if i didn't introduce that hack myself :D
coherently_olive
Engineer
Engineer
Posts: 14
Joined: 24 Apr 2015 17:16

Re: [v0.5] Template based train replacement r27527 + binaries

Post by coherently_olive »

New post because this is separate information from the last one:
  • Broke template selection in my repo which would lead to GUI bugs and crashes if a vehicle with wrong template was replaced -- Should be fixed now
  • Allowed refitting template trains (again?)
  • Fixed a crash that happened when clicking the refit button for an empty template train. Now the button does nothing if there are no vehicles in the train.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.5] Template based train replacement r27527 + binaries

Post by ffpp »

coherently_olive wrote:New post because this is separate information from the last one:
  • Broke template selection in my repo which would lead to GUI bugs and crashes if a vehicle with wrong template was replaced -- Should be fixed now
  • Allowed refitting template trains (again?)
  • Fixed a crash that happened when clicking the refit button for an empty template train. Now the button does nothing if there are no vehicles in the train.
Hi,

thanks for the contributions. I agree to most of them and have merged those into my patch.
Wahazar
Tycoon
Tycoon
Posts: 1451
Joined: 18 Jan 2014 18:10

Re: [v0.701] Template based train replacement r27742 + binaries

Post by Wahazar »

I'm using this patch introduced in JGRPP and I would say, that it is a great and useful tool (pity it was not implemented in original game 20 years ago ;)

But I got another idea: change groups dynamically with use schedule orders, together with template replacement it would give possibility for scheduled change of train consist.
Now I see, that you has similar idea:
ffpp wrote:Todo
[*]The ability to use a template replacement as a vehicle order
[*]the template would be used independently of a vehicle group
[*]replacement order can be inserted similar to 'visit depot for maintenance'
It would be awesome, to change consists via depot orders, imagine that one can:
  • switch from electric to diesel/steam back and forth,
  • add helpers to pass mountains,
  • drop some empty cars after intermediate station and pick them on return trip,
  • or even drop some cars to be picked by another train!
I hope that such feature would be implemented.
One important notice: I think that template via order should be restricted to use only vehicles in depot and leave vehicles in depot options,
to avoid cheats like 'constantly sell/buy locomotives to increase station rating' or 'sell empty freight cars, go back with and 1 locomotive only and buy new stock'. On the other side, template order should not check bank balance criteria, to avoid jams due to temporary lack of funds to change electric to diesel even if there is diesel waiting in depot. There is no need to check funds, if order driven template replacement is restricted only to use existing stock from depot.
Formerly known as: McZapkie
Projects: Reproducible Map Generation patch, NewGRFs: Manpower industries, PolTrams, Polroad, 600mm narrow gauge, wired, ECS industry extension, V4 CEE train set, HotHut.
Another favorite games: freeciv longturn, OHOL/2HOL.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.701] Template based train replacement r27742 + binaries

Post by ffpp »

McZapkie wrote:I'm using this patch introduced in JGRPP and I would say, that it is a great and useful tool (pity it was not implemented in original game 20 years ago ;)

But I got another idea: change groups dynamically with use schedule orders, together with template replacement it would give possibility for scheduled change of train consist.
Now I see, that you has similar idea:
ffpp wrote:Todo
[*]The ability to use a template replacement as a vehicle order
[*]the template would be used independently of a vehicle group
[*]replacement order can be inserted similar to 'visit depot for maintenance'
It would be awesome, to change consists via depot orders, imagine that one can:
  • switch from electric to diesel/steam back and forth,
  • add helpers to pass mountains,
  • drop some empty cars after intermediate station and pick them on return trip,
  • or even drop some cars to be picked by another train!
I hope that such feature would be implemented.
One important notice: I think that template via order should be restricted to use only vehicles in depot and leave vehicles in depot options,
to avoid cheats like 'constantly sell/buy locomotives to increase station rating' or 'sell empty freight cars, go back with and 1 locomotive only and buy new stock'. On the other side, template order should not check bank balance criteria, to avoid jams due to temporary lack of funds to change electric to diesel even if there is diesel waiting in depot. There is no need to check funds, if order driven template replacement is restricted only to use existing stock from depot.
Hi McZapkie,
I agree about the depot stuff. It should not be too hard to allow a template-replacement being added as a vehicle order.
Maybe there would be the need for helping the player with handling the extra complexity as well. Otherwise it could get a little out of hand that trains/vehicles are being bought left and right or are just sitting around in depots in bunches when a lot of trains use orders that involve train recomposition :)
Wahazar
Tycoon
Tycoon
Posts: 1451
Joined: 18 Jan 2014 18:10

Re: [v0.701] Template based train replacement r27742 + binaries

Post by Wahazar »

ffpp wrote:...Maybe there would be the need for helping the player with handling the extra complexity as well. Otherwise it could get a little out of hand that trains/vehicles are being bought left and right or are just sitting around in depots in bunches when a lot of trains use orders that involve train recomposition :)
There is little to do without messing with current openttd philosophy (train defined for leading engine only), but one thing should to be resolved: handling with depot schedule replacement in case of train delay.
Traffic deadlock is guaranteed, if some train cannot wait for full load because won't pick group of cars due to breakdown of other train feeder, or electric locomotive will not continue its journey because there was temporarily lack of diesel.

There is simply workaround to deal with such situations:
because I proposed, that depot scheduled replace is restricted to use vehicles from depot, without purchase of new ones,
in case of lack of these vehicles, train should be stalled in depot (not permanently stopped, just suspended like these waiting in depot for free track).
From time to time, scheduled replacement should be retry and warning message popped in case of failure.

In such case either situation will be solved by approaching late train, or fixed manually (if user messed something up with symmetry of replacing, for example forgot to switch back diesel to electric), without risk of traffic jam.
Formerly known as: McZapkie
Projects: Reproducible Map Generation patch, NewGRFs: Manpower industries, PolTrams, Polroad, 600mm narrow gauge, wired, ECS industry extension, V4 CEE train set, HotHut.
Another favorite games: freeciv longturn, OHOL/2HOL.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v0.701] Template based train replacement r27742 + binaries

Post by ffpp »

Right, there are a few things to be considered. But it sounds quite interesting.

But first, I want to replace this awful 3-tiered window mechanism that is currently necessary to create new template or modify existing ones. It is so clumsy to use.
User avatar
ffpp
Engineer
Engineer
Posts: 125
Joined: 29 Jan 2010 12:56

Re: [v2.0.0] Template based train replacement

Post by ffpp »

done

version 2.0.0 is out
added linux binaries
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 1 guest