[Freeciv-Dev] Re: PATCH: rand_pos function and usage (PR#1017)

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

rand_pos() is fine, as the normal condition should be a noop for
all current map topologies, and (hopefully) all but a small number
of positions for any forseeable properly implemented ones, i.e.
this while loop is expected to run once on average.

Ditch RAND_POS_CHECKED (and any places where you tried to apply it) 
since as coded it does map.xsize*map.ysize tests to return a single 
result. You clearly don't understand what you are doing here, so 
should not touch the current code until you have figured out
something sensible to do :-).

Cheers,
RossW
=====

At 12:49 AM 01/10/26 -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>Raimar Falke wrote:
>
>...
>
>> If we now adjust rand_pos accordingly I will be happy.
>
>I'm still not sure what exactly you (and, separately, Ross) want out of
>rand_pos.
>
>Anyway, here is another attempt.  Like the other two candidates, this
>makes effectively no difference to the speed of the code.  It combines
>some of the simplicity of the rand_pos function with the power of the
>RAND_POS_CHECKED macro by providing both independently.
>
>Personally, I like this implementation more.
>
>jason? rc
>? old
>? topology
>? tilespec_patch
>? corecleanup_08.ReadMe
>? topology.c
>? civserver-regular
>? client/mapview_common.c
>? client/mapview_common.h
>Index: common/map.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
>retrieving revision 1.97
>diff -u -r1.97 map.c
>--- common/map.c       2001/10/15 13:42:50     1.97
>+++ common/map.c       2001/10/26 05:47:09
>@@ -1340,6 +1340,17 @@
> }
> 
> /**************************************************************************
>+Random square anywhere on the map.  Only "normal" positions will be found.
>+**************************************************************************/
>+void rand_pos(int *x, int *y)
>+{
>+  do {
>+    *x = myrand(map.xsize);
>+    *y = myrand(map.ysize);
>+  } while (!regular_map_pos_is_normal(*x, *y));
>+}
>+
>+/**************************************************************************
> Random neighbouring square.
> **************************************************************************/
> void rand_neighbour(int x0, int y0, int *x, int *y)
>Index: common/map.h
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
>retrieving revision 1.100
>diff -u -r1.100 map.h
>--- common/map.h       2001/10/19 08:12:52     1.100
>+++ common/map.h       2001/10/26 05:47:10
>@@ -274,6 +274,11 @@
> enum known_type tile_is_known(int x, int y);
> int is_real_tile(int x, int y);
> int is_normal_map_pos(int x, int y);
>+/* Determines whether the position is normal given that it's "regular"
>+   (in he range 0<=x<map.xsize and 0<=y<map.ysize).  It's primarily a
>+   faster version of is_normal_map_pos since such checks are pretty
>+   common. */
>+#define regular_map_pos_is_normal(x, y) (1)
> /*
>  * A "border position" is any one that has adjacent positions that are
>  * not normal/proper.
>@@ -286,6 +291,30 @@
> void nearest_real_pos(int *x, int *y);
> 
> void rand_neighbour(int x0, int y0, int *x, int *y);
>+void rand_pos(int *x, int *y);
>+/* This picks a random position (x, y) from the map that satisfies
POS_CHECK.
>+ *   x, y: identifiers to hold the new position
>+ *   POS_CHECK: an expression that evaluates if position (x, y) (as above)
>+ *              is a valid choice. */
>+#define RAND_POS_CHECKED(x, y, POS_CHECK)
  \
>+{
  \
>+  /* Structured algorithm: assemble a full list of valid points */
  \
>+  struct map_position *_pos_arr = fc_malloc(sizeof(*_pos_arr) *
  \
>+                                          map.xsize * map.ysize);         \
>+  int _count = 0;
  \
>+  whole_map_iterate(x, y) {
  \
>+    if (POS_CHECK) {
  \
>+      _pos_arr[_count].x = x;
  \
>+      _pos_arr[_count].y = y;
  \
>+      _count++;
  \
>+    }
  \
>+  } whole_map_iterate_end;
  \
>+  if (!_count) abort(); /* what else can we do? */
  \
>+  _count = myrand(_count);
  \
>+  x = _pos_arr[_count].x;
  \
>+  y = _pos_arr[_count].y;
  \
>+  free(_pos_arr);
  \
>+}
  \