Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The MoreHeightLevels Patch #4126

Closed
DorpsGek opened this issue Sep 15, 2010 · 37 comments
Closed

The MoreHeightLevels Patch #4126

DorpsGek opened this issue Sep 15, 2010 · 37 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

ic111 opened the ticket and wrote:

This patch contains all new gui text strings necessary for the more heightlevels patch. It only adds new strings, and does not alter existing ones. Thus, when applied, it is not supposed to change gameplay in any way, it just alters strings needed later. The main reason why I put them in a separate patch is that this way, patch generation is less complicate and less merging of files has to be performed.

See the patch file for what strings will be added.

No other parts of the moreheightlevels patch are needed before this patch can be applied.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/4126
@DorpsGek
Copy link
Member Author

SmatZ wrote:

This patch doesn't meet OpenTTD quality requirements.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8787

@DorpsGek
Copy link
Member Author

ic111 wrote:

Well, I started with simple patches to get a feeling for the right formal criteria. However, from your comment I don't get which one I missed. Could you maybe be a bit more verbose about this?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8789

@DorpsGek
Copy link
Member Author

Alberth wrote:

  1. Don't touch any strings but english.txt

  2. Only supply strings when you need them in the code, as part of that patch.

  3. Trying to be funny is not recommended:
    +STR_TERRAIN_TYPE_FALLEN :Fallen ... cant get up
    +STR_CONFIG_SETTING_ROUGHNESS_OF_TERRAIN_GO_WITH_THE_FLOW :Go with the flow

  4. Start with the core of the patch, if that is not accepted, the other stuff is not useful to consider.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8792

@DorpsGek
Copy link
Member Author

ic111 wrote:

Concerning (1): I didn't know that, maybe something for the checklist http://wiki.openttd.org/Patch_Checklist ?
Concerning (2): Well, I can do it that way as well, then this patch can be discarded and the parts go to the individual sub-patches
Concerning (3): Ok.
Concerning (4): This is exactly the idea. I started with this patch, because additional strings obviously don't break anything existing, and several other sub-patches need them. But given (2), this argument doesn't hold any longer for this sub-patch. For an overview of the relationships between the sub-patches, I add a pdf and two xls files I already added (in a bit older version) to the forum thread.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8794

@DorpsGek
Copy link
Member Author

ic111 wrote:

I have finished the terraforming part of the patch. It can be applied on plain trunk, i.e. it is a patch on its own which does not depend on other parts of the more heightlevels patch. After being applied, the game is supposed to work as it used to work, so this patch could theoretically be applied (significantly) before the other patches.

The core part of this patch is a new terraforming algorithm. Why was the old algorithm not usable for me? Well, look at TerraformerState.tile_table in the old algorithm. This is an array whose size scales with the size of the biggest possible area affected by one terraforming action. And the size of this area is MAX_TILE_HEIGHT * MAX_TILE_HEIGHT. For max. heightlevel 16, we have an array of size 609. For max. heightlevel e.g. 50 it would have size 5301, for max. heightlevel 255 (the biggest possible after the more heightlevels patch was applied), it would have size 131581. This is the first half of the story. The second half can be found e.g. in function TerraformAddDirtyTile. Here, a linear search in that array is being performed. Thus, in the old code TerraformAddDirtyTile is O(MAX_TILE_HEIGHT^2).

The idea of the new algorithm is as follows: The heights of tiles in the game are obviously not independend of each other. For example, if tile (10, 17) has height 3, then tile (11, 17) can have either height 2, or height 3, or height 4. If it would have another height, then the very basic assumptions about geometry in the game would be violated.

Now, suppose we want to raise this tile to height 4. After this change, tile (11, 17) may have height 3, 4 or 5. If for tile (11, 17) any of these heights is ok, then for tile (12, 17) any of the heightlevels 2, 3, 4, 5, 6 is ok with respect to that specific terraforming action. Thus, the algorithm maintains a map TileIndex to Legal Heightlevel Bounds, where all conditions regarding legal heightlevels raised from the above argument are stored. A recursive algorithm adds neighbor tiles with their respective bounds to that map, until the current heightlevel of such a neighbor tile is within the bounds that will be active after the terraforming.

For example, if tile (11, 17) currently has heightlevel 3, and we raise (10, 17) from heightlevel 3 to heightlevel 4, then we can stop at (11, 17), its neighbors are not relevant for us here. If however, tile (11, 17) would have heightlevel 2, then we know that we have to do something because a tile of height 2 cannot exist next to a tile of height 4. Thus, we have to inspect the neighbors of (11, 17) as well, because maybe e.g. (12, 17) has heightlevel 1, which would mean that we would have to raise it either. This way, we have a data structure of size actually affected tiles.

I have profiled the code today. Basically, I measured the duration of CmdLevelLand under the following conditions: Have a scenario at height 15 with one city and plain land. Play this scenario, pause game, activate build while paused. Pick a position, lower land, lower land at the same position again, etc. Thus, after measurement (1), at the position we have height 14, after measurement (2), we have height 13 and so on. Each of those measurements were repeated five times, getting measurements 1a, 1b, 1c, 1d and 1e and so on, and the average calculated.

Results (each number is the number as returned by TOC, for one call of CmdLevelLand (average of five times test + execute). 175 means 175000.
Measurement / New code / Old code
(1) / 175 / 341
(2) / 421 / 709
(3) / 764 / 2202
(4) / 1168 / 2828
(5) / 1696 / 3602
(6) / 2326 / 4581
(7) / 3051 / 5672
(8) / 3870 / 6897
(9) / 4929 / 8487
(10) / 6031 / 9120
(11) / 7372 / 12151

We see an advantage for the new code, however I always had a relatively big variance in the tests. And I didn't mainly rewrite it because of speed considerations for such relatively simple terraforming operations (after all they are one time anyway, so the player won't generally notice the difference), but because of its intrinsic problems with more heightlevels as described above.

I have added the file terraform_cmd as well, since the diff output doesn't look that readable and basically I replaced almost the whole file anyway.

If there are question, notes etc. please ask.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8799

@DorpsGek
Copy link
Member Author

Yexo wrote:

I see absolutely no reason to include MarkTileDirtyByTileOutsideMap (in this patch)

"Note: With the current terraforming tools, you should never see this error." <- so why include it?

MAX_BRIDGE_HEIGHT and the code that returns it shouldn't be in this patch. It has nothing to do with only a new terraforming patch.

Currently not used debug functions that maybe useful at some time in the futura. <- If they are not used don't include them. Unused functions means dead code.

The most interesting test for speed would be trying to flatten a complete map. Did you also test that? Next to speed, how much memory does openttd need if a user tried to flatten a 256x256 map in one command?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8800

@DorpsGek
Copy link
Member Author

ic111 wrote:

"Note: With the current terraforming tools, you should never see this error." <- so why include it?

You can call the algorithm with an input that actually enforces this message. But, the current terraforming tools don't do so. So, from an algorithmic point of view I need that message there, otherwise the algorithm might behave incorrect, if someone decides to use it under certain circumstances (i.e. it would run into a case that is not solvable => error message). One example for such a at least possible usecase would be non-interactive usage in a map generation algorithm. Of course we can change the message, if you don't like the "you should never..." part, then we'll drop it.

The most interesting test for speed would be trying to flatten a complete map. Did you also test
that? Next to speed, how much memory does openttd need if a user tried to flatten a 256x256 map in > one command?

Space: Currently I would say about 10, perhaps 12 bytes per affected tile (mainly mapping a 32 byte value to a pair of 16 byte values in a map). Is this too much? If yes, I would have to find some solution for that usecase, which of course would add some additional complexity to code.
Conceptionally one might think about splitting up the area in regions to level separately, but in practice I run into the problem that then namely cost calculations for testing become difficult.

Is there an easy way to actually measure space consumption of OpenTTD?

The other things will be fixed in a v3 I will probably publish tomorrow.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8801

@DorpsGek
Copy link
Member Author

Yexo wrote:

Oh, and about "After being applied, the game is supposed to work as it used to work": Without your patch if I try to flatten an area and part of it fails than the rest is still terraformed. With your patch the complete operation failed.

We've discussed this behavior in the past and the result at that time was: terraforming as much as possible is the wanted behavior. Your patch breaks that.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8802

@DorpsGek
Copy link
Member Author

ic111 wrote:

I see your point, and it was not my intention to break that.

However I so far run into a technical problem: The old code achieves this behaviour by calling CmdTerraformLand via DoCommand from CmdLevelLand. I.e. it levels by terraforming each tile on its own to the desired heightlevel. I can do this too, then I get the desired behaviour, use little memory as a good side effect, but loose the advantage that I can level by touching each tile exactly one time.

Profiling results (leveling a 256x256 map in scenario editor, always the same map, multiple tests, M denotes millions):
Old code: About 1050M
New code, but with old CmdLevelLand calling CmdTerraformLand: About 5000M
New code, as I wrote it originally: About 400M.

Thus, in the version you had I am about a factor 2.5 faster than in the old code (while using a fair amount of memory for a short moment), in the version that fixes your bug I am about a factor 5 slower.

Maybe I can combine both versions, getting advantages from both, however to me these manual calls to CmdTerraformLand look a bit well hacked. At this place, to me the code doesn't look very intuitive... Possible solution: Maybe I can find out initially, which tiles in the area can't be demolished, and register them with their current height instead of the desired height of levelling in the map.

EDIT: BTW, is there some howto etc. for profiling OpenTTD using the usual gnu profiling tools? I mean, TIC and TOC work well, but require me to alter the code.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8803

@DorpsGek
Copy link
Member Author

Terkhen wrote:

The terraforming patch gives warnings under mingw32.

About the "fail completely if any obstacle is found" change: Players and AIs will rely in the old behaviour while building. I agree that this should not be changed.

I did some profiling of this part of the code a few months ago, see (http://www.tt-forums.net/viewtopic.php?p=864183# p864183). Doing a terraform that would have nothing to terraform (for example, levelling flat land) would run the execution phase in the patched method even if it was not required.

For profiling, check the --enable-profiling option of configure.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8804

@DorpsGek
Copy link
Member Author

ic111 wrote:

The terraforming patch gives warnings under mingw32.

Which ones? Could you post them?


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8805

@DorpsGek
Copy link
Member Author

Terkhen wrote:

Here they are. IIRC they already have been mentioned elsewhere.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8806

@DorpsGek
Copy link
Member Author

ic111 wrote:

I completely rewritten and much smaller version of the patch. I dropped the original approach and stayed much closer to the existing code. Namely, I replaced the arrays and the linear searches on them by a map for the heightlevels and a set for the dirty tiles, respectively.

Profiling results for (1) levelling a random 256x256 map to height 6, and (2) for levelling a 250x250 part of a 512x512 map from heightlevel 15 to heightlevel 0.

(1): New Code: About 1300M (test), 2000M (execution)
(1): Old code: About 700M (test), 1090M (execution)
(2): New code: About 66000M (test), 9000M (execution)
(2): Old code: About 62000M (test), 6500M (execution)

So, the fixed-size arrays are actually faster in this usecase. Now, I might experiment with using vectors instead of a map/set, with the well-known problems when datastructure grows. But, ususally it will not do too much (as in levelling, each point is terraformed separately), so maybe vectors are actually better here in terms of performance. But probably don't lead to better code...

Independent from that: For in-game terraforming, the exact speed doesn't matter too much I think, the operations are simply too small and seldom to dominate the workload. The critical point, you are right, is levelling in the scenario editor. But, in the scenario exact costs don't matter, do they? Also, is the form of partial levelling you require in-game required in the editor? I ask, because either the form of levelling "simply get rid of anything in the way" and the form of levelling "abort in case of first error" could be implemented easily with my first algorithm, which for levelling large regions as common in the editor is considerably faster according to profiling. One could even give the player the choice which variant he likes.

Given this, one might think about using the first algorithm in the editor and the second in-game (i.e. at some later point in time add a patch that replaces the scenario editor algorithm). Which of course means there would exist two terraforming algorithms. However, I think once implemented a terraforming algorithm will probably stay quite stable.

What is your opinion?

BTW. concerning the MingW warnings: The where caused by printing costs in DEBUG statements. In the way DEBUG(map, 0, "...%lld...", (int64)cost.GetCost()) as someone told me. But seems that this variant of debugging costs is not platform independent...

EDIT: For test purposes, I replaced (a) the set by a vector and (b) both set and map by vectors. The result then is that both above usecases (1) and (2) get about five to ten percent faster if only the set is replaced. If the map is also replaced, the performance gets worse. So in summary, set and map are the conceptionally proper data structures, and to me it looks like that becoming significantly better using a variable sized datastructure and not an array is at least difficult.
Thus, I would prefer staying with v3 (or a similar version in case you find some bugs etc. in it), and maybe, if it turns out to become necessary, doing something special for huge levelling actions in the editor as I sketched out above.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8808

@DorpsGek
Copy link
Member Author

ic111 wrote:

Hello everybody,

this is the first version of the ViewPort part of the MoreHeightLevels patch.

Background: The old ViewportAddLandscape function assumed a fixed maximum height any mountain on the map could have (i.e. 15 * 16 = 240 pixels). This height defined the set of tiles to be repainted in case something on the map changes. With more heightlevels, this approach is no longer feasible, as repainting e.g. 50*16=800 pixels just because there might be a mountain of this height is not an intelligent approach.

The idea of this patch is: Think of the map being partitioned into rows and columns. If you go e.g. one tile to the east, then you increase the column by two. If you go one tile to the south, you increase the row by two. If you go one tile to the southeast, you increase both row and column by one. The algorithm first determines the leftmost and the rightmost column that will be affected by the current call of ViewportAddLandscape. Then, for each column in between, it determines the minimum and maximum rows that can be affected by this call for this column. Those minimum/maximum rows depend on the heightlevel distribution within some column.

Furthermore, for similar reasons, the patched versions paints the black area outside map by applying a concept of landscape outside map. This concept is quite simple: Imagine the landscape, starting at the edge of map, descending to height zero as fast as possible while not violating the assumptions about map geometry in OpenTTD (i.e. two neighbor corners can only have height difference one). Now, just paint these tiles using a completely black tile grf.

The attached files serve the following purposes:
mhl_PaintingVoidTiles_v2_r20854.diff contains some preparations for painting those black tiles outside map.
flat_blacktiles.grf is the corresponding grf (that needs to be dealt with in a more elegant way lateron).
mhl_GeometryOutsideMap_v1_r20854.diff provides functions for that geometry outside map.
mhl_ViewPort_v1_r20854.diff finally contains the changed painting algorithms.

Note that those patches don't depend on the new terraforming algorithm I published before. They are meant to be applied on plain trunk. However, the ViewPort part contains a change in the terraforming algorithm, i.e. the version published here is that change adapted to plain trunk.

Profiling: I used the intro game for profiling, i.e. started the game, did no user interaction at all, just waited for the profiling output.
Two types of measurements: First, the average running time of the first 100000 calls of ViewportDoDraw:
Old code: 115432
New code: 131218

Second, the average number of calls to the draw_tile_proc per call of ViewportAddLandscape, in the first 100000 calls of ViewportAddLandscape.
Old code: 60.86
New code: 58.92

Question: Did I do profiling right in the sense that that are the relevant results? Especially, I assume that at least some work is done in an asynchronous way, i.e. I will not see it in the running time of ViewportDoDraw. Am I right with this assumption? At least, this assumption was the reason for me to measure the number of calls to the draw_tile_proc.

So far from me, now I am interested in your comments about the idea of the algorithm, its implemention, the way I profiled it, etc.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8827

@DorpsGek
Copy link
Member Author

ic111 wrote:

Hello,

the patch part dealing with positioning the small map correctly. So far, when opening the small map or clicking into the small map to centering somewhere, the heightlevel at point is simply ignored. To reproduce this, open the scenario editor with default landscape at height 15, build a city, open small map, center on one of its buildings. This position will not actually be centered where you clicked afterwards.

This patch corrects this, adjusts the position calculations when
(1) opening the small map
(2) clicking into the small map
(3) painting the map indicators inside the small map.

It must be applied on top of the ViewPort patches published in the comment before, because it uses parts of the calculation code provided there (GetRowAtTile, and all functions called by GetRowAtTile, to be precisely). Technically, one might isolate these code parts into a separate patch to reduce dependencies, but I will only do this if there is a real need for it (e.g. this patch can go to trunk separately), as my time isn't endless either ;-)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8828

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 3, 2010

ic111 wrote:

A small patch introducing a new savegame version. I made it a constant. Giving it the value 250 for now has the sense of avoiding the need to change this patch every time the savegame version in trunk changes.

In the end, one just needs to change the value of the constant to the savegame version of the commit introducing more heightlevels.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8850

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 4, 2010

ic111 wrote:

This patch switches the z_pos of vehicles from type byte to type int32. In other words, the z_pos is treated the same way as x_pos and y_pos - both are treated as int32 both in code and in savegame anyway.

The patch depends on the SaveGameBase patch published before. A game with just that two patches applied worked in a short test, including saving and loading savegames, but it is not meant to be applied separately. The next patches I will publish will adjust the z_pos in various places in the road vehicle, disaster, train, etc. code.

The situation so far is that the z_pos is sometimes treated as int (e.g. TerraformTile_Road, DrawGroundSpriteAt), and sometimes as byte. Byte as data type is not big enough with more heightlevels, thus I think unifying this to int is the best option for that attribute. This patch increases the amount of uncompressed data saved per vehicle in a savegame by three bytes.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8855

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 4, 2010

ic111 wrote:

The (only) two changes affecting only roads and road vehicles. Exchanging the data type of z at one position in road_cmd.cpp (z in TileInfo is an int, so we can make the local variable where that value is stored also an int), and in roadveh_cmd.cpp exchanging the type of z in the signature of a local function.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8856

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 5, 2010

ic111 wrote:

These rather small patches change the data type of z in the train, tree, tunnelbridge and common vehicle code where necessary. Per patch, only about two or three locations in code are affected. Not included in these patches are changes in the afterload code, as well as specific changes like introducing a maximum bridge height.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8863

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2010

ic111 wrote:

This patch introduces a maximum height for bridges, being set to 15 heightlevels.

Reason: Taking track about redrawing the regions behind bridges is at least very difficult if the bridge gets higher than the 15 heightlevels which are the maximum without more heightlevels. At least I didn't find a feasable way for implementing this.

Given that a bridge higher than 16 heightlevels is quite unrealistic anyway (imagine how high it would be in reality) I decided to simply forbid such bridges. So, bridges possible without the patch are still possible, but very high bridges which would theoretically be possible if you have very deep valleys are not.

The patch is meant to be applied to plain trunk. To test it without more heightlevels, simply set the constant in tunnelbridge.h to a low value e.g. 2.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8869

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2010

ic111 wrote:

This patch adjusts the aircraft moving scheme. In an unpatched game, an aircraft ascends to some fixed flight altitude that is above the highest possible mountain on the map.

Having a fixed altitude is no longer senseful with more heightlevels. This patch instead defines a minimum and maximum altitude above ground. When an aircraft flies towards a high mountain, it will eventually come closer to ground than the minimum altitude. Then, it will start to ascend. To avoid an effect of stairclimbing, it will ascend until (minimum altitude + (maximum altitude - minimum altitude) / 2) above ground. To keep track about this, there are two new attributes for aircrafts: aircraft_is_in_min_height_correction and aircraft_is_in_max_height_correction.

The patch depends on the GeometryOutsideMap, SaveGameBase and VehiclesBase patches published before.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8871

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 7, 2010

ic111 wrote:

This patch is quite similar to the previous one, except that it is for disasters. Unfortunately, aircrafts and flying disaster vehicle don't share the movement code, thus there must be separate patch for disaster vehicles.

The patch depends on the GeometryOutsideMap, SaveGameBase, VehiclesBase and Aircrafts patches published before.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8878

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 8, 2010

Terkhen wrote:

These are my results for profiling trunk against mhl_Terraformer_v3 under various conditions. They show both the cost of the testing and execution part (DC_EXEC) of the leveling, unless the execution part was not needed.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8883

@DorpsGek
Copy link
Member Author

ic111 wrote:

Thanks for profiling! Given my own results, your results don't surprise me. The question (I already wrote some ideas regarding this above in a previous comment) is now what's the conclusion from that.

My observations are: Terraforming with v3 is considerably slower than the current terraforming algorithm. However, to make more heightlevels work, I need some variable sized data structure here. So, how to deal with this situation? In-Game terraforming usually is small enough for ignoring the somewhat lower speed. I don't think that terraforming 64x64 in one command is / should be the usual way to play OpenTTD.

Thus, in my opinion if there is a problem then it is in the scenario editor. Here, I can think about three possible ways to terraform:
(1) Abort at any error
(2) Terraform as much as possible (present behaviour)
(3) Terraform anything and get rid of any objects that are in the way.

The algorithm I have posted in v1 and which is much faster for huge terraforming commands works well for (1) and (3). However, unfortunately it is not really suitable for (2) because the checks needed for performing (2) are somewhat against the idea of the algorithm. Also for reasons of algorithm design, the speed advantages of v1 is the bigger the bigger the heightlevel change is, i.e. its computational complexity doesn't have a O(delta_h) part.

Another possibiliy would be switching to variable-sized data structures only if the array overflows (which will happen fairly seldom in practice I think).

So, what is your opinion about this issue?

The attached patch contains the adjustments regarding signs, i.e. changing the type of variable z for signs. It depends on the SaveGameBase patch.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8895

@DorpsGek
Copy link
Member Author

ic111 wrote:

This patch adjusts the y offset when taking a screenshot of the whole map, depending on the heightlevel at (0,0). If we would not do that, a very high mountain at (0,0) might be outside the map.

This patch doesn't depend on any other patch and can be applied on plain trunk.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8896

@DorpsGek
Copy link
Member Author

ic111 wrote:

The Core patch provides the changes for actually activating more heightlevels. In detail:
- a new array for storing the additional heightlevels
- a new setting allow more heightlevels
- the code needed for taking track of new heightlevels with respect to savegames

The new array is used if and only if the allow more heightlevels setting is activated. If not, the heightlevels are stored as in the past. Thus, the new feature "more heightlevels" is optional, the players can activate it, but they are not forced to do so.

For existing games, switching to more heightlevels is possible. Also, if more heightlevels are activated in the settings, deactivating them is possible, as long as there is no mountain higher than 16 on the map.

The Core patch only depends on the SaveGameBase patch in terms of compiling. However, in this minimal configuration, things like terraforming, aircrafts, viewport drawing etc. would go wrong. Thus, the patches published previously should be applied together with the Core patch.

To make things easier, I put them into an archive and wrote a simple shell script for applying them. Note that the archive contains two mhl_Help* patches. They are needed, because certain patches (e.g. Terraformer and ViewPort) both are meant to be applied on plain trunk, thus if they are applied on top of each other they will fail. The mhl_Help patches corrects the problem in this special situation, otherwise some manual work would be needed. The BridgeTooHighMessage patch also suffers from this, but as I don't consider it essential for testing, I simply left it out of the script.

I tested this on r20915, and a test was successful, taking plain trunk and applying the provided patch set actually activated more heightlevels. Not yet published are changes to the map generator, the heightmap loader and regarding the snow line. At least the latter two can be published completely separately to the more heightlevels patch according to ChillCore. In my test, also the map generator (tgp to be precisely) generated a map with max. heightlevel 16 as usually.

As a side note, I noticed some glitches in the north west outside the map. They are not there in a configuration where v32 of the complete patch is applied to plain trunk. Also, there are some differences between viewport.cpp in v32 and the version of viewport.cpp generated by this patch set. Thus, I assume that this is a problem introduced when splitting things up, which I will have to fix in a future version of e.g. the ViewPort patch.

Any feedback and comments are welcome.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8909

@DorpsGek
Copy link
Member Author

ic111 wrote:

The next version of the more heightlevel patch set. I fixed the issue regarding glitches outside map I pointed out above (new version mhl_ViewPort_v2), and added an additional patch file mhl_ViewPortAfterCore, which adds the remaining changes in the viewport code, which depend on the Core patch.

Tested on r20920. Uncompress it in the OpenTTD base directory, and execute the script applyPatches. And if the tar.bz2 format and the sh-Script are a problem, since they are not supported on your system, please complain.

EDIT: mhl_v2.tar.bz2 contained two corrupted files, due to a technical problem. Thus, v3 is the same as v2, but without corrupted files. I also added a zip version.

BTW: Am I right that my permissions are not sufficient for deleting the corrupted file? I see a delete checkbox, but can't activate it.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8920

@DorpsGek
Copy link
Member Author

ic111 wrote:

This patch is independent of the rest of the more heightlevels patch. It introduces an additional "Height: " line in the land info window. Sense: If you have much more heightlevels, at least I think giving the heightlevel information its own line is convenient. You can see it more easily than if it is hidden e.g. in the string 137x873x14

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8925

@DorpsGek
Copy link
Member Author

ic111 wrote:

The next version of the more heightlevels patch set. Changes: It includes the mhl_HeightInLandInfo_v1 patch I published before, and a v2 of the VehicleCommon patch, which fixes the type of z when flooding vehicles.

Most remaining changes (I do diffs between trunk plus v32 of the whole patch applied and trunk plus this patch set) are regarding

(1) the small map
(2) the map generator and
(3) the height map.
(4) the snow line

I will not do anything about them before Sunday, October 17th.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8926

@DorpsGek
Copy link
Member Author

ic111 wrote:

The next version of the more heightlevels patch set. I integrated the smallmap, map generator and heightmap patches generated by ChillCore.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8942

@DorpsGek
Copy link
Member Author

ic111 wrote:

I integrated the changes ChillCore provided.

The snowline changes are still missing, but not essential either. Patched games work perfectly with the present snowline behaviour, the only thing is that having a snowline at height 13 if mountains have height 40 or 60 is maybe a bit low...

Beside this, map generation with the original map generator algorithm at present seems to not work. I will discuss this in the forum thread. Map generation with the TerraGenesis algorithm works to the best of my knowledge.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8980

@DorpsGek
Copy link
Member Author

ic111 wrote:

I integrated the changes of ChillCore regarding SnowLine, map generator and comments.

As we are short before declaring the patch finished from our point of view (minus comments from dev side of course), I think this would be a good moment to test this patch set, i.e. download it, apply it on clear trunk and see wether you can find bugs we did not find so far.

I myself am currently playing a game with at present about 50 trains using this patch, and in this game, my impression is that it works :-)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8989

@DorpsGek
Copy link
Member Author

ic111 wrote:

With this version, I declare the process of splitting up the patch finished.

Changes to v7:
- Comments in the applyPatches.sh script, documenting dependencies etc.
- Adjusted the order the patches are applied in the script, according to their dependencies.
- Generated Helper patch for the BridgeTooHighMessage patch

In some cases, there is an outcommented call to a patch (e.g. BridgeTooHighMessage), and an active call to a helper patch (e.g. Help_BridgeTooHeightMessage). In these cases, the outcommented patch is generated relatively to trunk, but can't be applied at this point of file, because it interferes with another patch changing the same file. The helper patch is then generated relatively to a changed trunk.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment8993

@DorpsGek
Copy link
Member Author

ic111 wrote:

This version contains fixes for Yexos comments at http://www.tt-forums.net/viewtopic.php?f=33&t=40844&start=640 (of October 28th 2010).

In details:
(1) Removed mhl_HeightInLandInfo as it was not accepted
(2) Improved comments for GetMin/MaxTileCoordsIgnoringHeight in viewport.cpp, refactored those function to reuse existing code
(3) Fixed variable names in mhl_SmallMapPos_v1
(4) Removed a useless cast in mhl_Train_v1
(5) Used IsInsideMM in mhl_Aircrafts
(6) Called aircraft code from disaster code as far as possible, avoiding code duplication
(7) Using ReallocT in mhl_Core
(8) Fixed off-by-one error when savegame-checking
(9) Created mhl_Help_TileMap as unified diff

Remaining TODO: Put sprites into openttd.grf instead of separate grf.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment9000

@DorpsGek
Copy link
Member Author

DorpsGek commented Nov 7, 2010

ic111 wrote:

A new version. Changes include:

- Fixed bug in heightmap generation (index[array] instead of array[index] - I was shocked that the compiler accepted that nonsense...)
- Put the changes to the old map generator into a separate patch
- Made patch compile/run with r21105

EDIT: A second version, as in the first I forgot to delete several unneeded files.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment9034

@DorpsGek
Copy link
Member Author

ic111 wrote:

Fixing some rejects. No functional changes.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126#comment9081

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Implemented

In r27010


This comment was imported from FlySpray: https://bugs.openttd.org/task/4126

@DorpsGek DorpsGek added Core flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant