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

["Raimar Falke via RT" <rt@xxxxxxxxxxxxxx>]

On Mon, Dec 09, 2002 at 08:24:30AM -0800, Mike Kaufman via RT wrote:
> On Mon, Dec 09, 2002 at 01:00:18AM -0800, Raimar Falke via RT wrote:
> > 
> > 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...).

So there are non-immediate effects (persistent) which stay and effect
other objects. And there are immediate effects which are one-shot and
get removed immediately.

> 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 I follow this logic wouldn't it also be nice to have a pending2
condition which is true if the effect has Player or World range and is
"almost finished" but only the island is the wrong one/undefined.

Basically I don't like the special case.

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

Can you explain it more?

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

I expected this answer ;) This is another possibility to point you at
PR#2350. The same is the solution here.

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

Isn't it possible to design something like this:
 effect A depends that no effect B is active
 effect B depends on A


A is active
B is active

A will get deactived
B will also get deactived
A will get actived

Is this possible?

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

I agree with you that it is a complex problem. However the worst case
I can think of goes as followes: we add this patch and also all the
others and than see that even with caching and some performance
kludges we end up performing 5 times slower than now. The AI doesn't
cope with it and to tech it would make it slower by factor of 10. At
this point we can't back out. I want to prevent this by having some
numbers now.

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

It is possible that Ben's implementation was faster here because it
cached the per city bonuses? I can remember Ben wrote this.

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

Intelligent caching since I agree that effects shouldn't change often.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Your mail could not be delivered to the following Address:
  VTCMC.VTLPR@xxxxxxxxxxxxx        ** Unassigned error message **"