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

patch: upgrade airports #1562

Closed
DorpsGek opened this issue Dec 16, 2007 · 17 comments
Closed

patch: upgrade airports #1562

DorpsGek opened this issue Dec 16, 2007 · 17 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

cirdan opened the ticket and wrote:

I would like to submit my upgrade-airport patch for inclusion in trunk.

The patch allows an airport to be built over another one, much like it can already be done with railroad stations. When doing so, the old airport is destroyed and the new one is built in an atomical way. The advantages of this are:

- First of all, it simplifies upgrading airports (replacing an airport type with another one, usually bigger). You just select the new airport and build it over the old one.

- Being an atomical operation means that there will be no "window of opportunity" between removing the old airport and placing the new one, so the town can't suddenly build a road or, worse, a building in the area in the interim.

- Also, town authority ratings are checked before execution starts, so you won't find you've removed an airport and then you can't build the new one, nor rebuild the old one.

Requirements for the airport remain the same, except that the 2-airports-per-town rule is not checked if replacing an airport.

This patch addresses the following feature request:
http://bugs.openttd.org/task/877

Of course, this patch is most useful coupled with the close-airport patch. Together they make upgrading an airport really easy. :-)

Attachments

Reported version: 0.5.3
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/1562
@DorpsGek
Copy link
Member Author

Belugas wrote:

impressive...
Could you comment some of your work a little more?
CanRemoveAirport, for example.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3012

@DorpsGek
Copy link
Member Author

cirdan wrote:

Could you comment some of your work a little more?

Sure. Is this new version better?

CanRemoveAirport, for example.

CanRemoveAirport is code extracted from RemoveAirport. Now it is also needed in CmdBuildAirport, so I factored it out.

By the way, the new version also changes CMD_ERROR in CanRemoveAirport into more meaningful error messages ("Aircraft in the way").

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3031

@DorpsGek
Copy link
Member Author

cirdan wrote:

Rebased against r11818, to make up for the new handling of EXPENSES_*.

I still don't know if the comments I added are enough or more are needed...

Also, could someone change the 'reported version' from 0.5.3 to 'devel'? I failed to set it properly in the original post.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3240

@DorpsGek
Copy link
Member Author

cirdan wrote:

I've updated the patch to fix a nasty bug that would crash the game if an airport was upgraded with CTRL pressed (to build an airport adjacent to another).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3301

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 3, 2008

cirdan wrote:

Rebased against 0.6.0.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3827

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 4, 2008

SmatZ wrote:

When you move the airport, the sign doesn't move

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3828

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 5, 2008

cirdan wrote:

Oops, good catch. Fixed, and updated against r12565.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment3830

@DorpsGek
Copy link
Member Author

cirdan wrote:

I've updated the patch against r13232, per a request in the forums, and I thought that I could as well post it here, too.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment4181

@DorpsGek
Copy link
Member Author

DorpsGek commented Aug 9, 2008

cirdan wrote:

Rebased against r14013.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment4573

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 4, 2008

cirdan wrote:

Rebased against r14432.

May I ask what the devs' opinion about this patch is? I have been keeping it up to date for nearly a year now, and periodically posting it here, but I still do not know if you just do not like it, if it has a chance but there are issues with it, if it happens to touch things in the code which are currently unsettled... Any comment would be appreciated.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment4813

@DorpsGek
Copy link
Member Author

dejv wrote:

Wiki says this feature is 90 % complete:
http://wiki.openttd.org/Requested_features# Stations_and_Depots
What needs to be done to merge the patch to trunk?


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment7133

@DorpsGek
Copy link
Member Author

Alberth wrote:

#1292 (Out-of-service stations (proposal)) is a more generic description of closing & upgrading stations in general


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment10208

@DorpsGek
Copy link
Member Author

cirdan wrote:

Err, sorry, no, this patch is completely independent of #1292. I think you have posted the same comment in #1497 and here without noting the difference.

#1497 is indeed similar to (or a particular case of) #1292. This patch, on the other hand, addresses a different problem, namely that of atomically overbuilding an (empty) airport, and has nothing to do with routing vehicles and the like. Think of it as follows: To upgrade a station (road, rail or airport--docks are different since ships do not enter the dock tiles), first you must ensure that there are no vehicles on the station, then you do the actual replacing. The first part is where airport closure or out-of-service stations help, to tell vehicles to "stay away" from the station (temporarily or permanently) so that they do not interfere. But this patch addresses the second part, removing and rebuilding the station, by making the operation atomic and more UI-friendly (fewer clicks, less hassle). In fact, this patch brings airports in line with road and rail stations, which can already be overbuilt in the current code (this makes me wonder if airports are so special that they do not deserve this feature...).

Anyway, now that someone is actually looking at this, can I please have this patch reviewed? I am providing an up-to-date patch which should apply against current trunk, and I can also provide the same patch split into three smaller patches if desired (this is the way I maintain it).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment10217

@DorpsGek
Copy link
Member Author

Alberth wrote:

My main motivation for posting issue #1292 in both your issues is to make sure all related issues can be found easily.
I do understand that the three issues aim for different things (otherwise I'd merged them).

Unfortunately, I am not enough familiar with airport buy/sell code to decide about your patch at code level.
However, I can see some other potential problems.

- This patch assumes the airport is empty. In a normal game that never happens. As such, the current patch does not provide an upgrade path for a user.

- Adding the 'close airports' patch #1497 would give the user a more useful way. That however means that the 'close airports' patch needs to be added to trunk before this one can be considered.
- The 'close airports' patch to me does not go far enough. I don't see a convincing reason to not allow closing of other types of station, even if they are easier to modify while in use. Other devs may disagree here.

- In the NewGRF airports proposal, a different way described to upgrade. That means a decision needs to be made first which of both proposals is better.


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment10218

@DorpsGek
Copy link
Member Author

cirdan wrote:

  • This patch assumes the airport is empty.

I am not sure I understand what you mean by "assumes the airport is empty". If you mean that the patch blindly upgrades an airport without checking whether there are aircraft on it or not, this is not the case. If you mean that the patch is only useful when the airport is empty, then that is evidently true: The aim of the patch is to simplify the double user action "demolish old airport+build new airport" into a single one, and to demolish an airport it has to be empty. There is no semantic change with this patch.

In a normal game that never happens.

In my games it does happen... In fact, I cannot see another way of turning an existing airport into a better one at the same station.

As such, the current patch does not provide an upgrade path for a user.

Right. The patch does not provide an upgrade path, it simplifies the existing one. You may claim that this is not a big win, but it is a win nevertheless. And I remember reading several posts in the forum asking precisely for this simplification.

  • Adding the 'close airports' patch patch: close airports #1497 would give the user a more useful way. [...]

  • The 'close airports' patch to me does not go far enough. [...]

You seem to think that this patch is only useful if the airport-close patch is applied first, but I have to disagree. While the airport-close patch greatly enhances its usefulness, this patch remains useful (albeit not as much) on its own, for the reasons above. Plus it makes airports behave like road and rail stations, which can already be overbuilt. Apparently, in #1497, coherency with other station types was important...

  • In the NewGRF airports proposal, a different way described to upgrade. That means a decision needs to be made first which of both proposals is better.

Why? Whichever proposal is better, why cannot we have both? The airport GRF spec is currently... vague, to say the least, about overbuilding airports. It gives some hints about what should happen to aircraft, but does not address all possible issues. Will it be possible to overbuild an airport in such a way that a terminal building ends up right on top of a currently loading airplane? As I see it, the upgrade spec can only make sense for certain overlap configurations, at best, while this patch would apply to the demolish-the-whole-thing-and-start-over case (at the cost of requiring the airport to be empty). They would complement each other rather nicely, in fact.

Perhaps part of the problem is that I called this patch "airport upgrade" instead of "airport overbuild", which would better convey what it does...


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562#comment10222

@DorpsGek
Copy link
Member Author

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).


This comment was imported from FlySpray: https://bugs.openttd.org/task/1562

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 2018
@giordy
Copy link

giordy commented Mar 14, 2020

I'm writing to support the inclusion of this patch, if possible. It would make dealing with airports fairly easier and consistent with the other stations.

I hope that sooner or later this can be worked into the game.

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

2 participants