New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #6574 #6636 #5405 #6493: Aircraft hangar issues #6925
Conversation
Code-wise this looks fine, but I don't know enough about the code to know whether these are the correct solutions or if it's what's desired. A couple of things: Can you elaborate on why the fix for #5405 is necessary, when it was supposedly fixed already? Also, it would be really nice if you could merge the compiler warnings commit into which ever commit caused the warning - the changes are small enough that this is probably easiest to achieve with some manual commit editing - interactive rebase, "pick" the necessary commits & amend them |
#5405 fixed it for automatic servicing, but was not fixed for orders of type 'service at nearest hangar'. Savegame attached. |
c7741c6
to
405b39a
Compare
{ | ||
OrderBackup *ob; | ||
FOR_ALL_ORDER_BACKUPS(ob) { | ||
for (Order *order = ob->orders; order != NULL; order = order->next) { | ||
OrderType ot = order->GetType(); | ||
if (ot == OT_GOTO_DEPOT && (order->GetDepotActionType() & ODATFB_NEAREST_DEPOT) != 0) continue; | ||
if (ot == OT_IMPLICIT || (IsHangarTile(ob->tile) && ot == OT_GOTO_DEPOT)) ot = OT_GOTO_STATION; | ||
if (ot == OT_IMPLICIT || (IsHangarTile(ob->tile) && ot == OT_GOTO_DEPOT && !hangar)) ot = OT_GOTO_STATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this additional complexity necessary? What cases does it prevent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to clear up (invalidate) the 'to go hangar' orders when replacing an airport with hangar with another without hangar, while still keeping the 'go to same airport' orders. Since I couldn't invalidate only the 'go to hangar' orders, I had to change this part of the code to let me keep the 'go to airport' orders. Otherwise, both 'go to hangar' and 'go to same airport' would be made invalid.
Aircraft *a = Aircraft::From(v); | ||
DestinationID destination = a->current_order.GetDestination(); | ||
if (a->targetairport != destination) { | ||
/* The aircraft is now heading for a different hangar than the next in the orders */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a bit of trial and error to understand the issue being fixed here. Rewording it in a way I think I would understand: If aircraft is currently heading for airport A, you you click Skip Order, and the next order is go to hangar at airport B, the aircraft will continue heading to airport A and use the hangar at that airport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug is this.
1 - Go to hangar A
2 - Go to hangar B
3 - Go to hangar C
If the aircraft is executing order 1, it is going to hangar A. Now you manually skip to order 2. The bug is that the aircraft is still heading to hangar A, while it should be going to hangar B. If you skip it again, it still goes to hangar A, instead of hangar C. That is the bug.
|
||
uint max_range = v->acache.cached_max_range_sqr; | ||
if (max_range != 0) { | ||
/* Determine destinations */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate a comment here more thoroughly describing the situation being tested for/prevented by these checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is trying to prevent Aircraft with a max range from getting a 'destination too far' after being serviced at a nearest hangar. The nearest hangar is an order (let's call it H) that is placed between orders A and B. The code is trying to ensure that the range from H to B and H to A are still within the max range limit of the aircraft. It is based on the assumption that A to B are within the range limit.
Determining the correct last and next destinations is the complicated part, and I'm not 100% sure that part of the code is fool proof, but it's the best I could do to. GetTargetAirportIfValid
doesn't update in the same manner as the others, and then there's the 'Service at nearest hangar' which also behaves different during the v->current order.GetDestination()
, so sometimes the last_dest is shifted an order behind, and next_dest is shifted an order ahead.
I don't know how to make this better. From my testings, it seems to be working fine enough, but without garantees.
src/aircraft_cmd.cpp
Outdated
|
||
if (nearest_hangar != INVALID_STATION && ((!CanVehicleUseStation(v, st) && v->state >= TAKEOFF && v->state <= FLYING) || (!st->airport.HasHangar() && | ||
/* is nearest hangar closer than destination? */ | ||
DistanceSquare(vtile, Station::Get(nearest_hangar)->airport.tile) <= DistanceSquare(vtile, st->airport.tile)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all the CanVehicleUseStation and HasHangar checks here? Isn't it already established that the current station either does not have a hangar, or cannot be used by the vehicle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is because of the first if condition:
if (st->airport.HasHangar() && CanVehicleUseStation(v, st))
I only know that either st->airport.HasHangar()
or CanVehicleUseStation(v, st)
failed. CheckIfAircraftNeedsService
is called daily, and it's part of an attempt to fix helicopters travelling between two heliports without hangars, but I also intend to maintain the original behaviour for the other cases. There's also situations where airports could be demolished in-between, and knowing this function is called every day, I went with more checks to try avoid abnormal behaviour.
868f981
to
9d696a8
Compare
We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles! |
Sends the aircraft to takeoff if the airport it's currently at, got no hangar even if the order is to go to hangar.
When replacing an airport with another, cancel current orders of type 'go to depot' from aircraft still heading to it if the rebuilt airport doesn't have a hangar (helicopter vs heliport), or if the airplane can't land on the rebuilt airport (airplane vs helistation). Removes 'go to hangar' orders from all aircraft when replacing an airport with hangar with another without hangar (heliport).
…kipping to a go to hangar order When manually skipping to a 'go to hangar' order in the order list, while the aircraft is flying, direct the aircraft to the correct location of the hangar.
…next destinations and then ensure these are within range of each other This fix is to avoid out of range when using service at nearest hangar orders
…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.
…d or when no_servicing_if_no_breakdowns is disabled Helicopters with 'service at helipads' enabled, will only do so when 'disable servicing when servicing set to none' is disabled, or, if enabled, when 'vehicle breakdowns' is not set to none.
…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
…pending replace Check if an airplane needs automatic servicing or if a helicopter has a pending replace and send it to hangar before sending to takeoff This replaces the old method which was attempting to do the same, but was ultimately canceling automatic servicing orders upon landing.
…n checks Results in less detours when handling aircraft heading to unreachable stations, such as airplanes trying to service at helistations
63c83d4
to
da215cd
Compare
This one appears to be stuck, and reviewers are finding it difficult to providing constructive reviews. Also it appears difficult to be confident that these solutions are correct. On this basis I am closing this one. Thanks for contributing! |
Some of the changes here may be valid improvements, but they need to submitted as smaller patches that are more easily testable. |
Pack of fixes about aircraft heading to hangar (mis)behaviours. Expand
…
below to read details or explanation.