Generally the number of reported bugs depends mostly on the number of players using a feature. Stable releases have factor 100 more players than betas, which have factor 100 more players than nightlies, ...
So, a bug, which was introduced 5 years ago and went through 5 stable release branches before being noticed, could be considered hardly important: Whether it is fixed or not is equally unlikely to be noticed. (Mind that this only applies for gameplay/usability issues. Security issues use a different scale )
A big share of bug reports are actually by new players, who use features in non-intended ways, which an experienced player would not do; just that OpenTTD is unable to tell the player what is wrong.
However, new features cannot be granted that kind of slack. A bug which is noticed within 10 days in a nightly, is noticed within 1 day in a beta, and is reported by 10 people within 3 days in a stable release (only some people check whether the bug is already reported).
More heightlevels is currently in such a state. FS#6156 ("Top of tall(?) buildings appear/disappear at random") first looked like it only needs fixing some inconsistencies in some constants (which were actually introduced by extra-zoom 2 years ago). But when I looked at it in more detail, the analysis of the code quickly derailed.
You can find a detailed analysis below, but the short story is: It likely cannot be fixed, unless it is rewritten. Unfortunately, the current code looks more like doing trial-and-error until it passes a few tests, rather than truly understanding what needs doing.
So, how to progress from here?
It does not seem likely that anyone is going to fix the current code before 1.5. The amount of work is just too big, especially with all the added work-arounds in the new code.
We also cannot release an OpenTTD version where buildings glitch all over the place, even in old maps.
The only available option is to pull the emergency brakes.
If there are no convincing ideas that the drawing is fixed before 1.5, more heightlevels will be disabled before beta 1.
- The setting will be restricted to the old value of maximum height 15.
- The viewport drawing implementation will be reverted.
- Other parts of more heightlevels remain in trunk, in particular savegames are not changed.
- You can no longer create new games with more heightlevels (that is, the max height becomes 15 again).
- Savegames created with more heightlevels will load fine, but glitch all over the place to an extent that is no longer playable.
- If patch packs continue to provide the old implementation, the savegames will be compatible.
- If a new implementation reaches trunk, the savegames will work again (without glitches).
The internal game structures of 256 height levels will stay, it's just not usable above height level 15 because the render code does not understand how to draw those height levels without glitches.
For a coder this means:
- You can start from a simple drawing algorithm (the one that works for 15 levels from before MHL) to implement more heightlevels, instead of having to refactor a complex renderer that fails for every map.
- With implementing a new drawing algorithm you get the praises for making MHL return to main OpenTTD, instead of being an unnoticed bug fixer.
Sorry for the mess. If you miss MHL, you can likely continue playing with it in a patch pack, resp. finish your current game without updating the nightly.
There is nothing bad in using a patch pack, the smaller and more experienced user-base enables them to provide more experimental features.
Stable OpenTTD cannot get away with that (sadly).
Just use the version you have fun with
Detailed analysis of the MHL drawing algorithm and the issues with it:
Basic idea:
[list]
[*] Drawing viewports in OpenTTD happens by drawing tiles and stuff on them.
[*] To do this, it is first determined which tiles have graphics that potentially reach into the viewport, and thus need to be considered for drawing.
[*] The old implementation before MHL just extended the drawing area by a fixed amount of pixels to account for tall buildings and bridges on tiles, which are below the viewport by themself.
[*] With more height levels this strategy becomes nonviable. Bridges can be way above the actual tile, so in the worst case you have to consider bridges on tiles 255 height levels below the viewport border.
[*] MHL solves this by scanning tiles below the viewport for bridges, and only consider these tiles for drawing if there actually is a bridge. Unlike the old algorithm which just always drew them.
[*] In the details you also have to account for the height of building sprites on tiles, and the height of bridge sprites above the actual bridge level.[/list]
The implementation:
When analysing the details of the code in order to fix FS#6156 I encountered this piece at the core of the algorithm.
[code]
/* Calculate, how many tiles below upper_tile, a worst case bridge intersecting upper_tile in
* terms of painting can be located. Lets inspect that formula in detail:
* ... - 5: The magic constant near the beginning of ViewportAddLandscape accounts for 5 harmless heightlevels a bridge can have. Thus subtract them.
* ... / 2: Four heightlevels account for one tile height. On the other hand, if landscape ascends from upper_tile southwards, this can account for
* as many additional heightlevels as we step southwards. In combination: A division by two gains the number of tiles to step southwards.
* ... + 1: Avoid rounding errors, and fall back to the safe side.
*/
int worst_case_steps_southwards = max(0, ((int)_settings_game.construction.max_bridge_height - 5) / 2 + 1);
[/code]Honestly, this is kind of jaw-dropping. For context, the first item refers to:
[code]
int viewport_bottom = _vd.dpi.top + _vd.dpi.height + 116;
[/code]The "-5" corresponds to the "116".
For the non-coders, what happens here is fixing by adding more duct-tape (add a "-5") rather than removing all duct-tape (remove the "+116" so you don't need to add the "-5") and handle the problem properly.
Adding a single number may not look very bad to you, but this is happening at a lot of places (at many places because elsewhere a constant was added too), and the new duct-tape is indistinguishable from the old duct-tape, and from the numbers that should really be used. It's like making a package, but it doesn't close correctly, so you start adding more and more rope until it seems to stay together. This is fun as a surprise gift to keep the receiver busy for a long time, but for code that needs to stay clean and maintainable, this is deadly as you cannot get in and understand the structure any more.
A second problem is that numbers themselves don't tell how they were derived. An example you may know is the "The hitchhikers guide to the galaxy" book, where they know the answer is 42, but they don't know the question that resulted in this answer.
The entire OpenTTD code is filled with numbers, but in the form of names like TILE_HEIGHT or TILE_PIXELS. This makes the code readable and understandable so you can change things and understand how that will affect the result. The drawing code of the MHL code does not do this, so to fix this code from its current state, a long journey is ahead. Note that in many cases, a number is not a single name, but the result of a calculation. For example, 56 could be 5 * TILE_HEIGHT + TILE_PIXELS / 2, but also many other expressions. Needless to say this makes deriving the formula from the number a lot harder.
To summarise the issues with this piece of code (which is just a representative example, there are many more):
[list]
[*] The "-5" should not be there is the first place. Instead the "116" should be removed. There is no point in reverting constants from other places, if there is nothing inbetween that relies on them.
[*] There are many magic numbers here. The code does not use symbolic constants like TILE_HEIGHT or TILE_PIXELS.
[*] The constants are not expressed as terms of symbolical values, instead the terms are evaluated and the result is inserted as plain new magic value.
[*] Height of buildings and height of bridge sprites above the actual bridge level are not handled by the current code.
[*] The rounding towards a specific direction at the end is to be expected. But there are about 5 other places above and below that code, which apply another level of rounding. Rounding is needed, but only once at the end. Not after every intermediate step.[/list]
Conclusion:
The basic idea of the MHL algorithm is correct. But the implementation of the drawing part suffers from bad practices which make it hard to maintain and impossible extend further with new features. Some issues are not addressed at all.
This slipped the attention of all of us and it likely will need some time to figure out a right way. This means we need to pull the emergency brake now, so that the 'stable branch' keeps to deserve its name.
Reverting back to 15 height levels is not nice, but by doing it before the 1.5 release means that only users of the nightlies are affected, which is better than all OpenTTD users (most users only play stable releases).
Secondly, reverting the drawing code gives a stable and working starting point for fixing, which is better than starting from mostly but not quite fully working code, where nobody knows what part is good and what part is not.