Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #892050

open

gold_upkeep_style Mixed doesn't seem to work as advertised

Added by Anonymous about 4 years ago. Updated 8 months ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
Server
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

The description for gold_upkeep_style Mixed in the comments of game.ruleset [1] goes:

; "Mixed" - In the first step, the player`s total gold must be non-negative
; after paying upkeep for all buildings within a city. If for any
; city the player`s gold is negative, random buildings in the city
; are sold off.
; In the second step, gold upkeep for all units is paid in a lump
; sum. If the player does not have enough gold, random units with
; gold upkeep are disbanded.

Also, with that setting, city gold surpluses are shown without unit upkeep costs deducted, and
those upkeeps are only included in the nation-wide income (shown in the sidebar on the client).
This seems to indicate that a city that can pay for its buildings, and hence shows a nonnegative
balance, should be free of trouble, regardless of how little money the nation has.

However, this is not the case. update_city_activity() [2] ignores the calculated surplus value, and instead
deducts both unit and improvement upkeep city during city processing whatever the gold_upkeep_style
is. It then goes on to check if the nation's treasure is negative, and fixes that by selling buildings.

Hence, depending on the order the cities are processed, a city that can pay for its buildings
(showing a nonnegative balance), in a nation that can pay for all its buildings and units (showing
a nonnegative balance) can still be forced to sell a building.

For example, with the nation as in the attached screenshot and savegame, if Castellum happens
to get processed first, you get "Can't afford to maintain Barracks in Castellum, building sold!" (If
Aquincum gets processed first, the income from it prevents this.)

Tested on 2.6, master looks similar.

[1] https://github.com/freeciv/freeciv/blob/S2_6/data/civ2civ3/game.ruleset#L150
[2] https://github.com/freeciv/freeciv/blob/S2_6/server/cityturn.c#L3049


Files

Actions #1

Updated by Anonymous about 4 years ago

Here's a patch to make the behaviour more like what the ruleset comments seem to describe.
(The other one has some extra log_*() calls for testing.)

Actions #2

Updated by Anonymous about 4 years ago

That said, I feel all the possible gold_upkeep_styles are somewhat wanting. "City" and "Mixed" rely on city processing order, in that
buildings/units only get sold if a city running a deficit gets processed before enough profitable cities have been. This may make a player
rely on the total balance over all cities being the number that matters, likely causing surprise and confusion when the city processing
order is not in their favor. On the other hand, with upkeep style "Nation", the final balance is what matters, but there if the nation runs
a deficit, the buildings/units sold may come from any cities, even the most profitable ones.

I thought about an upkeep style that would combine "City" and "Nation" so that a) upkeep costs would be included in the calculated
surplus for the cities, meaning that cities running a deficit would be visible in the Cities list, and that the total balance of the nation would
match the sum of the city balances (building Coinage being still an exception), BUT b) negative nation treasury would only be checked
after processing all cities, and not with individual cities. Then, c) if the nation was in debt after all cities, buildings would be sold from
the cities running the worst deficit.

In my mind, this would combine the best parts of all options. Only the final total would matter, which makes sense since the nation
only has one treasury, not individual ones for each city; but the player could still predict which city would suffer the consequences of the
deficit, instead of having it happen randomly. (And it would prevent possibly selling city walls from the one city under attack, even after
carefully balancing the budget there to avoid it...)

If there's no strong opposition to this, I'll take a look at implementing it.

Actions #3

Updated by Marko Lindqvist about 4 years ago

Ilkka Virta wrote:

If there's no strong opposition to this, I'll take a look at implementing it.

-> Open a new ticket. Target to master only.

About the patch here:
- Why do you turn switch - case constructs to if ... else if constructs? This loses compile time checking that all the enum values are handled.
- When you have a comment "This should not be possible" you probably should be adding fc_assert()
- Coding Style nit: In the very last 'if' there should be no space after '!'

Actions #4

Updated by Marko Lindqvist about 4 years ago

  • Category set to Server
  • Status changed from New to In Progress
  • Sprint/Milestone set to 2.6.3
Actions #5

Updated by Marko Lindqvist about 4 years ago

  • Assignee set to Anonymous
Actions #6

Updated by Marko Lindqvist about 4 years ago

Will you make an updated revision of the patch?

Actions #7

Updated by Anonymous about 4 years ago

Yes, thanks for the reminder. Good point about -Wswitch, might want to consider adding that to CodingStyle, too. I moved setting the nation_*_upkeep vars after city processing, to keep the current behavior where new units and buildings pay upkeep immediately; the first patch changed that unintentionally.

~

I looked at applying the patch against master, but I'm not sure what semantics punit->server.upkeep_payed (sic.) should have -- It looked it was meant to track the upkeeps that had already been paid this far on the current turn, but I can't see it being reset anywhere so it appears to just shadow punit->upkeep. What the difference is, seems to escape me. It's added here: https://github.com/freeciv/freeciv/commit/92f41c487bae5ed28798bc109c9ec4f7f54c801b
If the original bug report is still online, I can't find it.

Actions #8

Updated by Marko Lindqvist about 4 years ago

Ilkka Virta wrote:

It's added here: https://github.com/freeciv/freeciv/commit/92f41c487bae5ed28798bc109c9ec4f7f54c801b
If the original bug report is still online, I can't find it.

I think it's this archive page: https://web.archive.org/web/20170307091019/http://gna.org/bugs/?24132

Actions #9

Updated by Marko Lindqvist about 4 years ago

Ilkka Virta wrote:

punit->server.upkeep_payed (sic.) ... I can't see it being reset anywhere so it appears to just shadow punit->upkeep.

Maybe it's punit->upkeep that gets recalculated when server.upkeep_payed is not, after the upkeep has already been paid. I remember there being a related issue in stable branches (unit getting "free upkeep" when it actually has already paid for upkeep). I'll try to find the ticket later...

Actions #10

Updated by Anonymous about 4 years ago

Yeah, it makes sense if support gets recalculated and the unit getting a free pass changes, so it could then "forget" about an upkeep already paid. I thought initially about undoing a payment that hadn't been done, but I don't think that can happen, upkeep_payed is only looked at for the units that just got paid. (It does come to mind that it might be simpler if the free upkeep wasn't marked on individual units at all but just reduced from the city's upkeep total, but that's hindsight..)

Could probably change to update upkeep_payed only from inside (player|city)_balance_treasury_units*(), since the unitlist set up there is the only place that uses it.

Actions #11

Updated by Marko Lindqvist almost 4 years ago

  • Sprint/Milestone changed from 2.6.3 to 2.6.4
Actions #12

Updated by Marko Lindqvist almost 4 years ago

Ilkka Virta wrote:

if the free upkeep wasn't marked on individual units at all but just reduced from the city's upkeep total

That's ancient code, but my guess would be that it was needed that way for the clients to display unit upkeep icons at least somewhat sensibly. Of course the icons are not accurate in that even if the unit has no upkeep icon shown when you disband it, total upkeep will reduce (and some other unit will lose upkeep icon)

Otherwise; any updates about this patch?

Actions #13

Updated by Anonymous almost 4 years ago

Marko Lindqvist wrote:

Ilkka Virta wrote:

if the free upkeep wasn't marked on individual units at all but just reduced from the city's upkeep total

That's ancient code, but my guess would be that it was needed that way for the clients to display unit upkeep icons at least somewhat sensibly. Of course the icons are not accurate in that even if the unit has no upkeep icon shown when you disband it, total upkeep will reduce (and some other unit will lose upkeep icon)

Exactly.

The way it's shown makes it slightly hard to see on a glance if some unit doesn't cost upkeep because its type doesn't have upkeep to begin with, or if it doesn't cost upkeep because it happened to be one that got a free pass.

Otherwise; any updates about this patch?

Forgotten under all other things. I'll see if I can dig it up on the weekend.

Actions #14

Updated by Marko Lindqvist over 3 years ago

  • Sprint/Milestone changed from 2.6.4 to 2.6.5
Actions #15

Updated by Marko Lindqvist over 3 years ago

  • Sprint/Milestone changed from 2.6.5 to 2.6.6
Actions #16

Updated by Marko Lindqvist almost 3 years ago

  • Sprint/Milestone changed from 2.6.6 to 3.0.1
Actions #17

Updated by Marko Lindqvist over 2 years ago

  • Status changed from In Progress to New
  • Sprint/Milestone changed from 3.0.1 to 3.0.2
Actions #18

Updated by Marko Lindqvist over 2 years ago

  • Sprint/Milestone changed from 3.0.2 to 3.0.3
Actions #19

Updated by Marko Lindqvist over 2 years ago

  • Sprint/Milestone changed from 3.0.3 to 3.0.4
Actions #20

Updated by Marko Lindqvist about 2 years ago

  • Sprint/Milestone changed from 3.0.4 to 3.0.5
Actions #21

Updated by Marko Lindqvist almost 2 years ago

  • Sprint/Milestone changed from 3.0.5 to 3.0.6

Still keeping 3.0 target, though my best guess is that this can be addressed in 3.2 (as I'm working my way through some related server code changes - thankfully also meaning that it's not generic "future" but more likely 3.2 than 3.3)

Actions #22

Updated by Marko Lindqvist almost 2 years ago

  • Sprint/Milestone changed from 3.0.6 to 3.0.7
Actions #23

Updated by Marko Lindqvist over 1 year ago

  • Sprint/Milestone changed from 3.0.7 to 3.0.8
Actions #24

Updated by Marko Lindqvist over 1 year ago

Anonymous wrote:

update_city_activity() [2] ignores the calculated surplus value, and instead
deducts both unit and improvement upkeep city during city processing whatever the gold_upkeep_style
is.

This part causes also trouble other than gold_upkeep_style breakage, so is the most critical part to fix.

Actions #25

Updated by Marko Lindqvist 8 months ago

  • Sprint/Milestone changed from 3.0.8 to 3.1.1

Also available in: Atom PDF