Codechange: Rework automatic service for helicopters (#6493) #7111
Conversation
…dingReplace In order to verify if a helicopter with service at helipad has a pending replace, it's needed to call HasPendingReplace directly, as a seperate function that can be used by some aircraft_cmd.cpp functions
…ft at a hangar outside the order list If a helicopter needs to be sent to a hangar during automatic service, and there are no airports with hangars in the orders (travelling between heliports), a nearby hangar outside the orders will be searched. If the distance to a found hangar is shorter than the distance to the helistation/airport the helicopter is headed to, the helicopter is sent to that hangar.
SamuXarick
force-pushed
the
rework-automatic-service-for-helicopters
branch
from
1cd6f23
to
8dd4379
Compare
| bool needs_automatic_servicing = v->NeedsAutomaticServicing(); | ||
| bool has_pending_replace = v->HasPendingReplace(); | ||
| if (Company::Get(v->owner)->settings.vehicle.servint_aircraft == 0 || (!needs_automatic_servicing && | ||
| (!has_pending_replace || v->subtype != AIR_HELICOPTER || !_settings_game.order.serviceathelipad))) return; |
There was a problem hiding this comment.
serviceathelipad is a legacy setting only present for compatibility with TTDPatch savegames, and OTTD savegames from before the helistations with hangars were introduced. It should not be considered for this change, to reduce cyclomatic complexity (which is already being increased significantly).
Presumably, helicopters on a helipads-only route with the setting enabled would be serviced very often regardless, so the usual NeedsAutomaticServicing will likely not kick in before an autoreplace or autorenew triggers. That's an additional reason I think the checks for serviceathelipad are unnecessary.
| const Station *ost = Station::Get(o->GetDestination()); | ||
| if (ost->airport.HasHangar() || | ||
| /* Helicopters can be serviced at helipads as long as there is no pending replace and serviceathelipad is enabled */ | ||
| (v->subtype == AIR_HELICOPTER && !has_pending_replace && _settings_game.order.serviceathelipad && ost->airport.GetFTA()->num_helipads)) { |
There was a problem hiding this comment.
Will be simplified significantly by not considering serviceathelipad.
| */ | ||
| bool Vehicle::NeedsServicing() const | ||
| bool Vehicle::HasPendingReplace() const |
There was a problem hiding this comment.
This is not a critique, but I think the diff will look nicer if you move HasPendingReplace to below ´NeedsServicing` :)
| /* v->tile can't be used here, when aircraft is flying v->tile is set to 0 */ | ||
| TileIndex vtile = TileVirtXY(v->x_pos, v->y_pos); | ||
|
|
||
| if (nearest_hangar != INVALID_STATION && ((has_pending_replace && v->subtype == AIR_HELICOPTER && _settings_game.order.serviceathelipad && !needs_automatic_servicing) || |
There was a problem hiding this comment.
Will be simplified significantly by not considering serviceathelipad.
| @@ -1529,6 +1565,14 @@ static void AircraftEventHandler_AtTerminal(Aircraft *v, const AirportFTAClass * | |||
| /* airport-road is free. We either have to go to another airport, or to the hangar | |||
| * ---> start moving */ | |||
|
|
|||
| if (Station::Get(v->targetairport)->airport.HasHangar()) { | |||
| if (v->NeedsAutomaticServicing() || (v->subtype == AIR_HELICOPTER && v->HasPendingReplace() && _settings_game.order.serviceathelipad && | |||
There was a problem hiding this comment.
Will be simplified significantly by not considering serviceathelipad.
|
This is not an actual problem that needs fixing. The Closing as not a bug. |
In order to verify if a helicopter with
serviceathelipadhas a pending replace, it's needed to callHasPendingReplacedirectly, as a seperate function, that can be used byAircraftEventHandler_AtTerminalandCheckIfAircraftNeedsService.If a helicopter needs to be sent to a hangar during automatic service, and there are no airports with hangars in the orders (travelling between heliports), a nearby hangar outside the orders will be searched. If the distance to a found hangar is shorter than the distance to the helistation/airport the helicopter is headed to, the helicopter is sent to that hangar.
Also, when an aircraft (of any type) is at a terminal, it is checked whether it needs automatic servicing, and in the case of helicopter, due to the complexity of
serviceathelipad, whether it has a pending replace, and then sent to the hangar of the airport, before being sent to takeoff.These changes thus replace the two methods which were attempting to do the same.
CheckIfAircraftNeedsServiceevery day.