More height levels (in trunk since r27010)

[ChillCore]

Thank you.

My pleasure.

I will probably have to ask you for help regarding the smallmap patches. They are your code as far as I remember, and as trunk has changed as well at some of the locations, I don´t actually know what to do there.

I do not have much time to actually code this week and coming weekend, but extracting/looking up something that I already did in my patchpack should not take too long. Feel free to ask or just post the conflict, I'd be happy to help.
No, I do not expect you to look things up yourself in there as the smallmap file is heavily merged with CargoDist.

I will probably publish a version where they are left out for now.

As you wish, I should have a bit more time next week and can then update the smallmap patches myself. The needed changes are not that many IIRC.
I have also included an option to disable the floodwarning colour, as some people do not really like it.

Also thank you for updating the split up version, I know I said I would do it this month but things came up, as they do in RL.
Are you still using HG?

[ic111]

Another question:

In terraform_cmd.cpp, I find the following piece of code:

Code: Select all

	Company *c = Company::GetIfValid(_current_company);
	if (c != NULL && (int)GB(c->terraform_limit, 16, 16) < ts.modheight_count) {
		return_cmd_error(STR_ERROR_TERRAFORM_LIMIT_REACHED);
	}

In the old terraforming algorithm, modheight_count basically was an upper bound for terraforming activities in one terraforming action, enforced by the fact that a fixed size array was used for that. The more heightlevels patch switches to using a variable sized datastructure for that, i.e. it supports terraforming actions regardless how big they are. So, the patch removes that upper bound modheight_count as it is no longer necessary.

Now, what´s unclear to me is the sense of that particular piece of code. Probably it entered trunk after the patch was written, i.e. in the last 8 months? Is this a technical limit one can savely remove when switching to variable sized datastructures? Or is this some new rule to terraforming activities allowed for one particular player? In the latter case, does anyone have a clue what to do with it?

Thanks for any hints

[ChillCore]

Yes, new feature to stop the "level land idiots" in multiplayer games.
When terraforming or clearing land the number of tiles affected is counted and there is a limit that can be set in the config file by these options:

Code: Select all

terraform_per_64k_frames = 4194304
terraform_frame_burst = 4096
clear_per_64k_frames = 4194304
clear_frame_burst = 4096

Burst is the limit for a single terraform action, per 64K frames is multiple terraform actions over a period of time.
When the limit is reached you have to wait a while to be able to continue terraforming but with the default values the limit is hard to reach ...
It was included by Rubidium IIRC, but I do not remember the exact details nor the correct revision when it was included.
If you need more details about the actual implemantation, I suggest asking him.

I have made the changes in the full version of the more height levels patch and it was already included in the last (complete) version I posted.
What you want to do is re-introduce "modheight_count" in three places and introduce "limit" once near the end of the file.
Attached is the diff for terraform_cmd.cpp as it is in the newest version I have. Just search for both in the diff to see where exacly.

Basically it works except for the fact that it counts terraform actions instead of affected tiles (this is also still the case in my patchpack unfortunately).
The number of affected tiles should be introduced in both places marked as TODO:, for which I need your help, and everything should be fine.

Other modifications in terraform_cmd.cpp (for this feature) are not needed and the clearing land code should need no modifications at all as it work/s/ed correctly, IIRC.
If you still have other problems just shoot and I will have a looksie in my patchpack and/or bump my complete version to trunk, although it does not yet include a few additional changes I made in my patchpack.


ps:
When checking the file in my patchpack it sais that the last modification was done in r22451. Adjustment is minor, just adding an if and fixing indentation.
Included by Alberth, code (HackaLittlebit)


EDIT: levelling land != clearing land

[ic111]

Hello,

probably I will have no more time for this for at least one week, so I at least want to publish what I have so far.

Attached is a new split up version of the more heightlevels patch, adjusted for trunk version 22611. It will however NOT run without some further fixing, in detail:

(1) the terraforming issue discussed in the comment just above
(2) some of the SmallMap patches
(3) the SnowLine patch
(4) possibly one or two more, unknown problems

Problem (1) at the moment leads to a compiler error because of a no longer existing symbol (see details above)
Problems (2) and (3) need to be fixed, however I temporarily removed the corresponding patches, basically the game should work without them

During the last two weeks, I spent several evenings with updating the patch, I think I´ve done most of the updating work with the mentioned problems left. Thus it would be great if someone would find the time for having a look at those problems and fix them, feel free to publish a improved version of the attached file.

For installing, just extract the zip, and execute the shell script applyPatches.sh on a plain trunk.

[ChillCore]

I have bumped my complete version and added some bits from my patchpack this morning.
Not sure what went wrong for you as my version still works ... just generated a few games without actually playtesting though.
I will compare it with what you have posted to see what is missing, both in my and in your version. I have not yet compiled yours and pulled a (complete) diff to be able to compare ...
If you have some time, would you mind trying to compile mine to see if you get compiler warnings, please?

The version attached is the previous version bumped to trunk.
I added:
- Enabling/disabling the floodwarning colour in the smallmap.
- Configurable heightlevel for the extra logic when generating industries on new game.

I have not yet added the extra smoothness setting posted previously by olkmernok. (I was going to do so before posting my version but as you seem to have problems I posted sooner.)
The issue with the terraform limit counting actions instead of tiles affected still exists.

Also with recent trunk a newer version of the flat_blacktiles.grf (DOS palette instead of Windows) is needed to avoid pink pixels at the borders. Also attached below.

[rini17]

Played r22612 patch on linux x64, after 6 years it crashed - see attachment. It seems reproducible, when you load earlier autosaved game (also attached).

[ic111]

Hello,

I am currently merging the split up version with the all-in-one-file version and adjusting it to current trunk. For my current version, see the attachment. For installing it, you simply have to extract the archive and execute applyPatches.sh.

Note that this is a highly experimental version. Under svn version 22673, it seems to work more or less, under svn version 22929 (current trunk), it crashes during startup with a segfault, with the following stacktrace:

Code: Select all

Stacktrace:
 [00] ./openttd(_ZNK12CrashLogUnix13LogStacktraceEPcPKc+0x43) [0x82f0893]
 [01] ./openttd(_ZNK8CrashLog12FillCrashLogEPcPKc+0x102) [0x82135b2]
 [02] ./openttd(_ZNK8CrashLog12MakeCrashLogEv+0xa0) [0x8213870]
 [03] ./openttd() [0x82f07e1]
 [04] [0xb77ec400]
 [05] ./openttd(_Z12GetTileSlopejPj+0x15e) [0x83a215e]
 [06] ./openttd() [0x83ca7f7]
 [07] ./openttd(_Z9GetSlopeZii+0x4a) [0x8266dea]
 [08] ./openttd(_Z24InitializeWindowViewportP6Windowiiiij9ZoomLevel+0xbf) [0x83eb0ff]
 [09] ./openttd(_ZN15NWidgetViewport18InitializeViewportEP6Windowj9ZoomLevel+0x3f) [0x83ff9df]
 [10] ./openttd(_Z28SetupColoursAndInitialWindowv+0x11b) [0x826a3fb]
 [11] ./openttd() [0x82df51f]
 [12] ./openttd(_Z12SwitchToMode10SwitchMode+0x2e6) [0x82e10e6]
 [13] ./openttd(_Z15DrawDirtyBlocksv+0x239) [0x823d939]
 [14] ./openttd(_Z13UpdateWindowsv+0xed) [0x840504d]
 [15] ./openttd(_ZN15VideoDriver_SDL8MainLoopEv+0x1e2) [0x83e98b2]
 [16] ./openttd(_Z8ttd_mainiPPc+0xd11) [0x82e0891]
 [17] ./openttd(main+0x48) [0x813c288]
 [18] /lib/i386-linux-gnu/i686/cmov/libc.so.6(__libc_start_main+0xe6) [0xb736ae46]
 [19] ./openttd() [0x815795d]

Question: Does anyone have a clue for the reason for this crash? It must be something that changed between 22673 and 22929. It apparently crashes in GetTileSlope. One possible reason would be that it is called before the map array is initialized.

Thanks for any help regarding this!

Furthermore, for the moment the smallmap patches are not included. The reason is that they are quite difficult to merge, as in the all-in-one-file-version, rather big changes to some new feature regarding an extra color for flooding areas are included. As far as I understand those changes, I don´t think that they should go into this patch, however, currently I have to live with the fact that they are included in the all-in-one-file-version, which I took as reference during merging (as I know by testing that it works).

So, I ask anyone implementing or fixing anything for this patch for two things:
(1) Please do ONLY change the split-up-version. The split-up-version is the version that is being prepared for going to trunk at some time. Merging the all-in-one-file version into the split-up-version is a pain, I spent the whole evening with doing that. I will not do that again.
(2) Please don´t add things not being essential to the patch. That flooding-color-change took me a lot of time today, and in the end it won´t be in this patch anyway I think.

Thanks for any feedback and hints!

[Eddi]

i have not checked the revision numbers, but my first guess would be the new river generator.

[planetmaker]

On the risk of repeating myself: It proved VERY helpful to choose version control which can work with a patch queue representation (hg or git) along these lines (which is a full patch queue with all meta information). More important, for reading such queue the numering in the filenames is very helpful, too.

[ic111]

Well, I don´t think that merging a git/hg/whatever repository against a all-in-one-file version of the patch would have been easier than what I did yesterday. The order of my patches is well-defined by the shell script provided, as the order in the patch-queue is well-defined by the meta data.

For merging, I generated one working copy with the all-in-one-file version of the patch applied, and one working copy with the split up version applied. Then, using a small shell script, I generated one diff file per source code file. Diff files having size greater than zero indicated that there is some issue, some problem to look for. And for the small map code, the corresponding diff file had size 60K. Going through all those differences, and in the end deciding that for now I ignore that code because I don´t yet fully understand the differences (compared with the feeling that they are not actually necessary), was the complicate thing to do.

If there would be more persons working on that patch in parallel, one would need version control, no question, but as long as in fact there is no parallel development (in fact, for three months there was no development at all), I don´t see the point in including the complexity of a decentralized version control system into the project.

[Terkhen]

It's all about reviewing source code. Checking a huge change file by file is a nightmare. There is no way to know why something is changed, as everything is inside the same change.

Checking small, escalated changes that are separated in different diff files is another story. Each patch contains a small change (Codechange, Fix, Feature, Add, etc) and a small description that tells the reviewer (and the developer) what the patch does. As you can see if you check the commit log, all changes made by developers (and all accepted patches) are split this way. This way is chosen because it is easy to review even huge changes as long as they are split in smaller ones. Splitting a huge change between different files (with each file containing part of those huge changes) does not provide any benefits for reviewing.

For an example, you might want to review the old Improved acceleration for road vehicles patch file and try to guess what it is doing. The version that was committed in the end was splitted in a lot of patches. A few patches were commited earlier, but the biggest part of the patch starts at r19336 and you can check it here: http://vcs.openttd.org/svn/changeset/19336/

EDIT: You can check an example of a feature split on several patches here: http://devs.openttd.org/~terkhen/patche ... ed_graphs/

[ic111]

It's all about reviewing source code. Checking a huge change file by file is a nightmare. There is no way to know why something is changed, as everything is inside the same change.

Checking small, escalated changes that are separated in different diff files is another story. Each patch contains a small change (Codechange, Fix, Feature, Add, etc) and a small description that tells the reviewer (and the developer) what the patch does. As you can see if you check the commit log, all changes made by developers (and all accepted patches) are split this way. This way is chosen because it is easy to review even huge changes as long as they are split in smaller ones.

One moment. I did only talk about the difference maintaining the different diffs manually vs. using git/hg/etc. In my previous post, I did not propose an all-in-one-file-version of the patch. So, did we misunderstand each other, or do you see some fundamental problem with the version I posted yesterday? It currently contains 30 different diff files, each is responsible for the changes to one aspect of gameplay, e.g. adding a bridge too high message, or a new viewport algorithm.

My idea in fact is that several of these small patches can live on their own, and thus are candidates for finally being committed separately from other small patches.

[planetmaker]

You don't seem to believe it: but maintaining patches is much easier if you use a patch queue, also when only you yourself maintain that patch queue. It also makes it then feasible to modify the same line in subsequent patches...

Btw, a simple thing like "make the height variable an int (or uint16? (u)int32 - it's a bit inconsistent) is nothing which needs to be split across different patches just on the grounds one being for road vehicles, the other for trains,... Splitting is about functional steps or thought steps, less about files.

[ic111]

You don't seem to believe it: but maintaining patches is much easier if you use a patch queue, also when only you yourself maintain that patch queue. It also makes it then feasible to modify the same line in subsequent patches...

What I don´t believe is that git and hg are simple to maintain. For example, the last time I was forced to use git, I frequently had the task "throw away local changes to file x and get the version of that file from person y". I was told that the only way to deal with that particular situation was doing an explicit merge, although conceptionally there was nothing to merge, just to overwrite.

Seems that I have to give those tools another chance if you really want them, but I so far regard them just as cumbersome - compared with something like svn.

Btw, a simple thing like "make the height variable an int (or uint16? (u)int32 - it's a bit inconsistent) is nothing which needs to be split across different patches just on the grounds one being for road vehicles, the other for trains,... Splitting is about functional steps or thought steps, less about files.

[/quote]

I did not split it against files. I split it against changes for road vehicles, trains, aircrafts, etc. If you say it´s ok, if the changes for trains and for road vehicles and for etc. are in one patch, then there will be such a patch.

But to be honest, in this forum I always heard the order "split things up", "no big patches" and so on, but never a really concrete feedback on the way I finally split it up. So I tried to make such small patches where possible, to avoid the situation that in the end, after I have spent all the work, someone tells me "they are still to big, start again".

[planetmaker]

Personally I'd build this as mercurial (patch) queue: http://mercurial.selenic.com/wiki/MqTutorial

Your previous perception of git and hg and how to work with it (or the perception of the person whom you talked to) seems to be somewhat distorted. A merge can also be a "take everything from this version" or "keep everything from the patch". There's nothing different there when using svn up on a patched svn repo.

EDIT: maybe also http://hginit.com/00.html

[ic111]

i have not checked the revision numbers, but my first guess would be the new river generator.

Thanks for the hint, but as far as I understand it it is not the river generator.

I did a binary search on the svn revision numbers between 22673 and 22929. The last revision that worked with the patch was 22794, the first revision that crashes is 22795. Checked in by Rubidium with the comment

-Codechange: load the intro game the first time without NewGRFs

More precisely, it is exactly the addition of LoadIntroGame(false) near line 717 of openttd.cpp that makes the difference wether the patched game crashes or not. I noticed that the intro game starts up normally in a 22795-game, even with that line outcommented. Which rises the question what the intended effect of that line is. So, before I dive deeper into this, questions:

Can anyone give me some context about what´s the goal of this change? Does it change any basic rules how one should initialize map etc. during startup of an (intro) game? Because my impression (not yet checked 100%) is that the segfault is caused by some map initialization being not yet done at the time the painting code causing the segfault is called the first time.

[Eddi]

That's part of the new method of scanning for newgrfs in a separate thread, so a progress window can be shown. so when starting up, the intro game is loaded first, and then scanning for newgrfs happens afterwards. because of this, the location of the newgrfs is not known at the time of loading the intro game, so they cannot be loaded. (introgame may not contain newgrfs anyway)

[planetmaker]

Random guess: this means that the method to supply the black tiles as NewGRF fails. They should be supplied via a new action5 entry anyway.

[ic111]

Analysis (probably the last stacktrace and the conclusion below it is the most interesting one):

Obviously, the intro game doesn´t know anything about more heightlevels. According to gdb, the game crashes in the first return statement of

Code: Select all

static inline uint TileHeight(TileIndex tile)
{
	assert(tile < MapSize());

	if (AllowMoreHeightlevels()) {
		return _map_heightdata[tile].heightlevel;
	} else {
		return GB(_m[tile].type_height, 0, 4);
	}
}

So, AllowMoreHeightlevels() returns true here. That function returns the setting _settings_game.construction.allow_more_heightlevels, so this setting is true here. That setting being true is the error, it should be false when loading a game without more heightlevels. Now, I wonder what I did wrong. First, settings.ini says

Code: Select all

[SDT_BOOL]
base     = GameSettings
var      = construction.allow_more_heightlevels
from     = MORE_HEIGHTLEVEL_SAVEGAME_VERSION
to       = SL_MAX_VERSION
guiflags = SGF_NO_NETWORK
def      = false
str      = STR_CONFIG_SETTING_MORE_HEIGHTLEVELS
proc     = AfterChangeOfAllowMoreHeightlevels

Second, in AfterLoadGame(), I have the code

Code: Select all

	if (IsSavegameVersionBefore(MORE_HEIGHTLEVEL_SAVEGAME_VERSION)) {
		/* Maybe it is still filled from the previous game. If the currently loaded
		* game was saved with an OpenTTD version where the allow more heightlevels
		* patch was not yet introduced, we know for sure that it was NOT filled
		* during SlLoadChunks (because no MAPH chunk can exist in such a savegame). */
		free(_map_heightdata);
		_map_heightdata = NULL;

		/* Also, the default for such a game is that this patch is turned off.
		* However, the player may turn it on using the menu afterwards. */
		_settings_game.construction.allow_more_heightlevels = false;
	}

And, MORE_HEIGHTLEVEL_SAVEGAME_VERSION is

Code: Select all

#define MORE_HEIGHTLEVEL_SAVEGAME_VERSION 250

So, allow_more_heightlevels is set correctly by AfterLoadGame. But, my stacktrace says:

Code: Select all

#0  0x0839a1fe in TileHeight (tile=8224, h=0xbffff0cc) at /home/wol/openttd/_trunk_test/src/tile_map.h:81
#1  GetTileSlope (tile=8224, h=0xbffff0cc) at /home/wol/openttd/_trunk_test/src/tile_map.cpp:69
#2  0x083c0e77 in GetSlopeZ_Trees (tile=8224, x=512, y=512) at /home/wol/openttd/_trunk_test/src/tree_cmd.cpp:522
#3  0x08264f6a in GetSlopeZ (x=512, y=512) at /home/wol/openttd/_trunk_test/src/landscape.cpp:281
#4  0x083e115f in InitializeWindowViewport (w=0xb23332a0, x=0, y=0, width=848, height=480, follow_flags=8224, zoom=ZOOM_LVL_BEGIN) at /home/wol/openttd/_trunk_test/src/viewport.cpp:213
#5  0x083f5a8f in NWidgetViewport::InitializeViewport (this=0xb2333310, w=0xb23332a0, follow_flags=8224, zoom=ZOOM_LVL_BEGIN) at /home/wol/openttd/_trunk_test/src/widget.cpp:1812
#6  0x08268486 in MainWindow () at /home/wol/openttd/_trunk_test/src/main_gui.cpp:250
#7  SetupColoursAndInitialWindow () at /home/wol/openttd/_trunk_test/src/main_gui.cpp:526
#8  0x082db83f in LoadIntroGame (load_newgrfs=<value optimized out>) at /home/wol/openttd/_trunk_test/src/openttd.cpp:280
#9  0x082dd5b6 in SwitchToMode (new_mode=SM_MENU) at /home/wol/openttd/_trunk_test/src/openttd.cpp:984
#10 0x0823c6ef in DrawDirtyBlocks () at /home/wol/openttd/_trunk_test/src/gfx.cpp:1571
#11 0x083fb01d in UpdateWindows () at /home/wol/openttd/_trunk_test/src/window.cpp:2456
#12 0x083df84a in VideoDriver_SDL::MainLoop (this=0x862d008) at /home/wol/openttd/_trunk_test/src/video/sdl_v.cpp:581
#13 0x082dcd51 in ttd_main (argc=1, argv=0xbffff534) at /home/wol/openttd/_trunk_test/src/openttd.cpp:727
#14 0x0813b928 in main (argc=1, argv=0xbffff534) at /home/wol/openttd/_trunk_test/src/os/unix/unix.cpp:266

So, TileHeight is called indirectly from SetupColoursAndInitialWindow, which is called just _above_ SaveOrLoad which triggers AfterLoadGame.

Now, my question to the experts is: Why? Why is TileHeight called before the game is loaded? Do I deal correctly with allow_more_heightlevels, do I make some other mistake, or is this some bug independent from more heightlevels?

(EDIT: Added missing lines of stacktrace. Looking on it again: Is it possible that the first call of LoadIntroGame generates a landscape, the second one (that stacktrace is apparently from another call of LoadIntroGame) reinitializes the allow_more_heightlevels variable for some reason, but leaves the map array as it is, and in this situation calling TileHeight is harmful?)

[Eddi]

not entirely sure about the internals of startup, but it might create a completely empty map first, before trying to load the intro game.