Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Erroneous behavior of Get/SetGrowthRate GS methods #6378

Closed
DorpsGek opened this issue Oct 13, 2015 · 9 comments
Closed

Erroneous behavior of Get/SetGrowthRate GS methods #6378

DorpsGek opened this issue Oct 13, 2015 · 9 comments
Labels
component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) enhancement Issue would be a good enhancement; we accept Pull Requests! patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

dp opened the ticket and wrote:

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

```
-> |
```

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 #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).

Attachments

Reported version: 1.5.2
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/6378
@DorpsGek
Copy link
Member Author

dp wrote:

Meh, I though flyspray supports markdown :/

http://pastedown.ctrl-c.us/# 3sD-yGknOr8mq0TbVhzkl-fjseg.markdown


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14074

@DorpsGek
Copy link
Member Author

dp wrote:

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).


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14080

@DorpsGek
Copy link
Member Author

dp wrote:

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 %)


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14091

@DorpsGek
Copy link
Member Author

dp wrote:

Oopsies, forgot to attach files xD

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14092

@DorpsGek
Copy link
Member Author

dp wrote:

Omg, can't delete attached file even though flyspray shows checkbox for that %)
Here is correct one:

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14093

@DorpsGek
Copy link
Member Author

dp wrote:

Moar comments + coding style fixed.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14096

@DorpsGek
Copy link
Member Author

Zuu wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14099

@DorpsGek
Copy link
Member Author

dp wrote:

  • 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

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6378#comment14100

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) Goal/Game script labels Apr 7, 2018
@TrueBrain TrueBrain added patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay enhancement Issue would be a good enhancement; we accept Pull Requests! and removed bug from FlySpray labels Apr 10, 2018
@frosch123 frosch123 added component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) and removed Goal/Game script labels Apr 14, 2018
michicc pushed a commit that referenced this issue Jun 24, 2018
Takes some code and ideas from #6378 patch, but doesn't change anything GS-related.
@andythenorth andythenorth added the stale Stale issues label Jan 5, 2019
@andythenorth
Copy link
Contributor

Oops, looks like I missed that some or all of this was merged in Jun 2018. Closing as it appears to be done.

@ldpl if I'm wrong, I can re-open. Thanks for contributing!

@andythenorth andythenorth removed flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) stale Stale issues labels Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) enhancement Issue would be a good enhancement; we accept Pull Requests! patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

4 participants