[Freeciv-Dev] Re: (PR#4770) client-side patrol routes aren't safe

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Jason Short wrote:
> The client just reverses the path to find the way from the end of the 
> path back to the beginning.  This isn't safe; e.g., for tririemes which 
> may have pauses in their paths.  Instead a new path should be generated 
> for the return.
> 
> Question: should all of the waypoints be revisted in turn?  I don't 
> think so - all we should do is complete the cycle.  This allows more 
> flexible patrol routes, like patrolling in circles around an island.

Here's a patch that IMO makes it better (though not perfect; see the FIXME).

This overlaps with PR#4758.

jason

? rc
Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.54
diff -u -r1.54 goto.c
--- client/goto.c       2003/07/21 19:01:01     1.54
+++ client/goto.c       2003/08/06 15:42:39
@@ -439,48 +439,59 @@
 }
 
 /********************************************************************** 
+  Send the given path for the unit to the server.
+***********************************************************************/
+static void send_goto_path(struct unit *punit, struct pf_path *path,
+                          enum goto_route_type route_type)
+{
+  struct packet_goto_route p;
+  int i;
+
+  p.unit_id = punit->id;
+
+  /* we skip the start position */
+  /* FIXME: but for unknown reason the server discards the last position */
+  p.length = path->length - 1 + 1;
+  p.first_index = 0;
+  p.last_index = p.length - 1;
+  p.pos = fc_malloc(p.length * sizeof(*p.pos));
+  for (i = 0; i < path->length - 1; i++) {
+    p.pos[i].x = path->positions[i + 1].x;
+    p.pos[i].y = path->positions[i + 1].y;
+    freelog(PACKET_LOG_LEVEL, "  packet[%d] = (%d,%d)",
+           i, p.pos[i].x, p.pos[i].y);
+  }
+
+  send_packet_goto_route(&aconnection, &p, route_type);
+
+  free(p.pos);
+}
+
+/********************************************************************** 
   FIXME: the packet interface need to be changed to support danger
   paths.
 ***********************************************************************/
 void send_patrol_route(struct unit *punit)
 {
-  struct packet_goto_route p;
-  int i, j = 0;
+  int i;
   struct pf_path *path = NULL;
 
   assert(is_active);
   assert(punit->id == goto_map.unit_id);
 
+  /* Complete the patrol cycle. */
+  add_part();
+  draw_line(punit->x, punit->y);
+  /* FIXME: after one circuit is completed, the next circuit may be started
+   * with a different number of moves_left.  This could be a problem if
+   * dangerous positions are taken into account. */
+
+  /* Build a single path. */
   for (i = 0; i < goto_map.num_parts; i++) {
     path = pft_concat(path, goto_map.parts[i].path);
   }
 
-  p.unit_id = punit->id;
-
-  /* we skip the start position */
-  /* FIXME: but for unknown reason the server discards the last position */
-  p.length = 2 * (path->length - 1) + 1;
-  p.first_index = 0;
-  p.last_index = p.length - 1;
-  p.pos = fc_malloc(p.length * sizeof(struct map_position));
-  j = 0;
-  for (i = 1; i < path->length; i++) {
-    p.pos[j].x = path->positions[i].x;
-    p.pos[j].y = path->positions[i].y;
-    freelog(PACKET_LOG_LEVEL, "  packet[%d] = (%d,%d)", j, p.pos[j].x,
-            p.pos[j].y);
-    j++;
-  }
-  for (i = path->length - 2; i >= 0; i--) {
-    p.pos[j].x = path->positions[i].x;
-    p.pos[j].y = path->positions[i].y;
-    freelog(PACKET_LOG_LEVEL, "  packet[%d] = (%d,%d)", j, p.pos[j].x,
-            p.pos[j].y);
-    j++;
-  }
-  send_packet_goto_route(&aconnection, &p, ROUTE_PATROL);
-  free(p.pos);
-  p.pos = NULL;
+  send_goto_path(punit, path, ROUTE_PATROL);
   pf_destroy_path(path);
 }
 
@@ -490,34 +501,18 @@
 ***********************************************************************/
 void send_goto_route(struct unit *punit)
 {
-  struct packet_goto_route p;
-  int i;
   struct pf_path *path = NULL;
+  int i;
 
   assert(is_active);
   assert(punit->id == goto_map.unit_id);
 
+  /* Build a single path. */
   for (i = 0; i < goto_map.num_parts; i++) {
     path = pft_concat(path, goto_map.parts[i].path);
   }
 
-  p.unit_id = punit->id;
-
-  /* we skip the start position */
-  /* FIXME: but for unknown reason the server discards the last position */
-  p.length = path->length - 1 + 1;
-  p.first_index = 0;
-  p.last_index = p.length - 1;
-  p.pos = fc_malloc(p.length * sizeof(struct map_position));
-  for (i = 0; i < path->length - 1; i++) {
-    p.pos[i].x = path->positions[i + 1].x;
-    p.pos[i].y = path->positions[i + 1].y;
-    freelog(PACKET_LOG_LEVEL, "  packet[%d] = (%d,%d)", i, p.pos[i].x,
-            p.pos[i].y);
-  }
-  send_packet_goto_route(&aconnection, &p, ROUTE_GOTO);
-  free(p.pos);
-  p.pos = NULL;
+  send_goto_path(punit, path, ROUTE_GOTO);
   pf_destroy_path(path);
 }