More height levels (in trunk since r27010)

[Rubidium]

I got some feedback on this set of patches, see the attached diff.

[ChillCore]

Thank you Rubidium fo your comments.
Many of your remarks relate to code we needed before the now removed setting to allow MHL was removedl. I was aware of some but you found a few more that I missed while reviewing yesterday. eg. snowline enums

The debug message spamming the console can safely be taken out. The assert in that part of the code was removed (or commented out, I need to check) to be able to generate small maps at al (256 *256 and lower).
It is there to make sure things did not get out of hand unnoticed (reaching 10 or higher).
I never saw problems there.

Some quick questions in regards of the extra smoothness settings:
The last three settings are some bonus settings I added to be able to have some more options and be able to generate nice(r) maps on mainly very large maps.
Setting 5 and 6 are indeed smoother then 3 and the names need to be changed, never got to it due to lack of inspiration.
Would you prefer them taken out provided I finish up my custom smoothness patch or rather sorted properly and renamed?
The 'prob' with sorting them is that on small-ish maps there is/will be very little visual differnce between subsequent settings. In my patchpack they are marked as bonus settings in the gui.

Regardless of keeping these settings or not I will finish my custom TGP patch at some point, which is currently written for plain trunk but can be adjusted to accomodate MHL with 7 presets quite easily later on.

[Gigigonzalez]

Very interesting stuff, but this should definately coincide with desert height patch included!

Im working on a map and so far i have loaded a new map 500 times and no issue

[ic111]

I got some feedback on this set of patches, see the attached diff.

Thank you very much for the comments. I will have a detailed look on them shortly.

Concerning the "change max. heightlevel cheat": This is based on a long discussion two or three years ago, where the message I remember from dev side was that (1) the maximum heightlevel setting is needed to make certain NewGrfs work correctly, and that (2) changing that setting in-game is at least dangerous. Thus, I made a cheat of it. (what I just now don´t remember is wether that also applies to the scenario editor, i.e. wether without that cheat changing the setting on a existing scenario wouldn´t be possible either).

The details can probably be found somewhere above in this thread, I haven´t searched yet.


And concerning 50_Viewport: The following piece of comment is from readme.txt in the patch directory. At least rows and columns are described there, maybe it already makes things clear to you, otherwise please ask again (and also if you already read that).

IMHO the key aspect with respect to rows and column is, that the map coordinates go from north-east to south-west and from north-west to south-east, whereas things in the south shadow things in the north. Thus, I have to recalculate coordinates in a north-south / west-east coordinate system based on the usual coordinates.

The old painting algorithm repainted an area in the size of the worst
case regarding heightlevels. I.e. it looked how high a mountain could
be in the worst case, and repainted accordingly. This approach is not
feasible if mountains can have height 255*16 pixels.

The new painting algorithm works using a concept of rows and columns.
Each tile coordinate on the isometric map can be expressed by a
coordinate consisting of one row and one column. Rows go from west to
east, whereas columns from north to south. The painting algorithm
first determines the leftmost and rightmost column being affected by
the repainting activities. Then, it inspects the landscape inside the
area to be repainted, and for each column, it determines the top and
bottom row to the repainted. Based on this information, it then
repaints an area being as small as possible.

In the new painting algorithm, the area outside map is painted the
same way as the area inside map, just using the completely black tiles
from patch 30_PaintingVoidTiles. To make this work, a concept of
landscape geometry outside map is used. In this concept, outside map
landscape descends to height zero as fast as the general assumptions
about map geometry in OpenTTD allow (with general assumptions the
maximum height differences between neighbor tiles are meant). Of
course, those tiles outside map are not saved explicitely, they only
exist as concept in code when dealing with the edges of the map.

Basically, the thing about rows is that there is no closed formular for determining (given a particular column) which is the top and the bottommost affected row. This is then the binary interval search.

[ChillCore]

Very interesting stuff, but this should definately coincide with desert height patch included!

Im working on a map and so far i have loaded a new map 500 times and no issue

That is not going to happen in this patch, sorry.
The desert amount already scales according to the height of the map generated in almost the same way as trunk does, from 4 tiles away from water up to generated heightlevel devided by 4 (instead of just 4), thus allowing more desert.

It has been agreed upon that any further adjustments will have to be made as a seperate patch.

[ic111]

Rubidium, what do you mean with that comment (related to patch 060)?

The point of this patch is that the old code here could ignore the heightlevel, since if there are only up to 15, the user ends up near to the desired location anyway. With much higher heightlevels, I have to include the heightlevel into the calculation.

In that line, pt_with_height actually is not the same as pt. Or do you just mean the newline?

Code: Select all

@@ -103,6 +104,7 @@
 -	Point sxy = this->ComputeScroll(pt.x / TILE_SIZE, pt.y / TILE_SIZE, max(0, (int)wid->current_x / 2 - 2), wid->current_y / 2, &sub);
 +	Point sxy = this->ComputeScroll(pt_with_height.x, pt_with_height.y,
 +			max(0, (int)wid->current_x / 2 - 2), wid->current_y / 2, &sub);
+#### needless change?
  	this->SetNewScroll(sxy.x, sxy.y, sub);
  	this->SetDirty();
  }

[Rubidium]

Looks like I overlooked the part that actually changed somehow. If the pt_with_height.x/y would be of different length that pt.xy / TILE_SIZE, it would probably have caught my eye... but that's not something you can do anything about. It was the last patch that I commented on though; first the simple ones, then the more complex stuff.

[ic111]

I have implemented the simple changes, some more complex changes remain to be done. Nevertheless, here an intermediate release.

In detail:

Comment fixes and code improvements in patches 10 and 20 are fixed.

+#### I like 999 much better. Especially because this way is going to break horribly when adding sprites to openttd.grf, because it reduces the number of GRFs that can be loaded by one, and finally because it's a blob without source and a few Linux distro's want to build everything from source. If not, then it's deemed 'non-free'.

I perfectly agree, the only reason why I didn´t yet switch to 999 is that at least some weeks ago, 999 needed a recompile of a patched nforenum. As soon as the infrastructure is in a state that allows a 999 patch being applied and successfully compiled without problems, I happily switch to 999.

I'm missing a complete picture here. Columns, rows, regions and some other words are used, but I'm not really getting what they mean exactly when reading this. What is the basic meaning of those words, and what is the basic idea of the drawing algorithm?

I try to explain a bit more about this here:

The drawing algorithm receives a rectangular section of screen, and needs to redraw that section. The problem now is, how to determine what tiles are affected by this. To the left and to the right, things are quite simple, the x screen coordinate where a tile is painted does not depend on things like heightlevel, objects on the tile, etc.

More complicated is the vertical direction. The higher a tile is, the more it is painted in the north. The higher a building or bridge on a tile is, the more it is painted into the north either. Thus, if a tile has a relatively big heightlevel, it may need to be painted, although a tile with the same TileIndex, but heightlevel zero would be far south of the visible area. In other words, the decision if some particular tile intersects the painting area in vertical direction highly depends on the tile state.

What one knows for sure (in the absence of bridges, as the recent discussion showed) is that a Tile with smaller TileX/Y is painted north of a Tile with bigger TileX/Y, just the difference can vary. Thus, given some screen coordinate that is the boundary of the top be painted area, one can use binary search for finding out which tile is painted at, or at least near that screen coordinate.

Thus, if you do that binary search both with the upper and with the lower boundary of the screen, you get the area of tiles to be painted.

Now, the meaning of the words above: As I have said, to the left and to the right things are simple. Thus, I divide the set of tiles into columns. Tiles that are exactly north or south of each other are in the same column. A unique column index can be calculated quite simple based on the tile coordinates.

For each column, I determine the topmost and bottommost to be painted tile, as described above. Here, rows come into play: All tiles that area exactly west or east of each other are in the same row. Thus, the result of that calculation is not just a tile (that is at that stage not that interesting anyway), but a row.

Once this is done for all columns, I for each column have a top and a bottommost row to paint. The minimum top row and the maximum bottom row finally constitute the borders of the to be repainted area.

Does this explain your questions? If no, please ask again.

+#### Can't maximum_height_level be used as well? On 4kx4k map at tile 0,0 I doubt you need to search till 4k,4k. If maximum_height_level = 32, then I'd expect it to be at most a 32x32. This should shave off a couple of iterations on average.

Probably you are right, I thought about something similar in the last few days already. I will have detailed look on that.

Patch 060 should be clear now, no changes needed.

Patch 090 and 100: The minor code changes are fixed, however:

+### Can't this logic be shared with aircraft? It looks duplicated.

It indeed looks duplicated, unfortunately aircrafts and disaster vehicles don´t have a common superclass where the variables this code relies on could be defined. Thus IMHO, the answer is No, there is nothing to unify sensefully.

Patch 140: Comment and code issues fixed.

Patch 150, smallmap: I will have a look on it. Probably, I somehow need to scale the access to those color tables.

Patch 155, 157: Cheat vs. setting: I have just searched

+#### Why a cheat and not a setting?

I have just dived into three-years-old forum posts. See e.g.

http://www.tt-forums.net/viewtopic.php? ... 60#p980387

http://www.tt-forums.net/viewtopic.php? ... 80#p980488

and my answer

http://www.tt-forums.net/viewtopic.php? ... 80#p980514

I have just tested it: As far as I get it, I indeed cannot change the existing setting set on map generation in scenario editor. I.e. when I generate a landscape in scenario editor, and then realize, I want a higher mountain than allowed, I have a problem with the current setup, unless I use the cheat.

I think in-game changing that setting is indeed not that important, but in scenario editor the ability might sometimes be useful IMHO.

+#### Don't you know the 'column' and as such the tile height there? Can't that be used to calculate the max scroll outside of map? I fear that this way gets people into a black screen; it even took me a while to scroll out of this blackness when being zoomed in fully. As such, I think this will cause some bug reports of new users that are lost in a black screen.

I see the problem. Probably, I can do something by indeed inspecting all visible columns. I can restrict that calculation to the case that one is beyound the usual map border at height zero. Just code will become a bit more complicate than when I compare just with a constant value.

Landscape generation: I will have a look.

[Rubidium]

Regarding the aircraft/disaster vehicles. It can be done with the power of templates, see the attached patches. They are based on trunk, just need a savegame bump for the disaster vehicle state to be actually saved.

Regarding the cheat; I didn't think about the NewGRFs, and yes... that is troublesome so I guess a cheat is fine. I'd be useful to add a bit of reasoning to the comment why it isn't a setting. A simple "can't be safely changed in single and multiplayer due to NewGRFs having access to this value, as such a cheat that resets lots of the NewGRF state" should suffice I'd say.

[ic111]

I integrated the aircraft-related patches from Rubidium (thanks), and introduced a limit when looking which tiles to repaint in Viewport.

Thus, I have no pending changes in 50_Viewport left.

I found a small bug related to the scenario editor: It allows you to generate flat land with a height greater than the maximum allowed heightlevel, which then results in the absolutely logical message "Can´t lower land since it´s too high"...

[Rubidium]

Regarding the viewport changes in r26870; just overwrite them with your viewport changes. It was mostly to get the other bits before it tested and to get the void tile change actually working.

Regarding the aircraft thing. I didn't use the TileHeightOutsideMap; is that a huge oversight? I reckoned that it wouldn't make much sense for the aircraft to start flying lower because a fictional (lower) tile height triggers the max bounds. As a result, I just used the tile height at the actual edge of the map.

[ic111]

Yes, not using TileHeightOutsideMap at that point is probably the better decision.

I am currently improving the 160_ScrollZoom patch, if the user scrolls near the map edge, I basically calculate which map edge tiles are near the currently visible portion of the map border, calculate their actual max height, and use that number for getting a more tight bound on where the user may scroll.

[ic111]

I adapted it to your changes to trunk, the patch zip starts to shrink

I improved the logic in 160_ScrollZoom, for the northeast and northwest map edges, it now takes the height of the adjacent mountains into consideration for the decision, how far the user may scroll.

Please check wether it does what you expect from it.

BTW, I have seen that you added the void-tile-related changes to trunk. When testing this version of the patch, I see the usual brown bare land outside map, not the black land. Is this due to still needed changes in OpenGfx etc., or did I do some mistake?

Just to get sure I didn´t miss anything: Based on your changes to trunk, I removed patches 020_GeometryOutsideMap, 030_PaintingVoidTiles, 090_Aircrafts, and 999_PaintingVoidTilesImproved. If there is any change in one of those patches still pending, please object.

[Rubidium]

Just to get sure I didn´t miss anything: Based on your changes to trunk, I removed patches 020_GeometryOutsideMap, 030_PaintingVoidTiles, 090_Aircrafts, and 999_PaintingVoidTilesImproved. If there is any change in one of those patches still pending, please object.

There shouldn't be anything left from those. There is plenty left from others that I'm going to merge in just a while (wait at least till the savegame bump before amending).

OpenGFX is updated, just the compile failed because it didn't use the updated NML. That will solve the 'sea' of brown tiles around the map.

[ic111]

I just start having a look on the map generation patch.

[Rubidium]

The zoom scrolling now does what I expect and I merged some other 'simple' stuff. Back to the more difficult bits.

[ic111]

Map generation: I just thought about the variety of settings on map generation. I´m not the original author of this particular patch, thus I myself can only comment on what I see:

New terrain type Alpinist: Is IMHO needed, the original terrain types fail to produce a real mountainious area.
New terrain type Bumpy: Beside the name, I personally don´t see a big difference to Flat.

New smoothness "Steep coasts" (my new name for the old cereally rough setting): Needed, only the combination Alpinist / Steep Coasts gives mountains that descend steeply into the water, with not much space for cities etc.

New smoothness CommutorZ and Flowing: IMHO both can be characterized as producing huge, round forms. I could name them "Huge, round forms 1 and 2".



At least for Bumpy, I tend to leave it out.

Any comments / opinions?

[Eddi]

while revisiting map generation: the current TGP has a problem that i could never quite figure out. there's some overzealous rounding going on that prevents the max height level from ever being used, making hill-tops more flat than they need to be.

also, there is some scaling done to round off hill tops, which some of the height/smoothness settings may want to omit.

[ic111]

FYI:

As I´ve just seen, the map generation setting contains numbers for map sizes 8192 and 16384, yet maximum map size in trunk is 4196. Thus, I leave the last two columns of the following array out.

Nevertheless, someone has probably taken some time for optimizing them. But they in fact belong to a bigger-map-size patch.

Code: Select all

+/**
+ * Desired maximum height - indexed by _settings_game.difficulty.terrain_type and the map size.
+ *
+ * Sense of indexing by mapsize too:
+ * Below, h_max_new is calculated as min(map_size_x, map_size_y) / the constant defined here.
+ * Later, for calculating the height of some individual tile, the formula:
+ *  h_max_new * (h - h_waterlevel) / (h_max - h_waterlevel), where h_max is the maximal heightlevel
+ * existing anywhere on the map at the time of that computation step.
+ * So, to summarize:
+ * For mapsize 64x64, the above division gives 1. And, h - h_waterlevel is always
+ * smaller than h_max - h_waterlevel. So, the result is a map consisting just of water 
+ * - i.e. at heightlevel 0 :-/.
+ * My conclusion was: 
+ * Indexing this setting by the mapsize is a good thing, as one gains much more
+ * control on the heightlevel distribution of maps now and small maps work at all. 
+ * The n-th value of the second component is for log(min(map_size_x, map_size_y)) - 6, so
+ * index 0 is for map size 64, whereas index 8 is for map size 8192.
+ */
+static const int _max_height[7][9] = {   
+	{32, 48, 72, 170, 340, 680, 1360, 2720, 5440}, ///< Extremely flat
+	{21, 40, 50,  64, 128, 380,  760, 1520, 3040}, ///< Very flat
+	{16, 27, 38,  51, 102, 204,  408,  916, 1832}, ///< Flat
+	{12, 21, 30,  39,  60, 120,  240,  480,  960}, ///< Bumpy
+	{10, 13, 17,  20,  33,  66,  132,  264,  528}, ///< Hilly
+	{9,  10, 11,  12,  13,  24,   48,   96,  192}, ///< Mountainous
+	{5,   6,  7,   7,   7,  12,   24,   48,   96}  ///< Alpinist
 };

[ic111]

... and a small question to Rubidium:

What do you mean with your "Space" comments in the map generation patch?

A missing empty line? A spurious space character I didn´t yet find?