Timetable Improvement Patch

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

ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Ok, to formulate the problem in a more concise manner:

Apparently, in order related code (especially VehicleEnterDepot), at least for road vehicles, when I do a

Code: Select all

Order *curr_order = v->GetOrder(v->cur_real_order_index);
... I get a curr_order with partly different properties than v->current_order.

Thus, there must be somewhere code that creates a copy of an order, and makes v->current_order point to that order copy.

Questions that arise:

(1) Why? Is this necessary to keep network synchronized?
(2) Where is that code? I didn´t find it so far.

PS: I did those tests on a simple order list, with orders 1: Go to station Foo, 2: Go to station Bar, 3: Go to Depot; so I doubt that implicit orders are the answer.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Fixed.

The bug was, that I missed the fact that current_order.arrival/departure (or in the old concept, current_order.wait_time / travel_time) is *not* controlled via order_sl, but via vehicle_sl.

I.e.

Code: Select all

 SLE_CONDVAR(Vehicle, current_order.arrival,        SLE_INT32,   TIP_SAVEGAME_VERSION, SL_MAX_VERSION),
Thus, here is the next release.
Note: This release is savegame-incompatible with earlier versions of the Timetable Improvement Patch.

If you have TIP-based games you want to continue, please keep the old version for this purpose. The only new feature in this version is that waiting in depots now works.

Please test, and report wether it works for you, especially with respect to waiting in depots.
Attachments
stip_v18.zip
Version of stip, savegame-incompatible with earlier versions of the patch, with support for waiting in depots.
(164.05 KiB) Downloaded 152 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

A small bugfix release.

First, what I learnt yesterday helped me in getting rid of something which turned out to be a workaround.

Second, hopefully fixed the issue related to "header_height = -1" causing a compiler error on some systems (was reported via PM).
Attachments
stip_v19.zip
(163.98 KiB) Downloaded 166 times
User avatar
le_harv
Engineer
Engineer
Posts: 83
Joined: 27 Sep 2014 05:16

Re: Timetable Improvement Patch

Post by le_harv »

I am having some trouble compiling STIP19 with the current Trunk R27403. ic111 (the patch author) has been very kind and has helped me work through some compiling issues but we have run into a wall at the current issue so I am hoping for some help from the forum. Upon compiling I run into the following errors:
Error 4 error LNK2001: unresolved external symbol "void __cdecl ShowDepotTimetableWindow(unsigned short,bool,bool,enum VehicleType)" (?ShowDepotTimetableWindow@@YAXG_N0W4VehicleType@@@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\depot_gui.obj openttd

Error 5 error LNK2001: unresolved external symbol "void __cdecl ShowStationTimetableWindow(unsigned short,bool,bool)" (?ShowStationTimetableWindow@@YAXG_N0@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\station_gui.obj openttd

Error 6 error LNK2001: unresolved external symbol "void __cdecl ShowSetDateFastWindow(struct Window *,int,int,int,int,char const *,int const * const,unsigned short const * const,void (__cdecl*)(struct Window *,int,bool))" (?ShowSetDateFastWindow@@YAXPEAUWindow@@HHHHPEBDQEBHQEBGP6AX0H_N@Z@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\timetable_gui.obj openttd

Error 7 error LNK2001: unresolved external symbol "void __cdecl UpdateSetDateFastWindow(int,char const *,void (__cdecl*)(struct Window *,int,bool))" (?UpdateSetDateFastWindow@@YAXHPEBDP6AXPEAUWindow@@H_N@Z@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\timetable_gui.obj openttd

Error 8 error LNK2001: unresolved external symbol "void __cdecl ShowSetDurationWindow(struct Window *,int,struct Duration,unsigned short,void (__cdecl*)(struct Window const *,struct Duration))" (?ShowSetDurationWindow@@YAXPEAUWindow@@HUDuration@@GP6AXPEBU1@1@Z@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\timetable_gui.obj openttd

Error 9 error LNK2001: unresolved external symbol "void __cdecl ShowMoveTimetableWindow(struct Window *,int,int,unsigned short,unsigned short,void (__cdecl*)(struct Window const *,struct Duration,int,unsigned short,unsigned short))" (?ShowMoveTimetableWindow@@YAXPEAUWindow@@HHGGP6AXPEBU1@UDuration@@HGG@Z@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\timetable_gui.obj openttd

Error 10 error LNK2001: unresolved external symbol "void __cdecl ShowWaypointTimetableWindow(unsigned short,bool,bool)" (?ShowWaypointTimetableWindow@@YAXG_N0@Z) C:\Users\Lisa\Documents\OTTDTIP\Trunk\projects\waypoint_gui.obj openttd
I am a bit of a novice but I have been able to set up my environment and compile trunk without the patch following the appropriate guides on the wiki. My environment is MSVS Express 2012 v11.0.6.1219.00 Update 5 which I am also using to compile. I am using TortoiseSVN 1.9.0 to apply patches.

Any help would be most appreciated.
Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: Timetable Improvement Patch

Post by Eddi »

these typically mean that you forgot to compile a file. either you didn't add it to source.list, or you didn't run projects/generate[.vbs]
User avatar
le_harv
Engineer
Engineer
Posts: 83
Joined: 27 Sep 2014 05:16

Re: Timetable Improvement Patch

Post by le_harv »

Eddi wrote:didn't run projects/generate[.vbs]
Thank you, this looks to have solved the problem. I have a working copy with the patch to test.
User avatar
le_harv
Engineer
Engineer
Posts: 83
Joined: 27 Sep 2014 05:16

Re: Timetable Improvement Patch

Post by le_harv »

So I have been enjoying using this a lot since successfully compiling it and it is pretty flawless. In terms of user experience testing I have just two things to share, I've attached two gifs that animate the issue to hopefully show what I mean a little better:

Picking Dates for Time in Depot

When setting the arrival and departure dates for a station you can use the "choose and next" button to set the departure date after the arrival date. With depots clicking choose and next sets the departure date the same as the arrival date meaning to actually set waiting time in the depot you have to click back to the depot and set the departure date on its own.

Imperial Measurements

Imperial units seem to be broken. Even though its set to MPH, when entering a speed limit for your vehicle it still treats the input as KPH so trying to set 100 mph gets switched to 62 mph which would make sense if I was inputting KPH. I appreciate that KPH is used in most places but I think the game was built around MPH as a unit.

Hope that helps out. Great work!
Attachments
Pick Dates.gif
Pick Dates.gif (19.29 KiB) Viewed 7299 times
Imperial.gif
Imperial.gif (31.61 KiB) Viewed 7299 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

le_harv wrote: When setting the arrival and departure dates for a station you can use the "choose and next" button to set the departure date after the arrival date. With depots clicking choose and next sets the departure date the same as the arrival date meaning to actually set waiting time in the depot you have to click back to the depot and set the departure date on its own.
Yes, I didn´t change the original behaviour for depots so far. Basically, I have two options:

(1) The present behaviour, one chooses the arrival date, and it is automatically used as departure date as well for depots.
(2) Behaviour like for stations, but with default stay time zero days. I.e. it would present you the departure chooser, with the just chosen arrival as default value, which you can then alter freely as you already can for stations)

(I assume that the case, don´t spend any extra time in depots, is the more frequent one)

Which one is better / produces less clicks for the players in total, I don´t know.
Imperial units seem to be broken. Even though its set to MPH, when entering a speed limit for your vehicle it still treats the input as KPH so trying to set 100 mph gets switched to 62 mph which would make sense if I was inputting KPH. I appreciate that KPH is used in most places but I think the game was built around MPH as a unit.
This might very well be the case, thanks for the bug report.
User avatar
le_harv
Engineer
Engineer
Posts: 83
Joined: 27 Sep 2014 05:16

Re: Timetable Improvement Patch

Post by le_harv »

Which one is better / produces less clicks for the players in total, I don´t know.
Good question, I hadn't considered all the different user scenarios.
This might very well be the case, thanks for the bug report.
No problem, I think I have found something else unusual. I have mainly been playing around with trains so I started to try out some road vehicles. The road depots aren't quite working correctly. I tried recreating the issues in vanilla v1.5.2 but can't so I think it might be the patch. I took out all NewGRF's to eliminate them from testing too. This bug is a little trickier to explain because the behaviour is a little more random. I have experienced the following but inconsistently and not all the time:
  • Road vehicle is built in road depot but does not appear in the depot vehicle list. Clicking on the depot in the game will open the vehicle window but the vehicle is still invisible
  • Related... the same vehicle is unable to be refit and the refit icon is replaced with the u-turn icon
  • Cloned vehicles also experience the same issues
  • Vehicles sent to the depot are not visible
  • Road vehicles drive into depot but do not leave again even though there is no delay timetabled
I have attempted to GIF the issues but like I said I can't seem to find a pattern or narrow down the steps to recreate the bug. You can see in the first gif the problem happens right away but in the second it's not until I attempt to clone the vehicle. Apologies on the size of the files.
Attachments
Missing Vehicle 2.gif
Missing Vehicle 2.gif (2.78 MiB) Viewed 986 times
Missing vehicle.gif
(3.76 MiB) Not downloaded yet
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Strange, especially since I don't remember me changing the depot view (or did I?).

Smells like a debugging session...

Nevertheless, I doubt that I will do something about this in the short time, I am currently trying to finish the rivers patch, and that's more than enough work...

For now, thanks for the detailed bug report.
User avatar
le_harv
Engineer
Engineer
Posts: 83
Joined: 27 Sep 2014 05:16

Re: Timetable Improvement Patch

Post by le_harv »

ic111 wrote:Strange, especially since I don't remember me changing the depot view (or did I?).

Smells like a debugging session...

Nevertheless, I doubt that I will do something about this in the short time, I am currently trying to finish the rivers patch, and that's more than enough work...

For now, thanks for the detailed bug report.
No worries, glad I can help in some way. I'm enjoying using the copy I have. I do seem to recall that the Depot GUI is modified to have an extra button to show timetables.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

A small release to make the patch queue apply against trunk again.
Attachments
stip_v20.zip
(164.13 KiB) Downloaded 141 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Bugs / change requests according to chat conversation with LordAro:

- Prevent length zero of timetable, it´s senseless and triggers and endless loop
- Don´t make the timetable part of the window the default
- Maybe move code from vehicle_base.h to cpp, at least I cannot answer the question why it should be there currently...
- Minimum size of timetable window is far too wide

Regarding opening the timetable window: There is a setting "Open timetable view (with orders, arrivals, departures) by default"; so this request is just about which value the setting takes by default.

IMHO, offering the setting is quite senseful, as the probability that a player that preferred to open (Order|Timetable) window this time wants to open the same window next time is high.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

A new release dealing with the points mentioned by LordAro:
- Prevent length zero of timetable, it´s senseless and triggers and endless loop
The GUI should prevent entering length zero now. Additionally, I added an assertion before the place where the endless loop occurs if length is zero.
- Don´t make the timetable part of the window the default
There already is a setting that defines, which of the two windows is opened by default if the user hits the button. I changed the default value of that setting to open the order window by default.
- Maybe move code from vehicle_base.h to cpp, at least I cannot answer the question why it should be there currently...
Done.
- Minimum size of timetable window is far too wide
Didn´t do anything with this one yet, as I realized that especially the first line of the window (start / offset / length / delay) needs approximately the present length, and the order window seems to have quite a big minimum size either.

So, maybe I can gain some space by changing some of the labels (which on the other hand triggers trouble if a translation is longer than the english term), or I allow minimizing the window to a size where not all labels can be displayed anymore - but the latter option needs a somewhat deeper view into that GUI layout I suppose.
Attachments
stip_v21.zip
v21 of the Timetable Improvement Patch.
(326.87 KiB) Downloaded 124 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

A screenshot of the timetable window as implemented currently by the patch.
TimetableWindow.png
Screenshot of patched timetable window
(118.52 KiB) Not downloaded yet
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

A new version with some changes, to allow for smaller sizes of the timetable window.

- I removed most of the SetMinimalSize occurrences in the nested tree
- I removed two senseless occurrences of NC_EQUALSIZE
- UpdateWidgetSize does no longer set the minimum width of the summary panel
- Made the summary string in the first line of the timetable window somewhat shorter. It now writes "Offset" instead of "Vehicle Offset" and "Length" instead of "Timetable length". However, I don´t want to make that line too short, as it describes a very central idea of the patched timetable system, and IMHO the design of that window should help people in understanding that idea. At least, the width of the window doesn´t depend on the width of that line, it just ends with "..." if necessary.

Unfortunately, the effect of the above changes is quite limited. According to code like this in widget.cpp (NWidgetLeaf::SetupSmallestSize)

Code: Select all

		case WWT_TEXTBTN:
		case WWT_PUSHTXTBTN:
		case WWT_TEXTBTN_2: {
			static const Dimension extra = {WD_FRAMERECT_LEFT + WD_FRAMERECT_RIGHT,  WD_FRAMERECT_TOP + WD_FRAMERECT_BOTTOM};
			padding = &extra;
			if (this->index >= 0) w->SetStringParameters(this->index);
			Dimension d2 = GetStringBoundingBox(this->widget_data);
			d2.width += extra.width;
			d2.height += extra.height;
			size = maxdim(size, d2);
			break;
		}
where d2 is the calculated size of the button label, the minimum size is determined in a non-overridable way by the label of the button. If I use SetMinimalSize, the input of that function (size at the beginning of the code snippet above) is changed, but it does *not* influence the output, if d2.width > size.width.

For dropdowns, some lines below, similar code exists. In UpdateWidgetSize, I have (probably more or less copied from the original order window code) code that calculates the maximum size of any label of the dropdown, and uses this one as minimum size.

So, for me it looks like that there exists the hard rule, that no button and no dropdown label may overflow (ending in "..."). And as quite a lot of the button labels are well-known texts present also in trunk, I don´t think trying to shorten them would be senseful.

Conclusion: I don´t see what else I could sensefully do here.
Attachments
stip_v22.zip
(332.46 KiB) Downloaded 118 times
Alberth
OpenTTD Developer
OpenTTD Developer
Posts: 4763
Joined: 09 Sep 2007 05:03
Location: home

Re: Timetable Improvement Patch

Post by Alberth »

The idea behind the dimension calculations is that the text is what you want to show to your users. The widgets adjust themselves so it is clearly visible. Showing that text only partly would defeat the goal of showing the text.

Nobody claimed that Gui design is easy!


If you think the text is less important than the window size (which your description seems to suggest), perhaps the string shouldn't be so prominently visible. Guis don't tend to explain relations between the fields with a long string of text.

Excel doesn't have text explaining how one cell relates to another cell, or we don't explain in text that you need to setup the newgrf settings in the main menu before you press new-game. Obviously, our current main menu is very broken from a Gui point of view, our new-game should have a flow where you run into the newgrf settings before you reach the point of generating the world, instead. Still, no text explaining any of this.

If you feel you must explain it, perhaps
1. A tooltip somewhere explaining the relation
2. A "help" window (or area), like the settings window
3. A wiki page explaining time table in full detail (much more space than you could ever do in-game)
Being a retired OpenTTD developer does not mean I know what I am doing.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Alberth wrote: If you think the text is less important than the window size (which your description seems to suggest),
Actually, the start of this discussion was the feedback of LordAro, that the width of the window is far too big. So I tried to make it smaller.

But you are right, the default should be that all the text is visible.
perhaps the string shouldn't be so prominently visible. Guis don't tend to explain relations between the fields with a long string of text.

Excel doesn't have text explaining how one cell relates to another cell, or we don't explain in text that you need to setup the newgrf settings in the main menu before you press new-game.
Ok, then lets have a look at it in detail:
Version until yesterday: "Timetable start 1. Jan 1905 + Vehicle Offset 1 month = Vehicle start 1. Feb 1905 Timetable length 2 months"
Version now: "Timetable start 1. Jan 1905 + Offset 1 month = Vehicle start 1. Feb 1905 Length 2 months"

The relations I use for explaining are just the math operators "+" and "=", so IMHO they are not a big problem, the space they occupy is quite small. And if I would omit them, I would need to add some space between the different labels anyway.

Maybe, however, I could distribute things among those two lines in a different way, e.g.

First line: Timetable start 1. Jan 1905 Length 2 months Name R17
Second line: Vehicle start 1. Feb 1905 Offset 1 month <Autofill information if autofill is active>

(where an active autofill might increase the space somewhat (though less than currently implemented), but as an active autofill is a quite seldom case that temporary width increase might be more acceptable)
If you feel you must explain it, perhaps
1. A tooltip somewhere explaining the relation
2. A "help" window (or area), like the settings window
3. A wiki page explaining time table in full detail (much more space than you could ever do in-game)
Actually, at least (1) and (3) are needed anyway; (2) possible maybe in a separate window; for a separate area carrying static text, I probably don't have the space in that window. But I personally would give (1) and (3) priority.

My hope is / was just, that if I am able to present the information in that window understandable enough, I might be able to reduce the need to read that more extensive information significantly. I mean, IMHO, tooltips or external information is good, but if a GUI makes you understand how things work without the need to read that information, it is even better.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Reorganized the timetable start / offset / length information on top of the window. See screenshots in the next post.

(Disclaimer: I did not yet address all issues caused by that reorganizsation)
Attachments
stip_v23.zip
(333.29 KiB) Downloaded 119 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Timetable Improvement Patch

Post by ic111 »

Three screenshots showing how the patched timetable window currently looks like.

In the top area, I moved the Vehicle start to the second line, and also explicitely added the end date (so you don´t need to calculate yourself "1st March plus 16 months is...?").

As already noted in todays IRC discussion, I have implemented three buttons "Full", "Dest", "Time" which control which kind of information the timetable window shows.

Full shows both the well-known order information, and the timetable information:
TimetableFull.png
TimetableFull.png (12.46 KiB) Viewed 986 times
Dest shows only the order information:
TimetableDest.png
TimetableDest.png (10.42 KiB) Viewed 986 times
Time shows only destinations and timetable information. Arrivals and departures are shown as table.
TimetableTime.png
TimetableTime.png (12.4 KiB) Viewed 986 times
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: Google Adsense [Bot], Semrush [Bot] and 9 guests