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

["Mike Kaufman via RT" <rt@xxxxxxxxxxxxxx>]

On Sun, Jan 12, 2003 at 11:26:40AM -0800, Ben Webb via RT wrote:
>
> 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.

this is true. But as this will require reworking a lot of unit code, I
wanted to present this first, but now that I think about it, at least some
of it needs to go in before a commit since we don't want to change network
code twice.

> 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.)

I will investigate the use of effect vectors.

> 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

how much more memory?

> 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 ;)

heh. I'll get you the patch that 'somebody' used to do it if you want it :)
this is an idea. Another idea is to only allocate for those lists which
actually hold effects. The nice thing about your Local list idea is that
the effect struct already keeps track of the improvement id.

> > 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 figured you'd recognize it. I used a lot of impr-gen ideas. I never said
it was _all_ bad :)
> 
> 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.

Well, I don't think that you should have to get down to nuts and bolts
simply to take care of those few cases. Besides which, I wrote the iterator
to be as lean as I could. I didn't research all the possible case where
iterating through the effects is needed, so there are probably only a few
sets of ranges that are truly needed. Do you know what they are? Can they
be entirely indexed with pplayer, pcity, and id?

> 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.

hmm, I don't think so. If you need more than one effect, them you just use
EFT_ALL as type and then do the check as you outline. For lots of cases
though, you're only going to be looking for one type. You're right though,
a macro that looks like:

#define effect_iterate(iter, eff) \
 while ( eff = effect_iterator_get_next_active(&iter) ) {

to hide implementaion would look a lot better.

>
> > @@ -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?

this was mainly to placate Raimar, and his 'new' way of naming.

> > -#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.

hmm, there certainly has to be a capability, mandatory? yes, because we
remove (or at least change) the impr_effect stuff which the client is going
to expect.

> > +#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".

yes you're probably right.

>
> > +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!

yes, but I wonder at what speed cost in the cacheing? Probably little. I'll
look at it.

> > +  /* 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

just right now, I'm not so sure. It seemed to be quite clear when I wrote
the iterator. I'll think about this.

> 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.)

Please give me some examples of these AC effects and I'll see what's the
issue.

> 
> > 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?

maybe. All effects are added when the improvements are added when the
savegame is loaded, so when Apollo is added, for example, the Enable_Space
effect will get added to the game.global_effects list.

-mike