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 »

And why would you need to "add" things afterwards? You can add them during construction and hide them until needed.
What I wanted to avoid is having to touch the set of GUI widgets and the nested_array each time a new strategy for town placement is invented (or changed). Or, the other way round, I am not sure what becomes useful in advance, or ideas grow while one uses it.

The other point is, if some are based on some constant (e.g. a min_flow value) I might make that constant configurable as well, and then even adding multiple times the same thing might be useful. (e.g. with probability 200 and min_flow (indexed by max flow on map) 50 for some towns at rivers, and probability 400 and min_flow 300 for more towns at big rivers).

But, as I have said, I will implement something that works without adding things afterwards, take these posts as evaluation what infrastructure is available.
Eddi
Tycoon
Tycoon
Posts: 8257
Joined: 17 Jan 2007 00:14

Re: Patch: The Rainfall River Generator

Post by Eddi »

Alberth wrote:You can add them during construction and hide them until needed.
that would mean potentially destroying and recreating the window on every user interaction that adds a row. as you cannot know beforehand how often the user clicks that button.
Alberth
OpenTTD Developer
OpenTTD Developer
Posts: 4763
Joined: 09 Sep 2007 05:03
Location: home

Re: Patch: The Rainfall River Generator

Post by Alberth »

ic111 wrote:
Alberth wrote:And why would you need to "add" things afterwards? You can add them during construction and hide them until needed.
What I wanted to avoid is having to touch the set of GUI widgets and the nested_array each time a new strategy for town placement is invented (or changed). Or, the other way round, I am not sure what becomes useful in advance, or ideas grow while one uses it.
There is a pseudo-widget, that takes a function. On window construction the function is called, and it must return a tree of widgets.
Being a retired OpenTTD developer does not mean I know what I am doing.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Thank you, together with introducing a separate add & edit dialog, this will allow for senseful GUI design.

(in that I can separate town placer / parameter definition from setting up the GUI).

For the records:

Code: Select all

		case WWT_EDITBOX: {
			const QueryString *query = w->GetQueryString(this->index);
			if (query != NULL) query->DrawEditBox(w, this->index);
			break;
		}
... widget.cpp, function NWidgetLeaf::Draw.

If that function wouldn´t fail so silently when no QueryString is available, it would have saved me quite some minutes of trying different parameters to NWidgetLeaf (there is no code in OpenTTD yet constructing an EditBox using NWidgetFunction, thus I first suspected that some Fill, Padding, etc. was missing) until I realized that such a widget is only visible if there is a QueryString.

(I mean, I knew that I have to implement several things until it really works, but I was surprised that the widget wasn´t even visible...)
Alberth
OpenTTD Developer
OpenTTD Developer
Posts: 4763
Joined: 09 Sep 2007 05:03
Location: home

Re: Patch: The Rainfall River Generator

Post by Alberth »

It doesn't look like something I'd add as precaution without a need, so I am guessing it may have already existed (ie at the time of the conversion, some window used this to hide some edit boxes). Alternatively, it was added before there was an option to hide some widgets, and it was never fixed.
Being a retired OpenTTD developer does not mean I know what I am doing.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Small side patch I needed: The code for setting a string into a QueryString so far was buried inside QueryStringWindow. Bad if you don´t have a QueryStringWindow, but several QueryStrings in a Window.

This patch extracts that portion of code into a member function of QueryString, where it IMHO belongs to.
Attachments
337_MoveSetterCodeToQueryString.patch
(2.07 KiB) Downloaded 76 times
User avatar
wallyweb
Tycoon
Tycoon
Posts: 6102
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb »

Sorry for the delay. Real life got a bit busy.
Thank you Alberth, Eddi and JGR. :bow:
The following code worked marvelously well in MinGW: :D
JGR wrote:

Code: Select all

while read i; do echo "Applying $i"; patch -p1 -i "$i"; done < series
User avatar
wallyweb
Tycoon
Tycoon
Posts: 6102
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb »

ic111 wrote: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).
The problem seems to be within 245_BreadthFirstSearch.patch at the following chunk:

Code: Select all

@@ -18,6 +18,9 @@
 #include "slope_type.h"
 #include "debug.h"
 
+#include <set>
+#include <vector>
+
 /** The purpose of a HeightIndex is support for efficient access to all tiles
  *  of a given height.  E.g. a HeightIterator, which offers an interface for
  *  iterating over all tiles of a given height, is backed by a HeightIndex.
MinGW/msys tells me that chunk has already been applied. (By another patch?)
So I removed it from the patch and MinGW/msys stopped complaining, but this created a diff between your landscape_util.h and the one created in my run:
Your landscape_util.h has:

Code: Select all

#include "slope_type.h"
#include "debug.h"

#include <set>
#include <vector>

#include <set>
#include <vector>

/** The purpose of a HeightIndex is support for efficient access to all tiles
 *  of a given height.  E.g. a HeightIterator, which offers an interface for
 *  iterating over all tiles of a given height, is backed by a HeightIndex.
My landscape_util.h has:

Code: Select all

#include "slope_type.h"
#include "debug.h"

#include <set>
#include <vector>

/** The purpose of a HeightIndex is support for efficient access to all tiles
 *  of a given height.  E.g. a HeightIterator, which offers an interface for
 *  iterating over all tiles of a given height, is backed by a HeightIndex.
Is your second

Code: Select all

#include <set>
#include <vector>
necessary?
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

No, of course not :-)

Just header files are made safe against importing them twice anyway, thus this doesn't cause any error.

Thank you for finding this...
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Concerning saving it to the config, as I have said IMHO my only option is serializing it to a string (since I have a variable number of option items). I.e., I add

Code: Select all

	char* town_placers;
to the corresponding struct in settings_type. Then I serialize it as json string and store it to that config variable whenever needed. I.e.

Code: Select all

	void SerializeTownPlacerConfig()
	{
		std::string config_string = "";
		config_string += "[{phase:ONE_CITY,configs:";
		this->lists[TPP_PHASE_ONE_CITY].Serialize(config_string);
		config_string += "},{phase:TWO_TOWN,configs:";
		this->lists[TPP_PHASE_TWO_TOWN].Serialize(config_string);
		config_string += "}]";

		int length = config_string.size() + 1;
		_settings_newgame.game_creation.rainfall.town_placers = new char[length];
		strecpy(_settings_newgame.game_creation.rainfall.town_placers, config_string.c_str(), 
		             _settings_newgame.game_creation.rainfall.town_placers + length);
	}
In the config file, it looks like this:

Code: Select all

rainfall.town_placers = [{phase:ONE_CITY,configs:[{placer:1,weight:100,
    params:[{index:0,value:1},{index:1,value:300}]}]},{phase:TWO_TOWN,configs:[]}]
(if size matters, one could of course make the format more concise, but on the other hand, if size doesn´t matter that much, a more explicit format has its advantages either)

My question is, what is a senseful way to do the memory management here? The above solution works, but obviously leaves a memory leak, the char array is never deleted when the next call to that function occurs.

I might add a

Code: Select all

if (_settings_newgame.game_creation.rainfall.town_placers != NULL) {
   delete _settings_newgame.game_creation.rainfall.town_placers; 
}
but doesn´t that interfer with the config load/save code of OpenTTD?

Short: Is there any "OpenTTD way" to do that?
User avatar
JGR
Tycoon
Tycoon
Posts: 2557
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: Patch: The Rainfall River Generator

Post by JGR »

ic111 wrote:My question is, what is a senseful way to do the memory management here? The above solution works, but obviously leaves a memory leak, the char array is never deleted when the next call to that function occurs.
If the config is only loaded once at startup, you don't need to explicitly free the string as it'll be freed anyway when the process terminates.
Otherwise free the old value whenever you're about to update it with a new value.
Do note however that when _settings_game is cloned from _settings_newgame, any pointers such as these will need to be themselves cloned to avoid a double-free later on.
Perhaps also consider using a std::string or some such in the settings, and adjusting the savegame code.
ic111 wrote:

Code: Select all

if (_settings_newgame.game_creation.rainfall.town_placers != NULL) {
   delete _settings_newgame.game_creation.rainfall.town_placers; 
}
In the general, the array form of operator new is best avoided. You would need to use delete[] instead of delete in this case.

If you do go with JSON or similar I would advise against rolling your own parser.
Ex TTDPatch Coder
Patch Pack, Github
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Otherwise free the old value whenever you're about to update it with a new value.
Do note however that when _settings_game is cloned from _settings_newgame, any pointers such as these will need to be themselves cloned to avoid a double-free later on.
Thanks, I will have a look on that...
Perhaps also consider using a std::string or some such in the settings, and adjusting the savegame code.
Hm, would need conversions for existing code. Anyway, this kind of patch might easily be done at a later point of time, as my code uses std::string for the string manipulations anyway, and goes for char* only for saving it to _settings_newgame.
If you do go with JSON or similar I would advise against rolling your own parser.
As I don´t need a general purpose parser, I probably will do it on my own. After all, I have a clear structure of a few levels of nested brackets, support only positive integers and named constants at defined locations in the string, and if I don´t find what I expect because the string is corrupted, I can always stop and use a default config.

Or do you see any chance to use existing code in a feasible way?
Alberth
OpenTTD Developer
OpenTTD Developer
Posts: 4763
Joined: 09 Sep 2007 05:03
Location: home

Re: Patch: The Rainfall River Generator

Post by Alberth »

Perhaps look at code like station names, train names, and other things you can give a name?
I would expect it has the ability to store text data in a game.
Being a retired OpenTTD developer does not mean I know what I am doing.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Renaming a station uses a query window. Query windows (or rather their text buffer) allocate using MallocT, and free accordingly. The command CmdRenameStation uses stredup to duplicate that string, before it gets freed by the querystring / textbuffer destructor.

I.e. more or less what JGR suggested, one has to take care about freeing.
Alberth
OpenTTD Developer
OpenTTD Developer
Posts: 4763
Joined: 09 Sep 2007 05:03
Location: home

Re: Patch: The Rainfall River Generator

Post by Alberth »

I thought you were discussing saving such data in a file, right?

If you save a game with custom strings, and load it again, the custom string survives, so OpenTTD must have a way to save and load such data already.
My suggestion was to have a look how that happens.
Being a retired OpenTTD developer does not mean I know what I am doing.
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

The question was more about the memory management. Saving a char* in config to the file is already implemented, just as saving station.name (also a char*) to the savegame file is implemented.

(I did not inspect that to its very details though, you quickly end up in some code dealing with void* pointers, not that easily readable...).

So, the question was about how to do the memory management for the char*, and there the solution seems to be allocate it when you change it, and free it before. I just wanted to get sure that I don´t miss some technique that avoids that cycle, which as we all know is somewhat dangerous towards memory leaks.

So, problem hopefully solved :-)
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

Now, finally, town placement is configurable. Beside this, in this release I removed the savegame bump, as I was told some time ago that it isn´t necessary if one uses only settings with NEWGAME_ONLY flag (which I do). In other words, now this patch queue is savegame-compatible with trunk.

Town placement works using the config dialogs shown in the two attached screenshots. Hit the new "Details" button right of the town placer dropdown to enter the list view. Here, you see all currently configured town placers. Add or edit one using the corresponding dialog in the second screenshot.
I know, that the concept is somewhat sophisticated, thus I tried to keep that dialog self-documenting. Probably, I will try to improve those documentation strings in some of the next releases.

Now, you know where you can configure it, but how does it work?

- The whole map is partitioned into sections of 16x16 tiles.
- For each of those sections, several properties, expressed as integer values, are calculated. Things like, flow of the biggest river, maximum height, number of ocean tiles, and so on.
- A TownPlacer decides, based on those properties, wether such a grid section is interesting for it. For example, if a town placer follows the strategy, place towns near huge rivers, then it will only consider grid sections with sufficient flow of the biggest river in there. Another town placer e.g. might only conside coastal regions.
- Each TownPlacer is assigned some weight. weight / (the sum of all weights of all configured town placers) gives the probability with which it will be used. For example, if you want to place about 10 percent of your cities near a lake, you might assign the LakeTownPlacer a weight of 100. (if the sum of all weights is 1000).

The goal of the whole thing is being able to place cities and towns in a real-world-like manner, by using a combination of TownPlacers with appropriate weights. One thing I disliked always about the default town placement in OpenTTD is that frequently, Cities are placed at locations they usually don´t occur in reality, e.g. on top of a mountain.

Think of it as a framework for controlling town placement. Adding a new TownPlacer (i.e. a new strategy for placing towns) is possible with very little programmatic changes. The whole dialogs and generator code is implemented independent of the concrete town placer strategies. If one wants to enrich it with additional strategies, one might need some new value in the TownScore (i.e. the set of values calculated for each 16x16 grid section), and beyond that, just a new class extending TownPlacer (and registering it) is necessary. Those classes typically are in the region of 10 to 20 lines of code. For those interested, the API (structs etc.) lives in genworld.h

The reason why it lives in the Rainfall River Generator patch queue is that many of the strategies implemented so far depend on the information I gathered while the generator runs. With this information, e.g. finding out "I am in the valley of a huge river" is a very simple thing for me. Without it, one would need reasoning on sets of tiles, how is the water in the neighborhood distributed, does it look more like a lake or like a city, and so on.

I think that this should have been the last big change to the patch queue, what now remains to be done before I declare it finished are smaller improvements (e.g. help strings, comments, maybe moving code between patches, and so on),

A technical disclaimer: As I have said, I serialize the config to a json string. The corresponding parser is currently hand-written. It recognizes valid strings, and hopefully doesn´t segfault etc. for invalid ones, but it is not a complete parser. As JGR suggested, replacing it with some library code may be a good idea at a later time. But I see no point in trying to add another dependency to OpenTTD in such a patch, or similar. Thus, the main goal here was keeping it self-contained.

Have fun with it.
Attachments
rivers_v41.zip
(269.71 KiB) Downloaded 64 times
OptionList.png
(424.93 KiB) Not downloaded yet
OptionEdit.png
(368.84 KiB) Not downloaded yet
ic111
Director
Director
Posts: 608
Joined: 17 Jul 2007 17:56

Re: Patch: The Rainfall River Generator

Post by ic111 »

A bugfix release concerning the GUI for town placement.

Moved some code to the place where it belongs to, fixed the bug that on opening the "Add" dialog the parameter textboxes weren´t filled with the corresponding default values (they are defined by the TownPlacers).

Also I tried to improve the layout of the Add/Edit dialog. IMHO it looks good enough to use it, just the vertical alignment of the labels in the left column isn´t yet optimal. (some pixels moved towards the bottom). Unfortunately, I don´t fully understand what the layout algorithm does there. I will probably leave that for now.

If someone wants to play with it and improve that, you are welcome. The setup of that dialog is in town_rainfall_options_gui.cpp, lower section of the file; added in patch 334.
Attachments
rivers_v42.zip
(274.1 KiB) Downloaded 63 times
User avatar
wallyweb
Tycoon
Tycoon
Posts: 6102
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb »

ic111 wrote:...
I have found a new bug, this one when applying 345_TownPlacerConfigSettings.patch :

Never mind. The bug is gone with vesion 42
User avatar
wallyweb
Tycoon
Tycoon
Posts: 6102
Joined: 27 Nov 2004 15:05
Location: Canada

Re: Patch: The Rainfall River Generator

Post by wallyweb »

Here is the v.42 Windows 32 bit binary:
OpenTTD 1_5_2 Rainfall_v42_Win32.7z
(4.49 MiB) Downloaded 71 times
As far as I can tell all is good, except for the opening splash screen. It does not affect the game. Is this intended?
__Splash.png
__Splash.png (235.63 KiB) Viewed 2381 times
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 4 guests