Alberth wrote:

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 #1029, except for the change in SendAllVehiclesToDepot(), which I believe would be incorrect.

Attachments

• autorenew2.patch (17.52 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment3508

Alberth wrote:

The patch seems to cause unexpected state changes in aircrafts, it needs further investigation before committing to trunk

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment3531

Alberth wrote:

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.)

Attachments

• autorenew3.patch (18.01 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment3535

Alberth wrote:

Oops, with the check of the aircraft, the need for update is detected rather than maintenance. Sorry.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment3536

frosch wrote:

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)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment5078

Rubidium wrote:

Another thing is trains being sent to depots where their replacement can't be build (#2468).

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment5156

bilbo wrote:

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

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762#comment5954

frosch closed the ticket.

Reason for closing: Fixed

in r18551

This comment was imported from FlySpray: https://bugs.openttd.org/task/1762