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

Jump to bottom

Make subsidies scan for town cargo instead of wasting resources and risking desyncs by maintaining cargo caches #8159

Merged
merged 3 commits into from Jun 28, 2020
Merged

Make subsidies scan for town cargo instead of wasting resources and risking desyncs by maintaining cargo caches #8159

merged 3 commits into from Jun 28, 2020

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented May 18, 2020 •
edited

As mentioned in #7603 and #8157 there are currently a number of problems with town cargo caches. I also roughly measured the performance of updating them and it contributes about 25%-100% of the total monthly tick lag. Considering they're only used by subsidy generation which only happens once per month with a small chance it makes absolutely no sense to waste so many resources maintaining these caches.

I see 3 possible ways of removing caches:

  1. Reimplement subsidy generation completely (more CPU intensive and needs savegame bump).
  2. Break/alter subsidies a bit by reducing the probability of generating certain subsidy types.
  3. Remove default subsidy generation completely in favor of GS solution.

This PR basically does 2 but is also a partway to 1 or 3 if either of them is chosen in the future.

It's meant to be backported so savegame bump was avoided and a separate PR will be needed to remove the junk from the save in future. And there is almost no harm to zeroing it all as the only possible consequence I see is that there may be fewer subsidies if newer save is loaded in the old game version.

JGRennison
JGRennison reviewed May 18, 2020
View reviewed changes
src/town_cmd.cpp Outdated Show resolved Hide resolved
ldpl added a commit to ldpl/OpenTTD that referenced this pull request May 18, 2020
@ldpl
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
b0613ce 
…ing the savegame version
@ldpl ldpl force-pushed the remove-town-cargo-accepted branch from e6b254f to b0613ce Compare May 18, 2020 14:48
ldpl added a commit to ldpl/OpenTTD that referenced this pull request May 18, 2020
@ldpl
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
e6b6d41 
…ing the savegame version
@ldpl ldpl force-pushed the remove-town-cargo-accepted branch from b0613ce to e6b6d41 Compare May 18, 2020 14:49
michicc
michicc reviewed May 18, 2020
View reviewed changes
src/subsidy.cpp Outdated Show resolved Hide resolved
ldpl added a commit to ldpl/OpenTTD that referenced this pull request May 18, 2020
@ldpl
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
6b0f102 
…ing the savegame version
@ldpl ldpl force-pushed the remove-town-cargo-accepted branch from e6b6d41 to 6b0f102 Compare May 18, 2020 16:58
ldpl added a commit to ldpl/OpenTTD that referenced this pull request Jun 28, 2020
@ldpl
Revert OpenTTD#8157 due to allow OpenTTD#8159 in a backportable way
57430b3
ldpl added a commit to ldpl/OpenTTD that referenced this pull request Jun 28, 2020
@ldpl
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
6f555ed 
…ing the savegame version
@ldpl ldpl force-pushed the remove-town-cargo-accepted branch from 6b0f102 to 6f555ed Compare June 28, 2020 16:06
@ldpl ldpl force-pushed the remove-town-cargo-accepted branch from 6f555ed to 097a790 Compare June 28, 2020 16:09
LordAro
LordAro approved these changes Jun 28, 2020
View reviewed changes
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

LGTM

@ldpl
Copy link
Contributor Author

ldpl commented Jun 28, 2020

For anyone wondering what happened here xD

<dP> LordAro, also I'm more worried that #8159 is voided now
<LordAro> dP: i don't think so
<LordAro> dP: that said, if you rebase it quick enough to replace what we just merged, i'd be happy to merge it
<LordAro> (just "reuse" the save version)
<dP> LordAro, 8159 is backportable fix with the whole purpose of avoiding version bump
<LordAro> ah yes, i remember
<LordAro> the whole "change existing subsidy generation" thing scares me
<LordAro> can you explain how the subsidy generation is changed, and why that's necessary? (in the PR)
<dP> LordAro, didn't I already?
<LordAro> dP: not that i can see
<dP> LordAro, well, I guess I can add a bit of "how" but "why" there are plenty
<dP> LordAro, it's more effecient to do a map scan in subsidy than maintain those caches
<LordAro> dP: i get why the change has been made
<LordAro> i don't get why that results in different subsidies being generated
<LordAro> "Break/alter subsidies a bit by reducing the probability of generating certain subsidy types"
<dP> LordAro, because it now checks only acceptance of 13x13 area around town center instead of doing weird thing that was probably supposed to check everywhere but doesn't
<LordAro> i see
<dP> LordAro, well, it does check everywhere but what it actually checks is beyond description xD
<LordAro> without really thinking about it, i can't see how that would result in different subsidy generation?
<dP> LordAro, if you have special house accepting some unique cargo somewhere on the outskirts of the town it won't generate subsidy for it
<LordAro> if it's as edgecasey as that, i'm not bothered at all
<LordAro> you can add a Revert commit to the beginning of your PR :)

@nielsmh nielsmh merged commit 7045186 into OpenTTD:master Jun 28, 2020
@ldpl ldpl mentioned this pull request Jul 2, 2020
Merged
@ldpl ldpl deleted the remove-town-cargo-accepted branch July 14, 2020 08:25
@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Jul 27, 2020
LordAro pushed a commit to LordAro/OpenTTD that referenced this pull request Jul 30, 2020
@ldpl @LordAro
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
882b3e6 
…ing the savegame version
LordAro pushed a commit to LordAro/OpenTTD that referenced this pull request Jul 30, 2020
@ldpl @LordAro
Change OpenTTD#8159: Remove now unused town cargo caches without bump…
1c5a884 
…ing the savegame version
LordAro pushed a commit that referenced this pull request Aug 9, 2020
@ldpl @LordAro
Change #8159: Remove now unused town cargo caches without bumping the…
f8bbd8d 
… savegame version
@ldpl ldpl mentioned this pull request Sep 5, 2020
Merged
@glx22 glx22 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Nov 8, 2020
@JGRennison JGRennison mentioned this pull request Jan 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michicc michicc michicc left review comments

@JGRennison JGRennison JGRennison left review comments

@LordAro LordAro LordAro approved these changes

Assignees
No one assigned
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ldpl @michicc @JGRennison @LordAro @nielsmh @glx22