[Freeciv-Dev] Re: (PR#14904) Bugs in AI diplomacy code

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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

Hello,

Thanks for looking at the AI code.

On Mon, 19 Dec 2005, Guillaume Melquiond wrote:
> At ai/advdiplomacy.c:307, there is some obviously non-sensical code:
>     else if (adip->countdown >= 0 && adip->countdown < -1)
> I suppose the intent was to testfor (adip->countdown != -1) instead (as is
> done at line 318), so that the AI player refuses a peace treaty when there
> is a countdown going on.

No, merely adip->countdown != -1 is not correct. The line in 308 which
reads || instead of && is correct (it does not say != -1 though).

> At ai/advdiplomacy.c:1535, there is also wrong code:
>     worth = ai_goldequiv_clause(pplayer, aplayer,
>                                 &clause, ai, FALSE, CLAUSE_CEASEFIRE);
>     clause.type = CLAUSE_CEASEFIRE;
>     if (worth < 0)
> The code checks if a ceasefire is worth anything, but the diplomatic clause
> has not been initialized to ceasefire, so the game only checks garbage (and
> it is unfortunate no assertion failed). The code should follow the same
> coding style as at lines 1497 and 1514: no "worth" variable should be used
> and ai_gold_equiv should directly be in the conditional. This bug is
> responsible for the times the AI directly rejects a ceasefire it has itself
> suggested.

Ah, great catch. Thanks.

Also some calls of ai_goldequiv_clause() were called with CLAUSE instead
of DS enums (I wonder why the compiler didn't catch that?).

Patch attached with some fixes and added comments. Not tested.

  - Per

Index: ai/advdiplomacy.c
===================================================================
--- ai/advdiplomacy.c   (revision 11364)
+++ ai/advdiplomacy.c   (working copy)
@@ -239,7 +239,7 @@
                                struct Clause *pclause,
                                struct ai_data *ai,
                                bool verbose,
-                              enum diplstate_type ds_after)
+                               enum diplstate_type ds_after)
 {
   int worth = 0; /* worth for pplayer of what aplayer gives */
   bool give = (pplayer == pclause->from);
@@ -304,7 +304,7 @@
                "ceasefire for a bit longer first, %s."), pplayer->name, 
                aplayer->name);
         worth = -BIG_NUMBER;
-      } else if (adip->countdown >= 0 && adip->countdown < -1) {
+      } else if (adip->countdown >= 0 || adip->countdown < -1) {
         worth = -BIG_NUMBER; /* but say nothing */
       } else {
         worth = greed(pplayer->ai.love[aplayer->player_no]
@@ -625,9 +625,9 @@
     gift = (gift && (balance >= 0));
     ai_treaty_react(pplayer, aplayer, pclause);
     if (is_pact_clause(pclause->type)
-        && ai->diplomacy.player_intel[aplayer->player_no].countdown >= 0) {
+        && ai->diplomacy.player_intel[aplayer->player_no].countdown != -1) {
       /* Cancel a countdown towards war if we just agreed to peace... */
-      DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "countdown cancelled");
+      DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "countdown nullified");
       ai->diplomacy.player_intel[aplayer->player_no].countdown = -1;
     }
   } clause_list_iterate_end;
@@ -1142,6 +1142,12 @@
   above has been run for _all_ AI players.
 
   Only ever called for AI players and never for barbarians.
+
+  When the adip->coundown variable is set to a positive value, it 
+  counts down to a declaration of war. When it is set to a value 
+  smaller than -1, it is a countup towards a "neutral" stance of -1,
+  in which time the AI will refuse to make treaties. This is to make
+  the AI more stubborn.
 ***********************************************************************/
 void static war_countdown(struct player *pplayer, struct player *target,
                           int countdown, enum war_reason reason)
@@ -1251,7 +1257,7 @@
 
       if (!aplayer->is_alive 
           || aplayer == pplayer
-          || adip->countdown >= 0
+          || adip->countdown >= 0  /* already counting down to war */
           || ship->state == SSHIP_NONE
           || players_on_same_team(pplayer, aplayer)
           || pplayers_at_war(pplayer, aplayer)) {
@@ -1512,7 +1518,7 @@
       }
       clause.type = CLAUSE_PEACE;
       if (ai_goldequiv_clause(pplayer, aplayer, &clause,
-                              ai, FALSE, CLAUSE_PEACE) < 0
+                              ai, FALSE, DS_PEACE) < 0
           || (adip->asked_about_peace > 0 && !aplayer->ai.control)) {
         break; /* never */
       }
@@ -1524,20 +1530,15 @@
 
     case DS_NO_CONTACT: /* but we do have embassy! weird. */
     case DS_WAR:
-    {
-      int worth;
-
       DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "considering ceasefire");
       if (adip->asked_about_ceasefire > 0 && !aplayer->ai.control) {
         DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "... do not ask");
         break;
       }
-      worth = ai_goldequiv_clause(pplayer, aplayer,
-                                  &clause, ai, FALSE, CLAUSE_CEASEFIRE);
       clause.type = CLAUSE_CEASEFIRE;
-      if (worth < 0) {
-        DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "... unacceptable (w=%d)",
-                  worth);
+      if (ai_goldequiv_clause(pplayer, aplayer,
+                              &clause, ai, FALSE, DS_CEASEFIRE)) {
+        DIPLO_LOG(LOG_DIPL, pplayer, aplayer, "... unacceptable");
         break; /* Fight until the end! */
       }
       ai_diplomacy_suggest(pplayer, aplayer, CLAUSE_CEASEFIRE, 0);
@@ -1546,7 +1547,6 @@
              "bloodshed. May we suggest a cessation of hostilities?"), 
              pplayer->name);
       break;
-    }
     case DS_ARMISTICE:
       break;
     default: