Patch: The Rainfall River Generator

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: Patch: The Rainfall River Generator

Post by ic111 » 08 Nov 2015 19:43

Do I understand it right, your problem is that instead of the startup game, you see a blue area?

If yes, I have no clue what might cause that. It doesn´t smell like anything I remember to have changed.

Anyway, thank you for the binary.

User avatar
wallyweb
Tycoon
Tycoon
Posts: 5565
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb » 08 Nov 2015 20:20

ic111 wrote:Do I understand it right, your problem is that instead of the startup game, you see a blue area?
Yes. It can't be seen in the png but the blue area is water tiles.
Anyway, thank you for the binary.
:bow:

Eddi
Tycoon
Tycoon
Posts: 7420
Joined: 17 Jan 2007 00:14

Re: Patch: The Rainfall River Generator

Post by Eddi » 08 Nov 2015 21:37

a blue starting area means one of two things:
  1. it couldn't find the opntitle.dat file, or it is corrupted
  2. loading the opntitle.dat file failed due to savegame conversion errors
debug output may help determine which of these cases applies
You might not exactly be interested in Ferion, but if you are, have fun :)

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 08 Nov 2015 21:50

Here is version 43. Changes:

- Improved documentation around patch 40 (number of lower tiles)
- Removed the concept of a HeightLevelIterationMode. It is no longer necessary, as the improved code written in the last few weeks always used HLIM_ALL.
- Removed some unnecessary code regarding terraforming for rivers

Generally, I don´t think providing windows binary for this sort of change is necessary - a player shouldn´t notice that kind of change, they are purely improvements in the background.

Regarding the startup game: I only add config options with SGF_NEWGAME_ONLY, and recently removed the savegame bump. Maybe I somehow affected the ability to load the intro game by doing the latter?

Wallyweb: If you would give as that file (opntitle.dat), then I could try wether I can reproduce the problem.
Attachments
rivers_v43.zip
(283.24 KiB) Downloaded 41 times

User avatar
wallyweb
Tycoon
Tycoon
Posts: 5565
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb » 08 Nov 2015 22:15

ic111 wrote:Wallyweb: If you would give as that file (opntitle.dat), then I could try wether I can reproduce the problem.
I just ran a diff for opntitle.dat in trunk versus v.42 and ExamDiff tells me they are exactly the same so the file has not been corrupted.
Here is the file:
opntitle.dat
(162.84 KiB) Downloaded 43 times
I will compile v43 later this evening (after my dinner :wink: ) to verify if the problem followed your changes.

McZapkie
Tycoon
Tycoon
Posts: 1176
Joined: 18 Jan 2014 18:10

Re: Patch: The Rainfall River Generator

Post by McZapkie » 09 Nov 2015 11:31

Great patch. It now works with large maps, very useful (there is no more need to import all rivers from txt file into scenario editor).

One small improvement suggestion: is it possible to carve out river, if there is small terrain obstacle?
For example:
rfrivers.png
rfrivers.png (102.71 KiB) Viewed 1856 times
(or maybe I miss something, I have lake outflow probability 500, maybe other options count?)

EDIT: I had 'flow consumed per lake volume' 100, because I misunderstand this option.

EDIT2: unfortunately, 1.5.2 RFRG map does not work with regular 1.5.2 version - impossible to load map (for example, it is impossible to join 1.5.2RFRG server with normal 1.5.2 client).
Is it possible to fix that?
My experimental openTTD server: 149.156.194.203:3979 non-standard client, now testing: JGRPP http://tiny.pl/ggnch
Projects: Reproducible Map Generation patch, NewGRFs: Manpower industries, PolTrams, Polroad, 600mm narrow gauge, preindustrial houses, wired, ECS industry extension, V4 CEE train set.
Addicted to freeciv longturn.

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 09 Nov 2015 19:02

wallyweb wrote: Here is the file:
opntitle.dat
It reports "Unexpected end of chunk".

So, maybe I indeed do something which is forbidden unless one distributes a new savegame version (which I don´t do, and actually don´t want to do if I can avoid it). Are you (or someone else) able to load your favorite savegame (of unpatched trunk or an official OpenTTD release)? (question is basically answered below).
EDIT: I had 'flow consumed per lake volume' 100, because I misunderstand this option.
Indeed, this is the option you need. If you set that value to zero, all lakes will fill up, until they find an outflow. The bigger the value is, the more probable it is that flow gets exhausted before the lake overflows.
EDIT2: unfortunately, 1.5.2 RFRG map does not work with regular 1.5.2 version - impossible to load map (for example, it is impossible to join 1.5.2RFRG server with normal 1.5.2 client).
Is it possible to fix that?
Seems that this answers my question from above. Yes, of course this is a bug I want to fix...

User avatar
wallyweb
Tycoon
Tycoon
Posts: 5565
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb » 09 Nov 2015 21:10

ic111 wrote: and recently removed the savegame bump.
Do you remember in which version you did this?

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 09 Nov 2015 21:40

Two hours of debugging in the innermost sections of OpenTTD settings & saveload handling... Below, I describe the facts I found so far, if anyone gets a clue what might be wrong based on my findings below, hints are quite welcome.
Do you remember in which version you did this?
Revision 41, here: http://www.tt-forums.net/viewtopic.php? ... 0#p1159349

First of all, here is a new release, I adjusted some of the flags. Now, I define my settings in settings.ini using

Code: Select all

SGF_NEWGAME_ONLY | SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC
This seems to fix a part of the problem, but not all. Now, I run into "Invalid chunk size" in SlLoadChunk, in this code:

Code: Select all

/* Read length */
len = (SlReadByte() << 16) | ((m >> 4) << 24);
len += SlReadUint16();
_sl.obj_len = len;
endoffs = _sl.reader->GetSize() + len;
ch->load_proc();
if (_sl.reader->GetSize() != endoffs) {
	SlErrorCorrupt("Invalid chunk size");
}
If I get it right, then len is the chunk length actually stored in the savegame, whereas the reader increases its position based on the SaveLoad definitions. I definitely know that the error is thrown in Load_PATS, i.e. while loading the settings. Not surprising, as I don´t have any savegame-related changes beside new settings.

Here, I have (based on gdb):

Code: Select all

_sl.reader->GetSize() = 656114
endoffs = 656041
i.e., I seem to read more than I´m allowed to read based on the contents of the savegame. The difference is 73 bytes, the sum of my new config variables (except the length of the char*) is 34 bytes, i.e. this is no clear hint.

The SaveLoad definition is defined in the generated file objs/setting/table/settings.h, e.g.

Code: Select all

SDT_VAR(GameSettings, game_creation.rainfall.flow_for_river, SLE_INT32, 0, SGF_NEWGAME_ONLY | SLF_NOT_IN_SAVE |
 SLF_NO_NETWORK_SYNC, DEF_MODERATE_RIVERS_FLOW_FOR_RIVER,       MIN_FLOW_FOR_RIVER, MAX_FLOW_FOR_RIVER, 0,
  STR_CONFIG_SETTING_RAINFALL_FLOW_FOR_RIVER, STR_CONFIG_SETTING_RAINFALL_FLOW_FOR_RIVER_HELPTEXT, STR_NULL,
   NULL, 194, SL_MAX_VERSION,        SC_ADVANCED),
i.e., the flags SGF_NEWGAME_ONLY, SLF_NOT_IN_SAVE, and SLF_NO_NETWORK_SYNC are propagated. Looking at the definition of SDT_VAR, you end up at SDT_GENERAL, i.e.

Code: Select all

#define SDT_GENERAL(name, sdt_cmd, sle_cmd, type, flags, guiflags, base, var, length, def, min, max, interval, full, str, strhelp,
 strval, proc, load, from, to, cat)\
 	{NSD_GENERAL(name, def, sdt_cmd, guiflags, min, max, interval, full, str, strhelp, strval, proc, load, cat), 
 	SLE_GENERAL(sle_cmd, base, var, type | flags, length, from, to)}
Note: The flags are passed as fourth parameter. Now look at the definition of SLE_GENERAL, which if I get it right is supposed to produce a SaveLoad instance:

Code: Select all

#define SLE_GENERAL(cmd, base, variable, type, length, from, to) {false, cmd, type, length, from, to, (void*)cpp_offsetof(base, variable),
 cpp_sizeof(base, variable)}
... The fourth parameter (type) is passed to the third parameter of the resulting struct, which is...

Code: Select all

struct SaveLoad {
	bool global;         ///< should we load a global variable or a non-global one
	SaveLoadType cmd;    ///< the action to take with the saved/loaded type, All types need different action
	VarType conv;        ///< type of the variable to be saved, int
	uint16 length;       ///< (conditional) length of the variable (eg. arrays) (max array size is 65536 elements)
(...)
i.e. passed to conv. And indeed, in SlIsObjectValidInSavegame, called from SlObjectMember, we have

Code: Select all

static inline bool SlIsObjectValidInSavegame(const SaveLoad *sld)
{
	if (_sl_version < sld->version_from || _sl_version > sld->version_to) return false;
	if (sld->conv & SLF_NOT_IN_SAVE) return false;

	return true;
}
... SlObjectMember is implemented as follows:

Code: Select all

	VarType conv = GB(sld->conv, 0, 8);
	switch (sld->cmd) {
		case SL_VAR:
		case SL_REF:
		case SL_ARR:
		case SL_STR:
		case SL_LST:
			/* CONDITIONAL saveload types depend on the savegame version */
			if (!SlIsObjectValidInSavegame(sld)) return false;
			if (SlSkipVariableOnLoad(sld)) return false;
(...)
Everything seems fine, nevertheless I seem to read something I shouldn´t try to read.

Now, if anyone has a clue what might be going on here, this would be a quite good message, otherwise, this will probably become a second debugging session...
Attachments
rivers_v44a.zip
(237.92 KiB) Downloaded 42 times

User avatar
JGR
Tycoon
Tycoon
Posts: 1990
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: Patch: The Rainfall River Generator

Post by JGR » 09 Nov 2015 23:05

Your additional settings all include the line:

Code: Select all

guiflags = SGF_MULTISTRING | SGF_NEWGAME_ONLY | SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC
SLF_NOT_IN_SAVE and SLF_NO_NETWORK_SYNC are defined in and have the type of enum VarTypes, and should go in the flags field, not the guiflags field. Putting them in the guiflags will not have the desired effect.
Unfortunately C++ enums (before C++11 typed enums) are not type safe, and so you need to check manually that you are in fact matching the types correctly.

Have a look at the existing game_creation settings for what the fields should look like.
Ex TTDPatch Coder, Grumpy Greymuzzle
Avatar by MoonsongWolf.
Patch Pack, Github
Dad-Coder since April 2018

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 10 Nov 2015 19:51

Thank you very much, this indeed was the problem.

This release should fix the issues with loading savegames.
Attachments
rivers_v45.zip
(255.64 KiB) Downloaded 37 times

User avatar
wallyweb
Tycoon
Tycoon
Posts: 5565
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb » 10 Nov 2015 20:51

ic111 wrote:This release should fix the issues with loading savegames.
Indeed! The intro window now displays properly. :bow:

For those who would like to test this release in Windows, here is the Windows 32 bit binary:
OpenTTD 1_5_2 Rainfall_v45_Win32.7z
(4.49 MiB) Downloaded 37 times
Enjoy :D

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 10 Nov 2015 22:17

And another release. Changes:

- I removed the fan delta lake manipulation, as that setting didn´t have the desired effect.
- Improved source code comments
- I made the factor used for removing tiny oceans, and for removing tiny basins configurable. But only in the config file. If considered important, the GUI might be adjusted at a later time.

The idea behind that step is:
- Tiny oceans especially in tgp tend to consume huge rivers, which doesn´t look realistic at all. Thus, I remove all oceans smaller than MapSize() / small_oceans_removal_factor
- Small basins tend to trigger lakes too often, and additionally cost performance while setting up lakes. Thus, eliminating those small basins is a good idea before starting.

Removing in both cases means raising land. The default values are MapSize() / 1000 for tiny oceans, and 13 for tiny basins.
Attachments
rivers_v46.zip
(262.09 KiB) Downloaded 43 times

User avatar
wallyweb
Tycoon
Tycoon
Posts: 5565
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb » 11 Nov 2015 11:21

ic111 wrote:And another release.
And another binary ... 8)
OpenTTD 1_5_2 Rainfall_v46_Win32.7z
(4.48 MiB) Downloaded 44 times
Enjoy :D

McZapkie
Tycoon
Tycoon
Posts: 1176
Joined: 18 Jan 2014 18:10

Re: Patch: The Rainfall River Generator

Post by McZapkie » 12 Nov 2015 18:51

I found some graphical glitches:
riverrfg.jpg
riverrfg.jpg (38.28 KiB) Viewed 1577 times
it looks like river going up and down.
riverrfg2.jpg
riverrfg2.jpg (60.59 KiB) Viewed 1577 times
river going on the top of small hill (and many others with rivers traversing instead of flowing down the mountain gradient).

Is it possible to lower terrain 1 tile (except the lowest one) before river placement?

I know you have some erosion included, but maybe it need to be tweaked. What about flow speed?
High gradient should make much higher erosion than flat area.
My experimental openTTD server: 149.156.194.203:3979 non-standard client, now testing: JGRPP http://tiny.pl/ggnch
Projects: Reproducible Map Generation patch, NewGRFs: Manpower industries, PolTrams, Polroad, 600mm narrow gauge, preindustrial houses, wired, ECS industry extension, V4 CEE train set.
Addicted to freeciv longturn.

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 12 Nov 2015 20:57

McZapkie wrote: it looks like river going up and down.
Thank you, this is indeed a case my river-may-not-flow-upwards-detection misses so far. The problem here is that two rivers merge, but my detection works river-wise so far. It will be a bit tricky to improve, but I will give it a try.
river going on the top of small hill (and many others with rivers traversing instead of flowing down the mountain gradient).
On the one hand, I would to some extent consider that an artefact of OpenTTDs abstraction of the world.

On the other hand, the generator (very) roughly works in the following steps:
(1) Calculate flow based on number of lower tiles measure
(2) Wider rivers, wider valleys (involves random terraforming)
(3) Fine tune terrain to make slopes valid against rivers, while keeping them connected

The difficult thing about (3) is that I need to do terraforming that does damage landscape as little as possible. Thus, one of my strategies is, perform (among a number of possible terraformings) that one that fixes the greatest number of planned river tiles with respect to their slope.

If I would not do that, then I could easily run into situations where one accidentally lowers half of a lake, and similar things. Short: Situations that look considerably worse than anything you presented here as screenshots.

So, to conclude, such things trigger the question: Should one add another postprocessing step with even more code, or should one live with the result one sees here?
Is it possible to lower terrain 1 tile (except the lowest one) before river placement?
No, I would not like to do that. I think rivers flowing through a valley, being at the same heightlevel as the valley, fit more into the scale of OpenTTD than if the rivers would be one tile lower (and, I also think they look better).
I know you have some erosion included, but maybe it need to be tweaked. What about flow speed?
High gradient should make much higher erosion than flat area.
Would be a new concept. Erosion doesn´t exist as concept so far.

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 12 Nov 2015 21:57

Another post, as I want to write some sentences about the general state of this patch.

My goal during the last few weeks was improving it such that I can regard it more or less finished. Of course with the goal to make it (at least from my side) possible that it eventually becomes a / the river generator of OpenTTD. Which, obviously, is not my decision.

Thus, to summarize its contents and state:
- The patches until 019 deal with the genworld / GUI infrastructure (config settings & dialogs, etc.)
- Patches 020 to 105 deal with deciding where rivers and lakes are supposed to be created, and with placing them on landscape, including some basic terraforming.
- Patch 130 makes rivers look less straight
- Patches 140 to 190 modify the shape of lakes
- Patches 200 introduces wider rivers and valleys
- Patches 220 to 270 sets up a less landscape-damaging terraforming scheme than patch 105 does (which nevertheless is necessary as those later steps are not applicable to 100% of the tiles, just for, say, 98%), and fine tunes some river situations
- Patch 300 is the generation progress.

Here, the river generator is complete. Everything after this patch is for town placing, in detail
- Patch 310 sets up an API for that
- Patch 312 collects the information, often river-related
- Patch 314 introduces the concrete town placement strategies
- Patches 320 to 348 introduce the town placer config GUI
- Patches 360 and 370 finally builds things together.

- Side note: Patches 000, 035, 045, 255 are purely for debugging.

To summarize, a potential review of the river generator would need to cover the patches until 300. Due to the size of the queue, probably a reasonable approach would be, start at the first patch(es), and discuss them in increasing order, one-by-one or group-by-group, but not all at a time.
Maybe after a look on the general picture.

In fact, most of that queue is quite self-contained, and should not affect the remaining game. E.g., making the terraforming API accessible from landscape generation is not meant to change anything functional regarding the existing terraforming.

A few of them, e.g. patch 020, are quite library-oriented either, in that they provide general code that is not exactly river-oriented, but might be reused elsewhere when needed.

My primary goal now is, take that piece of work into a stable state. For this, people generating their favorite heightmap with this river generator and reporting on that (as McZapkie did) are needed.

For me, the most important question is, does it run stable (no crashes), and in a reasonable time?

Compared to that, I´m quite reluctant regarding more and more fine-tuning steps, which always have the danger of introducing new bugs (and, it also is a question of real world time).

So far some general words about that patch queue, have fun with it.

User avatar
JGR
Tycoon
Tycoon
Posts: 1990
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: Patch: The Rainfall River Generator

Post by JGR » 13 Nov 2015 00:26

I compiled v46 and got a few compiler warnings of note.
Most significantly was in patch 348, in genworld.cpp. std::string::npos is negative, and the result of std::string.find and the return type of FindMatchingCloseBracket are stored in a size_t, which is unsigned, so the tests for std::string::npos may be broken, and things may not won't work if the close bracket is actually missing. std::string::size_type should probably be used instead.
There were also a number of warnings about format strings in DEBUG macros. On a number of platforms, size_t is not the same size or signedness as int, so the compiler is liable to complain if using %i for size_t arguments. PRINTF_SIZE (%zu) should probably be used instead.

I had a (very) quick skim through the patches up to 300, when I get some more time I might have a closer look. Aside from whitespace, one thing I did notice is in patch 105, the variable names of clause_one to clause_eleven are a bit mystifying.
Ex TTDPatch Coder, Grumpy Greymuzzle
Avatar by MoonsongWolf.
Patch Pack, Github
Dad-Coder since April 2018

McZapkie
Tycoon
Tycoon
Posts: 1176
Joined: 18 Jan 2014 18:10

Re: Patch: The Rainfall River Generator

Post by McZapkie » 13 Nov 2015 13:31

ic111 wrote: If I would not do that, then I could easily run into situations where one accidentally lowers half of a lake, and similar things.
But terraforming is performed before river placement, if I understand correctly?
Therefore any terraforming would not destroy any river or lake, because there is none yet.
After flow calculations and terraforming with use of flow erosion, you can just perform standard river flow procedure
(of course with your width/curvature modifications, and with starting points calculated from flow calculations).
BTW, currently some rivers are very short, even 1 tile short. If I remember correctly, original procedure cancel river creation if length is below 'min_river_length'.
I think rivers flowing through a valley, being at the same height level as the valley, fit more into the scale of OpenTTD than if the rivers would be one tile lower (and, I also think they look better).
I agree if speaking about rivers flowing trough plain lowland (in case of openttd, usually level 1), but in case of any hills, there are usually canyons/narrower valleys. Despite of personal aesthetic thoughts, such valleys are practical, because allow to place nice flat bridges instead of bumpy ones.

P.S.
A general note - I'm not sure, if flow direction is necessary to store for each tile. Maybe only flow amount is important to store?
Flow direction is just a local flow gradient. Just in case, if you need some memory allocation optimisation.

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

Re: Patch: The Rainfall River Generator

Post by ic111 » 13 Nov 2015 20:16

JGR wrote:I compiled v46 and got a few compiler warnings of note.
Thanks, I´ll have a look at them. Do you use standard compiler warning settings, or do you use somewhat more restrictive settings?

I admit I never had a close look on those settings, but maybe I should, as this is not the first time people report about compiler warnings I never saw...
PRINTF_SIZE (%zu) should probably be used instead.
Indeed. And is already used at most points of code, what you found are the exceptions...
I had a (very) quick skim through the patches up to 300, when I get some more time I might have a closer look.
Thank you. Probably, at this point of time concentrating on those up to about patch 35 would be best, in a second step those including number of lower tiles, flow, and lake definition (i.e. up to patch 70).

The later patches hopefully won't change in the big scale, but might be affected if people find flaws, or suggest improvements during testing. E.g. what McZapkie found regarding upwards flow will trigger some changes in the patch determining logical rivers.

In contrast to that, things like the height index did not change for a very long time now.
Aside from whitespace, one thing I did notice is in patch 105, the variable names of clause_one to clause_eleven are a bit mystifying.
I'll have a look at it, at some point in time I needed those clauses in a separate way in the debug statements...
But terraforming is performed before river placement, if I understand correctly?
The general picture is:
(1) Decide which tiles are supposed to become rivers and lakes (compare functions like IsOrdinaryLakeTile).
(2) As those tiles can have any slope, perform terraforming such that all planned river/lake tiles have valid slope
(3) Actually make the tiles River tiles in the OpenTTD sense.
Therefore any terraforming would not destroy any river or lake, because there is none yet.
So, this is not true. The whole point is just, that terraforming things such that slopes are ok, and you not as a side effect destroy the river three tiles downwards or upwards some slope is a quite hard problem.

I have my helper functions for this, so I might even try to terraform lower land near a river, but it's just that such processing costs me at least two or three evenings, and leaves the patch by again some hundred lines of code bigger.
BTW, currently some rivers are very short, even 1 tile short. If I remember correctly, original procedure cancel river creation if length is below 'min_river_length'.
Yes, that's the case. I create a river if flow exceeds some limit, say 200, and if a flow patch exceeds that limit just one or two tiles before the ocean, you get such a river.
A general note - I'm not sure, if flow direction is necessary to store for each tile. Maybe only flow amount is important to store?
Maybe misunderstanding: None of those values is meant to be stored permanently. I only have some debug patches in the queue that show those values in the land info window as long as you did not exit the game after generation.

Needed during calculation are all of them. In fact, flow amount are 4 bytes, and in a fifth byte, flow direction is stored as 3 bit, and the remaining 5 bits are used for storing predicates like "planned river tile", or "planned ordinary lake tile".

Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 4 guests