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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=2521 >

Just a few notes.


The AI advisor under the effects patch is really bad.  All of the 
building-choosing code is removed so instead it is just the first 
available building that is chosen.

-----

In city_incite_cost this code is added at the beginning:

+  if (government_has_flag(get_gov_pcity(pcity), G_UNBRIBABLE)) {
+    return INCITE_IMPOSSIBLE_COST;
+  }

the code is obviously correct.  But why isn't it there now?

(Answer: the code never gets that far because the G_UNBRIBABLE check is 
done earlier.  Although it's clearly better with the check in.)

-----

pcity->airlift should eventually become an integer value.  EFT_AIRLIFT 
should specify the number of airlifts allowed.

-----

In ruleset.c there is this code:

-      for (j = 0; b->effect[j].type != EFT_LAST; j++) {
-       if (!tech_exists(b->effect[j].cond_adv)) {
-         freelog(LOG_ERROR,
-                 "improvement \"%s\": effect conditional on"
-                 " removed tech \"%s\" (%s)",
-                 b->name, advances[b->effect[j].cond_adv].name, filename);
-         b->effect[j].cond_adv = A_LAST;
-       }
-      }

This is problematic since this check should still be done since 
tech_exists() still needs to be checked (AFAIK).  However rather than 
keep it we should just remove tech_exists().  Note there could be a 
similar problem with terrain_exists() - there is no such function but it 
_is_ possible to have terrains that aren't used (as we had by default 
for a long time, but don't anymore).

jason