Allow for city level stockpiles to actually function #12710
Conversation
|
Oh wow! |
| fun gainStockpiledResource(resourceName: String, amount: Int) { | ||
| resourceStockpiles.add(resourceName, amount) | ||
| } | ||
|
|
||
| fun gainStockpiledResource(resource: TileResource, amount: Int) { | ||
| if (resource.isCityWide) resourceStockpiles.add(resource.name, amount) | ||
| else civ.gainStockpiledResource(resource.name, amount) | ||
| } |
There was a problem hiding this comment.
Very weird that in the first function you don't check if it's citywide or not, you get 2 different behaviours for civwide resources in the 2 identically named functions
| } else 0 | ||
| if (stat is TileResource) { | ||
| if (!stat.isStockpiled) return 0 | ||
| if (stat.isCityWide) return resourceStockpiles[stat.name] |
There was a problem hiding this comment.
What do we do when it's civ-wide? Fall down to "civ.getReserve(stat)" below?
I'd rather this was explicit in the "stat is TileResource", that it become a "return when()" so we see that we treat all TileResources cases exhaustively
There was a problem hiding this comment.
I had it like that originally, but wasn't sure if it should be explicit
| val resource = city.civ.gameInfo.ruleset.tileResources[resourceName] ?: continue | ||
| city.gainStockpiledResource(resource, -amount) |
There was a problem hiding this comment.
This, to me, indicates that the gainStockpiledResource(String) should do the resource lookup, and then call the gainStockpiledResource(Resource) function
There was a problem hiding this comment.
That's not entirely ideal unless we decide to add additional logic here, since the counter itself only cares about the resource name (for the sake of serialization)
There was a problem hiding this comment.
So, if this is what's wanted, the actual thing is to delete the function for strings (I added it for clarity, but the counter is still public rn, actually). Currently, the city version for strings can already be safely deleted
|
Interesting question: What happens when a city is conquered / bought / etc? |
Yeah, I would think the default behavior should be to retain the stockpiles |
Currently stockpiles for cities don't actually exist. Previously, I believe (but never checked) that they would function as if they were civwide regardless, which doesn't match up with the clear and obvious intent. During the last few PRs, I've since changed it so that under some circumstances, gaining a citywide stockpile from a city context would do nothing. This PR aims to do 2 things
As of writing this PR description, I'm not entirely sure the best way into the resource logic to add citywide stockpile resources, so I haven't immediately focused on adding it