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

Town::cargo_accepted, Town::cargo_produced saveload and consistency issues #7603

Closed
JGRennison opened this issue May 23, 2019 · 3 comments
Closed

Comments

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented May 23, 2019

Version of OpenTTD

Current master

Town::cargo_accepted is a TileMatrix with a 2x2 grid, of data type CargoTypes (i.e. uint64).
In src/saveload/town_sl.cpp it is saved and loaded with type uint32 for the data array, such that only the first half of the data array is actually saved and loaded.
The data array (but not the rectangular tile area extent) is unconditionally recalculated on load via AfterLoadGame -> UpdateHousesAndTowns -> RebuildTownCaches -> UpdateTownCargoes.

The 3 argument form of UpdateTownCargoes calculates the acceptance of each 2x2 square using an area up to 6x6 as returned by AcceptanceMatrix::GetAreaForTile with extent 1.
What is the rationale for: storing acceptance on a 2x2 tile basis, and using an area larger than 2x2 to populate the acceptance of that area?

In the case where a house is removed (in ClearTownHouse), the 2x2 square in which the northernmost house tile is, is updated by use of the 3 argument form of UpdateTownCargoes.
The case where the the house has multiple tiles which extend out of that 2x2 region doesn't seem to be handled, nor does the case where the immediately adjacent 2x2 regions also need to be re-updated due to the use of an enlarged region in the 3 argument form of UpdateTownCargoes.

In the case where the last house which accepts a particular cargo is removed, adjacent 2x2 squares can incorrectly retain the acceptance bit for that cargo, such that Town::cargo_accepted_total has the bit incorrectly set (see UpdateTownCargoTotal). This is used in subsidy generation.

UpdateTownCargoes is called both monthly (from TownsMonthlyLoop), and at load (as described above). In the case where Town::cargo_accepted/Town::cargo_accepted_total has become incorrect since the last TownsMonthlyLoop, and this a multiplayer server, if a new client joins it will recalculate Town::cargo_accepted and Town::cargo_accepted_total immediately with different values from those on the server, and this creates an opportunity for multiplayer desyncs.

Town::cargo_produced is also used in subsidy generation. It is added to from the 3 argument form of UpdateTownCargoes which is called on house add/remove, but removals are only made monthly or on load/join. In the event where the last house which produces a particular cargo is removed between a call to TownsMonthlyLoop and a multiplayer client join, the client and server would have differing values for Town::cargo_produced, and this also creates an opportunity for multiplayer desyncs.

What is the rationale for using a TileMatrix of bitmasks for acceptance and a single bitmask for production?

The acceptance and production of a house may be defined by means of NewGRF callbacks. If the results of the callbacks differ between the start the month and mid-month when a multiplayer client joins, this could also result in a multiplayer desync for the same reason as above.

Less importantly, enabling desync logging results in UpdateTownCargoes being called periodically from CheckCaches via RebuildTownCaches, however Town::cargo_accepted, Town::cargo_accepted_total and Town::cargo_produced are not among the variables which are checked for changes, and the old values are not restored. Consequently enabling desync logging masks the problem rather than highlighting it.

@JGRennison
Copy link
Contributor Author

@JGRennison JGRennison commented May 13, 2020

Bump

There are unresolved desync issues at present, and this could be an area of interest for investigation.

These are not directly rebasable but could point to potentially interesting areas of the code to look into for debugging:
JGRennison/OpenTTD-patches@326dfd3
JGRennison/OpenTTD-patches@9519c5c
JGRennison/OpenTTD-patches@3c2ebc4
JGRennison/OpenTTD-patches@546b524
JGRennison/OpenTTD-patches@5155afe
JGRennison/OpenTTD-patches@6d9f9ec
JGRennison/OpenTTD-patches@c2470a2

I will see if I can tidy all this up at some point.

JGRennison added a commit to JGRennison/Upstream-OpenTTD that referenced this issue May 17, 2020
In 11ab3c4 the number of cargo types was changed from 32 to 64.
The save/load of Town::cargo_accepted was not updated, such that
only half of the data structure is saved/loaded in savegame versions
199 to 218.
Discard and regenerate data from all savegame versions prior to 219.

Partially fixes: OpenTTD#7603
JGRennison added a commit to JGRennison/Upstream-OpenTTD that referenced this issue May 17, 2020
In 11ab3c4 the number of cargo types was changed from 32 to 64.
The save/load of Town::cargo_accepted was not updated, such that
only half of the data structure is saved/loaded in savegame versions
199 to 218.
Discard and regenerate data from all savegame versions prior to 219.

Partial fix for: OpenTTD#7603
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 3, 2021

I see several merged PRs in relation to this issue; what is left of this issue that needs resolving (if any)?

@JGRennison
Copy link
Contributor Author

@JGRennison JGRennison commented Jan 3, 2021

All of this stuff was nicely deleted by #8159, so this can indeed be closed.

@JGRennison JGRennison closed this Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants