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: Fix bugs in airport finite state machines #7710

Merged
merged 4 commits into from Oct 25, 2019

Conversation

@JMcKiern
Copy link
Contributor

JMcKiern commented Aug 30, 2019

Upon looking into #6219 I found that there were some issues with the finite state machines for the airports.

I have corrected the behaviour on a commuter airport of a helicopter in a hangar trying to takeoff while the helipads - actually only helipad2 - are/is occupied. With the first commit, it can now takeoff from just outside the hangar. Please let me know if this is not expected behaviour.

Note that there may be similar issues with the other airports that have helipads. I know that the international airport has the same issue.

This pull request is currently a WIP. I'm creating it now as I would be grateful for any feedback/assistance.

Additionally, with the pull request I would like to remove some of the magic numbers used:

  • the 0's used in the heading variable (TO_ALL)
  • the 255's used in the heading variable (unsure)
  • the 0's used in the block variable (unsure)

Finally, if anyone could answer the following, that would be great:

  1. What does 255 mean in the heading variable?
  2. What is the functional difference between NOTHING_block and 0? As far as I can tell, NOTHING_block doesn't block anything. But due to the bit setting nature of airport.flags surely 0 would have the same effect, right?
  3. Why are the takeoff and landing positions outside the depot for helicopters slightly different? For example, on a country airport, landing is at (44, 37) while takeoff is at (44, 40). This is also true for city airports. Surely they would be the same in reality.
@Eddi-z

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Sep 2, 2019

* What does 255 mean in the `heading` variable?

i haven't looked at the code, but my first guess would be some kind of INVALID value. The pattern "all bits set => INVALID" does appear a lot elsewhere in the code.

@JMcKiern

This comment has been minimized.

Copy link
Contributor Author

JMcKiern commented Sep 2, 2019

I had a chance to look at the code and found that AirportFindFreeTerminal() uses 255 to denote terminal groups (for example, international airports have 2 groups of 3 terminals each as indicated by the first entry in _airport_terminal_international). I believe that the 255 is used to route an aircraft to a specific terminal group. AirportFindFreeTerminal() loops through each terminal in each terminal group until it finds a free terminal. When heading is 255, next seems to store the terminal group number rather than the next position.

As far as I can tell, when heading is 255, there is no effect if there is only 1 terminal group. Country, commuter, city and metropolitan airports all have only 1 terminal group. However, all of these airports contain 255 as a heading in several of the entries in _airport_fta_XX. I suppose it is possible that these entries could be an artifact of the method used to generate the arrays but I'm not 100% sure about that...

JMcKiern added 2 commits Aug 30, 2019
Previously, a helicopter in the hangar of a commuter airport would have to wait until HELIPAD2 was free before it could takeoff. Now, a helicopter in the hangar can takeoff from just outside the hangar.
@JMcKiern JMcKiern force-pushed the JMcKiern:airportfta branch from 16d5025 to ad26078 Sep 2, 2019
@JMcKiern

This comment has been minimized.

Copy link
Contributor Author

JMcKiern commented Sep 8, 2019

This pull request is now ready for review.

In summary:

  • Helicopters can now take off from right outside the depots/hangars, mimicking the behaviour of airport without helipads. This required modifying the state machines of both the commuter airport (as identified in #6219) and the international airport. The intercontinental airport already behaved as expected.

  • The 255 in the heading variable denotes a terminal group (or terminalgroup as in the comments). I replaced the occurances of 255 with a TERMGROUP variable added to the AirportMovementStates enum with 168c0c9.

  • NOTHING_block and 0 do indeed appear to have different functions. These uses are contrasted with examples of code using NOTHING_block and 0. However, I do not completely understand the differences so I have chosen to leave it as is.

  • For slightly varying helicopter takeoff positions, I have used my best judgement. For the commuter airport I added 3 to the y value of the landing position to get the takeoff position, as in a country airport. For the international airport, I set the takeoff position to right outside the depots.

@JMcKiern JMcKiern marked this pull request as ready for review Sep 8, 2019
@JMcKiern JMcKiern changed the title WIP: Fix bugs in airport finite state machines Fix: Fix bugs in airport finite state machines Sep 8, 2019
@JMcKiern

This comment has been minimized.

Copy link
Contributor Author

JMcKiern commented Sep 8, 2019

@LordAro @nielsmh Let me know what you guys think when you get a chance

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Sep 8, 2019

Structurally this PR looks reasonable, but I'll have to dedicate some time to playtest it before giving a review.

@Eddi-z

This comment has been minimized.

Copy link
Contributor

Eddi-z commented Sep 8, 2019

There seem to be a few changes that are just whitespace, make sure these are following our code style for indentation (difficult to judge from the github interface)

src/airport.h Outdated Show resolved Hide resolved
@JMcKiern

This comment has been minimized.

Copy link
Contributor Author

JMcKiern commented Sep 8, 2019

There seem to be a few changes that are just whitespace, make sure these are following our code style for indentation (difficult to judge from the github interface)

I believe the only whitespace changes are in airport.h. I followed the style that was already there: a space after the equals sign, and then align the commas vertically (moving the numbers with them if necessary). Let me know if that's not appropriate. I couldn't find it in the coding style guide.

Copy link
Member

LordAro left a comment

Probably fine, can always be reverted if not :)

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Oct 22, 2019

Unsure if this should be squash-merged, or if the individual commits should be kept.
If the individual commits are to be kept, 168c0c9 and df4d7ee should be squashed to avoid putting in an unnecessary "revert" commit.

JMcKiern added 2 commits Sep 8, 2019
Helicopters can now take off from just outside the hangars.
@JMcKiern JMcKiern force-pushed the JMcKiern:airportfta branch from df4d7ee to ddf9315 Oct 24, 2019
@JMcKiern

This comment has been minimized.

Copy link
Contributor Author

JMcKiern commented Oct 24, 2019

Squashed the realignment and reversion commits into 5db82f7

@nielsmh nielsmh merged commit f52e605 into OpenTTD:master Oct 25, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20191024.1 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.