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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Tue, Oct 23, 2001 at 02:05:13PM -0400, Jason Dorje Short wrote:
> > Raimar Falke wrote:
> > >
> > > > +/* Determines whether the position is normal given that it's in
> > > > +   the 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 is_normal_map_pos2(x, y) (1)
> > >
> > > IMHO this name is still not acceptable.
> >
> > Ok, I agree...but what should we use?
> 
> We need a new name which describes map position for which
> 0<=x<map.xsize && 0<=y<map.ysize (this condition is independent of the
> topology used). If _for example_ the term regular is used that this
> function may be: is_regular_map_position_real or
> is_regular_map_position_normal. And there would be a
> is_regular_map_position.

Yes, I agree.  Although those names are unfortunately long, there's not
really any way around it.

"regular" isn't a particularly good name (certainly it has no mathematical
meaning that I know of), but I haven't thought of anything better.

Oh, yeah: and we should use "map_pos" everywhere instead of just "pos"
(which might be confused with GUI positions) or "map_position" (which is
unnecessarily long).

> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
> > retrieving revision 1.75
> > diff -u -r1.75 mapgen.c
> > --- server/mapgen.c   2001/10/14 21:02:16     1.75
> > +++ server/mapgen.c   2001/10/23 18:04:21
> > @@ -195,23 +195,30 @@
> >  **************************************************************************/
> >  static void make_forests(void)
> >  {
> > -  int x,y;
> 
> > -  int forestsize=25;
> 
> It looks like this assignment is useless in the old code.

So it seems.

> > -  forestsize=(map.xsize*map.ysize*map.forestsize)/1000;
> > -   do {
> > -    x=myrand(map.xsize);
> > -    y=myrand(map.ysize);
> > -    if (map_get_terrain(x, y)==T_GRASSLAND) {
> > -      make_forest(x,y, hmap(x, y), 25);
> > -    }
> > -    if (myrand(100)>75) {
> > -      y=(myrand(map.ysize*2/10))+map.ysize*4/10;
> > -      x=myrand(map.xsize);
> > -      if (map_get_terrain(x, y)==T_GRASSLAND) {
> > -     make_forest(x,y, hmap(x, y), 25);
> > -      }
> > -    }
> > -  } while (forests<forestsize);
> > +  int forestsize=(map.xsize*map.ysize*map.forestsize)/1000;
> > +  int *x = fc_malloc(sizeof(*x) * forestsize);
> > +  int *y = fc_malloc(sizeof(*y) * forestsize);
> 
> > +  int num = (forestsize+myrand(4))/4, num2, i;
> 
> A fifth of the total forrests created are from "tropical" forests: s|/4|/5|

Oops.

> > +  /* First make some extra tropical forests. */
> > +  RAND_POS_CHECKED(x, y, num,
> 
> > +                y >= map.ysize * 4/10 && y < map.ysize * 6/10 && 
> > map_get_terrain(x, y)==T_GRASSLAND,
> 
> You also have to exclude the poles for this. See make_forest. You may
> also introduce IS_POLE or IS_ARCTIC.

The check above does exclude the poles.  Not only will a pole never have
grassland, but it's y value will never be within the desired range.

Oh, wait...you probably mean the regular forest generation (rather than
tropical forests).  You're right, although the check for grassland should
exclude them anyway.

This has not yet been adjusted for general topologies; once we're decided on
how to handle rand_pos (oops, I guess that should be rand_map_pos) I'll do
that (complete with DISTANCE_FROM_POLE, which makes the calculations much
prettier).

> Still unknown to me is the difference between:
> 
>     if (y>map.ysize*42/100 && y<map.ysize*58/100 && myrand(100)>50)
>       map_set_terrain(x, y, T_JUNGLE);
> and
>       y=(myrand(map.ysize*2/10))+map.ysize*4/10;
> 
> These are similar but not equal. Odd.
> 
> > +                0);
> > +
> > +  /* Now finish by placing forests everywhere. */
> > +  num2 = forestsize-num;
> > +  RAND_POS_CHECKED(x+num, y+num, num2,
> > +                map_get_terrain(x, y)==T_GRASSLAND,
> > +                0);
> > +
> > +  /* Now make the forests. */
> > +  forestsize = num + num2;
> > +  for (i=0; i<forestsize; i++) {
> > +    make_forest(x[i], y[i], hmap(x[i], y[i]), 25);
> > +  }
> > +
> > +  free(x);
> > +  free(y);
> 
> If I see this example I vote for an action part in the
> RAND_POS_CHECKED macro. It is silly to accumulate positions and than
> do a for loop over them. So it should possible to pass a
> "make_forest(x, y, hmap(x, y), 25)" to the RAND_POS_CHECKED
> macro. This would also save us the malloc/free.

Yes, it would.  It would also allow other random position generation (like
making huts) to use this macro.

At this point, though, we see a descrepancy between the "random" method and
the "structured" method of choosing positions, since the random method may
happily choose the same position twice.  If we have an action element to the
loop as well, it will have to be done within both the random and structured
algorithms; it will no longer be possible to discard all the randomly-chosen
points if we move to a structured algorithm.  So, it would make more sense
IMO to separate these algorithms.

jason