Patch: Autoconvert

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
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Patch: Autoconvert

Post by cirdan »

Hi,

Following a recent topic in the suggestions forum, I am posting a patch that allows autoconversion of trains in depots when the depot is converted to a new rail type. To make use of this feature, first you have to set up a replacement for each of your engines/wagons, using an interface similar to that of autoreplace, only that the target rail type is also taken into account (see the attached screenshot). Once this is done, when you attempt to convert a depot to a new rail type, all trains inside will be checked for a replacement for that rail type and, if all of them have one (or can themselves run on the new track type), they are converted along with the depot. If an engine or wagon does not have a replacement, or the conversion cannot be otherwise carried out, then you get your typical 'train in the way' error message and nothing is changed.

Edit: Current status: Patch against r24411
Attachments
autoconvert.png
autoconvert.png (176.26 KiB) Viewed 9110 times
autoconvert-r22854.patch
(50.92 KiB) Downloaded 412 times
Last edited by cirdan on 19 Jul 2012 10:09, edited 3 times in total.
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

Nice idea. Me likes!

Maybe you should consider combining both windows, keeping the current behaviour when replacing trains with compatible railtypes and not replacing trains if the railtype is not compatible.

Add to that a news message "Cannot replace train #, incompatible railtype. Stop train in depot and then convert depot to replace train." to inform the user why replacing doesn't work and what to do to make it work.

That should both make the current dialog more clear as you specifically set a "from" and a "to". For incompatible railtypes you can first make all the rules and then send all your trains to the depot; upgrade the depot and track and the trains should come out changed according to the rules set up.
User avatar
adf88
Chief Executive
Chief Executive
Posts: 644
Joined: 14 Jan 2008 15:51
Location: PL

Re: Patch: Autoconvert

Post by adf88 »

Marvellous!
FooBar wrote:Maybe you should consider combining both windows, keeping the current behaviour when replacing trains with compatible railtypes and not replacing trains if the railtype is not compatible.
The GUI part can be combined into single window and it should be done I think. But I'm not sure if the autoreplace and autoconvert mechanisms should be combined together. From a one hand we gain simplification but from the other we have less possibilities. It would be good for small networks. In case of big, multi-rail-type networks we would loose some on management.

I would consider one more sub-feature. There is a bunch of cases where OTTD is played without any train replacement set (it may be the most common case). Thus I think there should exist implicit autoconversion. If a user don't specify replacement let the game to choose what vehicles to convert to. It may be some prosaic rule.
:] don't worry, be happy and checkout my patches
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

I don't think you'd necessarily have to loose functionality by combining the windows. But I'm only suggesting ;)
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: Patch: Autoconvert

Post by cirdan »

FooBar wrote:Maybe you should consider combining both windows, keeping the current behaviour when replacing trains with compatible railtypes and not replacing trains if the railtype is not compatible.

Add to that a news message "Cannot replace train #, incompatible railtype. Stop train in depot and then convert depot to replace train." to inform the user why replacing doesn't work and what to do to make it work.

That should both make the current dialog more clear as you specifically set a "from" and a "to". For incompatible railtypes you can first make all the rules and then send all your trains to the depot; upgrade the depot and track and the trains should come out changed according to the rules set up.
I am not sure I understand what you mean: You want to keep the current interface, only that allowing non-compatible replacements, right? If a compatible replacement is selected, it works as autoreplace; if a non-compatible replacement is selected, then a news message pops up when the train enters a depot. Is this right?

Combining both windows is something that could be done, and perhaps even beneficial, but I am no expert in UI design. If we can come up with something clearer and easier for the user, then I will code it. Feel free to throw in suggestions. :-)

On the other hand, I agree with adf88 that the underlying difference between autoreplace and autoconvert should be kept. Autoreplace is something that happens on its own, when a train enters a depot and without user intervention; autoconvert can only be user-triggered (through the convert tool), even if it requires prior preparation. Also, the current autoconvert system cannot be simply merged with autoreplace, because autoreplace only allows for one replacement (for a given vehicle type), while autoconvert allows for one replacement per rail type; that is, vehicle X can have replacement Y when converting to monorail and replacement Z when converting to maglev. This is something that cannot be achieved with the current autoreplace model.
adf88 wrote:I would consider one more sub-feature. There is a bunch of cases where OTTD is played without any train replacement set (it may be the most common case). Thus I think there should exist implicit autoconversion. If a user don't specify replacement let the game to choose what vehicles to convert to. It may be some prosaic rule.
Well, I'd rather go one step at a time, so first we put autoconversion in place and later we can discuss about implicit conversions. And, while I agree that this could be a good idea, I feel that defining such implicit rules can pose some problems. For instance, suppose you play without any additional train sets, you are converting to monorail and you already have both models available (X2001 and Millenium Z1). Which one should be picked as the replacement for a certain engine? The "best" one (Millenium Z1)? But changing a SH 40 into a Millenium Z1 can mess up full-load orders (since the Millenium Z1 carries passengers) or make a train no longer fit into a platform (since it is double-headed). So adding any implicit conversion would require some previous thinking (the standard wagons, for instance, would not really have any choice).
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

Basically I think your new window can replace the existing window entirely. The functionality of the existing window is then integrated in your new window.

Maybe my suggestion will be more clear with some examples:
- Replace steam train with electric train: If the steamers enters an electrified depot it is replaced immediately and released back out of it. If it enters a non-electrified depot, the train is not replaced, the user is informed of that by a message and the train is released back out unchanged.
- Replace train with maglev: same as the unchanged steam train; vehicle enters depot and is not changed because of incompatible depot.
- If you want to replace train with maglev: stop the train in the depot and then convert the depot to maglev. The train will now be converted to maglev by your patch.
- If you want to replace steam train to electric in non-electric depot: stop the train in the depot and then convert the depot.

So basically: if the depot is compatible, the train is converted immediately. If the depot is incompatible, you must stop the train in the depot and convert the depot to also convert the trains inside. If there is no rule set up to convert the train in the depot, the error message is shown.


This doesn't handle multiple replacement types yet, but that could work as follows:
If multiple replacement types are possible inside a depot, the first available replacement type (using the order of railtypes in the list) is used.

You need something like that anyways in your current patch. Imagine I set train type A to be replaced with monorail type B and maglev type C. I stop the train in a normal train depot. Now I convert the depot to the "universal railtype" that can handle trains, monorails and maglevs. What replacement must be used in this case?
Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: Patch: Autoconvert

Post by Eddi »

There is an old suggestion floating around to combine autoreplace and autorenew, which may be combined with this.

so the idea would be, to have the same window with (possibly) 3 buttons:
  • replace immediately (active when engine A and engine B have compatible railtype, but are not the same)
  • replace when old (also active when A and B are the same)
  • convert (active when A and B have no compatible railtype)
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: Patch: Autoconvert

Post by cirdan »

FooBar wrote:Basically I think your new window can replace the existing window entirely. The functionality of the existing window is then integrated in your new window.

Maybe my suggestion will be more clear with some examples:
- Replace steam train with electric train: If the steamers enters an electrified depot it is replaced immediately and released back out of it. If it enters a non-electrified depot, the train is not replaced, the user is informed of that by a message and the train is released back out unchanged.
I am not sure we want to pester the user with a message in that case; perhaps they set up the replacement just for a part of their network and don't want to be annoyed about the unelectified rest. Otherwise, fine.
FooBar wrote:- Replace train with maglev: same as the unchanged steam train; vehicle enters depot and is not changed because of incompatible depot.
(We certainly don't want a message in this case. If a user sets up a maglev replacement for a steamer, they should know pretty well it will not happen on its own.)
FooBar wrote:- If you want to replace train with maglev: stop the train in the depot and then convert the depot to maglev. The train will now be converted to maglev by your patch.
- If you want to replace steam train to electric in non-electric depot: stop the train in the depot and then convert the depot.

So basically: if the depot is compatible, the train is converted immediately. If the depot is incompatible, you must stop the train in the depot and convert the depot to also convert the trains inside. If there is no rule set up to convert the train in the depot, the error message is shown.

This doesn't handle multiple replacement types yet, but that could work as follows:
If multiple replacement types are possible inside a depot, the first available replacement type (using the order of railtypes in the list) is used.

You need something like that anyways in your current patch. Imagine I set train type A to be replaced with monorail type B and maglev type C. I stop the train in a normal train depot. Now I convert the depot to the "universal railtype" that can handle trains, monorails and maglevs. What replacement must be used in this case?
Fine, I now get your point. Overall, it could be an improvement. Regarding the fact that a vehicle type can have several simultaneous replacements depending on the target rail type, I think it should work like this:
  • When a train enters a depot, it should check for a replacement for the depot rail type. If it has one, it is converted; otherwise, nothing happens.
  • When a depot conversion is attempted, trains stopped inside are checked for replacements for the conversion target rail type. If all of them have one and can be converted, they are; otherwise, an error is thrown.
The first point would encompass autoreplace, the second one would be autoconvert (but the new name would be pointless now). In fact, this new scheme would actually improve autoreplace, because you could have, for instance, two different replacements for a steam engine, a diesel one for non-electrified depots and an electric one for electrified depots, and they would be upgraded accordingly.

And, for autoconvert, I don't think we should try to second-guess the user about compatible rail types. If they attempt to convert a depot to a universal rail type, we should only look for a replacement for that precise rail type, no matter if they have set replacements for other compatible rail types. (At least for now, since this is a small change in behaviour that could be tweaked later.)
Eddi wrote:There is an old suggestion floating around to combine autoreplace and autorenew, which may be combined with this.

so the idea would be, to have the same window with (possibly) 3 buttons:
  • replace immediately (active when engine A and engine B have compatible railtype, but are not the same)
  • replace when old (also active when A and B are the same)
  • convert (active when A and B have no compatible railtype)
As I see it, the best way to do this would be to have an additional target option "autorenew" when the user selects the same source and target rail types. In fact, it would be mostly like an ordinary autoreplace (with itself), only that with an age check.
Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: Patch: Autoconvert

Post by Eddi »

"autoreplace when old" should not only allow replacing with the same vehicle, also other vehicles like normal autoreplace.
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: Patch: Autoconvert

Post by cirdan »

All right, so it took me longer than I expected, but here it is. The autoconvert gui has been merged into the existing autoreplace gui, as per FooBar and adf88' suggestion.

- Now an engine can have a different replacement for each railtype; when it enters a depot, it is changed into whichever one is assigned for the depot's railtype. Thus, you can arrange to have your diesel locomotives replaced by a better diesel model or by an electric model depending on whether they are running on an already electrified part of your network or not.

- Additionally, replacements can be set up that would not be useful to the traditional autoreplace, such as an electric engine into a monorail one. These are used when you try to convert a depot to a different railtype. If all of the trains inside the depot can be converted using the autoreplace rules (or can themselves run on the new track), then they are replaced and the conversion succeeds. This should put an end to the silly contortions that are normally needed to upgrade a (part of a) network to a new track type.

Comments, suggestions and bug reports welcome.
Attachments
autoconvert-r22867.patch
(50.38 KiB) Downloaded 240 times
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

Doesn't build here unfortunately. I'm getting these errors:

Code: Select all

5>..\src\rail_cmd.cpp(1547): error C2059: syntax error : '('
5>..\src\rail_cmd.cpp(1555): error C2064: term does not evaluate to a function taking 4 arguments
5>..\src\rail_cmd.cpp(1561): error C2064: term does not evaluate to a function taking 4 arguments
5>..\src\rail_cmd.cpp(1564): error C2064: term does not evaluate to a function taking 4 arguments
This is the relevant piece of code:

Code: Select all

					if ((flags & DC_EXEC) != 0) {
						/* Make assert() work */
						void NORETURN CDECL (*const error) (const char *, ...) = &::error;
						CommandCost ret(EXPENSES_NEW_VEHICLES, 0);
						VehicleList engines, wagons;
						BuildDepotVehicleList(VEH_TRAIN, tile, &engines, &wagons, true);
						bool unused;
						SetRailType(tile, totype);
						for (uint i = 0; i < engines.Length(); i++) {
							const Train *t = Train::From(engines[i]);
							assert(t->IsStoppedInDepot());
							if (!t->IsFrontEngine()) continue;
							ret.AddCost(ReplaceChain((Vehicle**)&t, flags, dcd.c->settings.renew_keep_length, &unused, totype));
						}
						for (uint i = 0; i < wagons.Length(); i++) {
							const Train *t = Train::From(wagons[i]);
							assert(t->IsStoppedInDepot());
							ret.AddCost(ReplaceFreeUnit((Vehicle**)&t, flags, &unused, totype));
						}
						assert(ret.Succeeded() && ret.GetCost() == dcd.cost.GetCost());
						SetRailType(tile, type);
					}
With line 1547 being: void NORETURN CDECL (*const error) (const char *, ...) = &::error;

Trying to build using MSVC10; could build a clean (and patched) OpenTTD a couple of days ago.
Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: Patch: Autoconvert

Post by Eddi »

conceptual question: assume i am playing with NuTracks and assign one replacement for a low speed engine to a low speed engine, and the same low speed engine to a high speed engine (both are powered on/compatible with the same railtypes), which one does it get replaced to?
User avatar
cirdan
Director
Director
Posts: 539
Joined: 07 Apr 2007 18:08

Re: Patch: Autoconvert

Post by cirdan »

FooBar wrote:Doesn't build here unfortunately. I'm getting these errors:
That happens when you try to be too clever... The problem is that I tried to use assert() in that piece of code, but, in OpenTTD, at least on my machine, assert() is not the macro described in the standard, but a custom macro (yay for portable code) whose expansion involves a global function called error(). However, in that piece of code, the identifier 'error' has been reused as a variable and masks the global function, therefore rendering assert() unusable. Since I really wanted to put an assertion in that place, I tried to overcome this mess by re-redeclaring error as the global function. As it turns out, this wasn't such a good idea (but it worked for me, and that was all that I wanted back when I coded this patch).

To sum up: Try the attached new patch instead.
Eddi wrote:conceptual question: assume i am playing with NuTracks and assign one replacement for a low speed engine to a low speed engine, and the same low speed engine to a high speed engine (both are powered on/compatible with the same railtypes), which one does it get replaced to?
Replacements with this patch are bound to an (engine, railtype) pair, that is, a replacement is set for a certain engine when converting to a certain railtype. If you assign two different replacements for a given engine, obviously for different railtypes, the one that gets substituted is the one matching the target railtype, which is either the railtype at the depot (if replacement happens due to autoreplace) or the selected conversion tool railtype (if replacement is triggered by a manual conversion of a depot).
Attachments
autoconvert-r22867.patch
(51.95 KiB) Downloaded 245 times
Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: Patch: Autoconvert

Post by Eddi »

so if i have a low-speed replacement, and the engine happens to enter a medium-speed depot, nothing happens at all? that doesn't sound right...

something else: assume i play with breakdowns disabled and servicing disabled, the current behaviour is to still go to depot when an autoreplace rule is set, does your patch distinguish whether the replacement can happen in the chosen target depot, and skip the depot visit, if not?
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

cirdan wrote:To sum up: Try the attached new patch instead.
That works fine, thanks for update!

Attached is a win32 build for everyone on Windows who wants to give this a go and cannot build themselves.

I'm unsure when I'll be able to test this myself. Currently playing a custom build with yacd. I'll try to patch that with this patch as well. If that works I can test it in my current game, otherwise it'll have to wait.
From a quick look I must say that it looks good. If it does what it says on the tin you have my vote for trunk inclusion! :D
Attachments
openttd-custom-autoconvert-r22867-windows-win32.zip
(5.52 MiB) Downloaded 273 times
User avatar
adf88
Chief Executive
Chief Executive
Posts: 644
Joined: 14 Jan 2008 15:51
Location: PL

Re: Patch: Autoconvert

Post by adf88 »

cirdan wrote:All right, so it took me longer than I expected, but here it is. The autoconvert gui has been merged into the existing autoreplace gui, as per FooBar and adf88' suggestion...
We was talking about the GUI but you merged not only the GUI but the autoreplace mechanics too. That causes troubles. Current autoreplace tool is altered very badly and there are some bugs/inexactnesses.

My suggestion is...

Group rail types by compatibility. In case of no NewGRFs it will be:
  • rail (grou of two rail types - non electrified rails and electrified rails)
  • monorail (single rail type)
  • maglev (single rail type)
Let to chose separately autoreplace rule and autoconvert rules. The autoreplace is about compatible vehicles and autoconvert about incompatible. E.g. if we have some monorail engine selected the menu should be organized like this:
1) autoreplace (within monorail group)
2) autoconvert
  • a) to rail group
  • b) to maglev group
While setting autoreplace rule keep current behaviour - let the autoreplace to happen every time a vehicle enters a depot. In opposition to autoreplace, autoconvert rules should apply when a depot with trains inside is converted between invalid rail types.

There are no strings naming rail groups so you can use comma-separated list of all rail types inside e.g. in case of no NewGRFs:
  • "rail, electrified rail" - rail group name
  • "monorail" - monorail group name
  • "maglev" - maglev group name
Last edited by adf88 on 03 Sep 2011 08:27, edited 2 times in total.
:] don't worry, be happy and checkout my patches
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

problem with grouping by compatibility is exactly newgrf. Some newgrfs define everything compatible with everything. Then you only have one big group left. And some other newgrf may define A, B and C compatible in one depot and B, C and D compatible in the other (notice the overlap); now I want to replace B with C. What group should I set that in?
And imagine the group name in case of the universal railtype...
User avatar
adf88
Chief Executive
Chief Executive
Posts: 644
Joined: 14 Jan 2008 15:51
Location: PL

Re: Patch: Autoconvert

Post by adf88 »

FooBar wrote:Some newgrfs define everything compatible with everything. Then you only have one big group left.
That's not a problem. In that case we don't need autoconvert at all.
FooBar wrote:And some other newgrf may define A, B and C compatible in one depot and B, C and D compatible in the other (notice the overlap)
But how? You are saying that depots have their own types (different then rail types) and compatibility rules? Isn't that a compatibility relation is symmetric and transitive?
FooBar wrote:And how to name these groups in the first place?
See my previous post (I made an edit).
:] don't worry, be happy and checkout my patches
User avatar
FooBar
Tycoon
Tycoon
Posts: 6553
Joined: 21 May 2007 11:47
Location: The Netherlands
Contact:

Re: Patch: Autoconvert

Post by FooBar »

People may not be using the universal depot in all cases. And the attempting to use autoconvert for one of their 'normal' depots, finding the faced with an awefully large list.

Where I wrote depots you can read railtypes just as well. The game allows for 16 different railtypes and as such 16 different depots. Railtype A could be defined compatible with B and C. Railtype B could be defined compatible with C and D. That's what I meant to say there.

Also see my previous post. I made an edit as wel after I saw your edit :p
User avatar
adf88
Chief Executive
Chief Executive
Posts: 644
Joined: 14 Jan 2008 15:51
Location: PL

Re: Patch: Autoconvert

Post by adf88 »

adf88 wrote:
FooBar wrote:Railtype A could be defined compatible with B and C. Railtype B could be defined compatible with C and D.
Could you give an example?
Last edited by adf88 on 03 Sep 2011 09:53, edited 2 times in total.
:] don't worry, be happy and checkout my patches
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 11 guests