Changes to settings.c and settings_gui.c break patches

richk67 · Post by **richk67** » 08 Mar 2006 01:23

I was trying to update my TerraGenesis patch against the latest nightly, and discovered that the whole system for putting features on the Configure Patches panels has changed. Seriously, radically changed.

Code: Select all

Settings.c
	/***************************************************************************/
	/* Economy section of the GUI-configure patches window (52 - 62) */
	SDT_BOOL(Patches, inflation,                  0, 0,  true,            STR_CONFIG_PATCHES_INFLATION,        NULL),
...
STR_CONFIG_PATCHES_BRIBE,            NULL),
	 SDT_VAR(Patches, snow_line_height,SLE_UINT8, 0, 0,     7,   2,   13, STR_CONFIG_PATCHES_SNOWLINE_HEIGHT,  NULL),
	 SDT_VAR(Patches, colored_news_date,SLE_UINT, 0,NC,  2000,1900, 2200, STR_CONFIG_PATCHES_COLORED_NEWS_DATE,NULL),

and in settings_gui.c

static const PatchEntry _patches_economy[] = {52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62};

The hard coded numbers in settings_gui are a nightmare for any future mods. For example:

To try to separate out the terrain features, for instance, I want to extract Snow Line Height, and include it on a new page with my setting "Use TerraGenesis".

If I move snow_line_height, all the patch numbers are broken. If I dont move it to maintain the numerical structure, then the "groupings" of patches together becomes meaningless, as old patch entries have to be left scattered around just to stop the numbers breaking.

Also, it is impossible to tell looking at the code in settings_gui.c what patches are listed on what patch page. They are just numbers with no explanation. "Open settings.c in another window". Well, didnt have to in the previous incarnation - and why ever have hardcoded numbers!

Take this scenario: you decide to include 3 patches, each of which wants to add entries on different patch pages. Patch 1 adds 2 items, and expects them to be items 94 and 95 (given current usage up to 93). Patch 2 has 3 expecting 94,95,96. Patch 3 has 1 expecting 94. etc... total nightmare. Every new patch will conflict with every other patch.

[edit] I have recoded TGP to work with the new layout, and discovered how many problems this will cause. Although the comment line says:

Code: Select all

	/* Patches without any GUI representation (69 - 78) */

This is actually 69-77. The next section, commented as 79-93, is actually 78-91. So my new first entry is actually 92. But I had to work it out by counting the lines. No error is given at compile time, and if you get the number wrong it just CTDs out. (eg. because you accidentally picked one with a NULL in the display string.) *** THIS IS BAD NEWS ***

If the configure patches list were totally static, the current system is fine.

But it is the nature of patches to be dynamic, one person adds one selection, another person uses a different set. The old system for all its faults allowed patches to be added without conflicting with each other. Unfortunately, this system guarantees conflict!

This is seriously important!! (OK at least to me as Integrated Nightly creator.) The last IN had 28 patches, which included 30 additional entries into the Configure Patches screens.

Please, please find a way to use human readable tokens in the PatchEntry lists - yes, it aids readability, but mainly it avoids number-conflicts.

MeusH · Post by **MeusH** » 08 Mar 2006 09:11

I fully support you richk67, I learnt some time ago hardcoded numbers are really bad.

IMO patches_starting_date is much better than i.e. 54, and it won't cause problems.
I'd like to see the reasons of applying the new system...

gagarin · Post by **gagarin** » 08 Mar 2006 09:48

While updating the subsidiaries patch I had the same Problem.
I think i better aproach woud be to make an array for every patch and have referenzes to this array.
So you get something like this:

static const PatchEntry _patches_cooperation[] = {&(coop_sets[1]),&(coop_sets[2]),...};

an other approach could be:

static const PatchEntry _patches_cooperation[] = {1,2,3,...};

static const PatchPage _patches_page[] = {
...
{_patches_cooperation, lengthof(_patches_cooperation, coop_sets)},
};

coop_sets[]={ SDT_BOOL(Patches, subsidiaries, SLF_SAVE_NO, 0, true, STR_CONFIG_PATCHES_SUBSIDIARIES, NULL),
SDT_BOOL(Patches, allow_track_removal, SLF_SAVE_NO, 0, true, STR_CONFIG_PATCHES_SUBS_ALLOW_REMOVE, NULL),
SDT_VAR(Patches, shared_stations_tax,SLE_UINT8, SLF_SAVE_NO, 0, 10, 0, 100, STR_CONFIG_PATCHES_SHARED_ST_TAX, NULL),
...
}

Greetings,
gagarin

SirkoZ · Post by **SirkoZ** » 08 Mar 2006 10:57

You Stupid developers! Can't you see we're trying to maintain a quality version of OpenTTD here?

At any rate - this is just awful...
What's your strategy, Developers, OUT OF CHAOS COMES ORDER!?
It seems to be pretty popular one in these days.

richk67 · Post by **richk67** » 08 Mar 2006 14:00

SirkoZ wrote:You Stupid developers! Can't you see we're trying to maintain a quality version of OpenTTD here?

At any rate - this is just awful...
What's your strategy, Developers, OUT OF CHAOS COMES ORDER!?
It seems to be pretty popular one in these days.

I wouldnt go as far as your opening line! There is a good reason (having a unified structure for vars etc to ease saveload issues) , but I am 100% convinced this is not the impact they intended! (Nice devs

)

CobraA1 · Post by **CobraA1** » 08 Mar 2006 15:02

I agree. I'm sure the intentions were good, but these hardcoded numbers are a step backwards

.

hertogjan · Post by **hertogjan** » 08 Mar 2006 16:22

The new system is actually quite good, except for the indexing of the entries.
The entries should be identifyable by an invariant number, i.e. a number that doesn't change when extra lines are added.
I think that maybe the patch setting entries can be identified by their variable name or by the index of the displayed string.
For example, the list would then look like this:

Code: Select all

_patches_ui[] = {
"vehicle_speed",
"status_long_date",
(...)
}

Note that each entry gets its own line, to prevent conflicts from happening when patching. A simple function that runs only once on startup will create the correct indices (i.e. change them to the array of integers that is currently used), so all the other functions don't have to be changed.

CobraA1 · Post by **CobraA1** » 08 Mar 2006 16:56

Yeah, I agree it's mostly good. They just have to replace the hardcoded numbers.

SirkoZ · Post by **SirkoZ** » 08 Mar 2006 18:02

richk67 wrote:
SirkoZ wrote:You Stupid developers! Can't you see we're trying to maintain a quality version of OpenTTD here?

At any rate - this is just awful...
What's your strategy, Developers, OUT OF CHAOS COMES ORDER!?
It seems to be pretty popular one in these days.
I wouldnt go as far as your opening line! There is a good reason (having a unified structure for vars etc to ease saveload issues) , but I am 100% convinced this is not the impact they intended! (Nice devs )

I know I know - that's why I put :LOL behind - no fire really intended. I'm just having phun.

gagarin · Post by **gagarin** » 08 Mar 2006 19:16

The simplest thing could be, that you use the current struct and instead of an array wiith the entries a patch uses you only give the Number of Entries for that patch.
Drawing the Patches-Windows ist a little more complicated then, but you avoid the hard-coded indices.

Darkvater · Post by **Darkvater** » 12 Mar 2006 21:34

The original idea behind this list was that you will only add new entries at the end so it wouldn't matter anyways. But two things can go wrong here:

1. adding entries in between the list (conditionals)
2. making settings loadable/not loadable in between versions

Both screw up the order completely. So yes, you are all right in saying that this simple indexing is pretty crude.

I could go with the string representation of the patch-names and making the table on first load of the GUI. Any other suggestions are of course also welcome in the mean time.

richk67 · Post by **richk67** » 13 Mar 2006 18:39

Darkvater wrote:The original idea behind this list was that you will only add new entries at the end so it wouldn't matter anyways. But two things can go wrong here:

1. adding entries in between the list (conditionals)
2. making settings loadable/not loadable in between versions

Both screw up the order completely. So yes, you are all right in saying that this simple indexing is pretty crude.

I could go with the string representation of the patch-names and making the table on first load of the GUI. Any other suggestions are of course also welcome in the mean time.

Thanks for the attention DV. I would be very happy with using strings, in the sort of style hertogjan mentioned. Any system that will allow for easy creation of new Patch Pages and Patch entries, without disturbing the existing ones / ones from other patches. That would be great.

hertogjan · Post by **hertogjan** » 13 Mar 2006 19:47

The way I mentioned is really the most easy way to implement it. The names of the variables are in fact already used as an identifier. In the config file, that is. So there must already be a function that translates the names to the indices.
(I had not thought about this when I wrote my previous message in this topic, but this makes the idea only better.)

Darkvater · Post by **Darkvater** » 16 Mar 2006 00:27

Ok, I had a little work on it tonight.

How about this? Comments are welcome.

There is actually no function to translate the config file names to indices because it is not needed (at least in the current implementation). What it does is it loops the settings and then searches for it inside the config file. I do use indices already when setting (and getting) the value of a setting; for example in MP. You can't send the pointer, so you send the index.

I have the list of names for the gui, and on the first time the patch window is opened I build up the list of the corresponding structure.

richk67 · Post by **richk67** » 16 Mar 2006 00:36

Darkvater wrote:Ok, I had a little work on it tonight.

How about this? Comments are welcome.

There is actually no function to translate the config file names to indices because it is not needed (at least in the current implementation). What it does is it loops the settings and then searches for it inside the config file. I do use indices already when setting (and getting) the value of a setting; for example in MP. You can't send the pointer, so you send the index.

I have the list of names for the gui, and on the first time the patch window is opened I build up the list of the corresponding structure.

Sounds good. I looked through the code (dont want to integrate immediately into my dev build as it will affect my TGP development!). Method looks fine, and simple to add extra patches in too. Thumbs up from this side. (Of course, all that can change when I eventually compile it!!

). Thanks, DV.

SirkoZ · Post by **SirkoZ** » 16 Mar 2006 01:32

Nice - will this implementation stick?
I'll finally update my patches.... =)

hertogjan · Post by **hertogjan** » 16 Mar 2006 17:02

It works pretty well.
Only if you mistype one of the patch names, unexpecterd things may happen. It either crashes, or it (magically) shows the intended setting as if there was no typo at all (happens if the first few letters are equal), or maybe the wrong setting will appear (I haven't seen this one appear).

Maybe there should be some kind of fallback: something like a line "---invalid patch setting---" on the places where an unknown patch name is used.

Or maybe you should just trust programmers that they don't make typos. (In that case, it is better if the game crashes all the time if a typo is made).

sidew · Post by **sidew** » 16 Mar 2006 17:17

hertogjan wrote: Or maybe you should just trust programmers that they don't make typos. (In that case, it is better if the game crashes all the time if a typo is made).

Speaking about typo, this morning, after got the last revision of trunk, I found one in english.txt that screw me the string.h file and leave ma lots of compile errors and warnings...

I found :

Code: Select all

STR_512                                                         :512
7STR_1024                                                        :1024
STR_2048                                                        :2048

instead of

Code: Select all

STR_512                                                         :512
STR_1024                                                        :1024
STR_2048                                                        :2048

Darkvater · Post by **Darkvater** » 16 Mar 2006 20:28

hertogjan wrote:Or maybe you should just trust programmers that they don't make typos. (In that case, it is better if the game crashes all the time if a typo is made).

Hmm, interesting point there. My first thoughts was who cares, its non-user input, so have a little faith, but then I realized that the same method of getting the setting from the name is used in the console, where it is console-related.
The only thing I can think of is if a setting is found check that strlen() of both are the same and if not, return NULL and let it crash (the gui that is).

Darkvater · Post by **Darkvater** » 17 Mar 2006 08:21

hertogjan wrote:It works pretty well.
Only if you mistype one of the patch names, unexpecterd things may happen. It either crashes, or it (magically) shows the intended setting as if there was no typo at all (happens if the first few letters are equal), or maybe the wrong setting will appear (I haven't seen this one appear).

What do you mean? If the strings don't match exactly GetPatchFromName() returns NULL and it crashes. And it doesn't matter if you type "auotrenew1" or "autorene" as a mistype.

Transport Tycoon Forums

Changes to settings.c and settings_gui.c break patches

Changes to settings.c and settings_gui.c break patches

Who is online