OpenTTD

Tasklist

FS#6378 - Erroneous behavior of Get/SetGrowthRate GS methods

Attached to Project: OpenTTD
Opened by dP (_dp_) - Tuesday, 13 October 2015, 21:35 GMT
Last edited by dP (_dp_) - Thursday, 17 August 2017, 21:51 GMT
Type Bug
Category Script → Goal/Game script
Status With patch
Assigned To No-one
Operating System All
Severity Medium
Priority Normal
Reported Version 1.5.2
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

There are lots of cases SetGrowthRate can be used and, turns out, not all of them work well. Main problem that it leads to is town growth progress (grow_counter) being lost when you switch growth (in cb for example). Sometimes it even results in town not being able to grow at all. But generally it is just a bunch of closely intertwined bugs, so instead of trying to report them separately I did a comprehensive patch and will explain what's happening for every case possible (unless I missed some :).

For illustration I will be using test results that I got, they are all in form

```
<initial town state> -> <SetGrowthRate argument> | <current result town state> <patched result town state>
```

Where town state is `<growth_rate>:<grow_counter>`
Having * after growth_rate means it has TOWN_GROW_RATE_CUSTOM flag, [N] means town is not growing and [U] indicates that town state is not updated (can happen near the end of month for example).

Switching to normal growth
------
```
32:4[U] -> NORMAL | 12:4 12:4
12:4 -> NORMAL | 12:4 12:4
32*:4 -> NORMAL | 12:4 12:4
NONE:4[N] -> NORMAL | 12:4 12:4
12:4[N] -> NORMAL | 0:4[N] UNDEF:4[N]
```

Everything works fine except for the last test. When town is not growing UpdateTownGrowRate doesn't recalculate growth_rate leaving it at 0. It may seem harmless as it will be updated when it starts growing, but if another growth command is issued before that happens then grow_counter gets reset (loosing building progress):

```
0:4[N] -> 12 | 12*:12 12*:12
```

So I have to introduce a special value for this (TOWN_GROW_RATE_UNDEFINED) and check for it in CmdTownGrowthRate. This is done internally and doesn't affect GS at all.

```
UNDEF:4[N] -> 12 | 12*:0 12*:4
UNDEF:4[N] -> NORMAL | 0:4[N] UNDEF:4[N]
```

Stopping growth
-----
```
12:4 -> NONE | NONE:21845[N] NONE:4[N]
NONE:4[N] -> NONE | NONE:8[N] NONE:4[N]
16*:4 -> NONE | NONE:16383[N] NONE:4[N]
UNDEF:4[N] -> NONE | NONE:8[N] NONE:4[N]
```

As you can see grow_counter is being recalculated to some nonsense, because CmdTownGrowthRate doesn't check for TOWN_GROW_RATE_CUSTOM_NONE and treats it as normal value in calculations. Once again it leads to loosing building progress. So I fixed that by leaving growth_counter intact.


Switching to custom growth rate
-----
```
NONE:4[N] -> 32 | 33*:0 33*:4
12:4 -> 32 | 33*:11 33*:11
16*:4 -> 0 | 1*:0 0*:0
16*:4 -> 1 | 1*:0 0*:0
16*:4 -> 2 | 2*:0 1*:0
16*:4 -> 8 | 8*:2 7*:1
16*:4 -> 70 | 74*:18 73*:18
16*:4 -> 10700 | 11311*:2827 11310*:2827
31713*:31713 -> 30000 | 31714*:31714 31713*:31713
UNDEF:20[N] -> 12 | 12*:0 12*:12
1*:0 -> 96 | 101*:0 100*:0
2*:1 -> 96 | 101*:50 100*:50
```

First of all, and I already mentioned this in FS#5827, formula used for grow_rate conversion from days to tics is wrong. And that is particularly noticeable for small rates where town grows approximately every (growth_rate + 1) days. Also there clearly is a misconception as fastest growth speed is achieved when growth_rate is 0, not 1.

Next, for whatever reason (realism? ;) CmdTownGrowthRate actually tries to scale growth_counter (unfinished buildings progress) proportionally to growth rate. While there is nothing wrong with this on its own it seems a bit weird that this is the only case where it happens. Neither UpdateTownGrowRate nor switching from custom growth to normal does that (and that's understandable as it's kinda tricky and would require smth like saving prev growth_rate in town). Also, formula used for this scaling is also questionable. Furthermore due to values being discrete there is no clear notion of what that progress exactly is. For example (last two tests), with growth_rate 1 having growth_counter at 0 may mean any progress between 50% and 100% and for 2*:1 it is 33%-66%. Current values (100% and 50% respectively) fall into that range, so it's more or less fine. Due to all that I'm not sure what to do with this scaling, so I left it as it is. Probably would be better to just to remove it and leave grow_counter intact.

Also when switching from stopped growth (first test), grow_counter is once again "scaled" using special value. Fixed that one.

GetGrowthRate
-----
GetGrowthRate sometimes is not returning the same value that was used by SetGrowthRate (for 35, 70, 105 ...). So I fixed it to be the exact opposite of conversion I used in SetGrowthRate.

Other notes
-----
SetGrowthRate doc says days_between_town_growth <= 30000 so I relied on that and didn't design and test it with larger values. But it is not actually enforced in code, so it can break if that happens (as it could before).

Regarding GS compatibiliy I don't think this patch breaks anything. It does change TOWN_GROWTH_NORMAL value but that's quite weird if any GS rely on particular value of constant.

Also I attached some files I used for testing (too bag openttd doesn't have unit tests of its own).
This task depends upon

Comment by dP (_dp_) - Tuesday, 13 October 2015, 21:39 GMT
Comment by dP (_dp_) - Thursday, 12 November 2015, 19:16 GMT
Also noticed comment for CmdTownGrowthRate is wrong:
* @param p2 Amount of days between growth, or TOWN_GROW_RATE_CUSTOM_NONE, or 0 to reset custom growth rate.

p2 it's amount of TOWN_GROWTH_TICKS (70 ticks) intervals, not days (74 ticks).
Comment by dP (_dp_) - Monday, 16 November 2015, 15:55 GMT
I've updated my patch with regard to yesterday irc discussion

Changes:
- Always calculate growth rate instead of introducing TOWN_GROW_RATE_UNDEFINED
- Also scale growth counter in regular(non-gs) update of growth rate and when switching from custom rate to normal. Though, still no scaling is done if growth is stopped by gs and then started again with different growth rate, but I guess it's fine.
- Changed growth counter scaling formula a bit, now it always treats progress as lowest possible (counter = 0 - progress = 0%, counter = rate - progress<100%):
1*:0 -> 96 | 101*:0 100*:0
2*:1 -> 96 | 101*:50 100*:34
2*:2 -> 96 | 101*:101 100*:67
This also have additional benefit of preserving growth counter when switching to greater growth rate and back:
4*:2 --10--> 10*:4 --5--> 4*:2

P.S Splitting UpdateTownGrowRate turned out to be trickier than I though, ended up with 5 functions instead of 2. Hope I didn't break something %)
Comment by dP (_dp_) - Monday, 16 November 2015, 15:56 GMT
Oopsies, forgot to attach files xD
Comment by dP (_dp_) - Monday, 16 November 2015, 16:03 GMT
Omg, can't delete attached file even though flyspray shows checkbox for that %)
Here is correct one:
Comment by dP (_dp_) - Monday, 16 November 2015, 20:54 GMT
Moar comments + coding style fixed.
Comment by Leif Linse (Zuu) - Monday, 23 November 2015, 19:35 GMT
It would be useful to split the patch into smaller parts, as there are many things going on which make it hard to follow:
* Always re-compute growth counter.
* Changes to town growth constants. _NORMAL get a new value.
* Changes to formulas regard of rounding.

It can still utilize the same bug task, but it would help to separate these concerns as different patches.

Before starting with that I have some comments on the last patch after having discussed it with frosh123.
* Is it really necessary to use the _NORMAL constant instead of 0? Maybe the patch size can be reduced?
* Is this really correct? (TOWN_GROW_RATE_CUSTOM < TOWN_GROW_RATE_NORMAL, and p2 has to be smaller than ..._CUSTOM in order for t->growth_rate = p2 | TOWN_GROW_RATE_CUSTOM; to work correctly.)
- EnforcePrecondition(false, days_between_town_growth < TOWN_GROW_RATE_CUSTOM);
+ EnforcePrecondition(false, growth_rate < TOWN_GROW_RATE_NORMAL);
* Why do you change to use TOWN_GROW_RATE_* from town.h instead of TOWN_GROWTH_* from script_town.h in script_town.cpp?
Comment by dP (_dp_) - Monday, 23 November 2015, 21:01 GMT
> * Is it really necessary to use the _NORMAL constant instead of 0? Maybe the patch size can be reduced?
And how do you suggest to deal with 0 being a valid value for growth_rate? I could do +1/-1 to it but changing _NORMAL value is much better solution imo.

> * Is this really correct? (TOWN_GROW_RATE_CUSTOM < TOWN_GROW_RATE_NORMAL, and p2 has to be smaller than ..._CUSTOM in order for t->growth_rate = p2 | TOWN_GROW_RATE_CUSTOM; to work correctly.)
Right, it's not. It was supposed to be _NORMAL & ~_CUSTOM. But now that I think of it, just _CUSTOM will do, as _NORMAL is only used in CmdTownGrowthRate.

> * Why do you change to use TOWN_GROW_RATE_* from town.h instead of TOWN_GROWTH_* from script_town.h in script_town.cpp?
My bad, should've used those ones there. They are equal though.

Attaching fixed patch.


As for patch splitting I don't see any way to do it in a sensible manner. Changing _NORMAL value pulls Cmd changes and they require UpdateGrowRate split. I guess I could change counter scaling formula in old function first, but it's one-line patch and that would be just to move it to new function after.
For separate patch (and bug) I have one other thing in mind xD

Loading...