-
Notifications
You must be signed in to change notification settings - Fork 152
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
Economic Districts #6345
base: master
Are you sure you want to change the base?
Economic Districts #6345
Conversation
Assigned to Nordfriese |
Mirrored from Codeberg Does this change anything related to economy worker settings? In the attached save game the barracks produce soldiers endlessly, 'stealing' the wares needed to create heroes. Starting this save game in master the soldier production stops, because economy setting says 10 soldiers and there are already more than 40 sitting around. In debug builds the game stutters for me every time the district calculation is made. Probably because of the debug things. |
Mirrored from Codeberg Ah yes… So for districts where a ware/worker is not meant to be stored in warehouses this check won't work. And if some other (foreign) warehouse has a Prefer policy for that ware/worker but we don't Prefer it locally then too it won't work. I'll create a commit to take these stock policies into account and skip the local target check if the warehouse policies interfere with district target distribution.
For me debug builds with such large maps stutter on every economy update anyway ;) What times does the log print out for district recalculation and imports checking? For me this is always 1ms each with your savegame in a debug build (i.e. negligibly fast). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 15:45:41 CEST 2024, Tóth András (tothxa) commented:
I tested it in a game too and it seems to work nicely. Thanks a lot for solving this! :)
Some comments inline.
@@ -204,6 +216,10 @@ struct Flag : public PlayerImmovable, public RoutingNode { | |||
Coords position_; | |||
Time animstart_{0}; | |||
|
|||
Warehouse* district_center_[2] = { | |||
nullptr, | |||
nullptr}; ///< Warehouse at the center of our district, indexed by WareWorker (may be null). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 15:45:41 CEST 2024, Tóth András (tothxa) wrote:
Is indexing this directly by an unscoped enum safe?
src/economy/flag.cc
Outdated
@@ -1015,6 +1018,19 @@ void Flag::log_general_info(const Widelands::EditorGameBase& egbase) const { | |||
|
|||
Widelands::PlayerImmovable::log_general_info(egbase); | |||
|
|||
if (district_center_[0] == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 15:51:36 CEST 2024, Tóth András (tothxa) wrote:
Maybe wwWARE
(and wwWORKER
below) should be used for consistency?
src/economy/economy.cc
Outdated
++target_district; // Rounding up is important for wares with small targets | ||
} | ||
} else { | ||
target_district = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 16:26:04 CEST 2024, Tóth András (tothxa) wrote:
Simply initialise as 0 for readability instead of this else
clause?
|
||
Flag& supply_flag = imm->base_flag(); | ||
Warehouse* supply_district = supply_flag.get_district_center(type()); | ||
if (supply_district == nullptr || supply_district == target_district) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 17:24:19 CEST 2024, Tóth András (tothxa) wrote:
Is there a valid case when supply_district
can be nullptr
? Doesn't that mean there's some inconsistency somewhere?
src/economy/economy.cc
Outdated
wh->base_flag().set_district_center(type(), wh); | ||
astar.push(wh->base_flag()); | ||
} | ||
while (RoutingNode* current = astar.step()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 17:34:44 CEST 2024, Tóth András (tothxa) wrote:
Isn't this an implicit cast to bool?
constexpr uint32_t kClusterThreshold = 12 * 1800; // 12 nodes on flat terrain. | ||
std::map<std::pair<Warehouse*, Warehouse*>, uint32_t> warehouse_distances; | ||
|
||
for (Flag* flag1 : flags_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 18:02:55 CEST 2024, Tóth András (tothxa) wrote:
Why don't you only iterate over warehouses_
?
} | ||
} | ||
|
||
// Now find all existing clusters to merge this one with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 18:21:07 CEST 2024, Tóth András (tothxa) wrote:
Do I get it right that if there's a long chain of warehouses where neighbouring ones are within the threshold, then all of them will get clustered even if the distance between the opposite ends is many times the threshold? Do we really need clusters at all?
} | ||
|
||
upcast(PlayerImmovable, imm, location); | ||
if (imm == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 19:19:35 CEST 2024, Benedikt Straub (Nordfriese) wrote:
Since districts are recalculated lazily, they can be nullptr
for short periods during and after splits and merges and warehouse deletions. In this case the import will simply be caught on the next cycle, after the districts have been updated again.
Cluster current_cluster = {current}; | ||
for (auto& pair : warehouse_distances) { | ||
if (pair.first.first == current) { | ||
current_cluster.push_back(pair.first.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 19:17:28 CEST 2024, Benedikt Straub (Nordfriese) wrote:
The case I was thinking of was the strategy that some players use of building two or three warehouses in close proximity for training centres, one to store food, one to store weapons, etc. If each of these warehouses defined its own district then this would mess up all productionsites around this centre. This new feature should not break existing valid strategies and clustering was the solution I came up with.
A long chain would be clustered too, but IMHO a really long warehouse chain is an indication that the player is doing something highly unusual and unexpected and we can't really guess what he might wish to accomplish with this, except that such an economy seems to be tightly chain-linked and therefore may be meant to be treated as one unit.
Cluster current_cluster = {current}; | ||
for (auto& pair : warehouse_distances) { | ||
if (pair.first.first == current) { | ||
current_cluster.push_back(pair.first.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Tue Jun 11 20:34:08 CEST 2024, Tóth András (tothxa) wrote:
The case I was thinking of was the strategy that some players use of building two or three warehouses in close proximity for training centres, one to store food, one to store weapons, etc. If each of these warehouses defined its own district then this would mess up all productionsites around this centre.
Have you experienced that in testing before adding clusters?
Cluster current_cluster = {current}; | ||
for (auto& pair : warehouse_distances) { | ||
if (pair.first.first == current) { | ||
current_cluster.push_back(pair.first.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Wed Jun 12 01:12:10 CEST 2024, Tóth András (tothxa) wrote:
A long chain would be clustered too, but IMHO a really long warehouse chain is an indication that the player is doing something highly unusual and unexpected and we can't really guess what he might wish to accomplish with this, except that such an economy seems to be tightly chain-linked and therefore may be meant to be treated as one unit.
Or maybe it's just a player who took the advice to build warehouses a bit too overzealously. :) They shouldn't be punished for it. But I really don't know what we could do that's simple enough.
I also have a doubt about how we can decide what's an appropriate cluster threshold.
Cluster current_cluster = {current}; | ||
for (auto& pair : warehouse_distances) { | ||
if (pair.first.first == current) { | ||
current_cluster.push_back(pair.first.second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrored from Codeberg
On Wed Jun 12 11:38:08 CEST 2024, Benedikt Straub (Nordfriese) wrote:
Well, in the worst case aggressive clustering simply means that the entire economy (or a large chunk thereof) becomes one big district, which will simply behave exactly as in current master. So the only downside is that such an economy won't be able to fully utilize the benefits of this addition, but there's no regression or penalty compared to master.
On the other hand, splitting a zone that should logically form a single unit will lead to two or three close-by districts that are constantly importing wares from each other. While not super-harmful, this results in an overhead of production for goods that are needed only in smaller quantity (and thus waste of precious input materials), as well as an overhead for the many short-distance imports. That's why I think it is preferable to err on the side of clustering some more when in doubt.
Mirrored from Codeberg
Created on Tue Feb 06 19:59:44 CET 2024 by Benedikt Straub (Nordfriese)
Type of change
New feature for v1.3
Issue(s) closed
This addresses several points that arise frequently in the context of very large games – namely the problem of wares (and also workers) being transported across long distances instead of preferring local supplies.
How it works
These points all happen in the background. The UI is unchanged.
Each economy automatically divides itself into districts. A district is centered around a warehouse (multiple warehouses in close proximity are clustered for this purpose), and each flag belongs to the closest warehouse. Thus, a district is a circle around a warehouse or cluster of warehouses.
Districts try to be self-sufficient and minimize imports.
Supply/request matching always prefers to use supplies from the same district. Imports are only accepted if no local supply is available.
New: All active long-distance imports are frequently monitored, and if a ware becomes available in the local district, the import is cancelled and replaced with the local ware. This type of supply exchanging was previously always rejected as too CPU-intensive, but this approach is performant and catches all the cases where it matters most.
Additionally, economy targets are distributed across districts. Global targets (global here meaning within one economy) continue to work as before. In addition, if a district is short of a specific ware, then the productionsites within this district keep producing even if the global stock is above target. Each district's local target is simply the global target divided by number of districts, rounding up.
Possible regressions
Economic request/supply matching, economy targets.
Screenshots
Additional context
Nothing here needs to be saveloaded. This is all recalculated lazily every few seconds.
Benchmark for a nearly completely conquered Accurate Europe 1.0 map, Release build:
Flags at the border between two districts tend to jitter between the two possible districts due to pseudo-randomness in the routing algorithm. I would not consider this a problem.
Could do with more testing, both with big economies and with small ones. I'd therefore like to have this early in the release cycle for v1.3.