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')
@James103
Copy link
Contributor

@James103 James103 commented Dec 31, 2020

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

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 1, 2021

@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

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
…at the wrong height

Only the rotated intercontinental airport, don't get excited
@LordAro LordAro force-pushed the LordAro:bug8437 branch 2 times, most recently from a3ee01c to 922cc39 Jan 3, 2021
@LordAro LordAro force-pushed the LordAro:bug8437 branch from 922cc39 to 7a854ee Jan 3, 2021
@TrueBrain TrueBrain merged commit e21302f into OpenTTD:master Jan 5, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants