PR#595 Added mode of convoy replacement

[sou7]

Added a mode to replace all convoys belonging to the same line.

https://github.com/jamespetts/simutrans-extended/pull/595

Previously, there were three modes of exchanging convoys: (1) only that convoy, (2) the same vehicles of the same company, and (3) the same vehicle train of the same line.

In this pull request, we have added a mode to exchange all convoys belonging to the same line. In addition, the data management method has been changed from two bool values to one enum value.

[Ranran(retired)]

Your contribution should be appreciated.

But in my opinion there are some issues with this change.

Previously, there were three modes of exchanging convoys: (1) only that convoy, (2) the same vehicles of the same company, and (3) the same vehicle train of the same line.

The existing replacement modes (1) through (3) all respect the rule of replacing (vehicle order)A with (vehicle order)B, but the new mode you add ignores this rule.
That is, the vehicle order displayed above is before replacement, and the convoy displayed below is the new vehicle order after replacement.
In other words, in replacement modes (1) to (3) so far, the UI visualizes configuration changes, performance changes, and replacement costs, and conveys the information correctly.
The current UI doesn't support your additional rules, making it difficult for players to understand what you're trying to do.

This may result in unintended replacing, such as replacing convoys with new vehicles, which may incur extra costs. Commercial games will warn you about it, or players will complain about it.

[sou7]

I too think those are important issues. I was not aware of them.

When we select a new mode to add, I'll consider whether it is possible to (a) gray out part of the UI of the replacement original, and (b) perform the correct calculations.

[jamespetts]

Sou7 - thank you very much for your contribution and welcome to the forums. I agree with Ranran - this is a worthwhile improvement that is likely to make things easier for players, but it is important to ensure that the UI makes clear to players what the operations do.

[sou7]

I have made two changes.

(1) I grayed out the "vehicles:" and "station tiles:" display for current convoys when a new mode is selected. This indicated that the current convoys display was invalid and implied that vehicles other than current convoys would also be replaced.

(2) The cost of replacements is now calculated correctly. I made sure to display the total cost of updating each convoys.

When performing this operation, players usually do not care much about the origin convoys they are replacing. Therefore, I did not display other convoys and the performance of that.


The two windows in the screenshot show different convoys belonging to the same line, with the previous mode and the new mode selected.

[jamespetts]

Thank you for that - that is helpful. Can I check whether you have tested compatibility with the ex-15 branch?

[Ranran(retired)]

I have made two changes.

I'm afraid, but I don't think the fix is just a deception and does little to improve the issue.

(1) I grayed out the "vehicles:" and "station tiles:" display for current convoys when a new mode is selected. This indicated that the current convoys display was invalid and implied that vehicles other than current convoys would also be replaced.

The effect of graying out the label is very weak and ambiguous because the image of convoy remains as it is. It's not clear what the greyed out means.

When performing this operation, players usually do not care much about the origin convoys they are replacing.

Not limited to performance.

This may result in unintended replacing, such as replacing convoys with new vehicles, which may incur extra costs.

I don't see any improvement on this.
What makes this substitution procedure strange is that the management for the line are performed from the convoy dialog.


But the feature itself is very welcome.
Anyway I'm working on a patch with some improvements so please don't merge this until then.

[Ranran(retired)]

I'm still working on it, but I think this is the best way to go with line replacement, which is closely related to line management.
It still takes some time for adding rdwr and resolving conflicts in the two branches.

Therefore, change it so that you can open the line replacing dialog from the line management dialog.


The total capacity of the line and the convoy configuration of the line can be supplemented by using it together with the line management dialog.
The line replacing dialog is limited to replacing all lines and has no toggle options. Also there is no current convoy image.
I think that the operation will be simplified and the replacement will be performed in a more intuitive flow.
I hope this will resolve the issues I pointed out and reduce confusion for players.

[jamespetts]

I'm still working on it, but I think this is the best way to go with line replacement, which is closely related to line management.
It still takes some time for adding rdwr and resolving conflicts in the two branches.

Therefore, change it so that you can open the line replacing dialog from the line management dialog.


The total capacity of the line and the convoy configuration of the line can be supplemented by using it together with the line management dialog.
The line replacing dialog is limited to replacing all lines and has no toggle options. Also there is no current convoy image.
I think that the operation will be simplified and the replacement will be performed in a more intuitive flow.
I hope this will resolve the issues I pointed out and reduce confusion for players.

That is an interesting idea. Bear in mind that the whole way in which replacement works will inevitably change when we move into the 15.x code, but that may yet take some time to complete.

I wonder in this case how consistent that this is as an approach. Currently, we select "replace" from an individual convoy window, and can replace that convoy, or all of that type in a line. Replacing all convoys of all types from the line window suggests that we should be more specific about the description of "replace" in the convoy window - but it is difficult to see how this could be described shortly. Can we perhaps add a tooltip to "replace" so that we can have a longer description, and change "replace" in the convoy window to "replace type"?

[Ranran(retired)]

That is an interesting idea. Bear in mind that the whole way in which replacement works will inevitably change when we move into the 15.x code, but that may yet take some time to complete.

Yes, I'm testing this feature on the ex-15 branch and I'm having issues with the replace feature unrelated to this feature.
The convoy does not correctly belong to the original line after replacement.

I have absolutely no idea how the replacement and overhaul will interact and how the player will manage it.
However, this feature is useful in that it makes it easy to unify vehicles of lines into the same consist.

and change "replace" in the convoy window to "replace type"?

This is definitely possible.


The linemanagement dialog has been outdated and needs to be updated.
A line may have a separate window like standard.
One of my ideas is a line vehicle list. This will help with vehicle management and replacement planning.

[Ranran(retired)]

I found another issue with this function.
Since the replacement function was originally intended to replace convoys of the same consist, it was possible to flexibly set the number of convoys to be replaced, and there was almost no difference due to the difference in the replaced convoys.

However, by not specifying the consist of the convoys to be replaced, it is possible that only one of the newest convoys will be replaced when ordering to replace 1 convoy out of 100.

We can add processing that replaces the oldest convoy first, but there is no guarantee that it will be the best solution. It also requires extra effort in coding.
A simple solution is to make line replacing always an instruction to replace all convoys in the line.

[Ranran(retired)]

In the following branches I believe I've put in as some fixes and workarounds as possible for this feature.
https://github.com/Ranran-the-JuicyPork/simutrans-exp/tree/2301-replace-line

- Line's all convoys replacing can only be done with a dedicated dialog.
- It can only be opened from the schedule dialog.
- It replaces all convoys of the line.

I found another issue with this function.
Since the replacement function was originally intended to replace convoys of the same consist, it was possible to flexibly set the number of convoys to be replaced, and there was almost no difference due to the difference in the replaced convoys.

-- Only the cycle can be changed with the replace cycle option.
As already pointed out, this can cause confusion if different convoys are included, as the target of skip or sell becomes an indeterminate factor.


Code proofreading:
I found the words used in replace_mode_t difficult to understand. So I changed this, but I'm not an English speaker either so it would be helpful if you could check this out.


Translations to be added:

Code Select Expand

error_text_invalid_replace_cycle
multiline_helptext_replace_all_line_convoys
replace_line_convoys
helptxt_replace_all_convoys_of_this_line


Replacing all convoys of all types from the line window suggests that we should be more specific about the description of "replace" in the convoy window - but it is difficult to see how this could be described shortly. Can we perhaps add a tooltip to "replace" so that we can have a longer description

multiline_helptext_replace_all_line_convoys is for that, it appears at the top of the dialog only in the line replacement dialog.
This handles line breaks correctly with "\n" unlike regular labels.


The save revision is incremented by this patch for saving the line replace dialog.


Additional changes unrelated to this feature:
Changed to display a warning without closing the dialog when a meaningless replacing order is made with the input of the number of replacing set to 0.


Compatibility with ex-15 needs to resolve save revision conflicts.

[jamespetts]

Thank you both very much for your work on this. I have now had a chance to test this. Some feedback at this stage.

• The new button for replace_line_convoys overlaps the edge of the line management window when that window is first opened.
• The window for replacement vehicles that opens when clicking this button has a broken livery scheme selector.
• Attempting to merge this with the Ex-15 branch causes merge conflicts.

If these issues can be fixed, and subject to further functionality testing, this should be able to be incorporated.

Thank you both again for your work on this.

[Ranran(retired)]

• The new button for replace_line_convoys overlaps the edge of the line management window when that window is first opened.

I believe this has been resolved by moving it to a more optimal position - next to the retire button.

• The window for replacement vehicles that opens when clicking this button has a broken livery scheme selector.

The replace dialog's livery scheme selector has never worked. The change here makes no sense. I think I reported it earlier in another thread.
So it's not a problem with this patch.
But we'd like to fix this if possible, but I'm not sure if it's easy. Anyway it was never fixed.

• Attempting to merge this with the Ex-15 branch causes merge conflicts.

I believe the conflict for the ex-15 branch is resolved here.
https://github.com/Ranran-the-JuicyPork/simutrans-exp/tree/line-replace-ex15

[jamespetts]

Thank you for that - the button position is much better.

As to the livery scheme selector - the current master branch livery scheme selector has no effect, but the actual GUI component is not malformed. In this branch, the GUI component is malformed, which makes the dialogue box look untidy, which it does not currently. Can we perhaps just remove the livery scheme selector entirely in the replace dialogue? 

Thank you for your work on this.

[Ranran(retired)]

Can we perhaps just remove the livery scheme selector entirely in the replace dialogue? 

I changed it like that.

[jamespetts]

I changed it like that.

Thank you very much for this - the UI is substantially improved.

Testing, however, shows that there is a somewhat more fundamental (non-UI) problem with this feature which I had not considered before. Consider this saved game, for example. This has a single line served by vehicles of two traction types which require two different types of depot: (1) trolleybuses, requiring an electric depot; and (2) motor 'buses, requiring an internal combustion engine depot (the "petrol" traction type).

Performing a replace operation on this line will attempt to replace all vehicles no matter their traction type, but, in fact, attempting to replace one traction type with another in this way will fail. So, trying to replace all of the buses on the route with motor 'buses will result in the trolleybuses not being replaced and giving an error, and the motor 'buses replacing correctly. The converse will apply when attempting to replace motor 'buses with trolleybuses.

This is a complex problem to solve, as it requires replacement to do something very different to what it can currently do, which is to dispose of a vehicle in one depot and spawn its replacement in another depot. I do not think it wise to attempt this sort of change of behaviour before 15.x (where replacement mechanics will need to be overhauled in any event to allow replacement at the time of being maintained rather than immediately).

I wonder whether, in the interim, to make this feature usable, we might warn users in the case of lines with multiple traction types that replacement will not work unless all the replacements can happen in a single depot?

[Ranran(retired)]

Performing a replace operation on this line will attempt to replace all vehicles no matter their traction type, but, in fact, attempting to replace one traction type with another in this way will fail. So, trying to replace all of the buses on the route with motor 'buses will result in the trolleybuses not being replaced and giving an error, and the motor 'buses replacing correctly. The converse will apply when attempting to replace motor 'buses with trolleybuses.

Certainly this is an issue with this patch that cannot be overlooked and was overlooked.

I wonder whether, in the interim, to make this feature usable, we might warn users in the case of lines with multiple traction types that replacement will not work unless all the replacements can happen in a single depot?

It's complicated and I don't think we can do it without much effort at the moment.
It's possible to edit the message at the top of the window, but I don't think it will be clear or precise for beginners. It's just that the UI design is flawed...

Currently there is no code to detect this sort of error in advance as convoy does the check when it goes to the depot for replacing.
As already explained, this works by replacing many things with B instead of replacing A to B.
We need to check for convoys belonging to all lines that all vehicles in the convoy before and after being replaced are supported by the same depot and that the depot is reachable by the convoy.
However, all depots visited by line convoys need not be the same.

And I am afraid how much effort we'll pay to get this right now will be wasted on ex-15.
Specifically, ex-15 has a depot that the schedule visits, so it is clear what type of traction the line can support.
But I have no idea how it would work if the steam locomotive were replaced by a diesel or electric.

One of the issue is that the replace window allows player to construct multiple traction types consist that cannot be supported by any depot.
In this regard, I'm considering adding code to check in advance. This doesn't seem easy.

[Ranran(retired)]

An issue with the same sort of replace dialog that was originally present is that vehicles are displayed even though the player does not have a depot of the corresponding traction type.
For example, in the test saved game that james attached, the replace dialog shows a steam type tractor even though the player does not have a steam depot.

I made a fix for this. Vehicles of traction type for which the player does not own the corresponding depot will not be displayed. This will alleviate the unfriendly nature of this UI a bit.
Note, however, that this is still insufficient. It does not check if the corresponding traction type depot is visitable.

And some other bugs have been fixed.


I also suspect that convoy's algorithm for going to the depot is flawed.
The convoy's traction type only checks the lead vehicle. A convoy can have multiple traction types. Also, the leading vehicle may not be a powered vehicle.

Also, regarding visits to the depot for replacement, I think that whether the depot is reachable and whether the old vehicle can be scrapped at the depot is another matter.
I'm wondering if a vehicle needs to be sent the corresponding traction type depot in order to be scrapped or sold.

[Ranran(retired)]

I have added a makeshift warning display.
This only indicates that the line has multiple traction types, and does not actually conclude that replacement cannot be performed. Also the label has a tooltip.

I wonder whether, in the interim, to make this feature usable, we might warn users in the case of lines with multiple traction types that replacement will not work unless all the replacements can happen in a single depot?

Was it meant to be something like this?


(1) As already mentioned, I wonder if it's possible to scrap a powered type vehicle that the depot can't support.

(2) Enforcement obstruction due to inability to visit the garage is another matter. For example, if you want to perform a replacement on an electric type vehicle, the player has an electric type depot, but the replacing convoy is unable to reach that depot. The replacement will fail in that case.
However, the convoy before replacement needs to visit the corresponding depot to be replaced, and may not need to visit there for the purpose of being disposed of as shown in (1). On the other hand, if you want to store the replaced vehicle, it's not.


(3) There is no confirmation that the replacement vehicles will have multiple power types in advance. It may take some complication to know if this is exactly viable. We should search for depots where replacement is viable for that combination of traction types and see if line convoys can reach them.
For example, if you try to replace train with convoy containing steam and electric, there is no code to check if it is impossible.
So that kind of error is always known only by a message saying the convoy can't go to the depot after ordering replace.


Anyway, I think it's very difficult for players to understand what is done, how it is done, and what the result will be. Unintentional errors can lead to unexpected results.
Any advice on what to do about some of the issues listed would be helpful.


EDIT:
In summary, there is

- Problems with multiple traction types in line old convoys.
- Problems due to traction type relationship between line old convoys and new convoys and depot supporting traction types.
- Problem with new convoy having multiple traction types.

[jamespetts]

Excellent, thank you very much for this. I have updated my line-replacement branch and dealt with the translation texts. However, this does not seem to merge cleanly with the Ex-15 branch at present - I should be grateful if you could look into this so that I can finalise this.

I think that this interim fix is the best solution for the current master branch. I would strongly suggest that remedying the more fundamental issues be left to the Ex-15 branch. That is because the plan is to change how vehicle replacement works on that branch: instead of vehicles being replaced immediately that the replace button is pressed, the idea is that they will be replaced (at the user's choice) either at their next routine depot visit (routine depot visits will be a thing in the Ex-15 branch) or at the next overhaul (overhauls are also specific to the Ex-15 branch). This will require quite substantial changes to the way in which replacement works, and will make it much easier to deal with the traction type issues discussed here.

Thank you again for your work on this.

[Ranran(retired)]

However, this does not seem to merge cleanly with the Ex-15 branch at present - I should be grateful if you could look into this so that I can finalise this.

https://github.com/Ranran-the-JuicyPork/simutrans-exp/tree/ex-15-line-replacement2
Please check this branch.

[jamespetts]

https://github.com/Ranran-the-JuicyPork/simutrans-exp/tree/ex-15-line-replacement2
Please check this branch.

Excellent - thank you. Now incorporated.

I should note that replacing generally is not working in the ex-15 branch, but this was a pre-existing issue and, as noted previously, replacing will need reforming in ex-15 in any event.

Thank you both very much for your work on this.