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

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

On Sun, Jan 12, 2003 at 06:08:22PM -0800, Mike Kaufman via RT wrote:
> On Sun, Jan 12, 2003 at 11:26:40AM -0800, Ben Webb via RT wrote:
> > 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

Will it? I take it you've looked at the impr-gen unit effects code; I
don't recall having to muck about with very much unit code.

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

Hard to say in total; depends on the effects defined in the ruleset.
But let's take your City Walls example. This confers Unit_Defend and
Unit_No_Lose_Pop effects at City range, and two Spy_Resistant effects at
Local range. impr-gen will place these effects in an eff_city structure
in the relevant city's effect list. The eff_city structure has two int
elements (impr and active) so uses 8 bytes on a 32-bit machine.
gen-effects will add four effect structures (two to the City-range list,
and two to the City Walls Local-range list). The effect structure
contains 10 ints and 3 pointers - i.e. 52 bytes in total. So this one
building requires 52*4 = 208 bytes of storage - i.e. 26 times as much as
with impr-gen. I think that counts as "significant" ;) This ratio will
of course be reduced a little if the building has global effects, as in
this instance the Island, Player or World range effects require no
additional storage for gen-effects but impr-gen requires an additional
eff_global structure (weighing in at 16 bytes) for each additional
range. Note that I'm simplifying matters here by ignoring the memory
overheads of all those speclists or specvecs you're allocating.

N.B. 8 of the 10 ints in the gen-effects effect structure could be
packed into one by use of a bitfield, as I touch on below, which would
reduce the per-effect overhead significantly.

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

Saves me running "cvs diff" at any rate... ;)

> this is an idea. Another idea is to only allocate for those lists which
> actually hold effects.

Yes, although you still have to allocate the Local-range pointer array in
the first place.

> The nice thing about your Local list idea is that
> the effect struct already keeps track of the improvement id.

Exactly (as does the impr-gen equivalent, of course).

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

I'm not talking about nuts and bolts; I'm talking about your interface
being consistent. What's to stop a user feeding in a NULL city pointer
and then saying he wants City-range effects? You need to add error
checking to the function to enforce this. impr-gen just assumes that if
you put in a NULL city pointer you don't want City-range effects.

How does this iterator function deal with unit effects, by the way?

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

The only examples I can think of are where you want to consider only
Player- and World- range effects - i.e. you exclude Local- and City-
range. (This is done for the Have_Embassies, Reveal_Map and
Reveal_Cities effects, which make no sense at City range anyway.) This
can of course be achieved by passing in NULL for pcity and B_LAST for id.

> > type is also a little odd, IMHO, as it encourages spurious effect
> > iterations.
> 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.

Yes, obviously. But I still disagree with you. ;)

> For lots of cases though, you're only going to be looking for one type.

Less often than you might imagine. Unit_Vet_Combat, Spy_Resistant,
Unit_Veteran, Give_Imm_Adv, No_Anarchy, Adv_Parasite and Any_Government are
the only effects which are currently treated in this way (and out of
those, Any_Government and No_Anarchy should be cached per player anyway,
and will be when I bother to write the code). All others are handled by
loops that consider groups of effects simultaneously.

> > > -#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?
> hmm, there certainly has to be a capability

Oh, of course. No disagreement there.

> mandatory? yes, because we remove (or at least change) the impr_effect
> stuff which the client is going to expect.

Ah, OK, if you're not going to send it suitable old-style info, this
makes sense.

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

Speed cost? I'd imagine it'd be faster if anything. (By bitfield, I mean
that defined in the C language - e.g. int active:1; int pending:1 etc.)

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

See the documentation of the ".outside" member in
data/default/buildings.ruleset in FreecivAC. They're not AC-specific
effects, but more a consequence of the improvement-centric way in which
effects were originally defined. For example, the Unit_Defend effect at
Player range actually only affects units in all of a player's cities.
You could imagine a similar Player-range effect that affects units in
the field as well (e.g. a tech that improves armour). A similar
distinction is made by AC for city Science or Tax bonuses - does a
Player-range Science bonus boost the Science output of each city (like
the Civ SETI Program) or the total Science output of your Civ (as the AC
government science bonuses do) ?

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

No, I mean the effect of a destroyed Apollo. That won't be added in when
you load the savegame, because the improvement itself is never added.
(See FreecivAC for handling of Player- and World- range destroyed
effects.)

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"Never was a cornflake girl / Thought that was a good solution"