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

Fixes for issues with rotated (non-rectangular) airports #8458

Merged
merged 2 commits into from Jan 5, 2021

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Dec 28, 2020

Motivation / Problem

#8437

Description

First issue is caused by assumptions that the airport tile is the same tile as the rest of the airport/runway. Obviously the case in almost every case... except for the rotated (180deg) intercontinental airport
Second issue is again only an issue for rotated intercontinental, but seems to boil down to oilrigs being hacks - the "tile" that the aircraft/helicopter is landing at is always the airport tile, purely because the oilrig is actually only a 1x1 airport, pretending to be 2x2. Essentially, the fix for this just expands the special case that's already in place lower down (which doesn't actually seem to get triggered for oilrigs at all anyway

OpenTTD/src/aircraft_cmd.cpp

Lines 1037 to 1038 in edbb5f4

/* Oilrigs must keep v->tile as st->airport.tile, since the landing pad is in a non-airport tile */
gp.new_tile = (st->airport.type == AT_OILRIG) ? st->airport.tile : TileVirtXY(gp.x, gp.y);

Limitations

More hacks upon hacks.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged needs review: mad man size: small This Pull Request is small, and should be relative easy to process labels Dec 28, 2020
@James103
Copy link
Contributor

@TrueBrain What does the label "needs review: mad man" even mean?

@TrueBrain
Copy link
Member

@TrueBrain What does the label "needs review: mad man" even mean?

Exactly what it says: we need a mad man to review this PR. We already had one mad men making this PR ... only fair if another one does reviewing.
(in other words, reviewing this when you are tired or are hungry is a bad idea ... airport code is ... "special" in OpenTTD)

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise it looks okay to me. Tested it in a bunch of different setups, and I couldn't find any boo-boo, so yeah, I think this should be it.

Comments however do need work, as it took me more time to understand those than to understand the code :P Sorry :)

src/aircraft_cmd.cpp Show resolved Hide resolved
src/aircraft_cmd.cpp Outdated Show resolved Hide resolved
… drawn at the wrong height

Only the rotated intercontinental airport, don't get excited
@LordAro LordAro force-pushed the bug8437 branch 2 times, most recently from a3ee01c to 922cc39 Compare January 3, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review: mad man size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants