Transport Tycoon Forums

Coding style: I can do it.
Colors: I can also have a look on it. Maybe some hint from you, which lines are TODOs would be useful. But I also make no promise, as I never got in touch with those colors before...

An additional remark about the size discussion above, to get a complete overview about all technically possible options:

1. There was an ancient version of the patch with a switch "Use more heightlevels: Yes/No", where additional space was only used if the switch was in state "Yes". (i.e., there was a new struct, which was a Null-Pointer if the switch was in state "No").

The code for transitions between those states worked without problems as far as I remember.

Obviously, the inline Getter (TileHeight, I think) then was a short if (flag is set) <return data from new struct>, else <return data from Tile>.

At some point in time, at least someone (I don´t remember who) decided / proposed to drop that logic, thus the patch now is pure more heightlevels.

2. As I have said, if savegame size is considered a problem, one could at least still apply some of the patches that in the background make things more-heightlevel-capable. This would at least greatly reduce the future size of this patch.

3. Of course, for me it would by far be the simplest thing, if (1) and (2) would not be needed, since both involve significant additional work for me.

Just to have the options about this aspect completed here

Note that each colour is used for two levels by choice. My choice to be precise.

Light green: colours 229 and 230 are the same as the two before. I do not know why I did not fix that 1 colour at the time; I must have overlooked.
Dark green: coulours 199 to 230 are dublicates of 197 and 198.
Purple: colours 213 to 230 are duplicates of 211 and 212.


About the size of savegames, while yes they are increased in size now at all times, the code is much cleaner then it was before. In all honesty I do not mind the increase that much.

ic111 wrote:1. There was an ancient version of the patch with a switch "Use more heightlevels: Yes/No", where additional space was only used if the switch was in state "Yes". (i.e., there was a new struct, which was a Null-Pointer if the switch was in state "No").

that sounds like way more trouble than it is worth...

ChillCore:

I found and fixed two or three line comments with // that somehow survived previous codestyle-fixing-sessions (this is a typical error for me, since I use // comments in many other projects at work...).

But how did you manage to find "spaces to align code"? Do you have some regexp for that?

I know about https://wiki.openttd.org/Coding_style#R ... whitespace which does not report anything.

I use an editor that shows me the difference between spaces and tabs.

ic111 wrote: ChillCore:

I found and fixed two or three line comments with // that somehow survived previous codestyle-fixing-sessions (this is a typical error for me, since I use // comments in many other projects at work...).

But how did you manage to find "spaces to align code"? Do you have some regexp for that?

I know about https://wiki.openttd.org/Coding_style#R ... whitespace which does not report anything.

I did not mean leading or trailing spaces; I should have been more clear.
I meant alignment in the @param, @return parts of the code mostly and a few other places where either spaces are added or removed for no reason.

It's just some very very minor stuff that kinda jumped out to me and my OCD kicked in, sorry for the confusion caused.

Ah, you mean something like this (in viewport.cpp):
(where all whitespaces after the * are spaces, to get the alignment right)

Is there anything wrong with this piece of code, concerning coding style?

Regarding smallmap, actualy it would be greatly welcome, if someone else could have a look at those colors...



Beside this, I have found a somewhat bigger bug: Bridges may in worst case circumstances already trigger glitches when their height exceeds about 10 or 11, not 16 as I previously thought (and because of which I introduced a max bridge height). I will have to have a look on that either.

As far as I remember, the basic problem with bridges is, that they are not easily accessible for the algorithm deciding which area to repaint. For ordinary tiles, inspecting their heightlevel is quite easy, for bridges, I would have to inspect basically any tile within max length of a bridge.

Question to everyone interested: What is your expectation regarding max. bridge height - what height would you expect?

Yes and other niggles like this in patch 50
From this: to this: Edit: For some funky reason "Some" and "Tile" are aligned correctly on my pc (firefox) but not on my iPad (Apollo), anyhoo you get the idea.


About them colours ... I'll see what I can do over the weekend.

About bridges ... we had some serious glitches having them higher than 31 levels. That is why we kept it at 15.

ic111 wrote:Question to everyone interested: What is your expectation regarding max. bridge height - what height would you expect?

I don't think you'll find manys screenshots - if any - in the entire screenshot forum that has a bridge significantly higher than 6 or 7 maybe. It just stops looking good, and it's not very practical either. I doubt there's much demand for 15+.

it will be awesome if this would end up in trunk someday... and I am taking the last few posts as good sign.
10 levels high bridge is already quite high, so anything between 7 to 15 is just fine.

I sallute Chillcore and ic111. you have been working so long with this and still have motivation keep it closing the standards needed to get it reviewed. It has been a while since I did anything with OpenTTD sources (mods to original daylenght patch and some own use patch modifying base costs before newgrfs supporting it came along.), but I do have idea amount the work it is make own features and keep them up to date when revisions go up.

I just spent two hours inspecting that bridge issue deeper (and a third one writing that forum post ). See the attached screenshot illustrating the problem.

First a bit of background about how my viewport algorithm works: It basically performs the following steps:

1. Given the region to paint in screen coordinates, calculate which tiles would bound that region *if* the whole map would have height zero
2. For each column on the isometric map, determine the actual top and bottom row to paint.

Step 2 is a bit complicate, since it cannot be calculated in one closed formula. I solve it using the observation, that the higher a tile is, the more in the north it will be painted. This, in order words, means that if at some screen coordinate y, assuming height zero, tile T would be painted, the tile actually painted at that screen coordinate y (with height > zero) will always be south of T.

Thus, I can use binary search on the area defined by the tiles bounding the area assuming height zero, and the lower map edge, for finding the tile actually bounding the painting area given in screen coordinates above.

In each step of that search, log(map_height) times, I call GetViewportY(tile).

(more details about this algorithm can be found in the patched source code, a patched viewport.cpp contains extensive comments about that).


The problem with bridges now is, that a bridge tile in fact can be higher (painted more northwards) than a tile is actually more in the north. Something which otherwise only happens for buildings, which have a somewhat limited height.

If you look at the screenshot, you see a glitch. The reason for that glitch is, that the algorithm inspects the tile behind the glitch, calculates correctly, there is a tile with height 15. It cannot find the bridge tile southwards, which actually has height, say, 25, which can only be determined by IsBridgeAbove and GetBridgeHeight.

BTW, if there is a hill behind the bridge, the problem quickly disappears, i.e. what you see here is actually near the worst case landscape.

Now, what options do I have?

(1.) Inspect all tiles south of the chosen tile, wether there is maybe a bridge that triggers correction of the calculated tile.
This would result in a considerable performance loss, for an actually quite seldom case. Thus, I am very reluctant concerning thinking in this direction.

2. High buildings are treated using this piece of code: i.e. I artificially expand the region considered dirty by 116 pixels (the value is taken from the old code as far as I remember) into the south. Obviously, the higher the added constant is, the higher bridges can become without triggering problems. Furthermore, the height (in terms of screen coordinates) of one heightlevel is well-known (TILE_HEIGHT = .

Thus, one could think about introducing a configuration variable "Maximum Bridge Height", together with a warning "high values result in a performance loss". Then, I could make the 116 above a variable depending on the configuration variable. If the user chooses 5 or 6, the 116 can stay, if the user chooses e.g. 15, then we probably are somewhere between 200 and 300. Which of course means, that every render operation affects a considerable bigger region, i.e. it costs performance.

(actually, in a worst case landscape as seen in the screenshot, and given the quite high steel suspension bridge, the first glitches start at about height 6 - quite seldom, since the border of the repaint region additionally needs to be located exactly on the bridge, but they exist).

3. The third approach would be using some sort of an bridge index for finding out wether there is a bridge. One could, for each section of e.g. 16 x 32 tiles, store a vector with all tiles being containing a bridge higher than the minimal glitch height (or maybe those being shadowed). Assuming that high bridges are quite seldom, that map would be fairly small. Then, the algorithm from 2. above could, after it has calculated the searched tile, with one map get() determine wether in its region is a high bridge. In 99.9% of all cases, the answer will be no, in the remaining 0.1%, it will have to search southwards until some limit for a bridge.

Nevertheless, this would need quite complicate bookkeeping, additional code to be triggered on terraforming and on bridge building and destroying, also there would be the problem that the map would have to be calculated on savegame load().

===> I doubt that this actually makes sense.


Conclusion: I think that approach 2. is the most senseful one. The only problem is the support for old savegames, if one sets the setting to e.g. 10, and someone loads a old savegame with a bridge of height 15 (which is allowed, although probably quite seldom), then there will be glitches.

Maybe the AfterLoad() function could issue a warning, if an old savegame with a high bridge is loaded. One time in the support code for old savegames, scanning the whole map for a high bridge should be ok IMHO, similar on changes of the setting in-game.


What do you think?

One can also postpone that decision until the Viewport patch is reviewed, as discussing this is probably simpler if there is at least a second person knowing in detail how the algorithm works.

ic111 wrote:Maybe the AfterLoad() function could issue a warning, if an old savegame with a high bridge is loaded. One time in the support code for old savegames, scanning the whole map for a high bridge should be ok IMHO, similar on changes of the setting in-game.

i think the same behaviour as with station spread should apply here.

ic111 wrote: 2. High buildings are treated using this piece of code:

Code: Select all

int viewport_bottom = _vd.dpi.top + _vd.dpi.height + 116;

i.e. I artificially expand the region considered dirty by 116 pixels (the value is taken from the old code as far as I remember) into the south.

Thus, one could think about introducing a configuration variable "Maximum Bridge Height", together with a warning "high values result in a performance loss". Then, I could make the 116 above a variable depending on the configuration variable. If the user chooses 5 or 6, the 116 can stay, if the user chooses e.g. 15, then we probably are somewhere between 200 and 300. Which of course means, that every render operation affects a considerable bigger region, i.e. it costs performance.

(actually, in a worst case landscape as seen in the screenshot, and given the quite high steel suspension bridge, the first glitches start at about height 6 - quite seldom, since the border of the repaint region additionally needs to be located exactly on the bridge, but they exist).

Conclusion: I think that approach 2. is the most senseful one. The only problem is the support for old savegames, if one sets the setting to e.g. 10, and someone loads a old savegame with a bridge of height 15 (which is allowed, although probably quite seldom), then there will be glitches.

I think it does make sense to introduce a max bridge height parameter, yet having it as user parameter is not essential IMHO. It makes sens as a sort of advanced (=difficulty) setting - thus it's nothing which would actually be needed directly in a MHL series, only as an additional patch on top of core:

MHL then only needs to define an absolute maximum for the bridge height. That in turn defines the area which needs redraw below a bridge. Using an equivalent max height for bridges which gives about the same area as covered by a large building makes sense to me (which with the values you quote is around 15 height levels or so).

Just my 2ct

I agree, just one question:

Are their well-defined rules for (a) bridges and (b) buildings, how high newgrf authors may design them? Something like "at most x pixels height"?

The 116 I quoted above seem to work quite well for buildings as well, but honestly, I don´t know wether it´s too big or too small with respect to a worst-case allowed building.

Eddi wrote:

ic111 wrote:Maybe the AfterLoad() function could issue a warning, if an old savegame with a high bridge is loaded. One time in the support code for old savegames, scanning the whole map for a high bridge should be ok IMHO, similar on changes of the setting in-game.

i think the same behaviour as with station spread should apply here.

You mean, simply ignore it?

I just created a station of length 12, then reduced the station-spread to 5. Even after save / load, the station-spread continued to be 5, nothing stopped me in reducing it, and my station of course also happily stayed living

MHL then only needs to define an absolute maximum for the bridge height. That in turn defines the area which needs redraw below a bridge. Using an equivalent max height for bridges which gives about the same area as covered by a large building makes sense to me (which with the values you quote is around 15 height levels or so).

Actually, there is an additional factor 4 I missed yesterday. I.e., one heightlevel is equivalent to delta_y = 32, not 8, in that code. This also means, that the 116 are equivalent to about 3.6 heightlevels. This also makes much more sense, if you look at realistic house heights in OpenTTD.

Beside this, in the meantime I think that actually checking for bridge existence within (max bridge height / 4) tiles south of the chosen tiles should in average be cheaper than unconditionally doing all the drawing jobs for them. MayHaveBridgeAbove / IsBridgeAbove / GetBridgeEnd / GetBridgeHeight don´t look like being that expensive...

Once this will be implemented, we will need to test it with the highest bridges (measured above bridge level) and the highest houses that exist in the OpenTTD universe. But that will be not more than fine-tuning the constant values.

Finally, the release for the things we discussed above.

There is a new maximum bridge height setting (as doing so is a quite simple task anyway, I added a new config setting, as neighbor of the existing maximum bridge length).

The viewport code now searches in a range defined by the maximum bridge height setting for a potential bridge. If it finds one, it expands the to be repainted range of tiles accordingly.

ic111,

My TortoiseHG workbench keeps crashing over and over again for some reason (not related to MHL as it never worked properly on this sytem to begin with).
Could you please pull a 1 file patch so I do not have to apply all the patches manually and do it again if I want/need to start over.

I'll try and fix it later but for now I'd rather concentrate on them colours for a bit.
Thanks in advance.


Edit:
On second thought ...
For just fixing the smallmap colours I can use my patchpack or an older version so there is no rush.


Edit2:

Nappe1 wrote: it will be awesome if this would end up in trunk someday... and I am taking the last few posts as good sign.
10 levels high bridge is already quite high, so anything between 7 to 15 is just fine.

I sallute Chillcore and ic111. you have been working so long with this and still have motivation keep it closing the standards needed to get it reviewed. It has been a while since I did anything with OpenTTD sources (mods to original daylenght patch and some own use patch modifying base costs before newgrfs supporting it came along.), but I do have idea amount the work it is make own features and keep them up to date when revisions go up.

Thank you for the kind words, much appreciated.

I just imported the queue into my repo, so it was easy to get a all-in-one diff. It applies to svn r26856 (h357c24edb8ae).

Thank you planetmaker, much apreciated.
I prefer this over an old version or something as at the same time I can test some more relevant things that may need testing.

On my side, when importing the queque it would stall somewhere in the middle.
When doing some manual magic stuffs I get the queue imported but when I tried aplying the patchqueuque it complains about wanting to pop an non existant commit (or something along that line).
Anyway ... time to mess things up once more.