[Freeciv-Dev] (PR#7230) patrolling tririemes may sink

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [Gregory.Berkolaiko@xxxxxxxxxxxxx - Mon Jan 26 20:02:09 2004]:

> 1. I don't see where last_order is actually used in send_path_orders.

My mistake.

> 2. What if the unit finishes the cicle on full MP (say the last segment is
> RR).  It would still be forced to finish turn there wouldn't it?

Yep.

> 3. When you convert path to the packet (from the bits of code I can see), 
> you already have the directions in path->positions[].dir*, no need
> to get_direction_for_step.

Hmm, I guess so.

> 4. A suggestion for 2 above: instead of ORDER_FINISH_TURN introduce 
> ORDER_REFILL_MP.  This will be interpreted at the server at the time of 
> execution.  Them you just put this order in front for dangerous patrols 
> witout much checking.

This adds extra conditional logic to the server.  We already have two
such pieces of logic: cancelling orders for goto and (vigilant) patrol.

> 5. Applies to 4 and possibly to te current code: at the time of creating 
> the path, make sure the PF knows that the unit will have full move points!

I believe it does.  I edit the pf_parameter.

jason

Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.66
diff -u -r1.66 goto.c
--- client/goto.c       2004/01/20 21:52:06     1.66
+++ client/goto.c       2004/01/28 08:02:10
@@ -405,6 +405,11 @@
 
   fill_client_goto_parameter(punit, &goto_map.template);
 
+  if (hover_state == HOVER_PATROL) {
+    /* HACK */
+    goto_map.template.moves_left_initially = unit_type(punit)->move_rate;
+  }
+
   add_part();
   is_active = TRUE;
 }
@@ -483,51 +488,73 @@
 }
 
 /**************************************************************************
+  Append a single order to the unit packet.
+**************************************************************************/
+static void append_order(struct packet_unit_orders *p,
+                        enum unit_orders order, enum direction8 dir)
+{
+  if (p->length >= MAX_LEN_ROUTE) {
+    return;
+  }
+
+  p->orders[p->length] = order;
+  p->dir[p->length] = dir;
+  p->length++;
+}
+
+/**************************************************************************
   Send a path as a goto or patrol route to the server.
 **************************************************************************/
 static void send_path_orders(struct unit *punit, struct pf_path *path,
+                            enum unit_orders first_order,
+                            enum unit_orders last_order,
                             bool repeat, bool vigilant)
 {
   struct packet_unit_orders p;
-  int i, old_x, old_y;
+  int i;
 
   p.unit_id = punit->id;
   p.repeat = repeat;
   p.vigilant = vigilant;
+  p.length = 0;
 
   freelog(PACKET_LOG_LEVEL, "Orders for unit %d:", punit->id);
 
-  /* We skip the start position. */
-  p.length = path->length - 1;
-  assert(p.length < MAX_LEN_ROUTE);
-  old_x = path->positions[0].x;
-  old_y = path->positions[0].y;
-
   freelog(PACKET_LOG_LEVEL, "  Repeat: %d.  Vigilant: %d.  Length: %d",
          p.repeat, p.vigilant, p.length);
 
+  if (first_order != ORDER_LAST) {
+    append_order(&p, first_order, -1);
+    freelog(PACKET_LOG_LEVEL, "  packet[%d] = initial order",
+           p.length - 1);
+  }
+
   /* If the path has n positions it takes n-1 steps. */
   for (i = 0; i < path->length - 1; i++) {
-    int new_x = path->positions[i + 1].x;
-    int new_y = path->positions[i + 1].y;
-
+    int new_x = path->positions[i + 1].x, new_y = path->positions[i + 1].y;
+    int old_x = path->positions[i].x, old_y = path->positions[i].y;
+    
     if (same_pos(new_x, new_y, old_x, old_y)) {
-      p.orders[i] = ORDER_FINISH_TURN;
-      p.dir[i] = -1;
+      append_order(&p, ORDER_FINISH_TURN, -1);
       freelog(PACKET_LOG_LEVEL, "  packet[%d] = wait: %d,%d",
-             i, old_x, old_y);
+             p.length - 1, old_x, old_y);
     } else {
-      p.orders[i] = ORDER_MOVE;
-      p.dir[i] = get_direction_for_step(old_x, old_y, new_x, new_y);
+      append_order(&p, ORDER_MOVE, path->positions[i].dir_to_next_pos);
       freelog(PACKET_LOG_LEVEL, "  packet[%d] = move %s: %d,%d => %d,%d",
-             i, dir_get_name(p.dir[i]), old_x, old_y, new_x, new_y);
+             p.length - 1, dir_get_name(p.dir[p.length - 1]),
+             old_x, old_y, new_x, new_y);
     }
-    old_x = new_x;
-    old_y = new_y;
   }
+
+  if (last_order != ORDER_LAST) {
+    append_order(&p, last_order, -1);
+    freelog(PACKET_LOG_LEVEL, "  packet[%d] = last order",
+           p.length - 1);
+  }
+#undef APPEND_ORDER
 
-  p.dest_x = old_x;
-  p.dest_y = old_y;
+  p.dest_x = path->positions[path->length - 1].x;
+  p.dest_y = path->positions[path->length - 1].y;
 
   send_packet_unit_orders(&aconnection, &p);
 }
@@ -537,7 +564,7 @@
 **************************************************************************/
 void send_goto_path(struct unit *punit, struct pf_path *path)
 {
-  send_path_orders(punit, path, FALSE, FALSE);
+  send_path_orders(punit, path, ORDER_LAST, ORDER_LAST, FALSE, FALSE);
 }
 
 /**************************************************************************
@@ -550,6 +577,7 @@
   struct pf_path *path = NULL, *return_path;
   struct pf_parameter parameter = goto_map.template;
   struct pf_map *map;
+  enum unit_orders first_order = ORDER_LAST, last_order = ORDER_LAST;
 
   assert(is_active);
   assert(punit->id == goto_map.unit_id);
@@ -570,11 +598,34 @@
   }
   path = pft_concat(path, return_path);
 
-  pf_destroy_map(map);
-  pf_destroy_path(return_path);
+  if (parameter.is_pos_dangerous) {
+    /* For units that have dangerous positions it is important that we follow
+     * the path in exactly the same way each circuit.  If we start the first
+     * loop with 2 moves left, but start the second loop with only 1 move
+     * left, then on the second loop we may end up on a danger position.  The
+     * hack to get around this is to add an additional wait if needed, so
+     * that we guarantee each loop is started with full MP.  If we start with
+     * less than full MP, put in a wait at the beginning of each loop.
+     * Otherwise if we end with less than full MP, but in a wait at the end
+     * of each loop.  Otherwise we have full MP at beginning and end and
+     * there's no problem.
+     *
+     * Obviously this algorithm is less than optimal.  An optimal algorithm
+     * would continue making new paths around the circuit until a repeating
+     * condition was found.  The drawback of this is that it takes more
+     * work and sometimes results in longer routes.  See PR#7230. */
+    if (unit_type(punit)->move_rate != punit->moves_left) {
+      first_order = ORDER_FINISH_TURN;
+    } else if (return_path->positions[return_path->length - 1].moves_left
+              != unit_type(punit)->move_rate) {
+      last_order = ORDER_FINISH_TURN;
+    }
+  }
 
-  send_path_orders(punit, path, TRUE, TRUE);
+  send_path_orders(punit, path, first_order, last_order, TRUE, TRUE);
 
+  pf_destroy_map(map);
+  pf_destroy_path(return_path);
   pf_destroy_path(path);
 }