More height levels (in trunk since r27010)

[petert]

Here is an updated build with the updated small map works:

[ChillCore]

Thank you for the build petert.

Although there are a few things to mention.

- When you click the smallmap, the positioning of the main viewport is still a little off when clicking high levels.
- The dark_green and violet smallmap colours for showing the elevation are not quite finished.
- Dark_green needs 16 more colours added, what is there allready needs copy pasting to extend to 256 levels and the purple top from the original green smallmap elevation colours. Also there are two dark_green arrays, one will be gone by the next version. Until now I have 1 vote to reuse the purples I have already. Thank you hackelittlebit for the feedback.
- The violet array has some grays that will not be in the final version and also needs copy paste doubling.
- In terraform.cmd there is some outcommented code that needs moving to the correct place.(Leveling terrain.)
- The airport near borders crash still exists.

Other than that the patch is playable.
Please consider this version as a preview of things to come.(Coding style in smallmap.cpp is messed up but functional)

[ic111]

Hello,

I have just after a longer time again had a look at this thread. Looks like the patch is mostly finished. Maybe someone should update the feature list at

http://wiki.openttd.org/Requested_features

There, a more heightlevel patch is placed with 50 %, however this patch probably is much over 50 % in the meantime. Furthermore, the link in that list currently goes to the wrong thread (i.e. to the one someone opened more than a year before I opened this thread).

And thanks for taking this patch over, I currently would not have any time for it unfortunately (and this won't change during the next months). Hopefully, the OpenTTD developers eventually merge this into trunk, or at least place it on the roadmap for whatever version.

[Rubidium]

Maybe someone should update the feature list at wiki

What stops you from updating the wiki?

[Yexo]

Hopefully, the OpenTTD developers eventually merge this into trunk

Not as long as there are several unsolved problems. Also a single 220kb patch is very hard to review = less likely to get a review = less likely to end up in trunk.
Then there is also the issue that this patch uses extra memory, is that extra memory really worth a few more height levels?
Has anyone ever profiled this patch? How much extra cpu is needed to run this compared to clean trunk?

[ic111]

Maybe someone should update the feature list at wiki

What stops you from updating the wiki?

I played this patch last several months again. At this time, there were not much flaws left. But I have no detailed insight into how it behaves now.
So I don't think I am the right one to decide wether it is e.g. 75 % or 90 % right now. This should be done by someone being familiar with the current codeline - or by someone who tested it in a detailed manner.

And no, I don't have the time right now to get this insight. I had this time when I started the patch more than a year ago, but right now: Sorry.

[ChillCore]

Hello,
I have just after a longer time again had a look at this thread. Looks like the patch is mostly finished. Maybe someone should update the feature list at
...
And thanks for taking this patch over, I currently would not have any time for it unfortunately (and this won't change during the next months). Hopefully, the OpenTTD developers eventually merge this into trunk, or at least place it on the roadmap for whatever version.

Nice that you still drop by from time to time.
Again, I would have never gotten this far if your code was not written solid and documented the way it was.
The next update will be posted when I am finished extending the smallmap coulours.

Maybe someone should update the feature list at wiki

What stops you from updating the wiki?

I have been thinking about doing that but not understanding(read: not yet have taken the time) the formatting style has stopped me.
If somebody does feels like doing that now, it should say 75% completion untill all bugs are solved, 90% would be: only coding style remains too be finished.

Hopefully, the OpenTTD developers eventually merge this into trunk

Not as long as there are several unsolved problems.

Progress is slow but steady. I am struggling with airport placement and there are still a few other things that need taken care of, maybe even a bug or two I am yet not aware of.

Also a single 220kb patch is very hard to review = less likely to get a review = less likely to end up in trunk.

I can understand that. Maybe when finished we/I will be able to break it up in smaller patches. The only problem I see is that most code is related to one another in some way.

Then there is also the issue that this patch uses extra memory, is that extra memory really worth a few more height levels?

To make the difference between 16 levels and unlimited? IMHO yes.
I do not know if the patch uses more memory when disabled but I do know MAPH is zero bytes in the savegame at that moment. See codeblock.

Code: Select all

+/**
+ * The general idea behind loading and saving MAPH:
+ * ===============================================
+ * The _map_heightdata array is not NULL if and only if AllowMoreHeightlevels,
+ * defined in tile_map.h is true.
+ *
+ * Saving:
+ * ======
+ * So if AllowMoreHeightlevels is true, we have to save it and do so.
+ * However, if AllowMoreHeightlevels is false, we just save a junk named MAPH
+ * with length 0.
+ *
+ * Loading:
+ * =======
+ * If we load an old savegame without MAPH, then Load_MAPH is not even called.
+ * So, in AfterLoadGame, we set _map_heightdata to NULL and
+ * allow_more_heightlevels to false. (The player may turn on this setting
+ * afterwards in game.)
+ *
+ * If, however, we load a new savegame (where MAPH was already introduced),
+ * SlGetFieldLength() was already set to the length of the chunk by LoadChunk.
+ * So, if AllowMoreHeightLevels is false, we get SlGetFieldLength() == 0 here
+ * and set _map_heightdata to NULL accordingly.
+ * Otherwise, we just load _map_heightdata from the chunk..
+ *
+ * For background information about how and where _map_heightdata is
+ * constructed and freed see comment of AllocateMap() in map_func.h.
+ * @see AllocateMap() in map_func.h.
+ */

Has anyone ever profiled this patch? How much extra cpu is needed to run this compared to clean trunk?

To my knowledge, it has not been profiled yet. Even if I wanted to do that, I would not know how.
Somebody feels like doing so?

[Kogut]

Then there is also the issue that this patch uses extra memory, is that extra memory really worth a few more height levels?

To make the difference between 16 levels and unlimited? IMHO yes.

Also in my opinion. //now highest mountains are in reality funny hils. And nothing more.

[ic111]

Hopefully, the OpenTTD developers eventually merge this into trunk

Not as long as there are several unsolved problems. Also a single 220kb patch is very hard to review = less likely to get a review = less likely to end up in trunk.
Then there is also the issue that this patch uses extra memory, is that extra memory really worth a few more height levels?
Has anyone ever profiled this patch? How much extra cpu is needed to run this compared to clean trunk?

Concerning extra memory: I once designed it that way that the new feature is optional. You could / can enable it in the settings, if you don't do that the game should work the old style. ChillCores post indicates that this property hasn't changed in the meantime.

Converning efficiency: No, I did not profile it. I can say that I introduced no algorithms having in mind that they have a much higher computational complexity. For the repaint algorithm (things like heightlevel control for airplanes are not performance relevant anyway) it uses a mostly logarithmic interval search for finding out which region of the map is actually affected by the repaint. The algorithm in fact is more intelligent than the old one which as far as I remember simply took the height of the highest possible mountain as a basis for calculating the region to be repainted.

Finally, I think testing in the sense that you play a longer period of game-time with the patch being enabled should be a good indicator for wether there are major problems left or not. I don't think splitting it up is a good idea: It was tested over a long period of time, and if you try to split it up, you just introduce the danger of new bugs because you make mistakes during the split-up.

I think solving the problems left and then starting with a test phase where some people actually play a game with the patch being enabled would be the right way. In detail:
(1) Solve remaining known problems
(2) Some independent people test in the sense of actually playing a game
(3) Code-Review given that all problems that maybe occurred in (2) are fixed.

Somewhere in between maybe a profile session.

[Yexo]

Converning efficiency: No, I did not profile it. I can say that I introduced no algorithms having in mind that they have a much higher computational complexity. For the repaint algorithm (things like heightlevel control for airplanes are not performance relevant anyway) it uses a mostly logarithmic interval search for finding out which region of the map is actually affected by the repaint. The algorithm in fact is more intelligent than the old one which as far as I remember simply took the height of the highest possible mountain as a basis for calculating the region to be repainted.

If it really is more effecient then that might be an ideal candidate to split of and get in trunk first.

Finally, I think testing in the sense that you play a longer period of game-time with the patch being enabled should be a good indicator for wether there are major problems left or not. I don't think splitting it up is a good idea: It was tested over a long period of time, and if you try to split it up, you just introduce the danger of new bugs because you make mistakes during the split-up.

Unfortunately the past has shown that even if a patch is played by a lot of players, many don't report bugs "because it's a patch, so unstable anyway". Also as far as I can see there have been several people updating the version of a patch, which every time introduces a chance of bugs.

I think solving the problems left and then starting with a test phase where some people actually play a game with the patch being enabled would be the right way. In detail:
(1) Solve remaining known problems
(2) Some independent people test in the sense of actually playing a game
(3) Code-Review given that all problems that maybe occurred in (2) are fixed.

Somewhere in between maybe a profile session.

That is the correct order, but as I said before, you'll get a code-review much easier if you split up the patch.

[ChillCore]

Hello Devs,

Here is a the functional terraforming code part of our patch.
Please note that code has been slightly rewritten to function without changing maximum height.
The ONLY reason I have done so is for you guys to have a smaller chunk of code to chew on in case one of you wants to review the code.
If you want to include it in trunk, feel free to do so, however may I ask to not change the syntax or do other funy stuff with the way it is written for now because the rest of the patch depends on it.
Please do not make me regret having posted this, I have been looking at the code for a very long time now and I really do not feel like starting all over.
If some things need to change I will happily do so if I can.

To test the difference with the old code you can replace:
CommandProc CmdTerraformLand; with CmdTerraformLandOld; (2x)
and
CommandProc CmdLevelLand; with CommandProc CmdLevelLandOld; (2x)
in command.cpp as the old code is still in place.

The difference in speed is best seen while doing this with the normal patch and terraforming big chunks of land.
Anyway, thank you for the feedback so far.

I have also attached the complete patch even though I am not quite done with the smallmap colours ...

[Terkhen]

I have been profiling the provided terraforming code against the current implementation in trunk under various conditions. For this I have been using the TIC() TOC() macros found at debug.h (see attached diff files). There are three tests for each condition, and each test shows the results of the testing phase and the execution phase (DC_EXEC).

Lower 64x64 mountainous map to sea level

Trunk:

• Testing: 159245189, Execution: 184734613
• Testing: 159276286, Execution: 188236961
• Testing: 159971597, Execution: 191223031

new_terraform:

• Testing: 23447799, Execution: 25239295
• Testing: 23617035, Execution: 24717013
• Testing: 23785644, Execution: 24171768

Raise 64x64 mountainous map to high level

Trunk:

• Testing: 49760598, Execution: 104138559
• Testing: 49775716, Execution: 104245980
• Testing: 50239990, Execution: 104171406

new_terraform:

• Testing: 19499915, Execution: 20824259
• Testing: 18885634, Execution: 20737346
• Testing: 18966420, Execution: 20917939

Level 64x64 flat map (no changes)

Trunk:

• Testing: 63975, Execution: 0 (Not required)
• Testing: 60092, Execution: 0 (Not required)
• Testing: 63695, Execution: 0 (Not required)

new_terraform:

• Testing: 5388585, Execution: 4841995
• Testing: 5586814, Execution: 4693929
• Testing: 5572337, Execution: 4695961

Raise a 2x2 area

Trunk:

• Testing: 21862, Execution: 27669
• Testing: 22594, Execution: 27491
• Testing: 19903, Execution: 27064

new_terraform:

• Testing: 77471, Execution: 48457
• Testing: 72868, Execution: 43869
• Testing: 70052, Execution: 43384

Raise a 4x4 area

Trunk:

• Testing: 42593, Execution: 99060
• Testing: 40714, Execution: 98586
• Testing: 40044, Execution: 98496

new_terraform:

• Testing: 151075, Execution: 122406
• Testing: 148026, Execution: 119782
• Testing: 152235, Execution: 121277

Level a 2x2 flat area

Trunk:

• Testing: 2043, Execution: 0 (Not required)
• Testing: 2770, Execution: 0 (Not required)
• Testing: 2057, Execution: 0 (Not required)

new_terraform:

• Testing: 30081, Execution: 5237
• Testing: 28225, Execution: 5245
• Testing: 23697, Execution: 5075

The provided code is really fast when terraforming big areas and/or when there are many differences in the height of the tiles at the area. In usual games, though, it is more common to terraform smaller areas that can't have big differences in height anyways. A mixed implementation that would use the current method for small areas and the new method for greater areas is not desirable either, since it would add a lot of code that would not be used frequently but would still require maintenance.

EDIT: I forgot to attach the diff files.

[adf88]

I have been profiling the provided terraforming code

Terraforming is a one-time task, even if it's multiple times slower there will be no significant impact to overall performance. Much more interesting things are those computed every game loop.

[Terkhen]

Terraforming is a one-time task, even if it's multiple times slower there is no much impact to overall performance. Much more interesting things are those which are computed every game loop.

A code with the aim of achieving a better performance must be judged principally by its performance gain (of course other stuff must be taken into account too), even if terraforming is not usually a performance bottleneck.

[ChillCore]

Thank you very much Terkhen for doing that profiling.

The provided code is really fast when terraforming big areas and/or when there are many differences in the height of the tiles at the area. In usual games, though, it is more common to terraform smaller areas that can't have big differences in height anyways. A mixed implementation that would use the current method for small areas and the new method for greater areas is not desirable either, since it would add a lot of code that would not be used frequently but would still require maintenance.

So in other words we should keep the code in the patch until it is good enough for trunk inclusion. Once the patch is in trunk (If at all, but I do hope so) it can happen that one terraforms a long slope, then the new code is beneficial.
I agree that a mixed solution is not the way to go. I was thinking of removing the old terraforming code when the patch nears it final version.

For the maintenance part ... It is rare that I have to change something in the terraforming code.
As for the heightlevel change parts, most of the times it is write once and do not look back. Exept for changes in trunk that you guys (have to) do anyway.

Checking out your diffs to see how I can test other things myself later. Thank you for providing them.
Are there any other parts you would like to have seperated for testing purposes, airplanes changing altitude perhaps?

Terraforming is a one-time task, even if it's multiple times slower there is no much impact to overall performance. Much more interesting things are those which are computed every game loop.

With certain terrain type/smoothness combinations you will be wanting to terraform more than you do now. See attachments.
When using the old terraforming code and doing a big terraform operation, eg. a very long slope, the game tends to freeze for a few moments.


screenshot 1: My prefered map generation.
screenshot 2: ic111's prefered map generation.

[adf88]

A code with the aim of achieving a better performance must be judged principally by its performance gain (of course other stuff must be taken into account too), even if terraforming is not usually a performance bottleneck.

Don't understand me wrong, I just want to help, not to discard your work. Terraforming counts too somehow, but if we are talking about overall CPU usage, don't measure one-time tasks like command execution because it won't tell us much.

[el koeno]

Hopefully, the OpenTTD developers eventually merge this into trunk

Not as long as there are several unsolved problems. Also a single 220kb patch is very hard to review = less likely to get a review = less likely to end up in trunk.
Then there is also the issue that this patch uses extra memory, is that extra memory really worth a few more height levels?
Has anyone ever profiled this patch? How much extra cpu is needed to run this compared to clean trunk?

I'd just like to say that even if this feature degraded performance a bit, it would be worth it for me. And I play a lot on my low-end laptop.

I'd really like to test this patch, but I really can't live without my CargoDist and auto-separation. So I'm afraid I'm not of much use...

[ic111]

A code with the aim of achieving a better performance must be judged principally by its performance gain (of course other stuff must be taken into account too), even if terraforming is not usually a performance bottleneck.

Don't understand me wrong, I just want to help, not to discard your work. Terraforming counts too somehow, but if we are talking about overall CPU usage, don't measure one-time tasks like command execution because it won't tell us much.

Nevertheless, it is interesting on its own I think.

The speed advantages for huge terraforming operations were the reason why I rewrote it. Because a code review made it clear to me that even trying to use the old code in that case would be no good idea (I think because the old code needs some fixed sized array in the size of the max. heightlevel, if I remember correctly). But this only as side node...

Why it is somewhat smaller on small sizes would be interesting though, just because one can learn something for the future. I mean, I used STL containers in that code, so if they worsen performance much this would be an interesting message on its own, e.g. with respect to implementing code that actually runs in the game loop. Here, I can think one can live with the figures, and those STL containers simplified programming a lot

[ChillCore]

I'd really like to test this patch, but I really can't live without my CargoDist and auto-separation. So I'm afraid I'm not of much use...

Have a looksie here.
http://www.tt-forums.net/viewtopic.php?f=33&t=47622



Ic111,
the link and status on the requested features wiki page is updated.

[el koeno]

I'd really like to test this patch, but I really can't live without my CargoDist and auto-separation. So I'm afraid I'm not of much use...

Have a looksie here.
http://www.tt-forums.net/viewtopic.php?f=33&t=47622

Wow, looks good!