Fixing long-standing issues in calc_route for moving within station platform

[neroden]

Branch name is move-to-end-of-station-cleanup.  Compiles.  Might be worth doing some ad-hoc testing before merging (I did some but you might want to test a few more cases if you can think of any to test.)

While checking whether r7502 had been integrated into Extended, I discovered it *hadn't been*.

Some serious errors had developed in the code in Extended due to a subtle point -- "get_neighbour" actually changes the value of gr, meaning that

Code Select Expand

condition(gr) && gr->get_neighbour(gr, wegtyp, ribi)is NOT the same as

Code Select Expand

gr->get_neighbour(gr, wegtyp, ribi) && condition(gr)
Since get_neighbour was embedded in a very long while condition, this was confusing -- at some point someone moved something onto the wrong side and a bunch of hacks were made to try to work around it.

This restores the correct logic.  It also pulls a whole lot of code out of the while condition and adds a lot of comments to make it clearer what's going on.  AND it changes the name of an argument to reflect what it actually does.

The code in Standard is even more obscurantist in style so I did not try to copy the actual code.  But this restores the correct *logic* to the code.

Note that there is still a substantial difference in behavior between Standard and Extended.  Extended will ALWAYS pull to the middle of the platform, or to the end of the platform if reversing.  Standard will always pull forward *just* enough to get the train on the platform, but otherwise assumes the player "knows what they're doing" by picking a stopping location.

I'm not entirely satisfied with either of these behaviors.  I generally think we should assume the player "knows what they're doing".  But the middle of platform / end of platform thing is kind of pretty.  And most importantly, I want to be able to do complicated things where I stop two short trains on one long platform, and I want them to work even with "reversed" schedules and even with choose signals, and neither of these solutions really does the trick.

I'm currently thinking that the correct thing to do, for trains anyway, is to treat a mid-platform signal as a platform separator which breaks the platform conceptually into two platforms -- unless the train is so long it won't fit on one of them, in which case treat it as one whole platform.  That will take further coding however.

This was based off a slightly older commit than the bleeding edge, so if you want a proper "diff", do

Code Select Expand

git diff move-to-end-of-station-cleanup-branchpoint move-to-end-of-station-cleanup

[jamespetts]

Thank you very much for this. I can see that much work went into this. Can I check at this stage what noted incorrect behaviour that this fixes? Testing on demo.sve, convoy no. 107 seems to behave incorrectly some of the time (advancing to the end of the platform at a non-terminal stop and not doing so at a terminal stop) and also correctly some of the time both in the current version and in the version modified here.

I can see why I might have been confused by a "get_..." method that changed the value of the thing being passed to it! Simutrans has some very 1990s style code that can be very difficult to make sense of at times - and without a major rewrite of what is there, it is often difficult to build on it without coding in the same style, which can then be much more prone to bugs. For completely new work, I have tried to make the code rather easier to read, but a lot needs to integrate with the older code from Standard, so this is not always easy.

In any event, thank you very much for your work on this.

[prissi]

In standard the train always goes to the end of a platform unless someone has broken this functionality recently.

[neroden]

In standard the train always goes to the end of a platform unless someone has broken this functionality recently.

If Standard does this somewhere, it's certainly not doing so in this code, and it never has.

Code Select Expand

/**
* searches route, uses intern_calc_route() for distance between stations
* handles only driving in stations by itself
*/
route_t::route_result_t route_t::calc_route(karte_t *welt, const koord3d ziel, const koord3d start, test_driver_t *tdriver, const sint32 max_khm, sint32 max_len )
{
route.clear();

INT_CHECK("route 336");

#ifdef DEBUG_ROUTES
const uint32 ms = dr_time();
#endif
bool ok = intern_calc_route(welt, start, ziel, tdriver, max_khm, 0xFFFFFFFFul );
#ifdef DEBUG_ROUTES
if(tdriver->get_waytype()==water_wt) {
DBG_DEBUG("route_t::calc_route()", "route from %d,%d to %d,%d with %i steps in %u ms found.", start.x, start.y, ziel.x, ziel.y, route.get_count()-1, dr_time()-ms );
}
#endif

INT_CHECK("route 343");

if( !ok ) {
DBG_MESSAGE("route_t::calc_route()","No route from %d,%d to %d,%d found",start.x, start.y, ziel.x, ziel.y);
// no route found
route.reserve(1);
route.append(start); // just to be safe
return no_route;
}
// advance so all convoi fits into a halt (only set for trains and cars)
else if(  max_len>1  ) {

// we need a halt of course ...
halthandle_t halt = welt->lookup(start)->get_halt();
if(  halt.is_bound()  ) {

// first: find out how many tiles I am already in the station
for(  size_t i = route.get_count();  i-- != 0  &&  max_len != 0  &&  halt == haltestelle_t::get_halt(route[i], NULL);  --max_len) {
}

// and now go forward, if possible
if(  max_len>0  ) {

const uint32 max_n = route.get_count()-1;
const koord3d zv = route[max_n] - route[max_n - 1];
const int ribi = ribi_type(zv);

grund_t *gr = welt->lookup(start);
const waytype_t wegtyp = tdriver->get_waytype();
ribi_t::ribi way_ribi  = tdriver->get_ribi(gr);

while(  max_len>0  &&  ((way_ribi & ribi) != 0)  &&  gr->get_neighbour(gr,wegtyp,ribi)  &&  gr->get_halt()==halt
&&  tdriver->check_next_tile(gr)) {
// Do not go on a tile, where a oneway sign forbids going.
// This saves time and fixed the bug, that a oneway sign on the final tile was ignored.
ribi_t::ribi go_dir=gr->get_weg(wegtyp)->get_ribi_maske();
if(  (ribi&go_dir)!=0  ) {
break;
}
route.append(gr->get_pos());
max_len--;
way_ribi = tdriver->get_ribi(gr);
}
// station too short => warning!
if(  max_len>0  ) {
return valid_route_halt_too_short;
}
}
}
}
return valid_route;
}

As I said, this code is obscurantist in style and prone to confusion as a result.  But "max_len" as passed into calc_route is a convoy length (which is why I'm renaming this argument).  Standard will advance the convoy exactly enough spaces to fit the convoy on the platform (while max_len > 0) at which point it will stop.

[neroden]

Thank you very much for this. I can see that much work went into this. Can I check at this stage what noted incorrect behaviour that this fixes?

The actual change is quite subtle.

The code adds tiles to the end of a route if they're "on the same platform".  The correct code checks (a) that it's possible to advance from the current "end of route" tile to the new tile (to be added), including checking the masked ribi -- but because of an issue with signals, it doesn't check the masked ribi on track waytypes on tiles with signals.

The incorrect code in extended checks that it's possible to advance from the current "end of route" tile to the new tile (to be added), checking only unmasked ribi, and THEN, after changing the value of gr:
- on the first run through the loop, checks whether it's possible to advance into the new tile, but doesn't check this with masked ribi for track waytypes (whether there is a signal present or not)
- on the second and subsequent runs through the loop, checks (in code at the end of the loop which was set using the previous value of gr) that it's possible to advance from the tile to the tile after it, but only if there's a signal (whether it's a track waytype or not)

So the correction / behavior change actually relates to the check on whether there's a signal and the track waytype, the logic for which has been unified: it now checks that there's a signal and it's a track waytype both on the first time through the loop and the rest of the times through the loop.

The rest of the changes are all readability cleanups.  Since this code was sufficiently unreadable in Standard  that even Prissi didn't understand what it was actually doing.

Testing on demo.sve, convoy no. 107 seems to behave incorrectly some of the time (advancing to the end of the platform at a non-terminal stop and not doing so at a terminal stop) and also correctly some of the time both in the current version and in the version modified here.

I think I know what that bug is.  Currently you advance to the end of a platform *if the train is scheduled to reverse at that platform* and not otherwise, which is NOT actually the same check as "is it a terminal stop".  I can work on that; it's a bit complicated.

I can see why I might have been confused by a "get_..." method that changed the value of the thing being passed to it!

Yeah!  This took me days to figure out.

Simutrans has some very 1990s style code that can be very difficult to make sense of at times

A historical note:  This was not good coding style in the 1990s.  It's more 1970s-style code.

When Hajo wrote the first version in the 1990s, he was clearly used to an older coding style.  Apparently he was teaching himself object-oriented programming; this routine is written in a very pre-object-oriented-programming style, probably an artifact of him being new to object-oriented programming.

In fact, the loop structure with the decrementing max_len variable is characteristic of a style from an era when memory was in short supply, compilers didn't optimize, and you didn't want to create extra variables -- very 1970s.  I've written in that style occasionally because it's still used for assembly language programming in device drivers and embedded systems, but it's best avoided in other code these days.

- and without a major rewrite of what is there, it is often difficult to build on it without coding in the same style, which can then be much more prone to bugs. For completely new work, I have tried to make the code rather easier to read, but a lot needs to integrate with the older code from Standard, so this is not always easy.

In any event, thank you very much for your work on this.

I have several other things I want to clean up in this part of the code.
-- Currently (in extended only) a special value of "convoy_length + 8888" is passed through to indicate "advance to end of platform" (for historical reasons which make perfect sense to me but are obsolete); in my opinion, this should be an separate argument.  I think it should take a "platform behavior" enum, probably, since there are a number of choices as to how to behave when arriving at a platform.
-- As noted before I want some sort of rational behavior when there's a mid-platform signal.  I think I can figure out how to program this.
-- I should probably make a cleaned-up version of the code for Standard too, given how extremely confusing it is.  But the question of what the *desired* behavior is when the platform is significantly longer than the convoy remains an open question, since Prissi just said that the desired behavior isn't what's actually programmed here....

[neroden]

While I'm at it, I'm wondering how much work it is to implement diagonal platforms.  I know OTRP did it -- does anyone have links to the specific patches or git commits for that? 

This is one of the sections of code which makes explicitly non-diagonal-platform assumptions, and I think I can remove those assumptions while rewriting this, but I'm not sure what other sections of code would have to be touched.

[TurfIt]

If Standard does this somewhere, it's certainly not doing so in this code, and it never has.

rail_vehicle_t::calc_route():

Code Select Expand

// use length 8888 tiles to advance to the end of all stations
const uint16 convoy_length = world()->get_settings().get_stop_halt_as_scheduled() ? cnv->get_tile_length() : 8888;
return route->calc_route(welt, start, ziel, this, max_speed, convoy_length );


[neroden]

rail_vehicle_t::calc_route():

Code Select Expand

// use length 8888 tiles to advance to the end of all stations
const uint16 convoy_length = world()->get_settings().get_stop_halt_as_scheduled() ? cnv->get_tile_length() : 8888;
return route->calc_route(welt, start, ziel, this, max_speed, convoy_length );


Ah, right.  Settings-dependent.  That 8888 thing is UGLY.  I want to rewrite this with an added calc_route parameter (at this point, I think it should be an enum).

[neroden]

Found the OTRP diagonal stop code.  Thankfully it's quite recent. 

Starts after commit 83b871c in https://github.com/teamhimeh/simutrans/commits/OTRP-KUTAv6/?after=358f4d7c40b0e22e8970e09005423ef22ef478ac+69

For future reference the commits are, in order

Code Select Expand

5628d4c
cbfcbcf
cef1c1d
0638bc7
369627b
7424d2d
8c5e9e0 (interacts with coupling feature)
57280d4 (interacts with coupling feature)
3ec097c
064be33
c22a116
1c68885
0be02ec
894a728 (maybe)

This is... well, I would want to implement this more cleanly.

[TurfIt]

Ah, right.  Settings-dependent.  That 8888 thing is UGLY.  I want to rewrite this with an added calc_route parameter (at this point, I think it should be an enum).

Settings because at some point somebody wanted the option to not advance to the end of the station. Many many options like this in the game...

I don't think standards routine needs much rewrite - it's clear, well documented, etc.. IMHO.
The naming of get_neighbor and assumptions with get_  are not good, but looking at the function itself it's clear. Never assume what a function does off it's name - look at it. Especially with this mass german to english that happened, some rather questionable english names were assigned to things. (and I have no idea how appropriate the original german names were...)

8888 ugly, but simple. However, it does prevent any sensible functioning of 'valid_route_halt_too_short'. A separate parameter to trigger the advance to end behaviour would allow a warning message on rail trains that are too long, not just on road vehicles.

[jamespetts]

Thank you for that - that is helpful. I have now integrated this.

As for signalling - note that the current code assumes that signals will be placed at the end of platforms; placing signals part way along platforms will produce unintended results. If you would like to modify the code to allow signals part way along the platform to have a function, this will need careful consideration to ensure that the behaviour is consistent with real-world signalling principles.

Diagonal platforms are an interesting idea - I can see that this would be helpful to players, although would require some significant pakset work to implement (sadly, not as easy as re-rendering the graphics from .blends, as all the player colours have to be added manually for each individual rotation).

[prissi]

Partial loading of trains on too short platforms was requested by some players, like filling half the trains with iron ore and then next half with coal. Or just loading passengers at an intermediate stop.

[neroden]

Thank you for that - that is helpful. I have now integrated this.

As for signalling - note that the current code assumes that signals will be placed at the end of platforms; placing signals part way along platforms will produce unintended results. If you would like to modify the code to allow signals part way along the platform to have a function, this will need careful consideration to ensure that the behaviour is consistent with real-world signalling principles.

I think it should be treated as dividing the platform into two functionally independent platforms.  This is the only sensible meaning for it, IMO.  It may physically be one platform but the trains should treat it as two different stopping platforms -- perhaps one route stops on one of them, and one route stops on the other.
 

Diagonal platforms are an interesting idea - I can see that this would be helpful to players, although would require some significant pakset work to implement (sadly, not as easy as re-rendering the graphics from .blends, as all the player colours have to be added manually for each individual rotation).

Yeah. :-(  It was done for one of the OTRP paksets.

[neroden]

Settings because at some point somebody wanted the option to not advance to the end of the station. Many many options like this in the game...

I don't think standards routine needs much rewrite - it's clear, well documented, etc.. IMHO.

I disagree.  It's caused a lot of trouble and confusion in the past.  I will work up a no-functionality-change patch at some point to try to make it more readable.

[jamespetts]

I think it should be treated as dividing the platform into two functionally independent platforms.  This is the only sensible meaning for it, IMO.  It may physically be one platform but the trains should treat it as two different stopping platforms -- perhaps one route stops on one of them, and one route stops on the other.

This is potentially complex. Potentially challenging things to consider here include: (1) the treatment of trains that will fit in the whole length of the platform, but not one of the subdivided parts (players are likely to expect this to work without having to do anything); (2) the effect on stopping distances of having signals so close (nothing that needs to be done in the code, but this may be a disincentive for players placing signals in this way if trains may need to pass at speed through the platforms); and (3) how a choose signal should work if and to the extent that these are treated as separate platforms.

[neroden]

This is potentially complex. Potentially challenging things to consider here include: (1) the treatment of trains that will fit in the whole length of the platform, but not one of the subdivided parts (players are likely to expect this to work without having to do anything); (2) the effect on stopping distances of having signals so close (nothing that needs to be done in the code, but this may be a disincentive for players placing signals in this way if trains may need to pass at speed through the platforms); and (3) how a choose signal should work if and to the extent that these are treated as separate platforms.

(1) This I can do.
(2) OK, nothing which needs to be done in the code
(3) There are already issues with choose signals.  I've been trying to decipher that code in order to solve some other problems.  A choose signal should preferentially pick a "long enough" platform, but I think right now it does not. 

The actual main issue I see is determining which side of the platform the tile with the signal on it is on :-(

My plan is to break out the code into a find_platform subroutine: given a single tile (which is part of a halt) and a convoy's length (probably in carunits), find the associated "entire platform", as well as determining that platform's length.  This will contain all the fiddly code including code for tracking the length of diagonal platforms and allowing for curved platforms.  It will also determine whether the platform is a terminal.  (A platform, however, should never be able to cross a junction: a way should exit a platform in either one or two directions.)  This can then be called where needed.

[TurfIt]

Once the platform is split in two, allowing two trains in at once, a midplatform junction to form an escape path could be prototypical.

[jamespetts]

Once the platform is split in two, allowing two trains in at once, a midplatform junction to form an escape path could be prototypical.

Indeed - there is something like this in Cambridge, too.

[neroden]

I believe that is harder to implement given Simutrans's current design choices.  My plan was to track what platform I was on basically by going "move to the next tile -- is it still a platform? (and does it have a signal?)".  This handles diagonal and curving platforms.  Currently "next tile" when starting on a platform tile is currently unambiguous (it's either "none" for a terminal or another tile with the same waytype).  But mid-platform junctions would mean that "next tile" is ambiguous.  I'll think about this while dealing with the problem of "what do we do with the tile which the signal is on", however; there may be some logical implementation.
 
Since we don't actually track pedestrian movement within a station, if someone absolutely has to put a mid-platform crossover, they can delete that one tile of platform if they absolutely have to.  It would just be a kindness to the player to not require this under normal circumstances (i.e. when there is no mid-platform crossover needed)

[Octavius]

Such mid-platform crossovers are pretty common, in some countries at least. In the Netherlands, about 30 of 395 stations have at least one platform track with a junction in the middle. When there's such a junction, there's a signal too; when there's a signal, there's usually a junction. I've tried to use them in Simutrans in the past (and obviously failed).

If there's a junction or signal on the platform, we can expect the user to indicate which part of the platform track is to be used. The user knows why he made such a lay-out. If the station tile indicated by the user in the schedule is before the signal or junction, the train may proceed up to the signal or just before the junction (which is where the signal would normally be placed); if the train doesn't fit on that part of the platform, complain to the user that the platform is too short. If the station tile indicated by the user is in the second part of the platform, use that, and if the train is too long for just the second part, use the first part too. That just leaves the edge case, where the user schedules the train to stop on the junction. I suggest stopping on the junction then.

Basically, take the tile where the player has scheduled the train to stop; if this tile has no stop signal (distant signals can be ignored) and the next tile is no junction and is still part of the platform, go to the next; repeat.

If there's a choose signal, the user doesn't care where the train stops. A sensible user wouldn't use such a lay-out in combination with a choose signal; if he does so anyway, don't be surprised when strange things happen.

I see just one remaining issue. What if a long train enters the station on platform track 1a, then uses the crossover to the parallel track 2 and stops with the front of the train on platform track 2b? Simutrans would allow this, but in real life the middle of the train wouldn't be along the platform. Not a major problem I think.

[neroden]

Good advice.  I'll rely on the signal as the key information when I get around to this.