[Patch] Improved Build Station GUI [r19159]

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

User avatar
planetmaker
OpenTTD Developer
OpenTTD Developer
Posts: 9408
Joined: 07 Nov 2007 22:44
Location: Sol d

Re: [Patch] Improved Build Station GUI [r19159]

Post by planetmaker »

robotboy wrote: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).

User avatar
Thief^
Route Supervisor
Route Supervisor
Posts: 468
Joined: 10 Oct 2004 00:11

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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.
Melt with the Shadows,
Embrace your destiny...

scoz
Engineer
Engineer
Posts: 19
Joined: 21 Dec 2008 09:22

Re: [Patch] Improved Build Station GUI [r19159]

Post by scoz »

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

User avatar
Thief^
Route Supervisor
Route Supervisor
Posts: 468
Joined: 10 Oct 2004 00:11

Re: [Patch] Improved Build Station GUI [r19159]

Post by Thief^ »

So it does, I missed the id first time :?
Melt with the Shadows,
Embrace your destiny...

User avatar
Ammler
President
President
Posts: 953
Joined: 18 Jun 2006 18:18
Location: Switzerland
Contact:

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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
Tycoon
Tycoon
Posts: 1812
Joined: 10 Jul 2006 00:43
Location: Spain

Re: [Patch] Improved Build Station GUI [r19159]

Post by maquinista »

Thanks for update this patch. Tooltips should be used also in the window with way-points.
Sorry if my english is too poor, I want learn it, but it isn't too easy.[/list][/size]

User avatar
WWTBAM
Moderator
Moderator
Posts: 3682
Joined: 02 Apr 2005 07:01
Location: Sydney NSW Antipodea
Contact:

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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
Formerly known as r0b0t_b0y2003, robotboy, roboboy and beclawat. The best place to get the most recent nightly builds of TTDPatch is: http://roboboy.users.tt-forums.net/TTDPatch/nightlies/

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [v5.1-r19159]

Post by sbr »

scoz wrote:[...] not having the time to learn the nested widgets system [...]
I learned the basics of nested widgets while working on this patch.
scoz wrote:I'll update the first post [...]
Thank you. Glad to hear that the work done on this patch pleased you.
Ammler wrote: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.
robotboy wrote: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).
Attachments
station-build-gui_v5.1-r19159.patch
for subversion users
(29.82 KiB) Downloaded 106 times
station-build-gui_v5.1-hg-ea541d92bafc.patch
against r19159, for '-p1' users
(29.62 KiB) Downloaded 82 times

User avatar
Ammler
President
President
Posts: 953
Joined: 18 Jun 2006 18:18
Location: Switzerland
Contact:

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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

User avatar
WWTBAM
Moderator
Moderator
Posts: 3682
Joined: 02 Apr 2005 07:01
Location: Sydney NSW Antipodea
Contact:

Re: [Patch] Improved Build Station GUI [r19159]

Post by WWTBAM »

MSVC is happy now and builds it fine. :D
Formerly known as r0b0t_b0y2003, robotboy, roboboy and beclawat. The best place to get the most recent nightly builds of TTDPatch is: http://roboboy.users.tt-forums.net/TTDPatch/nightlies/

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [r19389]

Post by 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
Attachments
station-build-gui_v6.0-r19389.patch
for subversion users
(38.85 KiB) Downloaded 202 times
station-build-gui_v6.0-hg-671246bf9a84.patch
against r19389, for '-p1' users
(38.77 KiB) Downloaded 99 times

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [r19396]

Post by 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.
Attachments
station-build-gui_v6.1-r19396.patch
for subversion users
(36.72 KiB) Downloaded 165 times
station-build-gui_v6.1-hg-b753aec66152.patch
against r19396, for '-p1' users
(36.73 KiB) Downloaded 97 times
station-build-gui_v6.1-hg-b753aec66152_small-patches.tar
against r19396, small patches, for '-p1' users
(60 KiB) Downloaded 106 times

Terkhen
OpenTTD Developer
OpenTTD Developer
Posts: 1034
Joined: 11 Sep 2008 07:32
Location: Spain

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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
Tycoon
Tycoon
Posts: 2493
Joined: 26 Aug 2009 06:33
Location: Poland

Re: [Patch] Improved Build Station GUI [r19159]

Post by Kogut »

Maybe it changes after changing newgrfs ingame (yes that option with big, red, warning box).
Correct me If I am wrong - PM me if my English is bad
AIAI - AI for OpenTTD

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [r19396]

Post by sbr »

Terkhen wrote: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.
Terkhen wrote: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.
Kogut wrote: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.
Terkhen wrote: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'.
Terkhen wrote: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
OpenTTD Developer
OpenTTD Developer
Posts: 1034
Joined: 11 Sep 2008 07:32
Location: Spain

Re: [Patch] Improved Build Station GUI [r19396]

Post by Terkhen »

About the NewGRFs, I was referring to callbacks. I don't know if rail stations use callbacks, though.
sbr wrote: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.

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [r19396]

Post by 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: About callbacks:
Terkhen wrote: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: 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: 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:
Terkhen wrote:
sbr wrote: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
Terkhen wrote: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
Engineer
Engineer
Posts: 70
Joined: 23 Aug 2004 15:22

Re: [Patch] Improved Build Station GUI [r19159]

Post by 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
OpenTTD Developer
OpenTTD Developer
Posts: 1034
Joined: 11 Sep 2008 07:32
Location: Spain

Re: [Patch] Improved Build Station GUI [r19396]

Post by Terkhen »

sbr wrote: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.
sbr wrote: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?
sbr wrote: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.

User avatar
sbr
Engineer
Engineer
Posts: 13
Joined: 13 Apr 2008 18:35

Re: [Patch] Improved Build Station GUI [r19525]

Post by sbr »

Terkhen wrote: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.
Owen wrote: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.
Terkhen wrote:
sbr wrote: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}).
Terkhen wrote: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)
Attachments
station-build-gui_v6.2-r19525.patch
for subversion users
(39.56 KiB) Downloaded 142 times
station-build-gui_v6.2-hg-e7ca2b0ec896.patch
against r19525, for '-p1' users
(39.53 KiB) Downloaded 75 times
station-build-gui_v6.2-hg-e7ca2b0ec896_small-patches.tar
against r19525, small patches, for '-p1' users
(80 KiB) Downloaded 129 times

Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 1 guest