FS#1762 - Weird autoreplace behaviour

Attached to Project: OpenTTD
Opened by Alberth (Alberth) - Monday, 11 February 2008, 17:15 GMT
Last edited by frosch (frosch) - Saturday, 19 December 2009, 21:55 GMT
Type Bug
Category Vehicles → Autoreplace
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


In rev12110, train autoreplace shows some weird behavior.

In the save game attached, an autoreplace is activated for the train engines. However, I am quite out of money, so there is no point in sending them to the depot anyway (I run without vehicles breakdowns, and 'disable servicing when no breakdown' configure patch enabled).

In addition, the train-routing gets messed up. The train in the bottom center (moving from left to right) goes to the depot at the right for servicing.
(autoreplace was switched on after it left the station at the left). The replacement fails due to lack of money, then instead of delivering the goods at the right station, it returns to the left station! In this way, the train rides a few times back and forth before finally delivering the goods.
This task depends upon

Closed by  frosch (frosch)
Saturday, 19 December 2009, 21:55 GMT
Reason for closing:  Fixed
Additional comments about closing:  in r18551
Comment by Alberth (Alberth) - Sunday, 17 February 2008, 13:55 GMT
Wrote a patch to fix this behaviour (against rev12166).

The 'servicing' concept of all vehicle types has been split in 'maintenance' (periodically visiting the depot for resetting breakdowns, reliability, etc), and 'updating', renewal or replacement of the engine.

This is reflected in vehicle.cpp, the routines VehicleNeedsMaintenance() (almost the old VehicleNeedsService()), and VehicleNeedsUpdate(), the old CheckSendAircraftToHangarForReplacement(), generalized to all types of vehicles.
The VehicleNeedsUpdate() function performs a check on having enough renewal money before sending vehicles to the depot, hence my problem of getting my trains constantly re-directed to a depot disappears.

This patch also covers  FS#1029 , except for the change in SendAllVehiclesToDepot(), which I believe would be incorrect.
Comment by Alberth (Alberth) - Saturday, 23 February 2008, 09:31 GMT
The patch seems to cause unexpected state changes in aircrafts, it needs further investigation before committing to trunk
Comment by Alberth (Alberth) - Sunday, 24 February 2008, 10:30 GMT
Not sure whether my patch was the cause or just the trigger; I suspect the latter.

An aircraft at a terminal is tied to the station by means of the st->loading_vehicles list while exchanging goods/passengers. In the StateGameLoop() (openttd.cpp, line 977), the date is increased (line 1003, and vehicles are handled line 1005). If a new day is detected, v->OnNewDay() is executed, which for an aircraft, results in a call to CheckIfAircraftNeedsService() (aircraft_cmd.cpp, line 711). If at that point, the need for maintenance is detected, the order is changed to OT_GOTO_DEPOT. In particular, the aircraft is not uncoupled from the station 'loading_vehicles' list.

Two lines later in the game-loop, vehicles are handled. This makes a call to CallVehicleTicks() (vehicle.cpp, line 740), which does goods exchanging through LoadUnloadStation() (economy.cpp, line 1799). This function calls LoadUnloadVehicle() (line 1556) for vehicles in the station 'loading_vehicles' list. At line 1558 "assert(v->current_order.type == OT_LOADING)" is performed, which fails, since we just decided for the aircraft that it needs service.
As a result, the game crashes.

The fix for this problem that I implemented is to also detach the vehicle from the station data structures in CheckIfAircraftNeedsService() by adding a call to v->LeaveStation().
An alternative solution could be a more careful selection which vehicle to exchange goods with. I have not further explored that solution.

Attached you can find the additional fix, and an updated version of autorenew2.patch (now against r12225) in autorenew3.patch .

(with the autorenew2.patch file, the above bug is triggerable by settng up a replacement for an aircraft, and borrowing just too little money to get the replacement active (you need 'auto-renew-money + 2 * cost-of-new-engine'). The aircraft lands, goes to the terminal, gets paid (thus crossing the money boundary), and at that moment, a new day has to be activated.)
Comment by Alberth (Alberth) - Sunday, 24 February 2008, 10:33 GMT
Oops, with the check of the aircraft, the need for *update* is detected rather than maintenance. Sorry.
Comment by frosch (frosch) - Saturday, 29 November 2008, 18:21 GMT
To summarize some issues:
1. Vehicle::NeedsAutomaticServicing() sends engines to depot when an autoreplacement rule is set for them. No matter whether the vehicle was just serviced.
2. Vehicle::NeedsAutomaticServicing() does not test wagons for autoreplacement rules.
3. The vehicle is send to depot, even if there is not enough money for the replacement. (though there might be, when it reaches depot)
4. The vehicle is send to depot, even when autoreplace will fail anyway. (E.g. new vehicle not refitable to the needed cargo, newgrf restrictions, length restrictions, ...)

IMO there is no reasonable way to check 4. when the train is not in a depot resp. to check it every vehicle tick / day / ...
Some ways to circumvent the problem:
a) Remove automatic sending vehicles to depot when autoreplace/renew is set. Always let the user do it manually. (Autorenew does not encounter 2. or 4., so maybe some money rule would be enough)
b) Add some time constraint to checking for sending a vehicle to a depot for autoreplace. E.g. a certain number of days since last service (e.g. service interval, resp. some default constant if service_interval == 0).
c) Add some other constraint. E.g. only send a vehicle to a depot when it has just completed another order, e.g. finished loading/unloading, reached a waypoint, ... (but not when it reached a station and wants to start loading/unloading)
Comment by Remko Bijker (Rubidium) - Saturday, 27 December 2008, 09:58 GMT
Another thing is trains being sent to depots where their replacement can't be build ( FS#2468 ).
Comment by Bilbo (bilbo) - Tuesday, 21 April 2009, 22:27 GMT
This issue is much more serious for airplanes - if you set autoreplacement for airplanes, then airplanes start being autoreplaced, but if you run out of money in the process, then airplane will go to hangar for service, go right out (no money for autoreplace), then head to the terminal/loading spot, but before it manages to unload or pick something, it is immediately called back to depot for another autoreplace attempt. Such aircraft may block airport (at least the small size) for quite a long time and if unnoticed, it may lead to huge losses/getting bankrupt