FIX: some reserved tracks are not unreserved

[Dwachs]

See attached image. Originally the track was straight and completely reserved by the train. The I removed some parts of the tracks and build kind of sideway. The marked piece of track will never get unreserved. If there is a crossing on such a tile, the crossing will block forever.

Patch attached. The problem was in waggon_t::block_reserver. If a not reserved tile was encountered 'unreserve_now' was set to true, which means that when the next unreserved tile is encountered the loop will exit. However due to this building operations some unreserved tiles remain. I also changed convoi_t::move_to and betrete_depot to call the blockreserver to unreserve the route (betrete_depot was fine, but move_to could be a potential trouble maker, as it was not safe with regards to long vehicles on diagonals).

@prissi: I was not sure how to set the parameter count in waggon_t::block_reserver when called from convoi_t::unreserve_route. What would you suggest?

[prissi]

I think we should anyway make two seperate routine like block_reserver and block_unreserver, as the different logics and check makes it really hard to follow code and unvolce a lot of checks.

But I think this needs a little more testing and profiling to give a conclusive answer (but likely not this evening, I fear).

[Dwachs]

Why additional profiling? The force_unreserve parameter is only set to true, when a new route should be calculated (calc_route). All other calls of blockreserver have force_unreserve=false set, so the behavior is exactly the same.

convoi_t::move_to is called when a convoi needs to be realigned, there it is a good idea to call the blockreserver.

convoi_t::betrete_depot : the call to blockreserver only checks a short route (the distance from last vehicle to depot, there should be no performance impact)

---
The bug is easily reproducable and fixed by the changes in simvehikel.*

The changes in simconvoi.* are for completeness: betrete_depot needs unreserving but had its own method, move_to may give
problems. The call to the blockreserver is to have one place in code, where the unreserving for a convoi is done.

[prissi]

Code Select Expand


- if(unreserve_now) {
+ if(unreserve_now  &&  !force_unreserve) {
// reached an reserved or free track => finished
return false;
}
}
else {
- // unreserve from here (only used during sale, since there might be reserved tiles not freed)
- unreserve_now = true;
+ if (!force_unreserve) {
+ // unreserve from here (only used during sale, since there might be reserved tiles not freed)
+ unreserve_now = true;
+ }


should this not rather be

Code Select Expand


if(unreserve_now) {
// reached an reserved or free track => finished
return false;
}
}
else {
// unreserve from here (only used during sale, since there might be reserved tiles not freed)
- unreserve_now = true;
+ unreserve_now = !force_unreserve;


Your code seems somewhat doubled.

And calling it in move_to every time a train reverses in a station is really adding quite a lot extra tiles to be checked.

[Dwachs]

You are right. I will remove the call to unreserve in moveto.

Edit: updated patch attached.

[prissi]

Please commit for nightly testing ...

[Dwachs]

Done: rev 4185