Skip to content
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

Closed
wants to merge 10 commits into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Oct 1, 2018

Pack of fixes about aircraft heading to hangar (mis)behaviours. Expand below to read details or explanation.

@SamuXarick SamuXarick changed the title Fix #6574: Go to takeoff if no hangar Fix #6574 #6636 #5405 #6493: Aircraft hangar issues Oct 3, 2018
@LordAro
Copy link
Member

LordAro commented Oct 3, 2018

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

@SamuXarick
Copy link
Contributor Author

#5405 fixed it for automatic servicing, but was not fixed for orders of type 'service at nearest hangar'. Savegame attached.
Windside Heath Transport, 1985-03-05.zip

{
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;
Copy link
Contributor

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?

Copy link
Contributor Author

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 */
Copy link
Contributor

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.

Copy link
Contributor Author

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 */
Copy link
Contributor

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.

Copy link
Contributor Author

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.


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)))) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@SamuXarick SamuXarick force-pushed the aircraft-hangar-issues branch 2 times, most recently from 868f981 to 9d696a8 Compare November 6, 2018 23:02
@TrueBrain
Copy link
Member

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
@andythenorth
Copy link
Contributor

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!

@nielsmh
Copy link
Contributor

nielsmh commented Jan 25, 2019

Some of the changes here may be valid improvements, but they need to submitted as smaller patches that are more easily testable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants