More height levels (in trunk since r27010)

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

I just had a look on the bug that opening the small map for a high mountain leads to the wrong position. The reason is simply that height up to now is completely ignored in this case. As the changes for fixing this are not big, and my code currently is not ready for making a patch, I document them just here. Note that this only solves the case "Open small map, position it to the correct tile". The other case "Click into small map, position main map correctly", where heightlevel information is also ignored, remains to be solved.

Bugfix:
(1) Add function

Code: Select all

int GetRowAtTile(int viewport_y, Point tile);
to viewport_func.h

(2) Remove the static keyword for this function in viewport.cpp
(3) Add an include for viewport_func.h in smallmap_gui.cpp
(4) Replace function smallMapCenterOnCurrentPos in smallmap_gui.cpp as follows:

Code: Select all

	void SmallMapCenterOnCurrentPos()
	{
		// Goal: Given the viewport coordinates of the middle of the map window, find
		// out which tile is displayed there.

		// First find out which tile would be there if we ignore height
		const ViewPort *vp = FindWindowById(WC_MAIN_WINDOW, 0)->viewport;
		Point pt = InverseRemapCoords(vp->virtual_left + vp->virtual_width  / 2, vp->virtual_top  + vp->virtual_height / 2);
		Point ptWithoutHeight = {pt.x / TILE_SIZE, pt.y / TILE_SIZE};

		// Problem: There are mountains.  So the tile actually displayed at the given position
		// might be the high mountain of 30 tiles south.
		// Unfortunately, there is no closed formula for finding such a tile.
		// We call GetRowAtTile originally implemented for the viewport code, which performs
		// a interval search.  For details, see its documentation.
		int rowWithoutHeight = ptWithoutHeight.x + ptWithoutHeight.y;
		int rowWithHeight = GetRowAtTile(vp->virtual_top + vp->virtual_height / 2, ptWithoutHeight);
		int rowOffset = rowWithHeight - rowWithoutHeight;
		Point ptWithHeight = {ptWithoutHeight.x + rowOffset / 2, ptWithoutHeight.y + rowOffset / 2};

		// And finally scroll to that position.
		int sub;
		const NWidgetBase *wid = this->GetWidget<NWidgetBase>(SM_WIDGET_MAP);
		Point sxy = this->ComputeScroll(ptWithHeight.x, ptWithHeight.y,
										max(0, (int)wid->current_x / 2 - 2), wid->current_y / 2, &sub);
		this->SetNewScroll(sxy.x, sxy.y, sub);
		this->SetDirty();
	}
User avatar
ChillCore
Tycoon
Tycoon
Posts: 2879
Joined: 04 Oct 2008 23:05
Location: Lost in spaces

Re: More height levels

Post by ChillCore »

ic111 wrote: I just had a look on the bug that opening the small map for a high mountain leads to the wrong position. The reason is simply that height up to now is completely ignored in this case. As the changes for fixing this are not big, and my code currently is not ready for making a patch, I document them just here.
Thank you for the fix ic111.

Attached is the patch you wanted to post including coding style.
I have updated the logfile already but as the line numbering is no longer correct at the moment I will post it later.
Attachments
moreheightlevels_v28_r20739.diff
(259.58 KiB) Downloaded 45 times
-- .- -.-- / - .... . / ..-. --- .-. -.-. . / -... . / .-- .. - .... / -.-- --- ..- .-.-.-
--- .... / -.-- . .- .... --..-- / .- -. -.. / .--. .-. .- .. ... . / - .... . / .-.. --- .-. -.. / ..-. --- .-. / .... . / --. .- ...- . / ..- ... / -.-. .... --- --- -.-. .... --- --- ... .-.-.- / ---... .--.

Playing with my patchpack? Ask questions on usage and report bugs in the correct thread first, please.
All included patches have been modified and are no longer 100% original.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

Thank you for generating the patch ;-)

In the meantime, I had a look on the case "click in small map => positioning the main map". On my local copy here, my fix is already working, however some cleanup remains to be performed, and the map indicators are also not yet positioned correctly.

So I think I will post a patch with fully working smallmap on Monday or Tuesday.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

Ok, smallmap should work now.
Attachments
moreheightlevels_v29_r20756.diff
(264.2 KiB) Downloaded 49 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

I spent about two hours today in the evening with inspecting the "terraforming clears land even if money doesn't suffice" bug. I don't have a clue what's wrong so far.

Fact is, the unpatched code works. So it obviously is a bug somewhere in the patch. What is not 100 percent clear to me is how the original code makes the decision money is not sufficient.

I mean, the actual heightlevel change is within the "if (flags & DC_EXEC)" block. However, the call to terraform_tile_proc which is (as far as I know) responsible for clearing the tile is not within such a DC_EXEC-block.

I noticed that the whole code is called two times: The first time without DC_EXEC flag, the second time with DC_EXEC flag. So for me, it seems like the first call is for testing wether everything works, and the second call is for actually executing the changes.

So: Where - in the old code - is the decision that we don't execute the change if we have not enough money?

Does anyone of you have a clue what the bug is / might be?
User avatar
ChillCore
Tycoon
Tycoon
Posts: 2879
Joined: 04 Oct 2008 23:05
Location: Lost in spaces

Re: More height levels

Post by ChillCore »

Those fixes in the smallmap work like a charm ic111, thank you.

Edit:
I will have to take a better look at clean trunk first to be able to help you with the terraforming bug.
Hopefullly one of the Gurus can help.
-- .- -.-- / - .... . / ..-. --- .-. -.-. . / -... . / .-- .. - .... / -.-- --- ..- .-.-.-
--- .... / -.-- . .- .... --..-- / .- -. -.. / .--. .-. .- .. ... . / - .... . / .-.. --- .-. -.. / ..-. --- .-. / .... . / --. .- ...- . / ..- ... / -.-. .... --- --- -.-. .... --- --- ... .-.-.- / ---... .--.

Playing with my patchpack? Ask questions on usage and report bugs in the correct thread first, please.
All included patches have been modified and are no longer 100% original.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

I think I fixed it.

Both in the old and in the new terraforming code, CmdLevelLand is called both by the levelling and by the usual terraforming tools. Now, in the old terraforming code, CmdLevelLand called CmdTerraformLand using the DoCommand facility, i.e. this way:

Code: Select all

CommandCost ret = DoCommand(tile, SLOPE_N, (curh > h) ? 0 : 1, flags & ~DC_EXEC, CMD_TERRAFORM_LAND);
In the new code, I now longer need to call it this way, I instead directly call private functions of my own. This is conceptionally no harm, the only problem is/was:

If you look into command.cpp, then

Code: Select all

	DEF_CMD(CmdTerraformLand,           CMD_ALL_TILES | CMD_AUTO), // CMD_TERRAFORM_LAND
	DEF_CMD(CmdLevelLand, CMD_ALL_TILES | CMD_NO_TEST | CMD_AUTO),
CmdTerraformLand was declared without, but CmdLevelLand with the CMD_NO_TEST flag. Thus, in CmdLevelLand a test I need was not performed. In the old code, this problem was more or less elegant fixed when calling DoCommand explicitely from inside CmdLevelLand, thus I only had the problem in my new code.

Bugfix: I removed the CMD_NO_TEST flag from CmdLevelLand.

Question to all developers: I did not notice any side effects. Would you expect / do you know about any problems this may cause?
Attachments
moreheightlevels_v30_r20761.diff
(264.64 KiB) Downloaded 54 times
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

Concerning the future, i.e. how we can get it towards merging into trunk:

Some months ago, the developers told in this thread that they prefer it being split up into smaller parts before it could be merged into trunk. Reason: Smaller parts that can be tested more easily on their own.

Is this still your opinion? If yes, then I would start isolating parts that can be merged more or less independent of each other soon. In fact, I think one can (possibly among others) about the following categories of those small patches:

(1) Patches that take the heightlevel into consideration where it wasn't considered before, simply because considering it was not needed when one had no more than 16 heightlevels. Example: When opening the small map, the heightlevel at point was ignored so far. These patches I think can be generated and tested independently in a quite simple way.

(2) The basic data structures / access functions. For example, for this patch I replaced direct accesses to the map array by inline functions at various places in the code.

(3) In some areas, bigger changes were necessary, e.g. for terraforming or for painting the map. Here, one will have to exchange the whole algorithm in one patch.

If we go this way, then I would like to open bug tracker items for the individual patches. Maybe even adding a bug tracker category "MoreHeightlevels" would make sense?
Yexo
Tycoon
Tycoon
Posts: 3663
Joined: 20 Dec 2007 12:49

Re: More height levels

Post by Yexo »

ic111 wrote:Concerning the future, i.e. how we can get it towards merging into trunk:

Some months ago, the developers told in this thread that they prefer it being split up into smaller parts before it could be merged into trunk. Reason: Smaller parts that can be tested more easily on their own.
Is this still your opinion?
It certainly is. Reviewing a 250+kb patch is a lot of work. I'd estimate that reviewing 10 30kb patches is about ten times easier. For examples on how to split you could look at trunk commits done very soon after each other in the same code, for example r20648 to r20673 or r20681 to r20688. Mercurial queues are very helpful in creating/updating such a stack of patches.
If we go this way, then I would like to open bug tracker items for the individual patches. Maybe even adding a bug tracker category "MoreHeightlevels" would make sense?
Not in the official OpenTTD bug tracker. You could however ask the guys of dev.openttdcoop.org for a project so you get an issue tracker and a repository for the patches.

Splitting the patch in smaller patches is not the only thing that needs to be done to get in trunk however: It also needs some serious profiling to see how the speed is affected, both with the setting turned on and off. Also it needs a dev to review the smaller patches after you split it up and actually commit them to trunk. You shouldn't underestimate the time that the checking/fixing of a patch still costs, so you'll need to find a developer that is interested enough to do that. At this time I'm personally not interested enough to start a review of this very big patch, even if it were cut up in smaller pieces. For me it's at this moment just too much work.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

It also needs some serious profiling to see how the speed is affected, both with the setting turned on and off.
Is there already some kind of profiling environment for OpenTTD? E.g. some howto?
Also it needs a dev to review the smaller patches after you split it up and actually commit them to trunk. You shouldn't underestimate the time that the checking/fixing of a patch still costs, so you'll need to find a developer that is interested enough to do that.
My idea is posting small patches I regard as finished in a bug tracker, waiting for someone / some people having a look on them, maybe correcting them afterwards to get them into an acceptable state. The last patch version in a tracker ticket would then be one that is very close to being ready for commit.

However, I don't think that all patches have to be reviewed by the same person.
If we go this way, then I would like to open bug tracker items for the individual patches. Maybe even adding a bug tracker category "MoreHeightlevels" would make sense?
Not in the official OpenTTD bug tracker. You could however ask the guys of dev.openttdcoop.org for a project so you get an issue tracker and a repository for the patches.
This is an option. However: Is this site populated enough by people familiar with the OpenTTD code? I mean, a bug tracker (ok, in fact we talk about change requests, not bugs) is the more useful the more people have a look at it. And this was basically my idea with the main bug tracker: If I have a small patch that I recognize as finished, then what is needed is review and input from other people. If the reviewer complains about something, and does this at a place where other people read, then finding a solution for the problem by discussion is easier.

Maybe I use the bug tracker at dev.openttdcoop.org simply as repository, link the entries from here, and discussions can also stay here in the forum.
Yexo
Tycoon
Tycoon
Posts: 3663
Joined: 20 Dec 2007 12:49

Re: More height levels

Post by Yexo »

I think I was misunderstanding you about the bug tracker before. If you want to use it to post finished 'small' patches than of course bugs.openttd.org is the correct place.
User avatar
ChillCore
Tycoon
Tycoon
Posts: 2879
Joined: 04 Oct 2008 23:05
Location: Lost in spaces

Re: More height levels

Post by ChillCore »

ic111 wrote: Is there already some kind of profiling environment for OpenTTD? E.g. some howto?
See TIC() / TOC() in debug.h.
Also an example of its usage is here: http://www.tt-forums.net/viewtopic.php? ... &start=491
You may want to have a looksie at the old terraforming profiling patch first to see how and what you need to include in the code.



The clearing tiles bugfix works great.

That leaves just one bug open in regards of terraforming namely the missing "is already levelled" notice.
I have not fixed it yet but here is what I have noticed before when trying.
You can not compare the height of the start and end tile as that will give undesired effects if there is a dip or a bump inbetween two tiles of equal height. (The message will show in this case eventhough the terrain is not levelled).
Instead the cost of the operation has to be checked and if it is 0 then the message should show.
Maybe you already know this but I thought I'd mention it just in case.
-- .- -.-- / - .... . / ..-. --- .-. -.-. . / -... . / .-- .. - .... / -.-- --- ..- .-.-.-
--- .... / -.-- . .- .... --..-- / .- -. -.. / .--. .-. .- .. ... . / - .... . / .-.. --- .-. -.. / ..-. --- .-. / .... . / --. .- ...- . / ..- ... / -.-. .... --- --- -.-. .... --- --- ... .-.-.- / ---... .--.

Playing with my patchpack? Ask questions on usage and report bugs in the correct thread first, please.
All included patches have been modified and are no longer 100% original.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: More height levels

Post by ic111 »

I have made a plan how the patch can be split up. See the attachments.

patches.xls is a list of patches, with explanations and for each patch a list of files that go into that particular patch (either complete or partially)
files.xls tells, into which patch(es) contents of a file go. Sometimes just one, sometimes several different patches.

patches.pdf shows the same thing graphically.

I've ended up with 21 different patches, some small (below one KB), some have some dozens of KB. Of course, generating them makes only sense once all problems in that particular patch are fixed and tasks like profiling are performed. However, I don't think the distribution of changes among patches will stay quite stable.

Of course, there must be a final patch, I've called it Activating, which makes the changes finally visible to the player (i.e. allows him to activate the more heightlevel setting). Most of the patches before are more or less preparations, which perform things that have to be performed before you can allow the setting turned on, but which don't have (major) visible effects to the player as long as he plays without more heightlevels. And: The whole thing is designed as a setting that can be turned on optionally. You can even turn the setting of again in a running game given that there are no high mountains on the map.

What I have never tested is multiplayer. Did anyone of you test the patch already under multiplayer conditions. And how critical is that topic? I mean, a patch that e.g. makes the small map recognize the heightlevel correctly when being opened obviously is not multiplayer-relevant. But how much can one do wrong when dealing with things like configuration settings? Or, the other way round, did we do it right?

EDIT: Why does the forum neither allow gnumeric files nor csv files being uploaded? xls was not my first choice as file format ;-)
Attachments
files.xls
Files and into which patches they go - xls version.
(17.5 KiB) Downloaded 85 times
patches.xls
Patches and which files they contain, and how they are related. - xls version
(12 KiB) Downloaded 58 times
patches.pdf
Patches and how they are related - graphically
(11.45 KiB) Downloaded 112 times
Hirundo
Transport Coordinator
Transport Coordinator
Posts: 298
Joined: 27 Jan 2008 13:02

Re: More height levels

Post by Hirundo »

Splitting up your patch in small chunks sounds like a very sensible idea to me. In general, I would suggest changing all code that (implicitly) depends on MAX_TILE_HEIGHT being 15. For example, the small map could changed so the map colours are spread dynamically over all height levels and the legend is adjusted accordingly.

While I'm at it I would also like to post a suggestion. If I look at docs/landscape_grid.html, it seems to me that there are at least three bits that are currently unused for every tile type. With some shuffling, this would allow height levels up to 128. The most obvious approach to me would be to free m1 bits 7..5 and make type_height and m1 a single 16-bit value. Bits 11..5 of that value would then be the height level.

The advantage of this approach is that all of the tile data (except for m7) is in one location, so there are less cache misses. Also, you don't need to check the 'more height levels' setting explicitly each time you want to access height data. Instead, you could add a setting 'max height level' which sets the maximum height to any level with a default value of 15. There is of course a downside: you'd need quite a lot of shuffling to the map array, esp. if you want to keep the water class (currently m1 bit 6..5) in the same location for water/station/industry/object tiles.

These are just my 2c, whether my idea is sensible or not is yours to decide.
Create your own NewGRF? Check out this tutorial!
User avatar
ChillCore
Tycoon
Tycoon
Posts: 2879
Joined: 04 Oct 2008 23:05
Location: Lost in spaces

Re: More height levels

Post by ChillCore »

Cool to see the patch broken up in pieces like that.


Some comments on files.xls:
--------------------------
ai_tile.hpp:
I have not yet checked if more files need the comments adjusted.

heightmap.h:
The numbers in the array are just 1,2,4 etc (in reverse order)
There have been a few suggestion for having different numbers before I integrated the array and the option to choose the height.
http://www.tt-forums.net/viewtopic.php? ... &start=525 and there should be an earlier post too but I can not find it for the moment.
I was hoping someone would do some testing while tuning but unfortunately this has not happened (yet). Feel free to re-tune.

Industry_type.h:
IMHO the heightlimit below which the industries can level more than 1 heightlevel is too low.
In my patchpack I set it to 60 instead of 20 which gives some nicer results.

language files:
When merging patches in trunk only english is commited.
Of course nothing can stop translaters from re-using our strings. ;)

newgrf files:
Might need some more adjustments ...
George's variable snowline (included in ECS vectors) works.
planetmaker's variable snowline (online content) did not last time I checked.
There is a glitch with the dumptruck near the gravel/sand pit in FIRS.

saveload.cpp / saveload.h:
MORE_HEIGHT_LEVELS_SAVEGAME_VERSION is only introduced for easier bumping the patch during development.
it should/will be replaced by numbers when integrating the patch in trunk.
MORE_HEIGHTLEVEL_SAVEGAME_VERSION -> trunk + 1
MORE_HEIGHTLEVEL_SAVEGAME_VERSION - 1 -> trunk (at moment of merge)
Then both can be reverted. (Or not changed in trunk to begin with)

flat_blacktiles_grf:
These tiles could be included in OpenGFX perhaps?
I do not think that shipping official OpenTTD with an extra grf to function correctly is a good idea.



I will try to finish the smallmap height colours one of these days also.
Maybe I will just redo the purple top and extend that a bit instead of trying to find more colours to insert.
-- .- -.-- / - .... . / ..-. --- .-. -.-. . / -... . / .-- .. - .... / -.-- --- ..- .-.-.-
--- .... / -.-- . .- .... --..-- / .- -. -.. / .--. .-. .- .. ... . / - .... . / .-.. --- .-. -.. / ..-. --- .-. / .... . / --. .- ...- . / ..- ... / -.-. .... --- --- -.-. .... --- --- ... .-.-.- / ---... .--.

Playing with my patchpack? Ask questions on usage and report bugs in the correct thread first, please.
All included patches have been modified and are no longer 100% original.
User avatar
Zephyris
Tycoon
Tycoon
Posts: 2897
Joined: 16 May 2007 16:59

Re: More height levels

Post by Zephyris »

ChillCore wrote:flat_blacktiles_grf:
These tiles could be included in OpenGFX perhaps?
Ummm, there already is a flat black tile in the base graphics... how is flat_blacktiles different?
User avatar
ChillCore
Tycoon
Tycoon
Posts: 2879
Joined: 04 Oct 2008 23:05
Location: Lost in spaces

Re: More height levels

Post by ChillCore »

Zephyris wrote:
ChillCore wrote:flat_blacktiles_grf:
These tiles could be included in OpenGFX perhaps?
Ummm, there already is a flat black tile in the base graphics... how is flat_blacktiles different?
Our grf has all slopes (19 tiles), the base graphics not. They used to be there until OpenTTD-0.5.0 or so.
To avoid glitches outside the map we need all of them and that's why we had to make our own. :)
I still have the nfo and pcx around somewhere on my pc and they are hidden somewhere in the thread also. (Near v12 IIRC.)
-- .- -.-- / - .... . / ..-. --- .-. -.-. . / -... . / .-- .. - .... / -.-- --- ..- .-.-.-
--- .... / -.-- . .- .... --..-- / .- -. -.. / .--. .-. .- .. ... . / - .... . / .-.. --- .-. -.. / ..-. --- .-. / .... . / --. .- ...- . / ..- ... / -.-. .... --- --- -.-. .... --- --- ... .-.-.- / ---... .--.

Playing with my patchpack? Ask questions on usage and report bugs in the correct thread first, please.
All included patches have been modified and are no longer 100% original.
User avatar
Dimme
Transport Coordinator
Transport Coordinator
Posts: 277
Joined: 30 Jul 2008 12:42
Location: Trondheim, Norway

Re: More height levels

Post by Dimme »

ic111 wrote: Why does the forum neither allow gnumeric files nor csv files being uploaded? xls was not my first choice as file format ;-)
Maybe it's cheating, but you can zip them :wink:
Try my modular airports minigame!

Image
User avatar
Lord Aro
Tycoon
Tycoon
Posts: 2369
Joined: 25 Jun 2009 16:42
Location: Location, Location
Contact:

Re: More height levels

Post by Lord Aro »

looking at the above PDF file, i had a thought.
What changes does this mean for AIs? If none, what has been done so that the AIs can cope?
AroAI - A really feeble attempt at an AI

It is practically impossible to teach good programming to students that have had a prior exposure to BASIC: as potential programmers they are mentally mutilated beyond hope of regeneration. --Edsger Dijkstra
Eddi
Tycoon
Tycoon
Posts: 8289
Joined: 17 Jan 2007 00:14

Re: More height levels

Post by Eddi »

Dimme wrote:
ic111 wrote: Why does the forum neither allow gnumeric files nor csv files being uploaded? xls was not my first choice as file format ;-)
Maybe it's cheating, but you can zip them :wink:
or just name them .txt
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: Amazon [Bot] and 2 guests