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

Commit fef8b83 broke GSTown.SetGrowthRate #7112

Closed
Tahvohck opened this issue Jan 26, 2019 · 5 comments
Closed

Commit fef8b83 broke GSTown.SetGrowthRate #7112

Tahvohck opened this issue Jan 26, 2019 · 5 comments
Labels
needs triage This issue needs further investigation before it becomes actionable regression It used to work, and now it's broken.
Milestone

Comments

@Tahvohck
Copy link

Commit fef8b83 broke the way that GSTown.SetGrowthRate processes its input. Previous to this change, scripts could pass in numbers up to at least 50,000 (based on settings in Renewed City Growth), after the change, any value over 930 will cause the town growth to be set to 65,535 AND fail (return false). At the very least, this fail behavior is incorrect. It should definitely not display to the user that the value is 65,535, even if this is the internal state of the program.

Ideally, I don't see any reason why towns should be limited to growth rates of about 2.5 years. Less ideally, if the growth is going to be capped like that fail behavior should either disable growth or set it to the maximum time.

@nielsmh nielsmh added needs triage This issue needs further investigation before it becomes actionable regression It used to work, and now it's broken. labels Jan 26, 2019
@glx22
Copy link
Contributor

glx22 commented Jan 26, 2019

65,535 is TOWN_GROWTH_RATE_NONE

@nielsmh nielsmh added this to the 1.9.0 milestone Jan 27, 2019
@James103
Copy link
Contributor

Probable cause: While all town growth internal counters are scaled up by a factor of 70, the size of the integers used to hold those numbers remains the same, at 2^16 = 65536 (unit16).

Solution: Increase the town growth counter variables to 24-bit or 32-bit (uint32) instead of 16-bit (uint16). This should allow for town growth rates of 30 million days. For compatibility reasons, the town growth rate should be limited to 65534 days as usual (65535 beng reserved for TOWN_GROWTN_RATE_NONE).

A side effect of this change from town growth in days to ticks is that the town growth rate calculations could output a fractional number of days (i.e. be more accurate) then before commit fef8b83. This, if implemented, could theoretically raise the maximum population of a single town or city to tens of millions, from just hundreds of thousands as before.

@glx22
Copy link
Contributor

glx22 commented Jan 28, 2019

No when your call to GSTown.SetGrowthRate() returns false check the error (should be ERR_PRECONDITION_FAIL) and in this case growth rate is left unmodified.
If you call GSTown.GetGrowthRate() after the failing GSTown.SetGrowthRate() you get the unmodified old value, and it's TOWN_GROWTH_RATE_NONE in your case.

@glx22
Copy link
Contributor

glx22 commented Jan 28, 2019

After more reading of the code there's a bug

growth_rate = max(days_between_town_growth * DAY_TICKS, 2u) - 1;

DAY_TICKS should be TOWN_GROWTH_TICKS I think
return RoundDivSU(t->growth_rate + 1, DAY_TICKS);

this may be wrong too, but I'm not sure

glx22 added a commit to glx22/OpenTTD that referenced this issue Jan 28, 2019
@glx22
Copy link
Contributor

glx22 commented Jan 28, 2019

My previous comment is wrong, the problem is EnforcePrecondition()

EnforcePrecondition(false, days_between_town_growth <= MAX_TOWN_GROWTH_TICKS);

doesn't match
* @pre days_between_town_growth <= 880 || days_between_town_growth == TOWN_GROWTH_NONE || days_between_town_growth == TOWN_GROWTH_NORMAL.

allowing growth_rate to be at max 68819 (doesn't fit in 16bits), while it should be at max 65119 (based on the comment)

The precondition check was already wrong before fef8b83 allowing up to 30996 when the comment says 30000, but was safe because still within 16bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue needs further investigation before it becomes actionable regression It used to work, and now it's broken.
Projects
None yet
Development

No branches or pull requests

4 participants