[Patch] Improved Build Station GUI [r19159]

[planetmaker]

The patch suplied seems to look for english.txt under src/a/lang/english.txt which doesn't exist.
Edit: I am trying to patch on Windows 7 with Tortoise Merge.

Maybe it allows to set the dir hirarchy or 'patch level' (I know it's wrong word, what's the correct one) to which to apply the patch. This patch applies with -p1 (default for hg / git) instead of -p0 (svn).

[Thief^]

I don't understand why people use patch files which don't include any kind of source control changelist ID. They're so much harder to apply than one that specifies what version of the file(s) it was created from.

[scoz]

I believe its a mercurial patch, so it has the changeset info just not for subversion.

[Thief^]

So it does, I missed the id first time

[Ammler]

Maybe it helps if you add the prefix level and the tool you used to the filename, something like we did for is2: http://bundles.openttdcoop.org/is2/trunk-patches/

[maquinista]

Thanks for update this patch. Tooltips should be used also in the window with way-points.

[WWTBAM]

I finaly got the patch to apply but MSVC Pro will not compile it. The errors I get are:

Error 1 error C2059: syntax error : '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1033 openttd
Error 2 error C2143: syntax error : missing ';' before '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1033 openttd
Error 3 error C2143: syntax error : missing ';' before '}' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1033 openttd
Error 4 error C2059: syntax error : '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1072 openttd
Error 5 error C2143: syntax error : missing ';' before '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1072 openttd
Error 6 error C2143: syntax error : missing ';' before '}' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1072 openttd
Error 7 error C2059: syntax error : '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1146 openttd
Error 8 error C2143: syntax error : missing ';' before '{' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1146 openttd
Error 9 error C2143: syntax error : missing ';' before '}' c:\Users\Leo.Giusti\Documents\Visual Studio 2008\Projects\OpenTTD\r19145\src\rail_gui.cpp 1146 openttd

[sbr]

[...] not having the time to learn the nested widgets system [...]

I learned the basics of nested widgets while working on this patch.

I'll update the first post [...]

Thank you. Glad to hear that the work done on this patch pleased you.

Maybe it helps if you add the prefix level and the tool you used to the filename, something like we did for is2: [url]http://bundles.openttdcoop.org/is2/trunk-patches/[url]

I use mercurial and should have mentioned it. I've done a subversion checkout and will provide patch files for '-p1' and svn users.

I finaly got the patch to apply but MSVC Pro will not compile it. [...]

Seems like MSVC don't like multi-field assignments to struct variables, something that my gcc (4.4.3-20100108) doesn't complain about. I think it's fixed now but can't/will not test it on Windows.


I've found and fixed another bug: with an height-increased window, the station coverage area resize down also reduced size of the whole window.
The version number is bumped to 5.1 (considering that v3-sbr1 is v5 as scoz previously released a v4).

[Ammler]

He nice work.

But we generate the patches automatically from the repo, that is why we provide both versions. If you make them manually, one version should be fine, just mention it in the filename, so it is obviously how to patch (-p0 or -p1, a svn version is nice for the Tortoise users, as those can't patch with hg/git diffs)

--> so if you make a svn patch, that is fine for all, imo

Greets
Ammler

[WWTBAM]

MSVC is happy now and builds it fine.

[sbr]

Please find an updated version of this patch (r19389):

• Unified rail station widget descriptions (r19321)
• No more parameter setting: the current newstations gui is totally replaced
• Fix: Ensure that the last selected station's class/type are visible over gui invocation

[sbr]

A new update against r19396:

• Don't introduce a new widget for 'station coverage area' texts.
• Simplify code for restoring window size: don't depend of previous window size but on previous station types matrix rows/columns count. That allow to restore window size if 'line height' change between two gui invocation.
• Break up patch into smaller parts.

[Terkhen]

I have been checking the patch list. I don't know much about station NewGRFs or the complicated parts of the GUI, so I might be wrong at some details.

Before 1), the new station class list was checked each time it was required. After 1), the list is generated once and used until the window is closed. Generating the list once is more efficient, but maybe it was done every time because something can make this list change over time. If this is not the case, I think the new station should be generated only once too.

This comment in 2) could go directly in 1):

Code: Select all

+	uint line_height; ///< Height of a single line in the newstation classes and types selection matrices 
+	                  ///  (#BRSW_NEWST_STATION_CLASSES_LIST & #BRSW_NEWST_STATION_TYPES_LIST widgets).

After 5), I think that the window is complex enough to be made pinnable.

[Kogut]

Maybe it changes after changing newgrfs ingame (yes that option with big, red, warning box).

[sbr]

I have been checking the patch list. I don't know much about station NewGRFs or the complicated parts of the GUI, so I might be wrong at some details.

Thanks for having took the time to check it; to you and others who have/will do it. I'm (slowly) learning about the OpenTTD codebase while developing patches and should certainly miss a lot of its subtleties.

Before 1), the new station class list was checked each time it was required. After 1), the list is generated once and used until the window is closed. Generating the list once is more efficient, but maybe it was done every time because something can make this list change over time. If this is not the case, I think the new station should be generated only once too.

Maybe it changes after changing newgrfs ingame (yes that option with big, red, warning box).

The previous versions made by scoz already cached the station class list. When updating the code I should admit that I've not investigate implications of that choice.

I've looked deeper in the NewGRF loading code after reading your comments. As I understand it, station classes are stored in 'newgrf_station.cpp:_station_classes'. That array is fill when generating world, loading game/scenario or changing the NewGRF list.

In the first two cases, the BuildRailStation window can't exists. In the latter case, the whole screen is marked as dirty. If the BuildRailStation window is loaded, it will be repaint but not reinitialized. The cached station classes of the patched window isn't updated and the window is repainted with invalid data. In contrary, the actual window (and since its introduction in r4768) will correctly reflect all in-game's NewGRF changes.

The default behavior should be preserved either by refreshing the station classes cache on NewGRF changes or by not caching them anymore. I'll investigate these two options.

This comment in 2) could go directly in 1):

Code: Select all

+	uint line_height; ///< Height of a single line in the newstation classes and types selection matrices 
+	                  ///  (#BRSW_NEWST_STATION_CLASSES_LIST & #BRSW_NEWST_STATION_TYPES_LIST widgets).

Between '01' and '02' this comment differs by two things:

• one line comment in '01', two line in '02'
• '#BRSW_NEWST_LIST' in '01', '#BRSW_NEWST_STATION_TYPES_LIST' in '02'

I'll write in on two line also in '01'.

After 5), I think that the window is complex enough to be made pinnable.

Why not, that could be useful to draw eye-candy tiles while building, for example, a road complex. I need to examine the interactions with the window when the selected rail type/transportation type change.

[Terkhen]

About the NewGRFs, I was referring to callbacks. I don't know if rail stations use callbacks, though.

Why not, that could be useful to draw eye-candy tiles while building, for example, a road complex. I need to examine the interactions with the window when the selected rail type/transportation type change.

I meant pinning only the build rail station window, but I like this idea. But since it is complex and not directly related to this feature, you should implement it separately.

[sbr]

I've done a little bit more research about newstations in NewGRF and the OpenTTD code that deal with it. However, to anyone who's more skilled on this subject, feel free to correct me if I'm making wrong assertions.

From the NewGRF spec:

• The station class ID is defined by an Action 0 / Feature 04 / Property 08.
• The station class name is defined by one Action 4 / Feature 04 per language.
• There can be no more than 16 station class defined.

About callbacks:

I don't know if rail stations use callbacks, though.

In newgrf_callbacks.h I've found the following callbacks referable to stations:

Code: Select all

41:      CBID_STATION_AVAILABILITY            = 0x13, // 8 bit callback
44:      CBID_STATION_SPRITE_LAYOUT           = 0x14,
94:      CBID_STATION_TILE_LAYOUT             = 0x24, // 15 bit callback
177:     CBID_STATION_ANIM_START_STOP         = 0x140, // 15 bit callback
180:     CBID_STATION_ANIM_NEXT_FRAME         = 0x141, // 15 bit callback
183:     CBID_STATION_ANIMATION_SPEED         = 0x142, // 8 bit callback
204:     CBID_STATION_LAND_SLOPE_CHECK        = 0x149, // 15 bit callback, not implemented

All these callbacks refers to station types. Only CB 13 is of interest for the BuildRailStationWindow. CB 13 is checked, in both the trunk and patched version, inside DrawWidget() and OnClick().

How OpenTTD handle station classes:

• The station classes are stored in 'static StationClass _station_classes[STAT_CLASS_MAX]'.
• This array is indexed by StationClassID vars.
• There can be no more than 32 (STAT_CLASS_MAX) station class defined.
• The station class ID defined in the NewGRF is stored in StationClass.id.
• The station class names are mapped to a StringID in StationClass.name.
• Values of the station class array can be accessed by functions defined in newgrf_station.h.
• The station class array can be altered by ResetStationClasses() and AllocateStationClass(...) that are only called when the NewGRFs are (re)loaded.

In the patched BuildRailStationWindow:
The station class list is build inside OnInit() and is rebuild each time the window is reinitialized.
There is three case where BuildRailStationWindow::OnInit() is called:

• when building the window: Window::FinishInitNested(...) → Window::InitializeData(...) → Window::OnInit().
• when changing the used language: GameOptionsWindow::OnDropdownSelect(GOW_LANG_DROPDOWN, ...) → ReInitAllWindows() → Window::ReInit(...) → Window::OnInit().
• when updating the NewGRF list: NewGRFWindow::OnClick(..., SNGRFS_APPLY_CHANGES, ...) → NewGRFConfirmationCallback(...) → ReloadNewGRFData() → LoadStringWidthTable() → ReInitAllWindows() → Window::ReInit(...) → Window::OnInit().

To summarize, OnInit() is, in my opinion, the right place to cache the station class IDs and names list as it'll be reloaded when the cached values changes (in contrary to what I previously wrote).
However station class names are obtained by GetStationClassName() that do an array look up. I don't think it's worth caching them and am tempted to reduce _railstation.station_classes to a simple mapping between matrix position and station class ID.

On a side note, we can use OnInit() to ensure that _railstation.station_class is always valid (both in trunk and in patch):

Code: Select all

virtual void OnInit()
{
    if (_railstation.newstations && _railstation.station_class != STAT_CLASS_DFLT) {
        uint num_station_classes = GetNumStationClasses();
        if (_railstation.station_class >= num_station_classes) {
            _railstation.station_class = (StationClassID) (num_station_classes - 1);
            _railstation.station_type = 0;
        }
    }
}

About pinning the window:

Why not, that could be useful to draw eye-candy tiles while building, for example, a road complex. I need to examine the interactions with the window when the selected rail type/transportation type change.

I meant pinning only the build rail station window, but I like this idea. But since it is complex and not directly related to this feature, you should implement it separately.

and previously

After 5), I think that the window is complex enough to be made pinnable.

For me, inside a running game, pinned windows can only be closed by clicking on the close box or typing Ctrl+Del. Perhaps you were thinking of pinning the BuildRailStationWindow in the context of the rail toolbar?



I'll update patches and post an updated version soon.

[Owen]

You would be best doing the caching inside OnInvalidateData, and then calling InvalidateWindowData or InvalidateWindowClassData to invdlidate it whenever the NewGRF list is updated

[Terkhen]

To summarize, OnInit() is, in my opinion, the right place to cache the station class IDs and names list as it'll be reloaded when the cached values changes (in contrary to what I previously wrote).
However station class names are obtained by GetStationClassName() that do an array look up. I don't think it's worth caching them and am tempted to reduce _railstation.station_classes to a simple mapping between matrix position and station class ID.

I agree: it is not worth to cache the name too. Since it is a small vector with a known maximum size (STAT_CLASS_MAX) I think that a simple array and an int size should be enough to store it.

On a side note, we can use OnInit() to ensure that _railstation.station_class is always valid (both in trunk and in patch):

If I understood it correctly, OnInit always caches the station class array. Since OnInit is called after updating the NewGRF list, shouldn't it always be valid after that?

For me, inside a running game, pinned windows can only be closed by clicking on the close box or typing Ctrl+Del. Perhaps you were thinking of pinning the BuildRailStationWindow in the context of the rail toolbar?.

With pinnable I meant the same as you do, but right now if you change toolbars the build rail station window is always closed (probably even if made pinnable). A different behaviour is not trivial, since it would require to think and implement when it should be closed and when it shouldn't. Because of that, I think it is better to left it out for now.

[sbr]

I agree: it is not worth to cache the name too. Since it is a small vector with a known maximum size (STAT_CLASS_MAX) I think that a simple array and an int size should be enough to store it.

I've replaced the caching by a mapping between station class id and matrix position in a StationClassIDByte array.

You would be best doing the caching inside OnInvalidateData, and then calling InvalidateWindowData or InvalidateWindowClassData to invdlidate it whenever the NewGRF list is updated

The mapping is now done in OnInvalidateData(...) that is explicitly triggered in ReloadNewGRFData(). OnInit() had the advantage to work out-of-the-box but, as you pointed, OnInvalidateData(...) is the right method to use to reflect external changes that impact the window.

On a side note, we can use OnInit() to ensure that _railstation.station_class is always valid (both in trunk and in patch):

If I understood it correctly, OnInit always caches the station class array. Since OnInit is called after updating the NewGRF list, shouldn't it always be valid after that?

'_railstation.station_class' is the currently selected station class. When removing a NewGRF, it can point to a station class that doesn't exists anymore. I now check and update if needed the window settings (_railstation.station_{class, type, count}).

With pinnable I meant the same as you do, but right now if you change toolbars the build rail station window is always closed (probably even if made pinnable). A different behaviour is not trivial, since it would require to think and implement when it should be closed and when it shouldn't. Because of that, I think it is better to left it out for now.

I think too.



Attached is an updated version against r19525:

• Support NewGRF changes while the window is open:
  • always point to valid station class and type, update scrollbars
  • switch between default and newstations window if needed
• Rework small patches numbers:
  • patches 0* applies to the BuildRailStationWindow while keeping the same appearance as in trunk
  • patches 1* defines the new look of the window (station classes in matrix, preview of station types in right matrix)
  • patches 2* enhance the new window (tooltip, resizable, restore size)