Better map borders

[DorpsGek]

CommanderZ opened the ticket and wrote:

This patch changes the way how the TGP calculates map borders to make them smoother fit with the terrain and look more natural.

Most functions in tgp.cpp had to be altered to make this possible. The file now in my opinion much better follows the coding style. The alternations to other files are minimal (only one integer changed in unmovable_cmd.cpp).

The patch works for all map sizes and settings (the 64*64 maps are very similiar to those created with TGP though).

See this thread for more info and screenshots:
http://www.tt-forums.net/viewtopic.php?f=33&t=37853

The attached patch file is against r13631.

Attachments

• bmb_080726.diff (26.24 KiB)

Reported version: trunk
Operating system: All

---

This issue was imported from FlySpray: https://bugs.openttd.org/task/2114

[DorpsGek]

frosch wrote:

Hello, nice effort. The coasts look a lot nicer on mountainious maps.
However, we want to stay compatible with old random seeds - i.e. your algorithm should be an option in the generate world dialog. E.g. a third option in the "original"/"terragenesis"-combobox or similiar. But that does not need to be discussed now.

Basically we generally dislike:
- Makros (see r13645)
- floats (you are using doubles in HeightMapApplyBorders, which do not seem to be needed at all)
- magic values (if you change a magic constant from 20 to 60, I am sure you know what it means, and you can add an appropiate comment. Btw. why needs it to be changed?)

To cut this patch into smaller pieces I suggest to proceed this way (though if you know a better way, feel free to choose it):

1. Corrections/Additions to existing comments.
2. Remove the static _height_map variable and pass it to every needed function.
3. Make some basic function, that operate on the height_map, but which are not associated to a specific mapgenerator, member functions of the height_map type. E.g. IsValidXY(), AllocHeightMap -> constructor, FreeHeightMap -> destructor.
4. See what remains.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4388

[DorpsGek]

CommanderZ wrote:

However, we want to stay compatible with old random seeds - i.e. your algorithm should be an option in the generate world dialog. E.g. a third option in the "original"/"terragenesis"-combobox or similiar. But that does not need to be discussed now.

This really can come as the last.

Makros (see r13645)

This will be fixed.

floats (you are using doubles in HeightMapApplyBorders, which do not seem to be needed at all)

I see what you mean, they are there to prevent multiple type cast later when dividing the numbers (where doubles are necessary as the resulting number should be in range 0-1). If you think type casts are better, I will change it.

magic values (if you change a magic constant from 20 to 60, I am sure you know what it means, and you can add an appropiate comment. Btw. why needs it to be changed?)

I didn't think much while changing this value, I just needed to allow lighthouses to be spawned further from the map border as there is usually more water on borders with BMB

1. Corrections/Additions to existing comments.

This would be very very little patch :) I have added only several comments to the code I didn't change.

2. Remove the static _height_map variable and pass it to every needed function.

The reason both map arrays are still global is that I don't know how to pass them to GenerateWorldSetAbortCallback if they were local.

3. Make some basic function, that operate on the height_map, but which are not associated to a specific mapgenerator, member functions of the height_map type. E.g. IsValidXY(), AllocHeightMap -> constructor, FreeHeightMap -> destructor.

You mean...to turn it all into object? Okay, that would make the code very nice :)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4389

[DorpsGek]

frosch wrote:

1. Floats:
   + double coast_free_length = max(18, min(min(height_map.size_x, height_map.size_y) / 10, 80));
   This is not needed as the tenth do not really make a difference.

- 160 * (1 - distance_from_edge / coast_free_length)
The result is casted to an integer anyway. So you can as well just use -160 * (coast_free_length - distance_from_edge) / coast_free_length

2. "This would be very very little patch :)"
   There is nothing wrong with them :)

3. "The reason both map arrays are still global is that I don't know how to pass them to GenerateWorldSetAbortCallback if they were local."
   Ok, that might need further exploration. But at least the global-ness can be reduced to only a few functions.

4. "You mean...to turn it all into object?"
   Naa, not everything - but those things which would be needed by every believable future algorithm which wants to use the HeightMap struct.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4390

[DorpsGek]

CommanderZ wrote:

1. OK, I made the doubles uints and they are casted where necessary

2. Do you know any good method, how to extract them effectively? The only way I can come up with is to open my tgp.cpp along with original tgp.cpp in merger an go through lit line by line.

3. It seems GenerateWorldSetAbortCallback is the only one which needs globals, the rest of functions were already "localized".

4. OK

I'm attaching updated version of the patch with these issues corrected.

Attachments

• bmb_080728.diff (27.07 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4391

[DorpsGek]

frosch wrote:

Uhm, err - did you merge r13645 by hand? Your patch is still against r13631.

So you should read some tutorial/documentation about subversion (svn). Especially about 'update', 'revert', 'resolved'.

1. Please read my post above again. You do not need doubles at all. Just one integer subtraction, one integer multiplication, and one integer divison.

I don't know about any tool that can help with that as the half-manual way was sufficient for me up to now. However usually I create a diff-file, and open that one. In the diff file you can see the hunks (the code snippets starting at @@ until the next @@). You can classify them with three types: Those which you want to remove completely, those which you want to keep completely, and those which you want to split further. The first two types are easy. You can safely remove complete hunks and the patch will stay valid. You can also move complete hunks to a new file (including the header which file to patch) and apply this new patch to your second working copy. If you are lucky there will only be a few of the third type, as you have to manually copy and paste the + and - lines into your working copy.

5. The FOR_ALL_TILES_IN_HEIGHT macro is really bad. If there is no way to avoid it, please make at least 'height_map' a parameter of the macro.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4392

[DorpsGek]

CommanderZ wrote:

Updated to r13649.

All macros but the FOR_ALL_TILES_IN_HEIGHT were replaced with inline functions. I have no idea how to fix that one (except replacing all its occurences with its content).

Please read my post above again. You do not need doubles at all. Just one integer subtraction, one integer multiplication, and one integer divison.

I get it, sorry for being stupid. Fixed.

Now I'm working on extracting the comments as you wanted (seems to be pretty tedious work).

Attachments

• bmb_080729.diff (28.91 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4396

[DorpsGek]

CommanderZ wrote:

It is more difficult than I thought. The whole file is separated into 11 @@ blocks, which means all of them are of the "third type". Also, when deleting the + - lines, I often end up with something ambiguous, like "- for (uint y = 1; y <= _height_map.size_y; y++)", which appears many times troughout the file. I think I will simply copy the comments one by one into a unchanged tgp.cpp file and make a diff with it.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4398

[DorpsGek]

CommanderZ wrote:

Here it is. As I said,it is very small.

Against r13649.

Attachments

• bmb_080730_comments.diff (3.06 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4399

[DorpsGek]

CommanderZ wrote:

Another update.

- the key functions to work with tgen heightmaps were turned into object
- both height maps are no longer globals - the object destructor will now free the height array once the object is destroyed, so the callback shouldn't be necessary anymore

There is some commented stuff right now, I will remove it once I'm 100% sure the height maps can stay as locals.

Against r13674.

Attachments

• bmb_080705.diff (31.12 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4432

[DorpsGek]

CommanderZ wrote:

Synced to trunk.

One small fix: the world generation progress meter showed only 3 steps instead of 5 (I had added two map generation steps - noise map generation and shore generation). This was especially obvious with bilbo's extra map sizes patch, where the "4/3" and "5/3" was displayed for several seconds.

Attachments

• bmb_080716.diff (31.87 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4467

[DorpsGek]

CommanderZ wrote:

Sry I forgeto to add: the file is against revision 13706.

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4468

[DorpsGek]

CommanderZ wrote:

One more sync to trunk. Against revision 13784.

Attachments

• bmb_080723.diff (31.87 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4490

[DorpsGek]

CommanderZ wrote:

Update to r13992

Attachments

• bmb_080804.diff (31.09 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4533

[DorpsGek]

CommanderZ wrote:

Sync to r14110

Attachments

• bmb_r14110.patch (31.05 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4627

[DorpsGek]

CommanderZ wrote:

New version:

Coding style improvements.
Comments significantly improved.
Removed all the commented code from older versions.
Improved the way the noise map depends upon user defined map smoothness.
Updated to r14254.

Attachments

• bmb_080906.patch (32.25 KiB)

---

This comment was imported from FlySpray: https://bugs.openttd.org/task/2114#comment4697

[DorpsGek]

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).

---

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