SmatZ wrote:

Hello, I think this is useful patch- but I am not sure how problematic it can be with other devs (mainly the new airports maintainers)

There may be some problems (I may be wrong about them!)
code-wise:
CmdOpenCloseAirport() doesn't check whether the station has an airport (may cause problems in future)
InvalidateWindowWidget(WC_STATION_VIEW, st->index, 14); no a good idea, as the widget number may easily change, better invalidate whole window

+!HASBITS(st->airport_flags, AIRPORT_CLOSED_block) &&
+SetWindowWidgetLoweredState(w, 14, st->airport_flags & AIRPORT_CLOSED_block != 0);
+DoCommandP(0, st->index, st->airport_flags & AIRPORT_CLOSED_block ? 0 : 1, NULL, CMD_OPEN_CLOSE_AIRPORT | CMD_MSG(STR_CLOSE_AIRPORT_ERROR));
three ways to do the same thing, may be unified

Fatal problem for me:
I applied it, started a new game, and pressing the button doesn't change anything... the button doesn't get pressed
I loaded some game, there was the button still pressed and couldn't be depressed
(the command was successfully executed)

I am running on amd64, it may be a problem. But I cannot test it any further.

Attachments

• New Brendham Transport, 3rd Jan 1990.sav (88.84 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2823

SmatZ wrote:

I tried compiling on a 32bit machine and the button doesn't work there too.

so... it doesn't work for under these coditions:
gcc 4.2.2 amd64
gcc 3.4.6 amd64
gcc 4.1.2 x86

Maybe I am doing something wrong...

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2824

Belugas wrote:

The concept is not bad, it could be interesting.
But it will not be included in trunk (if ever) for 0.6. It might be integrated after release. I was thinking of a way to close airport after certain events, it looks like you wrote the mechanism for it :)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2828

cirdan wrote:

@smatz:

1. CmdOpenCloseAirport() doesn't check whether the station has an airport

That's true, but harmless. Even airportless stations have an airport_flags field, only that unused. If a modified client wants to fiddle with it, let it be. In fact, it could have some use: to close an airport even before it is built, so that it will start up closed.

2. InvalidateWindowWidget(WC_STATION_VIEW, st->index, 14); no a good idea, as the widget number may easily change, better invalidate whole window

Also true, but src/station_gui.cpp is full of hard-coded widget indices; see for instance StationViewWndProc. And all existing calls to InvalidateWindowWidget in src/station_cmd.cpp also have hard-coded numbers.

3. three ways to do the same thing, may be unified

Point taken, patch updated.

4. I applied it, started a new game, and pressing the button doesn't change anything... the button doesn't get pressed

Well, it did change something, only that not the widget. The airport was closed, but there was no visual indication. Updating the patch for point 3 fixed this.

@Belugas:
I'm not trying to push this patch for 0.6. I'd be happy if it gets in trunk, whenever that may be, but I won't push it too hard anyway. This was a feature that someone suggested in the forums and I decided that it was easy to code, so I made the patch and posted it. Afterwards, someone else asked for it to be included in ChrisIN, and it was, so I thought it could be generally useful.

On the other hand, I take from your words that the patch could actually be considered for inclusion after 0.6? (I don't mean "included", just "considered for inclusion".) That would certainly increase motivation for keeping the patch up to date. :-)

Attachments

• airport-close-v5-r11542.patch (6.71 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2836

SmatZ wrote:

That's true, but harmless. Even airportless stations have an airport_flags field, only that unused. If a modified client wants to fiddle with it, let it be. In fact, it could have some use: to close an airport even before it is built, so that it will start up closed.

It is not a good programming practise, and it may cause problems in future. Also, users may notice strange behaviour are report it as a bug.

Generally, it is not a good thing to have things blocked only in the GUI. It doesn't affect only 'hacked' clients, but also normal clients on laggy networks.

Also true, but src/station_gui.cpp is full of hard-coded widget indices; see for instance StationViewWndProc. And all existing calls to InvalidateWindowWidget in src/station_cmd.cpp also have hard-coded numbers.

These numbers are subject to change. It will likely change when the widgets are enumerized, and that would cause some strange behaviour later.

It is not possible to close only some airports of a station, but it is hard to do something with that...

There is a serious bug:
when destroying closed airport, aircraft just hang in the air. See savegame.

Attachments

• Shordston Transport, 18th Jun 1990.sav (10.72 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2843

Belugas wrote:

Point 1) Agreeing with smatz
Point 2) Widget numbers must be turned into enum, as it is the way to go and as it ease any king of change. And if there are still a lot of those, is that we do not have the time nor the energy to change them all at once. AS code is been touched, those magic numbers are been removed.

Considered for inclusion indeed, as it can be used with a new feature i have in mind and partly started coding, yes.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2845

SmatZ wrote:

There is a serious bug:
when destroying closed airport, aircraft just hang in the air. See savegame.

I am sorry, not your fault, even clean trunk has this problem

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2847

cirdan wrote:

Point 2) Widget numbers must be turned into enum, as it is the way to go and as it ease any king of change. And if there are still a lot of those, is that we do not have the time nor the energy to change them all at once. AS code is been touched, those magic numbers are been removed.

So, is it OK if I send a preliminary patch turning all of the hard-coded widget indices in the station view window into enumeration constants?

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2849

Belugas wrote:

Of course, given that the naming scheme used elsewhere is repected :)
This has a whole lot chance of been include than anything else, apart bug fixes and comments, of course ;)
hint hint hint...

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2851

cirdan wrote:

The attached patch introduces a new enumeration StationWidgets in src/station_gui.cpp, enumerating, well, the widgets in the station window, and changes the corresponding hard-coded widget numbers into enum values.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2867

cirdan wrote:

Oops, it seems that the patch didn't get attached.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2868

cirdan wrote:

All right, it appears that I'm unable to attach the patch.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2869

cirdan wrote:

I'm making another attempt.

Attachments

• station-widgets-r11557.patch (3.77 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2871

SmatZ wrote:

Thanks for your tries.

There are some things missing, refer to http://wiki.openttd.org/index.php/Coding_style

some personal opinions:
try to name StationViewWidgets, it is more accurate

STATION_WIDGET is too long prefix, something like SVW would be nicer

the enum should be documented too, "SWW_CLOSEBOX = 0, ///< Closebox"

the station_view_expanded_widgets struct with widgets should be commented too,
{ WWT_PANEL, RESIZE_NONE, 14, 71, 136, 17, 66, 0x0, STR..}, // BRDW_DEPOT_NE

correct the comment style - where "/** /" "/ */" and "//" is used

it would be nice to try to join _station_view_expanded_widgets and _station_view_widgets , but maybe this is too complex for you

if you would like to use to use these widgets outside station_gui.cpp, then you will need to move it to station.h
but you will poison the global namespace with this enum...

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2872

SmatZ wrote:

I will do the enumerization etc. as it is needed for http://bugs.openttd.org/task/1502

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2886

cirdan wrote:

I've just rebased the patch against r11580. The news are:

- The code should now disallow the airport-closed flag to be toggled for airportless stations, as requested (untested, as it would need a modified client).

- Invalidating the airport close widget now uses a symbolic constant instead of the hard-coded widget index.

- The widget is also invalidated when destroying/building an airport.

Attachments

• airport-close-v6-r11580.patch (7.99 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2893

Belugas wrote:

Looks nice:)
Question : why not using [Set|ClrBit instead for:
+ st->airport_flags |= AIRPORT_CLOSED_block;
+ } else {
+ st->airport_flags &= ~AIRPORT_CLOSED_block;

It would be clearer, more easy to read, wouldn't it?

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2894

cirdan wrote:

Looks nice:)

Thanks.

Question : why not using [Set|ClrBit instead for:

• st->airport_flags |= AIRPORT_CLOSED_block;
• } else {
• st->airport_flags &= ~AIRPORT_CLOSED_block;

Answer: Because there isn't a consistent use of {Set,Clr}Bit throughout the code, so it's difficult to decide what the preference is.

It would be clearer, more easy to read, wouldn't it?

Ease to read is in the eye of the beholder. I'd probably choose |= and &= rather than macros; even better, I would declare those variables as bitfields, although somehow bitfields don't have many supporters. But my position isn't too strong, either, so let's just go with the flow...

The attached patch uses {SET,CLR}BITS instead of direct bit manipulation.

Attachments

• airport-close-v6-r11588.patch (8.00 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2901

Belugas wrote:

Good. It is kept noticed and saved.
As soon as time permits it, i'll include it in my feature stuff to be done after 0.6
Thanls

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment2954

cirdan wrote:

Rebased against r11818 (to keep it up to date).

Attachments

• airport-close-v6-r11818.patch (8.01 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment3239

cirdan wrote:

Rebased against 0.6.0.

Attachments

• airport-close-v6-0.6.0.patch (9.82 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment3826

cirdan wrote:

Rebased against r13232.

Attachments

• airport-close-v6-r13232.patch (9.91 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment4183

SmatZ wrote:

It would be nice if any station could be closed for given type of transport separately.
There is a design problem with that:
trains, road vehicles and ships should just skip that order, but aircraft should fly around and wait?
I am not sure how to do that. Also, the GUI part is a question. Ctrl+click on small icon at the bottom right of the window to close station for given meaning of transport?

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment4362

cirdan wrote:

It would be nice if any station could be closed for given type of transport separately.
There is a design problem with that:
trains, road vehicles and ships should just skip that order, but aircraft should fly around and wait?

Yes, I also think that it would be nice. And no, I don't have a solution for the design problem, either. :-(

The fact is that the ability to close airports was asked for in the forums (and I know that the patch has seen some success since then), but the possibility of closing other station types was only raised once in the discussion about this patch, and not too strongly.

I think that "closing" an airport in the sense of preventing aircraft from landing is different from "closing" a station in the sense of making vehicles skip the station. The former is done, mainly, to replace the station (with a larger one, for instance), and airports are a notable source of trouble at that (a busy airport may never become clear). On the other hand, you never need to upgrade bus or lorry stations, or docks for that matter, and anyway they are easier to vacate. Train stations are similar to airports in that they may need upgrading (to monorail etc.) but trains are usually sent to depots beforehand, as they themselves also need un upgrade (unlike aircraft).

To sum up, "closing" an airport, as it is now, is more of a temporary measure to upgrade it with the minimum possible traffic disruption, and I see it as something different from making vehicles skip the station (you don't want an aircraft with only two stations in the schedule to return to its source airport just because you're about to upgrade its destination).

I am not sure how to do that. Also, the GUI part is a question. Ctrl+click on small icon at the bottom right of the window to close station for given meaning of transport?

My first patch (see http://www.tt-forums.net/viewtopic.php?t=33418 ) originally did that, but it was generally disliked, so I switched to the current implementation. I am, of course, open to new ideas and suggestions.

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment4384

cirdan wrote:

Rebased against r14013.

Attachments

• airport-close-v6-r14013.patch (10.07 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment4572

cirdan wrote:

Rebased against r14432.

Attachments

• airport-close-v6-r14432.patch (10.07 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment4812

planetmaker wrote:

small update: "close airport" is only shown, when the station is actually an airport.

I'm not sure whether I should have declared a whole new window type for this purpose. I didn't and changed the station_view window depending on the presence of an airport.

Based on the current gui layout, it could then be envisioned to space the close station for bus / trucks / trains / planes / ships in the space which is currently occupied by the close airport button.

Attachments

• close_airport-v7-r14640.diff (9.42 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment5077

cirdan wrote:

The attached patch is a new version, which hides the 'close airport' button, instead of disabling it, if the station lacks an airport. (The last patch by planetmaker can trigger an assertion if an airport is added to a station while its window is open.)

Attachments

• airport-close-v7-r14648.patch (8.53 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/1497#comment5085

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/1497#comment10209

michi_cc closed the ticket.

Reason for closing: Implemented

In r24127.

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