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

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

On Mon, Dec 09, 2002 at 01:00:18AM -0800, Raimar Falke via RT wrote:
> 
> > walls. In this case, we need to look at 2. the Local list (that the
> 
> I counted 5.

so it is. 

> > struct effect *effect_iterator_get_next_active(struct effect_iter *iter);
> 
> Can you also provide a "normal" iterate macro?

what does this mean? I think what you mean is see my response to vasco or
the end of effects.h. if it's not what you mean, explain.

> > effects, use EFT_LAST instead of EFT_SPY_RESISTANT.
> 
> There should a EFR_ALL. Same for EFT_ALL.

agreed. see my response to vasco.

> While the user documentation is good I miss some developer
> documentation. Example: what is an immediate effect and why it is so
> special? What is so special about "is effect active except for
> cond_bldg" that you create a special condition "pending" for
> this. What is EFR_NONE used for?

I didn't want to write a tome. the best thing is to simply read the patch.
of course, that can be frustrating without a guide, so here's one:

An immediate effect is one that needs to be immediately resolved when the
effect is added. An example of this is the "Giv_Imm_Adv". In this case you
need to get the advances immediately and then delete the effect from the
list. We can't go around giving advances every turn. This, or course, gives
rise to all sorts of problems. For example, the ai when doing it's
calculations, shouldn't apply the immediate effects, the client when it
gets enemy effect information shouldn't apply immediate effects, loading a
savegame shouldn't apply immediate effects (because the result is accounted
for elsewhere, or at least should be...).

cond_bldg is special because given an effect in an effect list (Island,
Player, and World) you don't know right away if the effect will be active.
cond_bldg means: this effect is _only_ active in a city that has this
building. So for an effect in the Player range, it could be active or not
depending which city we're looking at (if everything else is sufficient for
it to emit, then it is "pending") (thanks for asking, it's made me realize
there's a bug in the current code.)

If EFR_NONE is used, then the effect is not added to any lists. This is
needed (I think) for equiv_range not for effects.

> Why are there no real types in struct effect_defn cond_*.

that's right. That's because we can't include improvement.h, tech.h at the
top of effects.h because they must include effects.h at the top of them :(

> 
> effects_update_all has an almost unbound while loop. Looks like it
> won't return if the giver gives the right input?!

No it should always return eventually. If it doesn't then it's obviously a
bug and needs to be fixed. Note: that in the default ruleset, this loop is
never entered (which is very nice) as the default ruleset doesn't have any
conditional effects.

> 
> And last but not least: you add 1k lines of code. And you need another
> 1-5k to make use of all this. How good does the result perform? How
> long will effects_update_all take. How much time will an city_update
> need?

this is impossible to say until _all_ hardcoded effects are gone in the
code. This will _not_ be a hindrance to inclusion in the codebase. Please
look at the patch. A lot of code gets deleted and moved here as well.
Remember also that gen-impr via Ben's AC patches (just to get the effects
working) would require just as much or more code. that's just the way it
is. This is a complex problem.

To put this in perspective, city_updates _will_ take longer since we have
to iterate through an undetermined number of effects to determine
trade/science/happiness/etc bonuses. The looping will have some overhead,
but for the default ruleset, we'll be adding just the same number of
bonuses (and the nice part is that all the bonuses (?) will be in the same
place ---  the effects lists).

I was extremely worried about this performance of effects_update_all() and
I'm still a bit anxious, but for a ruleset without conditional effects, an
update will only need to iterate over all the effects (in all the lists)
once. This is not too bad. In the tests I've run, a typical player might
have 50 or so effects going in the late game.

The cacheing could also be made better since several of the cond_* don't
change often (like cond_nat) we could add some parameters to
effects_update_all() that takes this into account. The part that makes me
anxious is the AI. Normally, one wouldn't have to update very often (when
an improvement is created/destroyed, a tech is discovered, a government is
changed, a new island is discovered. There may be a couple others. The ai
is going to have to call it quite often when figuring out what to build,
research. This is going to have to require some thought, but right now, I
see no way around it. you?

-mike