[Freeciv-Dev] (PR#14443) city walls not visible (effects problem)

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [mstefek - Sun Oct 30 16:57:42 2005]:
> 
> > [chrisk - Mon Oct 24 20:38:12 2005]:
> > 
> > 
> > SVN HEAD 24 OCT 2005 GTK2
> > 
> > City Walls are not visible in the city icon, for at least trident
non-iso.

> This is because city_got_citywalls() always returns FALSE.
> get_city_bonus(pcity, EFT_LAND_DEFEND) is always 0, because the effect's
> requierements need also unit class.

This is an obnoxious problem.  Basically the issue is that (and this has
always been the case, but it is aggravated by the more generalized
effect types) you cannot query an effect type via get_xxx_bonus()
without using the /canonical/ get_xxx_bonus function.  In this
particular case you cannot use get_city_bonus to query citywalls, you
have to use get_unit_city_bonus, and this is impossible inside
city_got_citywalls because there is no unit.  The problem probably also
affects the AI in various places (probably not limited to the new effect
types).

A big part of the ugliness is it's difficult to tell where the problem
is even going to crop up.  If you do a wrong query it can happen.  But
how are you supposed to know what queries are right?  It's in the
documentation, but there is currently no compile-time or run-time
detection of errors.  Run-time detection would be easy enough (though
tedious) if each effect type told which query function it was to be used
with.  However there may be a more elegant way to merge query
functions...or to add a "needs_unit" flag to an effect type to show it
can only be used with a query function providing a unit.

Now, for the fix...there are numerous solutions and I really don't know
which is best.

1.  Revert the new effect-types patch.  This would solve the immediate
problem.  Of course it doesn't solve the design issue.

2.  Use the attached patch that changes the way unit* req queries work.
 In this patch if no unit is given the req returns TRUE not FALSE.  This
is basically a hack to get around the problem (the problem is such
queries should not be called at all).  However it would solve the
city-walls problem, potentially with unforseen side effects.  One
forseen side effect is that coastal defense and SAM batteries will be
drawn as city walls.

3.  Change city_got_citywalls to check building presence not effect
presence.  This is almost certainly a good idea anyway.  However it
still doesn't solve the underlying problem.

4.  We could add checks inside the reqs (NOT effects) code directly...if
a reqs check is done and the target is not given, we could crash or
print an error.  However this might have unforseen side effects as well
since it's not clear to me that these queries are /always/ invalid.

No matter how you slice it the biggest problem is always going to be the
AI.  Solving individual issues in the core code is pretty simple (fix#3
is probably right in this case) but in the AI code you have to be really
careful about what queries you do.

-jason

Index: common/requirements.c
===================================================================
--- common/requirements.c       (revision 11238)
+++ common/requirements.c       (working copy)
@@ -731,9 +731,12 @@
                                 enum req_range range, bool survives,
                                 struct unit_type *punittype)
 {
+  /* If not target_unittype is given, we allow the req to be met.  This is
+   * to allow querying of certain effect types (like the presence of city
+   * walls) without actually knowing the target unit. */
   return (range == REQ_RANGE_LOCAL
-         && target_unittype
-         && target_unittype == punittype);
+         && (!target_unittype
+             || target_unittype == punittype));
 }
 
 /****************************************************************************
@@ -743,9 +746,12 @@
                                 enum req_range range, bool survives,
                                 enum unit_flag_id unitflag)
 {
+  /* If not target_unittype is given, we allow the req to be met.  This is
+   * to allow querying of certain effect types (like the presence of city
+   * walls) without actually knowing the target unit. */
   return (range == REQ_RANGE_LOCAL
-         && target_unittype
-         && unit_type_flag(target_unittype, unitflag));
+         && (!target_unittype
+             || unit_type_flag(target_unittype, unitflag)));
 }
 
 /****************************************************************************
@@ -755,9 +761,12 @@
                                  enum req_range range, bool survives,
                                  struct unit_class *pclass)
 {
+  /* If not target_unittype is given, we allow the req to be met.  This is
+   * to allow querying of certain effect types (like the presence of city
+   * walls) without actually knowing the target unit. */
   return (range == REQ_RANGE_LOCAL
-         && target_unittype
-         && target_unittype->class == pclass);
+         && (!target_unittype
+             || target_unittype->class == pclass));
 }
 
 /****************************************************************************