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

Switch town growth rate and counter to actual game ticks #6763

Merged
merged 6 commits into from
May 2, 2018

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented Apr 29, 2018

This is more of a groundwork type patch rather one doing something particularly useful. It's needed to be settled first as it changes inner workings of town growth and my other town patches are obviously affected by that. But to keep it simple I actually tried to make this patch change as little as possible and not focus on GS API or gameplay changes. Though some stuff still got fixed along the way.

There are three main reasons for this tick conversion

  1. To allow towns to grow faster than 1 hous / 70 ticks
  2. To allow more granularity of town growth speed where it matters (<30 days/house)
  3. To make town growth more smooth and not affected by 70 tick steps or tick overflow.

So what it actually changes:

  1. growth_rate and grow_counter now use actual game ticks instead of big town ticks (70 ticks). That makes effective range of growth speed 1-65100 ticks instead of 70-~2219980 (with step 70). So instead of 30000 days slowest growth speed is now 880 which is still plenty (and unreachable without GS as it only goes to about 400 normally).
  2. Moves TOWN_GROWTH_RATE_CUSTOM to town flags. Just a nice thing to do as it frees one more bit for growth_rate and gets rid of &~TOWN_GROWTH_RATE_CUSTOM everywhere.
  3. Fixes sligthly uneven growth of towns due to tick overflow issue (since 2**16 % 70 != 0 for some towns tick handlers were not called).
  4. Fixes Incorrect town growth rate ticks to days conversion #5827 because, well, if in needs to be changed anyway why not do it right.
  5. Makes days to ticks conversion in GS growth methods more precise. Had my doubts about this one, but technically it's just a bugfixing not API change since it just didn't do it right before. Having to emulate completely wrong old behaviour here seems even more weird to me.

What it does NOT change:

  1. Does not significantly change normal town growth mechanics (without GS) including funding. In fact, except for a few minor things it should be tick-perfectly the same.
  2. Does not really address Erroneous behavior of Get/SetGrowthRate GS methods #6378, as most of that code is still intact despite the tick switch.
  3. Does not provide GS methods to fully take advantage of the new range to separate GS API stuff from this issue.
  4. Still updates growth rate on monthly tick, so no Keep town growth rate in sync with house count #6397 for now.

@@ -266,7 +266,7 @@
* 196 27778 1.7.x
* 197 27978 1.8.x
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add PR# to comment when bumping revision? Probably needs some discussion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git blame ftw? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git log -U0 -G "SAVEGAME_VERSION =" src/saveload/saveload.cpp
And get rid of this pointless comment altogether

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, loses a lot of otherwise easily findable history that way

Copy link
Contributor Author

@ldpl ldpl Apr 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean openttd version history? Yeah, will be a bit trickier to get, but saves from the need for someone to constantly update the comment. Like how the hell am I even supposed to know what's next OpenTTD version is going to be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important part here are the savegame versions of the stable branches.
The revisions are not important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't add the 1.8.x. That's wrong.

t->growth_rate = TownTicksToGameTicks(t->growth_rate & ~0x8000);
}
/* Offset t->grow_counter by t->index to emulate original behaviour of spreading tick calls. */
t->grow_counter = TownTicksToGameTicks(t->grow_counter) + ((uint32)_tick_counter + t->index) % TOWN_GROWTH_TICKS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop "_tick_counter" here.
It rather confuses than affects stuff.

src/town_cmd.cpp Outdated
t->growth_rate = 250;
/* Spread growth across ticks so even if there are many
* similar towns they're unlikely to grow all at once */
t->grow_counter = ((uint32)_tick_counter + t->index) % TOWN_GROWTH_TICKS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I would drop "_tick_counter" here.
It rather confuses than affects stuff.

Copy link
Contributor Author

@ldpl ldpl May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't like it from competitive CB perspective, but it makes sense since if you create a big map with a lot of towns many of them will have exact same growth_rate so will be growing in same tick which I assume can even be a bit laggy in some cases (haven't tested though). I was thinking of making default growth rate calculation to be smooth rather than jump in 50-house steps then it will probably be ok to remove this part but before that I'd leave it be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frosch123 or did you mean drop _tick_counter but leave t->index? Yeah, that would probably work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, leave t->index for the spreading of CPU resources.
But remove _tick_counter since it does not do anything.

src/town_cmd.cpp Outdated
/* If growth failed wait TOWN_GROWTH_TICKS before retrying */
i = TOWN_GROWTH_TICKS - 1;
/* If growth failed wait a bit before retrying */
i = min(t->growth_rate, TOWN_GROWTH_TICKS - 1);
Copy link
Contributor Author

@ldpl ldpl May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also reduced retry time as it was growing in waves on fast speeds.

@frosch123
Copy link
Member

Please don't add the "1.8.x" to the savegame version. It won't be 1.8.x.

@frosch123 frosch123 merged commit fef8b83 into OpenTTD:master May 2, 2018
@ldpl ldpl deleted the towns/tick_growth branch July 14, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect town growth rate ticks to days conversion
4 participants