[Freeciv-Dev] Re: (PR#2521) general effects framework

["Ben Webb via RT" <rt@xxxxxxxxxxxxxx>]

On Sun, Dec 08, 2002 at 04:44:48PM -0800, Mike Kaufman via RT wrote:
> Here is the first draft of the first step toward general-improvements.
> This is the initial framework.

I should perhaps clarify the situation with regards impr-gen here, for
the interested...

> improvements have certain "effects" defined in data/default/buildings.ruleset 
> that define what they can do when built. Read the ruleset for for specific 
> info. Except for the spaceship parts effect, these "effects" currently do 
> nothing. Everything is hardcoded by building all throughout the codebase.

This is true. The impr-gen patches (part of FreecivAC, at
http://freecivac.sourceforge.net/) implement these effects. (I should
also point out that the effects defined in current CVS do not combine
very well, and are inconsistent; impr-gen rearranges these a little so
they make more sense, and adds some new ones.)

> There is an access method to theses effects currently, but it is not
> complete, and after examining it, I have found it lacking and fairly
> unintelligable.

I don't know if you're talking about impr-gen here, but if you are I'd
be interested to know which aspects are lacking (other than the
intelligibility of the code, of course - that could do with some
commenting).

> This patch gives (what I think) is an improved and complete access method to 
> these effects.

Well, the structures seem easier to use. But the reason the equivalent
structures in impr-gen are harder is that they're significantly more
compact. More on that later. I'm not sure about "complete"; there is no
support for unit effects here, which impr-gen has supported for some
time.

> In a nutshell, each improvement has an "effect definition" that 
> it gets from the ruleset. on startup, these can be found in:
> 
> struct impr_type {
>  ...
>  struct effect_defn_list effect_defs;
>  ...
> }

Hasn't this been the case in CVS for years now? This just looks like a
rename to me. (I think it makes sense though - the word "effect" is
rather overloaded in current CVS, and particularly in the impr-gen
patches. effect_defs is clearer.)

> When an improvement is built, an effect [struct effect] is created (which 
> has a pointer to the definition) for each effect definition in the list.
> Each effect is then added to an effect list based on the range of its
> definition. 

This is the same as the current impr-gen behaviour, except that the
storage of the effects in the lists is different. impr-gen bundles all
of the effects of one improvement (or nation, unit, or whatever) into a
single eff_city structure, and marks the activity of each effect of that
improvement by means of an "active" bitfield in the structure. Effects
at other than Local or City range are put into a similar eff_global
structure. This uses rather less memory than the proposed effects
framework. The effect lists themselves are vectors (specvecs in Freeciv
parlance) rather than linked lists (genlists) to further reduce memory
utilisation. (Effects are added and removed sufficiently infrequently
for the increased CPU hit of adding to or reducing a vector to be
insignificant, IMHO.)

Code "intelligibility" arguments aside, the main difference between
impr-gen and general effects is the use of memory. Gen-effects looks to
be like it'll need significantly more than impr-gen; this seems to me to
be a high price to pay for slightly prettier code. (I see no reason why
either approach should be faster than the other, and all of this
low-level gubbins is hidden from 99% of Freeciv code at any rate - the
impr-gen API is just as simple as the gen-effects one.)

> Now to check to see if the Spy fails to sabotage the walls... For
> motivation, let's say she begins with a 25% chance of failure. To apply
> effects, we need to iterate through all the applicable effects, now we need 
> to check for the Spy_Resistant effect in ALL the lists that affect the
> walls. In this case, we need to look at 2. the Local list (that the
> improvement has (actually stored as pcity->impr_effects[B_CITY])

Ouch. You seem to have game.num_impr_types effect lists for every
city... this seems like a lot of memory to use when you consider that
City Walls is in fact the _only_ building in the default ruleset that
confers Local effects. (impr-gen puts such effects in the city's
City-range effect list, but flags them as Local effects. Thus the
City-range list requires two iterations to pick out both Local and City
range effects; this is also not ideal, but perhaps a solution better
than both current approaches would be to have a City-range effect list
and a _single_ Local-range effect list, into which all building Local
effects get lumped, per city. I would try this with impr-gen, except
that somebody seems to have deleted big chunks of the underlying code in
CVS, so that my patches no longer apply ;)

> void effect_iterator_init(struct effect_iter *iter, unsigned int ranges,
>                           struct player *pplayer, struct city *pcity,
>                           int id, enum effect_type type);

This is very similar to the equivalent impr-gen function. (The
differences are fairly minor when you consider that almost all usage of
effect iterators in FreecivAC is via standard Freeciv iterator macros
anyway.)

I don't like the ranges argument at all. There are very few occasions
(two or three, from memory, in FreecivAC) when you wish to exclude
ranges, and the choice of ranges is largely determined by the pplayer,
pcity and id arguments anyway.

type is also a little odd, IMHO, as it encourages spurious effect
iterations. The Freeciv code should instead be refactored to try and
evaluate as many effects as possible from each iteration. For example,
in FreecivAC all of the city bonuses, defence modifiers, etc. etc.
active in a city centre tile (as well as those that affect worked tiles)
are evaluated in a single effect iteration. It is of course trivial to
filter returned effects for a specified desired type (or collection of
types) if desired. For example, in the combat functions, this API
encourages coders to do multiple iterations, for each effect (e.g.
attack bonus, firepower bonus, defence bonus, etc.) when one would be
sufficient.

>   effect_iterator_init(&iter, EFR_LAST, pplayer, pcity, 
>                        walls_impr_id, EFT_SPY_RESISTANT);
>   while( (eff = effect_iterator_get_next_active(&iter)) ) {
>     failure_chance += eff->defn->amount;
>   }
>  
> my, wasn't that nice?

Well, the equivalent code in FreecivAC (server/diplomats.c, line 1010)
reads:-

      impr_effects_iterate(iter, improvement, pcity, cplayer) {
        if (iter.imeff->type == EFT_SPY_RESISTANT) {
          failchance += iter.imeff->amount;
        }
      } impr_effects_iterate_end;

I wouldn't call that enormously ugly either. ;)

> Improvements aren't the only things that can have effects. Nations,
> governments and techs can also have effects (any effects from these must
> be World or Player ranged). Note: non-improvements effects are known to be
> buggy in this version. Beware. Also easily extensible from this framework
> are unit effects (which will be restricted to Local ranged effects).

impr-gen does unit effects already (but I never bothered to write the
code for tech effects - it's easy though). Oh, and non-improvements
effects are _not_ known to be buggy in impr-gen. ;)

And now, the patch...

> @@ -281,7 +289,7 @@
>   } built_impr_iterate_end;
> 
>   if (effect_update)
> -    update_all_effects();
> +    effects_update_all();
> 
>    popdown_city_dialog(pcity);
>    game_remove_city(pcity);

There seem to be a lot of these. Why the name change?

> -#define CAPABILITY "+1.14.0 conn_info +occupied team"
> +#define CAPABILITY "+1.14.0 conn_info +occupied team +effects"

Why is this a mandatory capability? If it isn't shared, everything
should still work (since effects are calculated serverside anyway).
There may be some confusion between observed and reported behaviour
with modified rulesets, however.
   
> +#define MAX_EFFECTS 16          /* fairly arbitrary */

I see no reason for there to be a limit at all with gen-effects.
impr-gen imposes this limit because (as I mentioned earlier) the
activity of all effects is packed into a bitfield, and I'm sufficiently
cautious to give 16 as the minimum number of bits in an "unsigned int".

> +struct effect {
> +  bool active;            /* is effect active? */
> +  bool pending;           /* is effect active except for cond_bldg? */
> +
> +  bool has_nat;           /* has req. nation (or doesn't need one) */
> +  bool has_adv;           /* has req. advance (or doesn't need one) */
> +  bool has_gov;           /* has req. gov (or doesn't need one) */
> +  bool has_bldg;          /* has req. bldg (only partially cached) */
> +  bool has_eff;           /* has cond_eff (only partially cached) */
> +  bool has_no_equiv;      /* there is no equivalent bldg that suppresses */

Yikes! If you really must do all this for every effect, at least use a
bitfield. This lot could be packed into one byte!

> +  /* these are internal. They track the current position of the iterator */
> +  enum effect_range at_range;
> +  int island_pos;
> +  int city_pos;
> +  int impr_pos;
> +  int list_pos;

I'm not sure why you need a separate _pos for each range. This would
seem to make it tricky to add extra ranges in the future. (e.g.
additional ranges for units owned by, or within the radius of, a given
city, effects that affect a given player and any allies, etc. etc.) (Of
course, I wouldn't make this point if it were not for the fact that
a) FreecivAC makes such a distinction between Player-range effects that
affect units in cities, and similar effects that also affect units in
the field, and b) the impr-gen iterator code is generalised in just this
way.)

> diff -Nur -Xsnap/diff_ignore snap/server/savegame.c snap-i/server/savegame.c
> --- snap/server/savegame.c    Sun Dec  8 13:46:44 2002
> +++ snap-i/server/savegame.c  Sun Dec  8 13:49:58 2002

I don't see any code in here to deal with persistent effects (e.g. that
of Apollo or Manhattan). Am I missing something?

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"You know my methods, Watson."
        - 'The Crooked Man', Sir Arthur Conan Doyle