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

[Vasco Alexandre Da Silva Costa <vasc@xxxxxxxxxxxxxx>]

On Tue, 15 Jan 2002, Ben Webb wrote:

>       The attached patches should apply to current CVS. They correct a 
> few bugs in the generalised improvements code, while adding sufficient 
> functionality to cover the original effect-implement, effect-iterator, and 
> island-active-fix patches. (effect-init and part of effect-implement are 
> currently in CVS.) This then enables the remaining generalised 
> improvements patches at http://freecivac.sourceforge.net/ to be applied to 
> current CVS, to largely remove hard-coded building effects. The patches 
> make use of derived classes (or at least, the best C approximation 
> thereto) to cut down on code redundancy.
> 
>       Comments on code style and this implementation are welcomed; in 
> particular, if large changes in implementation are likely to be made 
> before CVS inclusion, I'd like to know ASAP, as this would most likely 
> affect current work on FreecivAC.
> 
> neweff-implement.patch:-
>  - Streamlines renumber_island_impr_effect() in client/climisc.c to use 
>    the append_geff function
>  - Fixes specvec_c.h so that it handles derived classes properly 
>    (eff_global in common/improvement.h is derived from eff_city; this 
>    patch then allows a vector of eff_globals - geff_vector - to be safely 
>    cast to a vector of eff_citys - ceff_vector)
>  - Properly implements update_all_effects(), using the new ceff_vector and 
>    geff_vector types
>  - Corrects get_eff_island to take an island number rather than a city 
>    pointer, so that the effects of things not in cities (e.g. units in the 
>    field) can be considered
>  - Adds a get_all_effect_vectors() function that returns a list of all 
>    effects that affect a given "thing", cast to ceff_vectors
>  - Fixes a bug in update_global_effect() so that Island-range effects are 
>    updated properly

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.

See also below.

> neweff-iterator.patch (requires the above patch to first be applied):-
>  - Adds a function eff_iterator_impr_init() to iterate over all effects 
>    that influence a given city improvement, or other city-related entity
>  - Adds a function eff_iterator_unit_init() to iterate over all effects
>    that influence a given unit at a specified map position (N.B. both 
>    these functions take often-redundant data, e.g. both a city and a
>    player pointer, so that unusual cases can be considered - e.g. for
>    prediction by the AI "what happens if I move this unit to these
>    coordinates?" etc.)
>  - Defines Freeciv-consistent iterator macros to iterate over improvement 
>    effects (uses the get_all_effect_vectors() function defined above)
>  - Adds is_city_affected() and is_unit_terrain_affected() to test the 
>    cond_bldg and the various aff_xxx fields of improvement effects
>  - Adds functions to common/unit.c and common/unittype.c to convert unit 
>    types and flags to "unit classes" as used in buildings.ruleset
>  - Clarifies the meanings of cond_bldg and cond_eff in 
>    data/default/buildings.ruleset

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

E.g. for units you usually just need to check for the presence of an effect.
You don't need to iterate over everything. So, say, instead of:

+  impr_effects_iterate(iter, impr, pcity, pplayer) {
+    if (iter.imeff->type == cond_eff) return TRUE;
+  } impr_effects_iterate_end;
+

You just use:

if (xIS_SET(get_impr_effects_bv(impr, pcity, plr), cond_eff))
  return TRUE;

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


= What i *really* dislike is stuff like this:

 void update_all_effects(void)
...
+  do {
+    changed = cond_eff = FALSE;
+    freelog(LOG_DEBUG,"Effect test pass");
+    players_iterate(pplayer) {
+      city_list_iterate(pplayer->cities, pcity) {
+        ceff = get_eff_city(pcity);
+        for (i=0; i<ceff_vector_size(ceff); i++) {
+          effect = ceff_vector_get(ceff,i);
+
+          /* Consider the effects of all active improvements */
+          if (effect->impr!=B_LAST &&
+              pcity->improvements[effect->impr] == I_ACTIVE) {
+            this_changed=FALSE;
+            for (imeff = improvement_types[effect->impr].effect, testbit= 1;
+                 imeff && imeff->type!=EFT_LAST;
+                 imeff++, testbit<<=1) {
+

but this one is much worse...

+static struct impr_effect *eff_iterator_next_internal(struct eff_iter *iter)
...
+ /* Loop over all valid ranges */
+  for (;iter->range<EFR_LAST;iter->range++) {
+    ceff = iter->effs[iter->range];
+    if (ceff) {
+      efflen = ceff_vector_size(ceff);
+      for (;iter->index<efflen;iter->index++) {
+        effect = ceff_vector_get(ceff, iter->index);
+        if (effect->impr != B_LAST) {
+          if (iter->imeff) {
+            /* Make sure we don't return the same effect again */
+            iter->imeff++; iter->bitmask<<=1;
+          } else {
+            iter->imeff=improvement_types[effect->impr].effect;
+            iter->bitmask=1;
+          }
+          for (;iter->imeff && iter->imeff->type!=EFT_LAST;
+               iter->imeff++,iter->bitmask<<=1) {
+            if (effect->active & iter->bitmask &&
+                iter->imeff->range == iter->range &&
+                (iter->imeff->range != EFR_BUILDING ||
+                 effect->impr == iter->impr) &&
+                is_city_affected(iter->imeff, iter->pcity)) {
+              return iter->imeff;
+            }
+          }
+          /* Force reinitialisation of imeff next time round */
+          iter->imeff=NULL;
+        }
+      }
+    }

Please reformat this in some way.

---
Vasco Alexandre da Silva Costa @ Instituto Superior Tecnico, Lisboa