More height levels (in trunk since r27010)

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

Eddi
Tycoon
Tycoon
Posts: 8258
Joined: 17 Jan 2007 00:14

Re: More height levels (in trunk since r27010)

Post by Eddi »

tbh, i always found it weird that there is an upper limit for house sizes other than sprite restrictions.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels (in trunk since r27010)

Post by ic111 »

Technically, there has to be one, regardless of its size. The code (both old and new code) simply has to get some notion how big the area below the to be repainted area is, which has to be repainted although its tiles aren´t visible, just because they might carry an object high enough for intersecting the to-be-repainted area.

If there wouldn´t be a limit, for every redraw operation, all tiles below until the map border would have to be searched for a high building.

Of course, one can increase the limit, at the cost of a somewhat more expensive drawing loop.

The other way round: Does the bug petern describes also occur with NewGrf, not-built-in buildings? Does anyone have an example where this happens?

Generally, this smells like a major debugging session for me, digging into quite old code of house animations. Thus any hints are welcome.
User avatar
planetmaker
OpenTTD Developer
OpenTTD Developer
Posts: 9432
Joined: 07 Nov 2007 22:44
Location: Sol d

Re: More height levels (in trunk since r27010)

Post by planetmaker »

ic111 wrote:The other way round: Does the bug petern describes also occur with NewGrf, not-built-in buildings? Does anyone have an example where this happens?
Yes, it also happens with NewGRFs. http://bugs.openttd.org/task/6156 is the related issue in our bug tracker and has an example with TTRS (which had some flickering before, but not the whole town being a flicker-o-rama).
Eddy Arfik
Transport Coordinator
Transport Coordinator
Posts: 260
Joined: 09 Apr 2014 11:10

Re: More height levels (in trunk since r27010)

Post by Eddy Arfik »

ic111 wrote:This is the piece of code in viewport.cpp one has to adapt:

Code: Select all

	/* The upper and lower edge of the viewport part to paint. Add some number
	 * of pixels to the lower end in order to ensure that we also take tiles
	 * south of the given area, but with high buildings intersecting the area.
	 * Subtract some pixels from the upper end in order to avoid glitches at the
	 * upper end of the top be painted area. */
	int viewport_top = _vd.dpi.top - 16;
	int viewport_bottom = _vd.dpi.top + _vd.dpi.height + 116;
More, precisely, the 116 pixels doesn´t seem to suffice for that building. I think I copied that number from the old code, but there might have been some extra margin I didn´t recognize.

Thus, it all reduces to the question: How high is a building in OpenTTD allowed to be? I raised this question already when we finally made MHL ready for trunk, but I didn´t get an answer then.

Once this number is known, one has to adapt that constant, and to get rid of that issue once and forever, test with a building that has exactly that height (maybe one has to shift things by some pixels because the term "height of building" and my algorithm refer to different baselines).
I've spent the last few hours playing around with various numbers here, taking screenshots of the flickering and looking at how many pixels were "lost". I noticed that with the value of 116, buildings would chop out to 16px, when I bumped it to 256 buildings chopped out at 156px, I couldn't find any building sprite larger than 256px, it looks like the 400 you tested is probably optimal giving some allowance for slightly taller sprites.

With regard to maximum allowed sprite size I'm a bit confused...
http://newgrf-specs.tt-wiki.net/wiki/RealSprites wrote: Ranges

The ranges for the four numbers after the compression are as follows:

ysize: 1..255

xsize: 1..65535

xrel: -32768..32767

yrel: -32768..32767

ysize*xsize may not exceed 65535.
but then this further down the page...
http://newgrf-specs.tt-wiki.net/wiki/RealSprites wrote: xsize, ysize

The width and height of the sprite within the graphics file. The valid value range is from 1 to 65535 for both.
If the value of 65k for ysize is correct, we could in theory model the "space elevator" at TTD scale
Netist
Engineer
Engineer
Posts: 12
Joined: 30 Oct 2014 20:16

Re: More height levels (in trunk since r27010)

Post by Netist »

Eddy Arfik wrote:If the value of 65k for ysize is correct, we could in theory model the "space elevator" at TTD scale
When can I download it? :P
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels (in trunk since r27010)

Post by ic111 »

Ok, I think I understand the problem: Seems that I (and all other people having looked at that code...) had a misconception concerning how things have to be scaled at the point of code where the 116 are added.

Please: Read my reasoning below carefully, and tell me if I make an error. That whole scaling magic, calculate in one coordinate system here, in another there, etc. simply is a bit tricky.

In ViewportAddLandscape, code reads as follows:

Code: Select all

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

	/* First get the position of the tile at the upper left / lower right edge,
	 * for now ignoring the height. (i.e. assuming height zero.) */
	Point upper_left_tile = GetMinTileCoordsIgnoringHeight(_vd.dpi.left, viewport_top);
The end result (upper_left_tile) is a tile coordinate in the sense of TileX and TileY. GetMinTileCoordsIgnoringHeight relies on

Code: Select all

	Point tile_coord = GetTileCoordFromScreenCoord(x, y);
GetTileCoordFromScreenCoord is implemented as follows:

Code: Select all

static inline Point GetTileCoordFromScreenCoord(int x, int y)
{
	/* First convert from the screen coordinate system (where the width of tiles
	 * is twice their height) to the tile coordinate system. That means, turn
	 * around by 45 degrees and make the tiles quadratic. */
	Point tile_coord = InverseRemapCoords(x, y);

	/* Scale from a 16x16-grid to a 1x1-grid as returned by TileX/TileY. */
	tile_coord.x /= (int)TILE_SIZE;
	tile_coord.y /= (int)TILE_SIZE;

	return tile_coord;
}
InverseRemapCoords finally calculates:

Code: Select all

Point pt = {(y * 2 - x) >> (2 + ZOOM_LVL_SHIFT), (y * 2 + x) >> (2 + ZOOM_LVL_SHIFT)};
where ZOOM_LVL_SHIFT is 2 according to zoom_type.h
If you feed InverseRemapCoords with (x=0, y=8), you get Point pt = (16/16,16/16) = (1,1). Thus, getting from the northest tile of map, to the tile just south of it is a step of 8 in the sense of InverseRemapCoords. In GetTileCoordFromScreenCoord, another division by TILE_SIZE = 16 (according to tile_type.h) occurs.

Thus, the step from tile (x,y) to (x + 1, y + 1) has a vertical difference of 16 * 8 = 128 if you think in the coordinate system _vd.dpi.top and _vd.dpi.height uses.

In the end, the 116 from above is a bit less than one tile difference, which of course is a way to small value. Looking at the savegame from the bug report, that skyscraper is about five tiles high. If I set the constant to 384, I occasionally can see that the antenna on its roof is cut. 5 * 128 = 640 might be a safe value.

Or maybe in the long-term, if this is possible from NewGrf side, one might make that constant a variable, that depends on the height of the highest object offered by any NewGrf added to the game.
Michi_cc
OpenTTD Developer
OpenTTD Developer
Posts: 619
Joined: 14 Jun 2004 23:27
Location: Berlin, Germany
Contact:

Re: More height levels (in trunk since r27010)

Post by Michi_cc »

ic111 wrote: Or maybe in the long-term, if this is possible from NewGrf side, one might make that constant a variable, that depends on the height of the highest object offered by any NewGrf added to the game.
This would be the NewGRF tile refresh offsets I guess: http://newgrf-specs.tt-wiki.net/wiki/Gl ... 95.2C96.29

TTRS might even be old enough to support them.
-- Michael Lutz
frosch
OpenTTD Developer
OpenTTD Developer
Posts: 988
Joined: 20 Dec 2006 13:31
Location: Aschaffenburg

Re: More height levels (in trunk since r27010)

Post by frosch »

NewGRF do not matter in this context.

In OpenTTD there are two functions:
  • ViewportAddLandscape
  • MarkTileDirtyByTile
Both functions limit how much the sprites belonging to a tile may extent from the tile origin. Historically these have always only matched approximately. So, it would be nice if we could add a proper "MAX_BUILDING_HEIGHT" constant and use it in both functions.

MarkTileDirtyByTile is a lot easier, so I only illustrate what I mean with that function :p

Code: Select all

static const uint MAX_BUILDING_HEIGHT = 122;

MarkTileDirtyByTile:
MarkAllViewportsDirty(
  pt.x - TILE_PIXELS * ZOOM_LVL_BASE,
  pt.y - MAX_BUILDING_HEIGHT * ZOOM_LVL_BASE,
  pt.x + (TILE_PIXELS + ZOOM_LVL_BASE - 1) * ZOOM_LVL_BASE, // adjust for rouding in max zoom-out
  pt.y + (TILE_PIXELS + ZOOM_LVL_BASE - 1) * ZOOM_LVL_BASE
The example uses "122" as maximum building height, since that matches current MarkTileDirtyByTile, but I think we should just increase it to something like 250 or so. Actually, my example already differs slightly with the rounding.

The old code in ViewportAddLandscape used 241, which is way bigger than above 122. IIRC this is because the old code before MHL ignored the Z-positions of tiles, and thus the 241 had to account for the worst case of 15 height levels (15*8=120 pixels in normal zoom => 120+122 ~ 241). The same reasoning is likely why MHL reduces the 241 to 116, possibly counting one height level too many.

If I understand ic111's post correctly, then the 241 resp. 116 lacks scaling for the newer zoom-in levels. ViewportAddLandscape should read:

Code: Select all

	int viewport_top = _vd.dpi.top - TILE_PIXELS * ZOOM_LVL_BASE / 2; // half a tile upwards
	int viewport_bottom = _vd.dpi.top + _vd.dpi.height + MAX_BUILDING_HEIGHT * ZOOM_LVL_BASE; // max building height downwards
So my theory about all this:
  • Originally the code accounted for buildings of height 120 on height level 15.
  • On lower height levels even taller buildings than 120 worked.
  • With additional zoom-in levels, the constants were not properly scaled, reducing the maximum height. However this was not noticed, since it would only trigger on higher height levels.
  • MHL removed the dependency on the height level (yay!), and thus made the bug introduced by zoom-in noticeable.
Is this about right?

If yes, I propose the usage of TILE_PIXELS and MAX_BUILDING_HEIGHT in ViewportAddLandscape and MarkTileDirtyByTile instead of random magic integers, and setting MAX_BUILDING_HEIGHT to 250 (not configurable).

Edit: Now that I look at the code, MarkTileDirtyByTileOutsideMap also lacks scaling for new zoom-in levels.
⢇⡸⢸⠢⡇⡇⢎⡁⢎⡱⢸⡱⢸⣭⠀⢸⢜⢸⢸⣀⢸⣀⢸⣭⢸⡱⠀⢰⠭⡆⣫⠰⣉⢸⢸⠀⢰⠭⡆⡯⡆⢹⠁⠀⢐⠰⡁
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels (in trunk since r27010)

Post by ic111 »

frosch wrote:NewGRF do not matter in this context.

In OpenTTD there are two functions:
  • ViewportAddLandscape
  • MarkTileDirtyByTile
Are you sure that by addressing the issue at both points of code, you don´t treat a problem that occurs once twice?

I mean, if I understand correctly, MarkTileDirtyByTile based on some TileIndex defines a range of viewport that is declared dirty. That information then is propagated through the call chain, and eventually ends up in one or more corresponding calls to ViewportAddLandscape.

In ViewportAddLandscape finally, the high building issue is addressed by expanding the area as defined by MarkTileDirtyByTile etc. to the south by a worst case building height.

Now, if also MarkTileDirtyByTile expands the area artificially, then ViewportAddLandscape already receives a expanded area, and expands it another time.

Or do I miss something here?
Both functions limit how much the sprites belonging to a tile may extent from the tile origin. Historically these have always only matched approximately. So, it would be nice if we could add a proper "MAX_BUILDING_HEIGHT" constant and use it in both functions.
Such a constant would be nice indeed.
If I understand ic111's post correctly, then the 241 resp. 116 lacks scaling for the newer zoom-in levels. ViewportAddLandscape should read:

Code: Select all

	int viewport_top = _vd.dpi.top - TILE_PIXELS * ZOOM_LVL_BASE / 2; // half a tile upwards
	int viewport_bottom = _vd.dpi.top + _vd.dpi.height + MAX_BUILDING_HEIGHT * ZOOM_LVL_BASE; // max building height downwards
So my theory about all this:
  • Originally the code accounted for buildings of height 120 on height level 15.
  • On lower height levels even taller buildings than 120 worked.
  • With additional zoom-in levels, the constants were not properly scaled, reducing the maximum height. However this was not noticed, since it would only trigger on higher height levels.
  • MHL removed the dependency on the height level (yay!), and thus made the bug introduced by zoom-in noticeable.
Is this about right?
IMHO the crucial point is: In what unit are your numbers? What I did in my post above was trying to set up a clear picture, how many pixels in terms of vd.dpi correspond to one tile height the user sees?

Furthermore, note that although my calculation above involves some constants named like ZOOM_Foo, it does not at all depend on the current zoom level. What you see above is just a scaling of coordinates between different points of code, completely independent of the current zoom level.

Thus, I suggest thinking in coordinates in terms of vd.dpi as I did above, where (if my calculation above is right), one tile the player sees has a height of 128. Then one can inspect various NewGRFs, having a look how many tiles in vertical direction their buildings shadow in the worst case, and decide upon a constant.

IMHO, writing a constant calculated as x * 128 = <your constant> into the code with some clear comment about the reasoning would be quite appropriate.
magic integers, and setting MAX_BUILDING_HEIGHT to 250 (not configurable).
As I have said, in what unit are your 250? In the 128 = one tile height coordinate space, 250 would be way to small already for the building in the savegame attached to the bug report.
frosch
OpenTTD Developer
OpenTTD Developer
Posts: 988
Joined: 20 Dec 2006 13:31
Location: Aschaffenburg

Re: More height levels (in trunk since r27010)

Post by frosch »

ic111 wrote:Are you sure that by addressing the issue at both points of code, you don´t treat a problem that occurs once twice?

I mean, if I understand correctly, MarkTileDirtyByTile based on some TileIndex defines a range of viewport that is declared dirty. That information then is propagated through the call chain, and eventually ends up in one or more corresponding calls to ViewportAddLandscape.

In ViewportAddLandscape finally, the high building issue is addressed by expanding the area as defined by MarkTileDirtyByTile etc. to the south by a worst case building height.

Now, if also MarkTileDirtyByTile expands the area artificially, then ViewportAddLandscape already receives a expanded area, and expands it another time.

Or do I miss something here?
MarkTileDirtyByTile is called when a tile needs redrawing. It then extents the are to be drawn *upwards* to include a potential building on the tile. ViewportAddLandscape on the other hand needs to determine the tiles whose graphics extent into the viewport, and thus it has to extent the area to be drawn *downwards*. As such the extensions do not add up. They complement each other to the same area. One from the bottom upwards, the other from the top downwards.
ic111 wrote:As I have said, in what unit are your 250? In the 128 = one tile height coordinate space, 250 would be way to small already for the building in the savegame attached to the bug report.
The unit is number of pixels in the base zoom level (ZOOM_LVL_BASE). Imho it makes no sense to use the unit of "vp.dpi", since the units of "vp.dpi" change when changing the maximum zoom-in level. So imho all pixel dimensions should be of the form "x * ZOOM_LVL_BASE". This is already the case for constants like TILE_PIXELS or the values in "SubSprite".
⢇⡸⢸⠢⡇⡇⢎⡁⢎⡱⢸⡱⢸⣭⠀⢸⢜⢸⢸⣀⢸⣀⢸⣭⢸⡱⠀⢰⠭⡆⣫⠰⣉⢸⢸⠀⢰⠭⡆⡯⡆⢹⠁⠀⢐⠰⡁
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels (in trunk since r27010)

Post by ic111 »

MarkTileDirtyByTile is called when a tile needs redrawing. It then extents the are to be drawn *upwards* to include a potential building on the tile. ViewportAddLandscape on the other hand needs to determine the tiles whose graphics extent into the viewport, and thus it has to extent the area to be drawn *downwards*. As such the extensions do not add up. They complement each other to the same area. One from the bottom upwards, the other from the top downwards.
Ah, ok, thanks for the explanation.
ic111 wrote:As I have said, in what unit are your 250? In the 128 = one tile height coordinate space, 250 would be way to small already for the building in the savegame attached to the bug report.
The unit is number of pixels in the base zoom level (ZOOM_LVL_BASE). Imho it makes no sense to use the unit of "vp.dpi", since the units of "vp.dpi" change when changing the maximum zoom-in level. So imho all pixel dimensions should be of the form "x * ZOOM_LVL_BASE". This is already the case for constants like TILE_PIXELS or the values in "SubSprite".

Ah, now I get your point. You mean the case that ZOOM_LVL_BASE changes. Yes, then I think you are right.
Wahazar
Tycoon
Tycoon
Posts: 1451
Joined: 18 Jan 2014 18:10

Re: More height levels (in trunk since r27010)

Post by Wahazar »

Alberth wrote:I was (am) terribly failing to understand what is exactly so great about MHL, as I can't see anything new with it.
Let me give an example of my real-world based map, here is the original heightmap:
https://www.dropbox.com/s/i7h8nhwnjwnqf ... 6x4096.png

In case of 1024x1024x16 map, high mountains at the south of map are rather flat, whereas post-glacial norhern parts are totally flat plateau.
Medium sized hills are flattened to one-tile height plateau:
Image


In case of 2048x2048 map, things looks even more flatten.
Of course I can edit height bitmap manually, and I've done this - adding some hills with false relative height, trying to give "look and feel" of some moraine hills.
But generally all relative heights were 1...5 tiles which is rather negligible unless:
a) high terraform base cost are applied
b) high tile slope is set (7% instead of default 3%) or/and mass modifier are involved.
But above restrictions have a negative drawback: making multilevel crossings is expensive and troulbesome.

With MHLs, map not only looks aestethically better, but also above restriction can be loosened.

Similar area on 2048x2048x50 map:
Image

Many thanks for all patch contributors and bug busters.
Formerly known as: McZapkie
Projects: Reproducible Map Generation patch, NewGRFs: Manpower industries, PolTrams, Polroad, 600mm narrow gauge, wired, ECS industry extension, V4 CEE train set, HotHut.
Another favorite games: freeciv longturn, OHOL/2HOL.
User avatar
FLHerne
Tycoon
Tycoon
Posts: 1543
Joined: 12 Jul 2011 12:09
Location: St Ives, Cambs, UK

Re: More height levels (in trunk since r27010)

Post by FLHerne »

Ok, so you answered a two-month-old question whose author already acknowledged the answers he got. :?

So I will too. :P

My problem with 16-level maps is that hills don't often have a real impact unless you go out of your way to confront them - because there's (almost*) no such thing as a large and tall hill.

You can have steep hills that are impassable without switchbacks or tunnels, but they're usually only a few dozen tiles wide so you can just build around them. Alternatively, you can have terrain features too big to build around, but with a dozen or so tiles' rise over a big chunk of map they're more just undulations and a slight nuisance (second track to compensate for slowdown?) rather than a challenge.
There's almost no such thing as a valley-crossing viaduct in a 16-level game, since any two points high enough to make sense are either dozens of tiles apart (smooth undulations) or the tops of tiny steep hills with no conceivable reason to link in the first place. A 12-tile-high ridge can't cut a continent in two - if it's too steep to build a mainline over, it's narrow enough to (sanely) tunnel through. A 50-tile-high one can!

With a 60-level map, you really can have entire towns and cities that are perched on mountainsides, have multiple mainlines running parallel above each other skirting the same hill. A pass in the steep mountains isn't something that comes every two dozen tiles, it's a precious thing converged on in all directions from hundreds of tiles away. A valley isn't just 'gap between two little humps', it can be big enough to have its own local economy and rail network.

Hail MHL (and ic111)! :bow:

[/melodrama]

*Ok, so you can have a big plateau with steep edges. But the actual sloped area is still quite small, and TGP doesn't often make them anyway.
Temporary Permanent signature filling text. Content coming soon delayed indefinitely! Oh, and I have had a screenshot thread.
Linux user (XMonad DWM/KDE, Arch), IRC obsessive and rail enthusiast. No longer building robots; now I ring church bells.
Author of an incredibly boring stickied post about NewGRFs.
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 10 guests