[Freeciv-Dev] Corruption Handling - Possible bug?

[Davide Pagnin <nightmare@xxxxxxxxxx>]

        Hi all!

After looking on how corruption handling algorithm works in
common/city.c file, I think to have spotted two potential bugs.

The code reads:
> int city_corruption(struct city *pcity, int trade)
> {
>   struct government *g = get_gov_pcity(pcity);
>   struct city *capital;
>   int dist;
>   int val, trade_penalty;
> 
>   assert(game.notradesize < game.fulltradesize);
>   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);
>   }
> 
>   if (g->corruption_level == 0) {
>     return trade_penalty;
>   }
>   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);

Here a maximum limit of 36 for the distance calculation is enforced.

>     }
>   }
>   dist = 
>     dist * g->corruption_distance_factor +
g->extra_corruption_distance;

Now distance is modified by government parameters

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

As the manual say, Courthouse and Palace reduce to half the corruption
present in the city.

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

What I want to point out is that, corruption handling is rather
counterintuitive, and above all, the government parameters have effects
that are not easy understandable.

Now to the points:

1. How fixed_corruption_distance, corruption_distance_factor and
extra_corruption_factor are supposed to work?

At the moment, effects of corruption_distance_factor and
extra_corruption_factor, are applied after the distance calculation,
thus they are applied even if fixed_corruption_distance is set.
This seems to me, rather counterintuitive.
And more, it is clear, reading the code, that 36 is the maximum fixed
distance, BUT, extra_corruption_factor and corruption_distance_factor,
are applied AFTER distance calculation, thus it they are different from
zero, the real maximum distance may be different from 36.
Obviously, the hard-coded 36 is a bad thing and would be very nice to
have it as a parameter in game.ruleset.

Mine proposal, is to have extra_corruption_factor and
corruption_distance_factor, apply only in the case fixed_distance_factor
is not set, and moreover, to apply them BEFORE the capping of the 36
maximum factor, and if this factor became a game variable, we can call
it maximum_corruption_distance_factor, without confusion.

I understand that this talking is about meaning and style, thus perhaps
this is mine point of view, but yours is very different and you're
content with the actual code and his way of working.
Any comment? 

2. This is another matter, related to corruption, but less disputable
IMHO. Reading manual, Courthouse effect is to half the corruption in a
city. When the corruption is low, the actual code is behaves correctly,
but when corruption is set to a greater degree and the city is distance
from the capital is big, strange things may happen.
Imagine a city with output of 10 arrows, all of them taken from
corruption. I expect that after having built the courthouse, total
corruption is reduced to 5, but this is not the case if val is MORE than
total trade in the city, as can be easily seen reading the code.
Thus, my proposal is to put an if before courthouse effect, for capping
the val value. (if (val>trade) val=trade;)

I'll sent an attachment with both of this changes.

        Ciao, Davide

corruption.hints
Description: Text document