[Freeciv-Dev] (PR#2574) RFC: (PR# 1762) corruption revisited

["Davide Pagnin via RT" <rt@xxxxxxxxxxxxxx>]

This is actual code of city.h city_corruption(), I want to open a
discussion on this whole topic and perhaps reach some agreement on what
need changes.

This is also important because we are discussing the possibility to
implementing waste (corruption concept applied to production), and this
flawed corruption handling function is used to calculating waste, then
replicating the same conceptual errors.

btw, we need a little more flexibility to let waste and corruption
differentiate totally (the max_distance_from_capital variable should be
game.ruleset defined *AND* should be different between waste and
corruption).


> if (pcity->size <= game.notradesize) {
>   trade_penalty = trade;
> } else if (pcity->size >= game.fulltradesize) {
>   trade_penalty = 0;
> } else {
>  trade_penalty = trade * (game.fulltradesize - pcity->size) /
>       (game.fulltradesize - game.notradesize);
> }

This first part handle fulltradesize and notradesize options, they have
been introduced to cut off ICS but are not used in the default ruleset
nor in the vast majority of games.

>
> if (g->corruption_level == 0) {
>   return trade_penalty;
> }

fine with this, but we need to have courthouse behavior not depending on
this parameter

> if (g->fixed_corruption_distance != 0) {
>   dist = g->fixed_corruption_distance;
> } else {
>   capital = find_palace(city_owner(pcity));
>   if (!capital)
>     dist = 36;
>   else {
>     int tmp = map_distance(capital->x, capital->y, pcity->x, pcity->y);
>     dist = MIN(36, tmp);
>   }
> }
> dist =
>     dist * g->corruption_distance_factor + g->extra_corruption_distance;

This whole distance calculation, is badly flawed, IMHO.

1. fixed_corruption_distance, I found counter-intuitive the fact that
distance_factor and extra_distance are applied to this parameter, and at
least this should be documented in the appropriate ruleset

2. We have a magical number (36), that is as always a bad thing, and who
nobody knows where it came from. 
NOTE: I have checked civ2 and civ2 uses 32.
This number is a cap for distance from the capital or in the event that
the capital isn't present.
But again distance_factor and extra_distance are applied to this
parameter, and again a I found this *VERY* counter-intuitive.

3. I've looked at map_distance, to understand which "distance" is used
by the code and found that map_distance = abs(dx) + abs(dy).
An example:
capital in 0,0

city in 10,0
map_distance --> 10

city in 5,5
map_distance --> 10!!!

This is clearly wrong! That city is 5 diagonal square away from your
capital and suffer same corruption of a 10 square away city!
We have 2 choices here, IMHO:

a) dist=sqrt(dx*dx+dy*dy)
b) dist=max(abs(dx),abs(dy))

The first one is used by civ2, but don't take into account that you can
move diagonally with the same cost of moving horizontally or vertically.
Then the second one is more appropriate for the move calculation we
adopted.


> val = trade * dist / g->corruption_modifier;
>
> if (city_got_building(pcity, B_COURTHOUSE) ||
>     city_got_building(pcity, B_PALACE)) val /= 2;

I know that Per isn't convinced of this, but I think that this check
should be moved down, after the CLIP function.
This is civ2 behavior and moreover increases the value of the
courthouse.
When this 2 choice differ??

Then you are very distant from your capital and there is a lot of
corruption!

Example:
despotism, city 32 square distant from the capital,
the city without corruption has 20 trade.

Without courthouse, in default ruleset, you get:
val = 20 * (32+5) / 27 = 27
val = CLIP(0, 20, 27) = 20
you end up with 20/20 trade lost to corruption

with courthouse, actually, you get:
val = 20 * (32+5) / 27 = 27
val = val / 2 = 13
val = CLIP(0, 20, 13) = 13
you end up with 13/20 trade lost to corruption

with courthouse effect applied *AFTER* clipping, you get:
val = 20 * (32+5) / 27 = 27
val = CLIP(0, 20, 27) = 20
val = val / 2 = 10
you end up with 10/20 trade lost to corruption

This change will imply that having a courthouse in a city will save *AT
LEAST* 50% of your total trade, this is IMHO the behavior you expect
from the courthouse.


> val *= g->corruption_level;
> val /= 100;
> val = CLIP(trade_penalty, val, trade);
> return (val);                 /* how did y'all let me forget this one? -- 
> Syela */