Freight list sorter - fix "by connection" mode issues

[Nazalassa]

Currently, the "by connection" sort mode groups freight as such:
- If two freight packets want to go to the same stop, they get merged;
- If there is a line or a lineless convoy that passes at two freight packets' next stops, they get merged.

However, in the second case, it does not check:
- Whether the line or lineless convoy actually passes at the stop where the packets are;
- Whether the line or lineless convoy can actually carry the packets (freight types).

This patch fixes this. (See example situations below for examples of when such things can happen.)

-- EDIT --
It also fixes more things:

• Packets are now only merged if they can take the exact same lines to their next stop. So there are different entries for "line 1", "line 2" and "line 1, line 2". The merger should now work correctly in all situations.
• There was a problem in line name retrieval (for displaying the list) which caused all lines to be displayed, even those which cannot transport the good packet. For example, a passenger packet was listed as "<passenger line>, <post line>".
• Added missing "case" statements in freight_list_sorter_t::compare_ware() that caused warnings in the log.
• Don't merge packets with different lines, but which share a common lineless convoy, if at least one of them can take a line.

Some example situations. Assume we are at stop A, good G1 wants to go to stop B, and good G2 wants to go to stop C.

• If there exists a line passing at B and C, but not at A, the current version still merges G1 and G2, even if they can not take the line.
• If G1 and G2 are e.g passengers, and there is a line with only postal vehicles passing at B and C, then G1 and G2 will be merged, even though they cannot take the line in question.

[Nazalassa]

Found (yet another) bug in the patch. How it escaped the compiler's wrath is still unknown.

[prissi]

I think one needs to compare the wares' catg, not the good index. Also I found the logic a little convoluted, as it has to be either two line ore not. How about

Code Select Expand

                        // not same via halt, but maybe same lines
                        for (linehandle_t const& line : h->registered_lines) {
                            if (line->get_goods_catg_index().is_contained(ware.get_catg())) {
                                // only merge if both can travel with the same line to their stop
                                bool has_line1 = w_next->registered_lines.is_contained(line);
                                bool has_line2 = wi_next->registered_lines.is_contained(line);
                                merge = has_line1 && has_line2;
                                has_line = has_line1 || has_line2;
                            }
                        }


[Nazalassa]

I think one needs to compare the wares' catg, not the good index. Also I found the logic a little convoluted, as it has to be either two line ore not. How about

Code Select Expand

                        // not same via halt, but maybe same lines
                        for (linehandle_t const& line : h->registered_lines) {
                            if (line->get_goods_catg_index().is_contained(ware.get_catg())) {
                                // only merge if both can travel with the same line to their stop
                                bool has_line1 = w_next->registered_lines.is_contained(line);
                                bool has_line2 = wi_next->registered_lines.is_contained(line);
                                merge = has_line1 && has_line2;
                                has_line = has_line1 || has_line2;
                            }
                        }


I see an issue with your code: it is equivalent to checking only the last line of h->registered_lines that can transport the wares. If it enters the "if", then merge and has_line will be overwritten and we will lose all information about the previous lines.

To be clear, the logic I have in mind is:
 - If no lines in common, try lineless convoys
 - If at least one line in common:
      - If one can take a line which the other cannot take: don't merge
      - Otherwise, i.e they can take the exact same lines: merge
Meaning wares will be merged if, and only if, they can take the exact same lines and (they can at least take one line, or they have a lineless convoy in common).

I would suggest:

Code Select Expand

                        // not same via halt, but maybe same lines
                        for (linehandle_t const& line : h->registered_lines) {
                            if (line->get_goods_catg_index().is_contained(ware.get_catg())) {
                                // only merge if both can travel with the same line to their stop
                                bool has_line1 = w_next->registered_lines.is_contained(line);
                                bool has_line2 = wi_next->registered_lines.is_contained(line);
                                has_line = has_line || has_line1 || has_line2;
                                if (has_line1 || has_line2)
                                    merge = has_line1 && has_line2;
                                if (has_line1 ^ has_line2)
                                    break;
                            }
                        }
- Do not overwrite has_line (that was my mistake).
- Do not overwrite merge if none can take the line; otherwise it may overwrite a previous "true".
- Break if one can take the line but not the other, because in this case we know they cannot be merged, so no reason to keep looping.

[prissi]

I still think the XOR should be an or, because if merge is true, we should also break ...

[Nazalassa]

Once there is a common line, maybe there is a line later, which one of the wares can take, but not the other. If we break too early, we risk missing it.

For example, if one of the wares can take line A and line B, and the other can only take line A. Assuming line A comes before line B in the loop. If we break once they have a common line, then only line A will be checked, then break. Line B wil not be checked, and the wares will be merged, even though they cannot take the same lines (because one of them cannot take line B). This means both wares will be listed in the "xxx > Line A, Line B" entry, even though one of them cannot take line B.

[prissi]

But the merge may then be resetted. So if both lines match, the search will continue, but of course only find single matches at best after that. But the search will likely break before that, as soon as a single line is found.

I think I have to dive into this a lot deeper again.

[prissi]

I would suggest this:

Code Select Expand

// not same via halt, but maybe same lines
for (linehandle_t const& line : h->registered_lines) {
if (line->get_goods_catg_index().is_contained(ware.get_catg())) {
// only merge if both can travel with the same line to their stop
bool has_line1 = w_next->registered_lines.is_contained(line);
bool has_line2 = wi_next->registered_lines.is_contained(line);
has_line |= has_line1 || has_line2;
merge = has_line1 && has_line2;
if (merge) {
break;
}
}
}
}
Break when merge; if a line is found, remember this, be continue to to check wheter a full match is there.

[Nazalassa]

I think that would always merge two wares with a common line, even if one of the wares can take a line the other cannot take. I'd say it doesn't make much sense to merge wares which cannot take exactly the same set of lines, because they are then reported as taking the same set of lines...

[prissi]

Yes, but if both wares can be served by the same line, they will travel with the same line when such a convoy will arrive. Even if there is an extra line. So those wares will overcrowd the joint line as well as individual lines.

Your code stopped at the first line found, and did not search on to find if there is not a mutual line. But if there was only one line serving both stops, it never stopped and searched to the end. The purpose is to merge as much lines as possible to have a compact display.

Anyway, even with merging, the next line could be random after each rerouting. It not unique when you have two circular routes clock and counterclockwise.

I see no easy way to fix this apart from listing more than one connection for a destination (ware), and even then have to count wares twice if there is a choice of convois or lines.

[Nazalassa]

ok, I see. But I still think a more "detailed" display should be available, too, like the "via (detail)" and "via (amount)" modes.

[prissi]

Listing the connection for each ware and via stop would be straightforward; but even then it may not be unqiue if more than one line serves the stop in question.

[Nazalassa]

Rather than listing the connection for each ware and via stop, we can list the connections for wares, grouped according to which lines they can take exactly (e.g wares taking line A only, wares taking line A or B, wares taking line B only, ...). If more than one line serve the stops, we can just display them all.

[prissi]

Not sure, if it helps with identifying which lines need more capacity with double lines.

[Nazalassa]

If it gives exactly the lines the wares can take, the player can see whether it is more one line or the other that needs strengthening, and whether it is the shared part that needs strengthening or not.

I think if wares are merged and not all lines are reported (ex. if wares taking A and B are merged with wares taking only A), that may be misleading. For example, one may get "1234 > Line A" and "768 > Line B", even though most of the 1234 reported as taking line A can also take line B, which is not shown because the original ware could not not take it.

Personally, I would prefer "1184 > Line A, Line B", "768 > Line B", "50 > Line A" - that tells me I better put more vehicles on line B, than putting more on both lines, because outside of the shared part Line A is doing well. Not sure that is optimal though.

[prissi]

As this is mostly relevant for passengers and mail, there are further complications, like express versus local lines and ringlines in two directions. One might break this by sorting at the number of intermediate stops to the next transfer. A lot of effort for that, but probably what the player expects.

Actually, keeping the number of intermediate stops and total transfers also in a ware package, it would avoid wares taking local lines for express lines or counterclock lines for clockwise connections. Moreover, this  enable fore more competition in networkgames.

It would be a fundmental addition to the simutrans routing system, but not so difficult to implement.

For the moment, I have submitted the system preferring the merge, but keep wares on individual lines separate. However, even this is not perfect, as if there are two possibe merging lines, their order may be change after a rerouting event.