More height levels (in trunk since r27010)

[ic111]

Looking at the stacktrace above, it seems to do something with trees (it calls GetSlopeZ_Trees). This doesn´t look like it trying to deal with an empty map.

I would really be surprised if calling things like GetSlopeZ_Trees would make sense before the map is loaded...

[ic111]

Question: qnew and qapplied are described in the tutorial you linked. But what do I have to do in order to generate a patch set that I can release, i.e. what is the mq counterpart to an svn diff?

[Terkhen]

This command will create a diff including all of the patches applied in the queue:

Code: Select all

hg diff -r qparent

[ic111]

Thanks!

hq qapplied says:

Code: Select all

~/openttd/_trunk_mq$ hg qapplied
0 A BridgeTooHighMessage
~/openttd/_trunk_mq$

However, your command produces no output, although there certainly are changes made between qnew and qrefresh:

Code: Select all

~/openttd/_trunk_mq$ hg diff -r qparent
~/openttd/_trunk_mq$ 

Is there anything I missed?

Also, I noticed that "A BridgeTooHighMessage" is probably no good name, so I wanted to rename it to "BridgeTooHighMessage". It says:

Code: Select all

~/openttd/_trunk_mq$ hg qrename BridgeTooHighMessage
Abort: /home/wol/openttd/_trunk_mq/.hg/patches/BridgeTooHighMessage already exists
~/openttd/_trunk_mq$

Looking into .hg/patches, there actually is a file "BridgeTooHighMessage" (I wondered why I took "A BridgeTooHighMessage" as name, but am not sure which one is the one I chose as name). So, does qapplied actually output the names of the patches, or did I manage to corrupt my hg repository?

Sorry for so many questions!

[planetmaker]

hg qseries gives you the name of all patches within your patch queue whether applied or not, hg qapplied the name of the patches currently applied.

You can also always un-apply all patches via hg qpop -a and then directly edit the file .hg/patches/series and rename patch files accordingly yourself within the .hg/patches directory. hg qdiff will give you the diff of the changeset of the currently active patch in the patch queue. hg diff -rqparent:tip will give you the diff from trunk up to and including the last active patch of your patch queue.

You can always look directly at the files in .hg/patches - they're just normal patch files except status and series

[Alberth]

As normal with renaming, you need to give it both the old and the new name, thus

hg qrename "A BridgeTooHighMessage" "BridgeTooHighMessage"



I normally prefix with a unique and increasing number, and end the name with ".patch" like

10_foo.patch
20_bar.patch

then you can copy the .hg/patches/*.patch files to other places, and still know they are patch files, and in which order to apply.

[ic111]

Ah ok, I think I see my error: Of course I have to do hg add, its not sufficient to do a hg init in the directory, right? In the tutorial it is not mentioned, but it seems logical.

This brings me to the next question: How do you organize which files hg has under control? Do I have to do that manually (i.e. hg add all files that are under svn version control, and do that again whenever there is a change in svn)?

As for the "A BridgeTooHighMessage": I tested again on a clean repository, and it seems that the A is the content of an additional column qapplied prints between the order number and the patch name, i.e.:

Code: Select all

~/openttd/_trunk_mq$ hg qapplied
0 A 10_BridgeTooHighMessage.patch
1 A 20_GeometryOutsideMap.patch
~/openttd/_trunk_mq$

Unfortunately, hg help qapplied doesn´t tell me how to read that column...

[Alberth]

Ah ok, I think I see my error: Of course I have to do hg add, its not sufficient to do a hg init in the directory, right? In the tutorial it is not mentioned, but it seems logical.

hg init is for creating an empty repository, you never use that, except when you need to make a new empty repository. In particular, you don't need hg init if you use hg clone.

If you add completely new files to your mq, yes, you need to do 'hg add newfile' before hg qrefresh or hg qnew. hg does not know whether an unknown file is just some temporary file or part of the source code.

You do use git-style patches, right? (http://mercurial.selenic.com/wiki/MqExt ... ial_Queues section 8.4).

This brings me to the next question: How do you organize which files hg has under control? Do I have to do that manually (i.e. hg add all files that are under svn version control, and do that again whenever there is a change in svn)?

Assuming you use a clone of openttd trunk, the repository knows what files are under version control, and a change in any of those files will always be considered as being needed to store in your patch queue (otherwise 'hg qpop' and 'hg qpush' would not work correctly). Only for files not known by hg (and since that hg repo is a copy of svn, files not known by svn) need to be added manually.

As for the "A BridgeTooHighMessage": I tested again on a clean repository, and it seems that the A is the content of an additional column qapplied prints between the order number and the patch name, i.e.:

Code: Select all

~/openttd/_trunk_mq$ hg qapplied
0 A 10_BridgeTooHighMessage.patch
1 A 20_GeometryOutsideMap.patch
~/openttd/_trunk_mq$

Unfortunately, hg help qapplied doesn´t tell me how to read that column...

I don't think such a column exists. I think you have patch filenames "0 A 10_BridgeTooHighMessage.patch" and "1 A 20_GeometryOutsideMap.patch".

Do a 'ls -l .hg/patches' to see the real filenames.

(and unlike in Windows, where you hardly ever type a filename manually, at Linux you enter those names often. So do yourself a favor, and stop using spaces in filenames. I also never UpperCase letters in filenames. The system does not mind, but both spaces and uppercase letters are such a pain to enter manually from the keyboard.

[Eddi]

"hg add" and "hg forget" change which files are under version control, "hg remove" (or "hg rm") forgets the files and physically remove the files from disk, and "hg addremove" forgets all files that have been deleted from disk by any other operation.

[ic111]

I think I made it working. Could someone have a look into the attached archive, wether this is the way you want to have patches published?

I consider the patch in the archive finished (look into the change for tunnelbridge.h for explanation), but mainly I post it to make sure that I do everything right concerning hg and mq.

I don't think such a column exists. I think you have patch filenames "0 A 10_BridgeTooHighMessage.patch" and "1 A 20_GeometryOutsideMap.patch".

I generated it using the command

Code: Select all

~/openttd/_trunk_mq$ hg qnew -m "Patch: More heightlevels: Adding an error message if bridges get too high" 10_BridgeTooHighMessage.patch

and, look into the attached archive, there is no "A" before the name.

So do yourself a favor, and stop using spaces in filenames.

I never use them anyway

[planetmaker]

Yes, that looks like a normal patch "queue" (in quotation marks as a queue usually needs more than one patch )

[ic111]

Good to hear

Question: What should I do with the flat_blacktiles.grf (a new grf containing completely black tile images, which is needed by the more heightlevels patch)? hg add it to bin/data and learn what mq does with binary files? Or something else / nothing at all?

[Eddi]

ultimately you should add it to media/extra_grf so it gets added to openttd.grf (and subsequently also to opengfx_extra.grf). this likely needs an additional action 5 block, which in turn needs a grfcodec/nforenum patch.

other than that, hg can handle binary files, but potentially only by "hg import"/"hg export"

[planetmaker]

Good to hear

Question: What should I do with the flat_blacktiles.grf (a new grf containing completely black tile images, which is needed by the more heightlevels patch)? hg add it to bin/data and learn what mq does with binary files? Or something else / nothing at all?

Like what Eddi said: it ultimately would need merging into the base grf(s), i.e. openttd.grf and ogfxe_extra.grf and adding a new action5 entry. As an example you can find inspiration in my patch queue wrt introduction of grid sprites. 010 adds the sprites to the TTD base grf, adding it to OpenGFX is also simple, and 050 adds the action5. You'll also find also a patch to add a new entry to nforenum/grfcodec.

Doing this, could be the last thing prior to merging the patch queue to trunk, especially as it doesn't make testing easier, especially for people who don't compile themselves, as one would have to obtain new basesets For now you could indeed just hg add the grf file as is and have that grf (and its reading) a separate patch to be replaced by the proper way later.

[ic111]

The first patch version split up using mercurial queues.

I regard most of the patches as finished. For details about the individual patches (why are they needed? what is their idea?), I added a readme.txt file inside the zip archive.

Known issues are:
(1) The crash during startup I analyzed above. I have included a workaround (patch 135_WorkaroundLoadIntroGame) which just comments out one LoadIntroGame(false) call that was added some weeks ago.
(2) Regarding levelling during industry placement, it seems to not do what I want. I will inspect that closer during the next days I think, this may change the IndustryLeveling patch.
(3) The SnowLine patch is still buggy, e.g. default values are not set correctly. I will inspect this also.

Beside this, I have no known issues to the other patches, so for all patches except IndustryLevelling and SnowLine, I will wait for comments, testing, bugreports, etc.

Regarding testing, I did short tests wherever possible, but I did not player any longer game with this patch version yet.

BTW, what is the correct way to update the repository during patch development? Pop all patches, pull and update, and push them again? Or is there some special command for this I missed so far - applying 23 patches again in the correct order would be a bit cumbersome...

[Alberth]

With respect to updating the repository:

Code: Select all

hg qpop -a   # pop all patches
hg pull
hg update
hg qpush -a  # push all patches

It knows the order to apply them.
It aborts when it finds errors, inviting you to fix them, refresh the patch (qrefresh), and do another 'hg qpush -a' to continue.

[ic111]

Concerning the IndustryLevelling patch, I think I remove it.

With the patch enabled, generate some mountainious or alpinist maps with setting very rough. You will get maps that are fine for playing (if you like that type of rough landscape - rough especially on a small scale, with many small hills and depressions), but that don´t contain actually flat places. This is a problem for the industry placement logic during map generation. The IndustryLevelling patch was formerly introduced to make terrain levelling to generate valid places easier for that logic. It seemed to work, but now I noticed that I have hardly any industries on maps as described above.

Some debugging showed me, that not CheckIfCanLevelIndustryPlatform and CheckCanTerraformSurroundingTiles in industry_cmd.cpp are the problem, but PerformIndustryTileSlopeCheck in newgrf_industrytiles.cpp. As far as I understand it, this function matches the slope at some randomly found candidate position against an allowed slope for this type of industry. I don´t know wether this check is new or altered, but it seems to say no for the vast majority of candiate positions before it can even try to level land.

So: If you don´t generate too rough maps (in terms of smoothness), you will see industries even with mountainious or alpinist maps. My conclusion from that is removing the issue of industry placement from the more heightlevels patch, and leave optimization of industry placement during map generation for a possible separate patch.

As a second question: What should we do with the 135_Workaround-issue, i.e. removing the LoadIntroGame(false) call to avoid a crash during startup? Does someone of you plan to have a look at this, do you expect me to dive further into this (however, my knowledge about what happens / is supposed to happen during startup is limited), should I open a tracker item on it? Do not understand me wrong, I don´t expect anyone to do anything soon, I just think that me waiting for you and you waiting for me would be a fairly unproductive state

I think the main question regarding this is: Is my code ok, and I just trap into some buggy or at least strange code in trunk, or should I go for searching for a bug in my code...?

[ic111]

And for the SnowLine patch: Assume the following situation:

allow_more_heightlevels is enabled in the settings.
Now I open the map generation settings dialog. The game evaluates GetMaxSnowLineHeight (new function in this patch) for determining wether it should use the old or new settings (e.g. max. allowed snow line). Debugging shows me that it evaluates allow_more_heightlevels to false here, thus it returns the wrong setting.

Maybe this is a side effect of me commenting out the LoadIntroGame call in the workaround patch?

[Alberth]

The IndustryLevelling patch was formerly introduced to make terrain levelling to generate valid places easier for that logic. It seemed to work, but now I noticed that I have hardly any industries on maps as described above.

Sounds to me like you are solving the problem in the wrong place.
You should do this in context of industry placement, not in context of game level creation.

Eg Andythenorth currently has problems with placement of some big industries during the game. If you solve this problem in context of industry placement, that would also be solved.

As a second question: What should we do with the 135_Workaround-issue, i.e. removing the LoadIntroGame(false) call to avoid a crash during startup? Does someone of you plan to have a look at this, do you expect me to dive further into this (however, my knowledge about what happens / is supposed to happen during startup is limited), should I open a tracker item on it? Do not understand me wrong, I don´t expect anyone to do anything soon, I just think that me waiting for you and you waiting for me would be a fairly unproductive state

If 'you' means me, I have no such plans. Whether other 'you's have such plans, I don't know (we are much less organized than you think ).

Reverting trunk to prevent a crash in a patch means to me the patch is not finished, and I am pretty sure other 'you's will see it the same way. The change was added to trunk for a reason. Reverting the change without addressing the issue it was introduced for, means that your patch would re-introduce that problem. That reduces your chances to get your patch into trunk to about 0.

I don't know what a tracker-item is supposed to do here. A "please change trunk so my patch does not crash" issue is not likely to get any attention, unless you have a convincing case that it is a problem in trunk (and having a patch that crashes does not count, making a program crash with a patch is extremely simple).

I think the main question regarding this is: Is my code ok, and I just trap into some buggy or at least strange code in trunk, or should I go for searching for a bug in my code...?

Do you accept bets on whether you are right, or at least one developer (and very likely more than one) is right?

You should first look at why it crashes, and what needs to be done to prevent the crash (other than undoing the change, as that re-introduces the problem that code is supposed to fix). Once you have that clear, decide the best way to fix it.

[ic111]

Thanks for your comments.

The IndustryLevelling patch was formerly introduced to make terrain levelling to generate valid places easier for that logic. It seemed to work, but now I noticed that I have hardly any industries on maps as described above.

Sounds to me like you are solving the problem in the wrong place.
You should do this in context of industry placement, not in context of game level creation.

Eg Andythenorth currently has problems with placement of some big industries during the game. If you solve this problem in context of industry placement, that would also be solved.

You are right, maybe he is solving the same problem. The (industry) levelling code I first wanted to fix is also in the context of industry placement. It is levelling to make room for an industry. I suspect that the first check where currently on rough maps most of the tries for industry creation fail is too hard, it doesn´t leave any chance for even trying to level an appropriate place.

As a second question: What should we do with the 135_Workaround-issue, i.e. removing the LoadIntroGame(false) call to avoid a crash during startup? Does someone of you plan to have a look at this, do you expect me to dive further into this (however, my knowledge about what happens / is supposed to happen during startup is limited), should I open a tracker item on it? Do not understand me wrong, I don´t expect anyone to do anything soon, I just think that me waiting for you and you waiting for me would be a fairly unproductive state

Reverting trunk to prevent a crash in a patch means to me the patch is not finished, and I am pretty sure other 'you's will see it the same way. The change was added to trunk for a reason. Reverting the change without addressing the issue it was introduced for, means that your patch would re-introduce that problem. That reduces your chances to get your patch into trunk to about 0.

I think you misunderstood what I mean with the term "Workaround". I do NOT intend that 135_Workaround patch being a permanent solution. For me, this is just a tool for making the game start at all, until a real solution for that problem is found. Without that 135-patch, I would not have been able to test any of the small patches after it.

I think the main question regarding this is: Is my code ok, and I just trap into some buggy or at least strange code in trunk, or should I go for searching for a bug in my code...?

Do you accept bets on whether you are right, or at least one developer (and very likely more than one) is right?

You should first look at why it crashes, and what needs to be done to prevent the crash (other than undoing the change, as that re-introduces the problem that code is supposed to fix). Once you have that clear, decide the best way to fix it.

Look again at the end of that comment: http://www.tt-forums.net/viewtopic.php? ... 40#p970372

I analyzed it up to the point that I found out that in LoadIntroGame, map-dependent code (e.g. GetSlopeZ) is called before the intro game is actually loaded. More precisely, SetupColoursAndInitialWindow indirectly calls things like GetSlopeZ. And SetupColoursAndInitialWindow is called before SaveOrLoad in that function.

The crash happens because at that point allow_more_heightlevels is true (this triggers it accessing the new heightlevel array from GetSlopeZ, which will be initialized by AfterLoadGame lateron (if and only if allow_more_heightlevels is true), and the segfault happens because at that point, initialization is not yet done). However, in settings.ini it is defined:

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

Unless I did something wrong in the description in settings.ini (did I?), it should have default value false. So, is it correct that at that point of startup, it not yet has its default value?
And, as AfterLoadGame obviously and sensefully is called in SaveOrLoad, I didn´t have any chance to initialize my data structure at that point, from within SetupColoursAndInitialWindow, above SaveOrLoad.

Another possibility: _If_ at that point some dummy game is already loaded, this might be an explanation why the normal arrays don´t trigger a crash at that point. So, does anyone know wether there actually is the concept of a dummy game being loaded before LoadIntroGame is called?

And at that point, qualified opinions wether at which point in that description (this one and the one I linked above) something sounds weird, something shouldn´t happen in your opinion would help much. Or, wether you see anything I definitely do wrong.