[Freeciv-Dev] Re: (PR#4758) client crash on goto

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Gregory Berkolaiko wrote:
> On Wed, 6 Aug 2003, Jason Short wrote:
>>Gregory Berkolaiko wrote:
>>
>>>The problem here is that find_nearest_allied_city finds a city to which 
>>>more rigourous client-goto cannot find a path.
>>>
>>
>>How about this?
> 
> Good.  I read it very briefly, so only one remark:
> the code that assigns pf_parameter in the correct way can be separated 
> into a special function imo.
> 
> I also hope that the comment to send_goto_route sufficiently highlights 
> the difference between itself and send_goto_path

Very well.

jason

? rc
Index: client/control.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/control.c,v
retrieving revision 1.107
diff -u -r1.107 control.c
--- client/control.c    2003/07/23 13:46:01     1.107
+++ client/control.c    2003/08/08 17:01:41
@@ -664,15 +664,12 @@
 void request_unit_return(struct unit *punit)
 {
   struct city *pcity;
+  struct pf_path *path;
 
-  enter_goto_state(punit);
-
-  if ((pcity = find_nearest_allied_city(punit))) {
-    draw_line(pcity->x, pcity->y);
-    send_goto_route(punit);
+  if ((pcity = find_nearest_allied_city(punit, &path))) {
+    send_goto_path(punit, path);
+    pf_destroy_path(path);
   }
-
-  exit_goto_state();
 }
 
 /**************************************************************************
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/08 17:01:41
@@ -357,6 +357,28 @@
 }
 
 /********************************************************************** 
+  Fill the PF parameter with the correct client-goto values.
+***********************************************************************/
+static void fill_client_goto_parameter(struct unit *punit,
+                                      struct pf_parameter *parameter)
+{
+  pft_fill_default_parameter(parameter);
+  pft_fill_unit_parameter(parameter, punit);
+  parameter->get_EC = get_EC;
+  if (unit_type(punit)->attack_strength > 0) {
+    parameter->get_TB = get_TB_aggr;
+  } else {
+    parameter->get_TB = get_TB_peace;
+  }
+  parameter->turn_mode = TM_WORST_TIME;
+  parameter->start_x = punit->x;
+  parameter->start_y = punit->y;
+
+  /* May be overwritten by the caller. */
+  parameter->moves_left_initially = punit->moves_left;
+}
+
+/********************************************************************** 
   Enter the goto state: activate, prepare PF-template and add the 
   initial part.
 ***********************************************************************/
@@ -367,17 +389,9 @@
   goto_map.unit_id = punit->id;
   assert(goto_map.num_parts == 0);
 
-  pft_fill_default_parameter(&goto_map.template);
-  pft_fill_unit_parameter(&goto_map.template, punit);
   assert(goto_map.template.get_EC == NULL);
-  goto_map.template.get_EC = get_EC;
   assert(goto_map.template.get_TB == NULL);
-  if (unit_type(punit)->attack_strength > 0) {
-    goto_map.template.get_TB = get_TB_aggr;
-  } else {
-    goto_map.template.get_TB = get_TB_peace;
-  }    
-  goto_map.template.turn_mode = TM_WORST_TIME;
+  fill_client_goto_parameter(punit, &goto_map.template);
 
   add_part();
   is_active = TRUE;
@@ -438,10 +452,39 @@
   update_last_part(dest_x, dest_y);
 }
 
-/********************************************************************** 
-  FIXME: the packet interface need to be changed to support danger
-  paths.
-***********************************************************************/
+/**************************************************************************
+  Send an arbitrary goto path for the unit to the server.  FIXME: danger
+  paths are not supported.
+**************************************************************************/
+void send_goto_path(struct unit *punit, struct pf_path *path)
+{
+  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_GOTO);
+
+  free(p.pos);
+}
+
+/**************************************************************************
+  Send the current patrol route (i.e., the one generated via HOVER_STATE)
+  to the server.  FIXME: danger paths are not supported.
+**************************************************************************/
 void send_patrol_route(struct unit *punit)
 {
   struct packet_goto_route p;
@@ -484,15 +527,14 @@
   pf_destroy_path(path);
 }
 
-/********************************************************************** 
-  FIXME: the packet interface need to be changed to support danger
-  paths.
-***********************************************************************/
+/**************************************************************************
+  Send the current goto route (i.e., the one generated via HOVER_STATE)
+  to the server.  FIXME: danger paths are not supported.
+**************************************************************************/
 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);
@@ -500,24 +542,8 @@
   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);
   pf_destroy_path(path);
 }
 
@@ -604,28 +630,41 @@
 
 /**************************************************************************
   Find the nearest (fastest to reach) allied city for the unit, or NULL if
-  none is reachable.
+  none is reachable.  If there is a reachable city and 'path' is NULL, it
+  will be set to a newly-allocated PF path indicating how to get there.
 ***************************************************************************/
-struct city *find_nearest_allied_city(struct unit *punit)
+struct city *find_nearest_allied_city(struct unit *punit,
+                                     struct pf_path **path)
 {
   struct city *pcity = NULL;
+  struct pf_parameter parameter;
+  struct pf_map *map;
 
   if ((pcity = is_allied_city_tile(map_get_tile(punit->x, punit->y),
                                   game.player_ptr))) {
-    /* We're already on a city - PF doesn't check for this (!). */
-    return pcity;
+    /* We're already on a city - don't go anywhere. */
+    return NULL;
   }
+
+  fill_client_goto_parameter(punit, &parameter);
+  map = pf_create_map(&parameter);
+
+  while (pf_next(map)) {
+    struct pf_position pos;
 
-  simple_unit_path_iterator(punit, pos) {
+    pf_next_get_position(map, &pos);
+
     if ((pcity = is_allied_city_tile(map_get_tile(pos.x, pos.y),
                                     game.player_ptr))) {
-      /* We use break so that the PF map can be destroyed. */
       break;
     }
-  } simple_unit_path_iterator_end;
+  }
+
+  if (pcity && path) {
+    *path = pf_next_get_path(map);
+  }
 
-  /* FIXME: For some reason this sometimes seems to find arbitrary but
-   * reproducable far-away cities instead of the obvious closest city. */
+  pf_destroy_map(map);
 
   return pcity;
 }
Index: client/goto.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.h,v
retrieving revision 1.9
diff -u -r1.9 goto.h
--- client/goto.h       2003/07/07 19:14:36     1.9
+++ client/goto.h       2003/08/08 17:01:41
@@ -15,6 +15,8 @@
 
 #include "map.h"
 
+#include "path_finding.h"
+
 void init_client_goto(void);
 void free_client_goto(void);
 void enter_goto_state(struct unit *punit);
@@ -27,9 +29,11 @@
 void draw_line(int dest_x, int dest_y);
 int get_drawn(int x, int y, int dir);
 
+void send_goto_path(struct unit *punit, struct pf_path *path);
 void send_patrol_route(struct unit *punit);
 void send_goto_route(struct unit *punit);
 
-struct city *find_nearest_allied_city(struct unit *punit);
+struct city *find_nearest_allied_city(struct unit *punit,
+                                     struct pf_path **path);
 
 #endif /* FC__GOTO_H */