[Freeciv-Dev] (PR#11571) What is the target_type for?

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=11571 >

I am confused about the target_type used for effects and reqs.

The idea of a target is simple enough.  is_req_active passes in a 
player, city, tile, building, etc., target.  Some or most of these 
targets may be NULL depending on what you're looking at.  But also the 
target type: PLAYER, CITY, or BUILDING is passed in.  And this I don't 
understand.

First of all, the target should be obvious from what actual target 
values are passed in.  If NULL is passed in for the target_city, then 
obviously there's not a city target.  In fact this is the way the code 
works already!  The only place where the target is actually checked is 
in count_buildings_in_range, where it returns 0 if the target has a 
greater range than the req's range.  But I think this should also be 
taken care of automatically below when we actually do the lookup to see 
how many buildings there are (if we don't have a city we can't do a 
lookup to see if there's a building in the city).

I ran into this problem when I considered what a terrain/special 
requirement means for a city.  The current idea of a terrain requirement 
presumes that the target is a city tile - a city and a tile target must 
both be given, and that tile is checked for the terrain (in 
get_city_tile_bonus()).  I wanted to allow a city-targeted (not 
city-tile-targeted) terrain requirement: the req would be met if the 
terrain was present anywhere within the citymap (similar to terr_gate, 
we could have terrain requirements for units or buildings).  So I 
figured I'd just check the target type: TARGET_TILE versus TARGET_CITY. 
  But of course there is no TARGET_TILE.  Nor should there necessarily 
be one; we could potentially have a tile target without a city target. 
This shows that the range of possible targets isn't linear and doesn't 
fit directly into an enum.  So I started wondering what the target type 
is actually good for, and I haven't been able to come up with any answer.

The attached patch removes the target enum entirely.  There are no ill 
effects that I can see.  But I'm not entirely certain of it.

-jason

Index: ai/aicity.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aicity.c,v
retrieving revision 1.185
diff -u -r1.185 aicity.c
--- ai/aicity.c 17 Dec 2004 08:21:19 -0000      1.185
+++ ai/aicity.c 17 Dec 2004 09:38:12 -0000
@@ -256,7 +256,7 @@
   /* Calculate desire value. */
   effect_type_vector_iterate(get_building_effect_types(id), ptype) {
     effect_list_iterate(*get_building_effects(id, *ptype), peff) {
-      if (is_effect_useful(TARGET_BUILDING, pplayer, pcity, id,
+      if (is_effect_useful(pplayer, pcity, id,
                           NULL, id, peff)) {
        int amount = peff->value, c = cities[peff->range];
         struct city *palace = find_palace(pplayer);
Index: common/effects.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/effects.c,v
retrieving revision 1.19
diff -u -r1.19 effects.c
--- common/effects.c    17 Dec 2004 08:21:20 -0000      1.19
+++ common/effects.c    17 Dec 2004 09:38:12 -0000
@@ -608,8 +608,7 @@
   source is the source type of the effect
   peffect is the exact effect
 **************************************************************************/
-static bool is_effect_redundant(enum target_type target,
-                               const struct player *target_player,
+static bool is_effect_redundant(const struct player *target_player,
                                const struct city *target_city,
                                Impr_Type_id target_building,
                                Impr_Type_id source,
@@ -626,7 +625,7 @@
     if (elt->source_building == source) {
       return FALSE;
     } else {
-      if (count_buildings_in_range(target, target_player, target_city,
+      if (count_buildings_in_range(target_player, target_city,
                                   target_building, elt->range,
                                   elt->survives, elt->source_building) > 0) {
        /* The effect from this source in the group makes peffect
@@ -645,15 +644,14 @@
   given target. (If the requirements are not met the effect should be
   ignored.)
 **************************************************************************/
-static bool are_effect_reqs_active(enum target_type target,
-                                  const struct player *target_player,
+static bool are_effect_reqs_active(const struct player *target_player,
                                   const struct city *target_city,
                                   Impr_Type_id target_building,
                                   const struct tile *target_tile,
                                   Impr_Type_id source,
                                   const struct effect *peffect)
 {
-  return is_req_active(target, target_player, target_city, target_building,
+  return is_req_active(target_player, target_city, target_building,
                       target_tile, &peffect->req);
 }
 
@@ -669,18 +667,17 @@
   source gives the source type of the effect
   peffect gives the exact effect value
 **************************************************************************/
-bool is_effect_useful(enum target_type target,
-                     const struct player *target_player,
+bool is_effect_useful(const struct player *target_player,
                      const struct city *target_city,
                      Impr_Type_id target_building,
                      const struct tile *target_tile,
                      Impr_Type_id source, const struct effect *peffect)
 {
-  if (is_effect_redundant(target, target_player, target_city,
+  if (is_effect_redundant(target_player, target_city,
                          target_building, source, peffect)) {
     return FALSE;
   }
-  return are_effect_reqs_active(target, target_player, target_city,
+  return are_effect_reqs_active(target_player, target_city,
                                target_building, target_tile,
                                source, peffect);
 }
@@ -697,19 +694,18 @@
   source gives the source type of the effect
   peffect gives the exact effect value
 **************************************************************************/
-static bool is_effect_active(enum target_type target,
-                            const struct player *plr,
+static bool is_effect_active(const struct player *plr,
                             const struct city *pcity,
                             Impr_Type_id building,
                             const struct tile *ptile,
                             Impr_Type_id source,
                             const struct effect *peffect)
 {
-  if (count_buildings_in_range(target, plr, pcity, building, peffect->range,
+  if (count_buildings_in_range(plr, pcity, building, peffect->range,
                               peffect->survives, source) == 0) {
     return FALSE;
   }
-  return is_effect_useful(target, plr, pcity, building,
+  return is_effect_useful(plr, pcity, building,
                          ptile, source, peffect);
 }
 
@@ -727,7 +723,7 @@
       /* We use TARGET_BUILDING as the lowest common denominator.  Note that
        * the building is its own target - but whether this is actually
        * checked depends on the range of the effect. */
-      if (!is_effect_redundant(TARGET_BUILDING, city_owner(pcity), pcity,
+      if (!is_effect_redundant(city_owner(pcity), pcity,
                               building, building, peffect)) {
        return FALSE;
       }
@@ -749,8 +745,7 @@
   source gives the source type of the effect
   effect_type gives the effect type to be considered
 **************************************************************************/
-static int get_effect_value(enum target_type target,
-                           const struct player *target_player,
+static int get_effect_value(const struct player *target_player,
                            const struct city *target_city,
                            Impr_Type_id target_building,
                            const struct tile *target_tile,
@@ -762,7 +757,7 @@
   /* Loop over all effects of this type provided by the given source. */
   effect_list_iterate(*get_building_effects(source, effect_type), peffect) {
     /* For each effect, see if it is active. */
-    if (is_effect_active(target, target_player, target_city,
+    if (is_effect_active(target_player, target_city,
                         target_building, target_tile,
                         source, peffect)) {
       /* And if so add on the value. */
@@ -786,7 +781,6 @@
   is done with it.
 **************************************************************************/
 static int get_target_bonus_sources(struct effect_source_vector *sources,
-                                   enum target_type target,
                                    const struct player *target_player,
                                    const struct city *target_city,
                                    Impr_Type_id target_building,
@@ -804,7 +798,7 @@
     int value;
 
     /* And for each source, add on the amount of effect provided by it. */
-    value = get_effect_value(target, target_player, target_city,
+    value = get_effect_value(target_player, target_city,
                             target_building, target_tile,
                             *pbldg, effect_type);
     bonus += value;
@@ -830,7 +824,7 @@
 int get_player_bonus(const struct player *pplayer,
                     enum effect_type effect_type)
 {
-  return get_target_bonus_sources(NULL, TARGET_PLAYER,
+  return get_target_bonus_sources(NULL,
                                  pplayer, NULL, B_LAST, NULL,
                                  effect_type);
 }
@@ -840,7 +834,7 @@
 **************************************************************************/
 int get_city_bonus(const struct city *pcity, enum effect_type effect_type)
 {
-  return get_target_bonus_sources(NULL, TARGET_CITY,
+  return get_target_bonus_sources(NULL,
                                  city_owner(pcity), pcity, B_LAST, NULL,
                                  effect_type);
 }
@@ -851,7 +845,7 @@
 int get_city_tile_bonus(const struct city *pcity, const struct tile *ptile,
                        enum effect_type effect_type)
 {
-  return get_target_bonus_sources(NULL, TARGET_CITY,
+  return get_target_bonus_sources(NULL,
                                  city_owner(pcity), pcity, B_LAST, ptile,
                                  effect_type);
 }
@@ -862,7 +856,7 @@
 int get_building_bonus(const struct city *pcity, Impr_Type_id id,
                       enum effect_type effect_type)
 {
-  return get_target_bonus_sources(NULL, TARGET_BUILDING,
+  return get_target_bonus_sources(NULL,
                                  city_owner(pcity), pcity, id, NULL,
                                  effect_type);
 }
@@ -876,7 +870,7 @@
 int get_player_bonus_sources(struct effect_source_vector *sources,
     const struct player *pplayer, enum effect_type effect_type)
 {
-  return get_target_bonus_sources(sources, TARGET_PLAYER,
+  return get_target_bonus_sources(sources,
                                  pplayer, NULL, B_LAST, NULL,
                                  effect_type);
 }
@@ -890,7 +884,7 @@
 int get_city_bonus_sources(struct effect_source_vector *sources,
     const struct city *pcity, enum effect_type effect_type)
 {
-  return get_target_bonus_sources(sources, TARGET_CITY,
+  return get_target_bonus_sources(sources,
                                  city_owner(pcity), pcity, B_LAST, NULL,
                                  effect_type);
 }
@@ -909,7 +903,7 @@
     int power = 0;
 
     effect_list_iterate(*get_building_effects(bldg, effect_type), peffect) {
-      if (is_effect_useful(TARGET_BUILDING, city_owner(pcity),
+      if (is_effect_useful(city_owner(pcity),
                           pcity, bldg, NULL, bldg, peffect)) {
        power += peffect->value;
       }
Index: common/effects.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/effects.h,v
retrieving revision 1.13
diff -u -r1.13 effects.h
--- common/effects.h    17 Dec 2004 08:21:20 -0000      1.13
+++ common/effects.h    17 Dec 2004 09:38:13 -0000
@@ -195,8 +195,7 @@
                              enum req_range range, bool survives);
 int find_effect_group_id(const char *name);
 
-bool is_effect_useful(enum target_type target,
-                     const struct player *target_player,
+bool is_effect_useful(const struct player *target_player,
                      const struct city *target_pcity,
                      Impr_Type_id target_building,
                      const struct tile *target_tile,
Index: common/requirements.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/requirements.c,v
retrieving revision 1.3
diff -u -r1.3 requirements.c
--- common/requirements.c       17 Dec 2004 08:48:55 -0000      1.3
+++ common/requirements.c       17 Dec 2004 09:38:13 -0000
@@ -237,27 +237,6 @@
 }
 
 /****************************************************************************
-  Is this target possible for the range involved?
-
-  For instance a city-ranged requirement can't have a player as it's target.
-  See the comment at enum target_type.
-****************************************************************************/
-static bool is_target_possible(enum target_type target,
-                              enum req_range range)
-{
-  switch (target) {
-  case TARGET_PLAYER:
-    return (range >= REQ_RANGE_PLAYER);
-  case TARGET_CITY:
-    return (range >= REQ_RANGE_CITY);
-  case TARGET_BUILDING:
-    return (range >= REQ_RANGE_LOCAL);
-  }
-  assert(0);
-  return FALSE;
-}
-
-/****************************************************************************
   Returns the number of total world buildings (this includes buildings
   that have been destroyed).
 ****************************************************************************/
@@ -355,17 +334,12 @@
   the number of available sources.  However not all source caches exist: if
   the cache doesn't exist then we return 0.
 ****************************************************************************/
-int count_buildings_in_range(enum target_type target,
-                            const struct player *target_player,
+int count_buildings_in_range(const struct player *target_player,
                             const struct city *target_city,
                             Impr_Type_id target_building,
                             enum req_range range, bool survives,
                             Impr_Type_id source)
 {
-  if (!is_target_possible(target, range)) {
-    return 0;
-  }
-
   if (improvement_obsolete(target_player, source)) {
     return 0;
   }
@@ -420,8 +394,7 @@
   for instance if you have TARGET_CITY pass the city's owner as the target
   player as well as the city itself as the target city.
 ****************************************************************************/
-bool is_req_active(enum target_type target,
-                  const struct player *target_player,
+bool is_req_active(const struct player *target_player,
                   const struct city *target_city,
                   Impr_Type_id target_building,
                   const struct tile *target_tile,
@@ -445,9 +418,9 @@
     /* The requirement is filled if there's at least one of the building
      * in the city.  (This is a slightly nonstandard use of
      * count_sources_in_range.) */
-    return (count_buildings_in_range(target, target_player, target_city,
-                                    target_building, REQ_RANGE_CITY, FALSE,
-                                    req->value.building) > 0);
+    return (count_buildings_in_range(target_player, target_city,
+                                    target_building, REQ_RANGE_CITY,
+                                    FALSE, req->value.building) > 0);
   case REQ_SPECIAL:
     /* The requirement is filled if the tile has the special. */
     return target_tile && tile_has_special(target_tile, req->value.special);
Index: common/requirements.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/requirements.h,v
retrieving revision 1.2
diff -u -r1.2 requirements.h
--- common/requirements.h       17 Dec 2004 08:21:20 -0000      1.2
+++ common/requirements.h       17 Dec 2004 09:38:13 -0000
@@ -40,15 +40,6 @@
   REQ_RANGE_LAST   /* keep this last */
 };
 
-/* An requirement is targeted at a certain target type.  For building and
- * unit reqs the target will be a city; for tech reqs it will be a player;
- * for effect reqs it may be anything. */
-enum target_type {
-  TARGET_PLAYER,
-  TARGET_CITY,
-  TARGET_BUILDING 
-};
-
 /* A requirement. This requirement is basically a conditional; it may or
  * may not be active on a target.  If it is active then something happens.
  * For instance units and buildings have requirements to be built, techs
@@ -77,15 +68,13 @@
 struct requirement req_from_values(int type, int range,
                                   bool survives, int value);
 
-bool is_req_active(enum target_type target,
-                  const struct player *target_player,
+bool is_req_active(const struct player *target_player,
                   const struct city *target_city,
                   Impr_Type_id target_building,
                   const struct tile *target_tile,
                   const struct requirement *req);
 
-int count_buildings_in_range(enum target_type target,
-                            const struct player *target_player,
+int count_buildings_in_range(const struct player *target_player,
                             const struct city *target_city,
                             Impr_Type_id target_building,
                             enum req_range range, bool survives,