[Freeciv-Dev] Re: [PATCH] Generalised improvements patches for current CVS

[Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, 16 Jan 2002, Vasco Alexandre Da Silva Costa wrote:

> Looks nice. From the quick glance i made of it i have the following
> comments to make (mostly minor stuff) prior to commiting:
> 
> 1) Rename ath_nth() to ath_get().
> 2) Reformat stuff to Freeciv style in a couple of places.
> 3) Rename get_base_ceff_vector() to get_geff_parent() or something like it.

Sounds fine to me.

> About the iterators: i still question myself about some of this stuff.
> I.e. if it can be optimized or not.

        But of course; I agree completely. In an ideal case you'd only do 
a single effect iteration per turn, and then cache all the results. But 
this requires rewrites of many Freeciv functions. For example, 
effect-city-bonuses.patch merges all the city effects (pollution, food, 
production, happiness, etc.) into one function. Previously these effects 
were calculated in several different places, which would require several 
effect iterations. Also, effect-unit-mods.patch caches several unit 
effects to cut down on effect iterations.

> E.g. for units you usually just need to check for the presence of an effect.
> You don't need to iterate over everything.

> This would require some extra caching. I believe that is fairly easy to add.
> But i guess we can leave this for later on.

        I don't think that keeping a cached list of active effects for 
units and cities is particularly useful. After all, that's pretty much 
what the existing effect lists are, except that you lose the ability to 
combine multiple effects with "amount" specifiers (which most effects 
use). Plus, you'd have to update this cache every time a unit moved, or 
whenever the AI wanted to consider a future move, for example, which would 
be rather prohibitive.

> = What i *really* dislike is stuff like this:
> 
>  void update_all_effects(void)

        It's not clear to me what you dislike about this function. The 
coding style? The presence/lack of comments? Long "if" statements? Too 
long a function? The inefficiency of the algorithm? (This last is the only 
one that really concerns me, and there are various ways in which the 
number of effects updates can be reduced, and the updates themselves can 
be made partial - e.g. you rarely need a full effect update when 
discovering a new tech or changing government - but nobody has shown me 
any game profiles to demonstrate that this really warrants the extra code 
complexity.)

If you tell me what it is _that_ you dislike, I can address it.

> but this one is much worse...
> 
> +static struct impr_effect *eff_iterator_next_internal(struct eff_iter *iter)

        Again, you've lost me here. It's not exactly complicated - it's 
just three nested loops (over ranges, improvements, and effects) plus an 
activity test. Granted, that conditional is rather long. What don't you 
like about it?

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