Patch: The Rainfall River Generator

[ic111]

In fact I usually do the changes (debug or not) in 400_Temp, and before a release, I distribute the code where it is supposed to belong to.
(i.e. what I meant with "I set them to 9 before a release" simply is that I dismiss the change in 400_Temp...)

I don´t know that happened here...

I hope at least the next release applies without fixing rejects...

[ic111]

Now I know what happened: The thing with the reject at patch 240 is: It is no longer part of the queue. I removed it from patches/series at some time manually (probably because at first, I wanted to have a look how landscape looks like if I omit that step), and it seems that when I finally decided that I actually don´t need that step, I forgot to also delete the file.

And it seems that you manually apply the patch files (it must be a lot of work, given their number), and not just hit "hg qpush -a" as I do, which only applys what´s in the patches/series file.

[ic111]

Changes in this release:

Hopefully fixed the seldom endless loop, the criterium now should be safe.

Improved the detection of logical rivers, and display them as debug info in the land info window. See the attached screenshot. It tells you, that the generator identified the tile at hand as part of river number 1, 1047 tiles [if one is only allowed to step NW,NE,SW,SE, not diagonal] away from its spring.

If someone wants to set up a concept of river names (and cities named accordingly) for OpenTTD, this could be done on the basis of the information I gather here. I so far use it just for finding out where rivers flow upwards, and correcting this in a postprocessing step.

Note that the river_map and river_iteration arrays the information in the land info is based on are *not* made persistent. The additional lines in the land info are only meant as debugging information available right after generation, as inspecting data like this is much easier using the land info window, than by inspecting tons of log output...

Quite a lot of small bugfixes, mainly regarding bookkeeping, that had at most limited influence on generation.

... and I cleaned up the patch directory, it now should contain only files that are really necessary. I just usually don´t like typing things like "rm patches/*~" right before a release...

[wallyweb]

Now I know what happened: The thing with the reject at patch 240 is: It is no longer part of the queue. I removed it from patches/series at some time manually (probably because at first, I wanted to have a look how landscape looks like if I omit that step), and it seems that when I finally decided that I actually don´t need that step, I forgot to also delete the file.

I will try v37 and not inlude patch 240 v38.

And it seems that you manually apply the patch files (it must be a lot of work, given their number), and not just hit "hg qpush -a" as I do, which only applys what´s in the patches/series file.

My compiler is MinGW. But the extra work is good exercise for me.

[JGR]

And it seems that you manually apply the patch files (it must be a lot of work, given their number), and not just hit "hg qpush -a" as I do, which only applys what´s in the patches/series file.

When I last applied your patch series I deleted all the temporaries and ran something along the lines of `cat *.patch | patch -p1` which works well enough.

[wallyweb]

I deleted all the temporaries and ran something along the lines of `cat *.patch | patch -p1` which works well enough.

Indeed it does ... with one small exception ... It does not name the patches as it runs. If there is an error or a fail, it can be a bit of a pain to determine the offending patch:

Code: Select all

patching file src/rivers_rainfall.h
patching file src/landscape_util.cpp
Hunk #1 succeeded at 523 (offset 5 lines).
patching file src/landscape_util.h
Reversed (or previously applied) patch detected!  Assume -R? [n] y
Hunk #2 FAILED at 284.
1 out of 2 hunks FAILED -- saving rejects to file src/landscape_util.h.rej
patching file src/landscape_util.cpp
patching file src/landscape_util.h

[wallyweb]

...

I found another problem, this time with 245_BreadthFirstSearch.patch:

Code: Select all

Adminguy@Wallwalk ~/_Patches/ic111/ic111_rivers_v38-cityOn-1_5_2_A
$ patch -p1 < 245.patch
patching file src/landscape_util.cpp
Hunk #1 succeeded at 523 (offset 5 lines).
patching file src/landscape_util.h
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] y
Hunk #1 succeeded at 21 with fuzz 2 (offset 3 lines).
Hunk #2 succeeded at 291 (offset 4 lines).

I was able to continue and complete the compile, and OTTD seemed to function ok, but I am not confident.

I will try again after you have a chance to fix 245_BreadthFirstSearch.patch

[ic111]

Hm, I don´t know exactly what the tool wants to tell you at that point (probably that it wasn´t confident either). But the fact that you are able to compile and run at least suggests that it did things the right way; since merging errors in a header file quickly lead to duplicate symbols etc., i.e. to a compile error.

Here is v39. I this time also added a zip with the most important files of this patch (landscape_util.h / cpp and rivers_rainfall.h / cpp). One would expect, that after you have applied the complete queue of v39, you don´t get any differences between the files resulting of that, and the files in that zip (if you use something like diff to compare them).

(as those files are created by the patch queue, they are not at all influenced by what possibly happens on trunk).


Changes in v39:
- Fixed a nasty bug in AddDirectionToTile, one comparison in the checks against map edge was missing...
- Fixed a bug in flow calculation, where tile instead of curr_tile was used.
- Made calculation of wider rivers and valleys a bit simpler, the extra cases for inclined slopes actually weren´t necessary.
- Implemented an extra treatment for the locations where rivers and ocean meet. They are a bit problematic for the generator, in that there, the rivers end. Thus, up to now, in some cases river and ocean wasn´t connected any longer after the generator completed. This additional algorithm detects such situations, and fixes them by terraforming and adding additional river tiles.

[Eddi]

I deleted all the temporaries and ran something along the lines of `cat *.patch | patch -p1` which works well enough.

Indeed it does ... with one small exception ... It does not name the patches as it runs.

try this one then:

Code: Select all

for i in *.patch; do echo Applying $i; patch -p1 -i $i; done;

[Alberth]

try this one then:

Code: Select all

for i in *.patch; do echo Applying $i; patch -p1 -i $i; done;

Or slightly more correct

Code: Select all

for i in `cat series` do echo Applying $i; patch -p1 -i $i; done

The 'series' files defines the official order of the patches, the back-ticks around 'cat series' dump the file content, and iterate over the content.

[Eddi]

Or slightly more correct

right. except i dislike ``. i would use $() instead.

[wallyweb]

try this one then:

Code: Select all

for i in *.patch; do echo Applying $i; patch -p1 -i $i; done;

Or slightly more correct

Code: Select all

for i in `cat series` do echo Applying $i; patch -p1 -i $i; done

The 'series' files defines the official order of the patches, the back-ticks around 'cat series' dump the file content, and iterate over the content.

Code: Select all

Adminguy@Wallwalk ~/_Patches/ic111/ic111_rivers_v39-cityOn-1_5_2
$ for i in `cat series` do echo Applying $i; patch -p1 -i $i; done
sh: syntax error near unexpected token `patch'

[Eddi]

missing a ; before "do"

rule #1: in a post correcting someone's mistake, there is at least one mistake (also known as Skitt's Law)

[JGR]

Or slightly more correct

Code: Select all

for i in `cat series` do echo Applying $i; patch -p1 -i $i; done

The 'series' files defines the official order of the patches, the back-ticks around 'cat series' dump the file content, and iterate over the content.

Strictly speaking this will go wrong if the filenames contain whitespace or shell metacharacters.
An even more correct version would be

Code: Select all

OLDIFS="$IFS"; IFS=$'\n'; for i in `cat series`; do echo "Applying $i"; patch -p1 -i "$i"; done; IFS="$OLDIFS"

or

Code: Select all

while read i; do echo "Applying $i"; patch -p1 -i "$i"; done < series

[ic111]

Now, finally, the question wether the river generator should also place towns is configurable.

I added a corresponding dropdown to the map generation dialogs. The details button right of that dropdown does not yet work, the corresponding dialog is not yet implemented.

As I now have a dropdown for that, I also added an item "No towns" that is only shown in scenario editor. Or, the other way round, I added the ability to generate towns during landscape generation in scenario editor.

After all, when one has a configurable town placer, IMHO letting it do some initial work, and then manually correct the places on map where the result doesn´t fit the personal taste is a reasonable approach to use the scenario editor.

At least, I see no reason to forbid that. The default value in scenario editor is "No towns" nevertheless, i.e. no change with respect to the current behaviour.

[ic111]

Concerning the details GUI for town placers, I face the question how I can do it.

TownPlacers are short (5 to 10 lines of code) snippets of code that implement one particular strategy to place a city, based some measures collected about map before. Such a strategy might read like "Place a city in this section of the map, if it contains a big river". Each such strategy is executed with a given probability, e.g. in 200 of 1000 cases strategy A, in 500 strategy B, and in the remaining 300 strategy C. Thus, conceptionally, there is no fixed number of them, in particular, I don´t want to bind GUI design too much to that (i.e. I don´t want to have to touch the GUI just because a new strategy enters the scene).

Saving this to the configuration should be possible using SLE_STRQ, which provides a string. I.e. encoding such a list of strategies into a string seems feasible, at least as far as I overlook the infrastructure I have. Will look like "[{STRATEGY_A, 200}, {STRATEGY_B, 500}, {STRATEGY_C, 300}], or maybe in a more json-like format [{name:STRATEGY_A, prob:200}, ...].

(assumption: SLE_STRQ has no tight limit on the string size, i.e. I hope nothing breaks if it has some hundred bytes)

But where I don´t see a good solution so far is the GUI list. In more modern GUIs, I would place some Add button somewhere, and add lines consisting of a Dropdown, a Textbox for the probability, and maybe more textboxes if there are more specialized parameters one might set.

In OpenTTD, we have CreateNestedTree in window.cpp, which stores the widgets in nested_array, which then has a fixed size. GetWidget then relies on a widget being part of that nested_array. Additionally, widget ids are only implicitely given by the named constants.

In other words, that whole widget concept doesn´t seem to be prepared for the usecase, call "Add" after some button was clicked in order to dynamically add a widget, and maybe remove it later. remove() BTW is not even implemented.

So what could I do? I might stick to a purely textual representation of the list (e.g. using WWT_MATRIX), and perform changes using a single dropdown somewhere below or above.

Or I drop the decoupling of GUI design and existing TownPlacers, and instead add all I can sensefully think of to a list, i.e. just add several lines hardcoded.

Any comments?

[Eddi]

maybe there is a case here to coordinate the GUI layout with stuff like programmable signals or a new order GUI.

[Simons Mith]

I was teaching a friend who hasn't played any of the Transport Tycoon games for /years/ how to play OTTD,
and watching him struggle with the UI was really eye-opening. It's really pretty hostile. But you get used to
it, and forget that. It doesn't compare very favorably with the Sim City 2000 UI, for example.

If this new Rivers feature warrants a UI overhaul - or even just an expansion - I think it might benefit
the game if we took the chance to do some UI work. I'm looking at writing up what I can of Wiz's experience,
but there was just /so much/ I haven't really decided where to start.

Perhaps I'll try to write down the 'rules' of the OTTD' UI, and then try to enumerate the places where the
game breaks its own principle, and then look at how they might be fixed. I have made some start on this
in other posts, but my last weekend's gaming gave me a shed-load of extra material.

[ic111]

What I don´t want is adding another big project to that patch queue...

After a look into the code: What might be possible relatively easy is, replace that

Code: Select all

NWidgetBase **nested_array

in window_gui.h by a

Code: Select all

std::map<int, NWidgetBase*> widget_map

(or however one calls it).

The existing WID_FOO_BAR constants (defined in src/widgets/foo_widget.h) are >= 0, so if one adds custom widgets programmatically outside the usual nested arrays, one might do that with negative index. Or, alternatively, use some _END constant for identifying the end of the already used space of indices.

In either case, the code controlling those dynamically added widgets would be responsible for keeping that indices unique.

Sure, a map is somewhat slower than indexed access in an array, but I don´t think that this really matters here, both the number of widgets per window, and the number of accesses into that array or map (using GetWidget) is probably rather small.

Nevertheless, this is something that needs to be discussed before implemented, and as this patch queue is already big enough (and my time not endless), I think I´ll implement a traditional static solution now.

[Alberth]

In other words, that whole widget concept doesn´t seem to be prepared for the usecase, call "Add" after some button was clicked in order to dynamically add a widget, and maybe remove it later. remove() BTW is not even implemented.

So what could I do?

And why would you need to "add" things afterwards? You can add them during construction and hide them until needed.