[Freeciv-Dev] Re: PATCH: remove map_adjust_[xy] invocations from server (PR#989)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Fri, Oct 05, 2001 at 01:43:57AM -0700, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> > Here is a correction to the previous patch.
> >
> > Changes include:
> >
> > - The client code was cut out; that wasn't supposed to be there anyway.
> >
> > - A redundante is_real_tile check was removed.  In this case I have now
> > simply replaced a logging-of-error with a simple assertion.  It's
> > cleaner but slightly less safe in non-debug mode.
> >
> > Unless I'm badly mistaken this should be ready for inclusion.
> >
> 
> >  /**************************************************************************
> > +Returns TRUE iff the map position is normal.  "Normal" here means that it
> > +is both a real/valid coordinate set and that the coordinates are in their
> > +canonical/proper form.  In plain English: the coordinates must be on the
> > +map.
> > +**************************************************************************/
> > +int is_normal_map_pos(int x, int y)
> > +{
> > +  return 0 <= y && y < map.ysize && 0 <= x && x < map.xsize;
> 
> This should be expresed in a way of normalize_map_pos. Or the other
> way round.

I don't understand.  You mean this function should call
normalize_map_pos?  That won't work; normalize_map_pos returns the
realness of the coordinate.  Having normalize_map_pos call this function
wouldn't work either, since normalize_map_pos must not only determine
whether the value is real but which direction it is missing in (you
could make the call, but you'd still have to check if it returned
false).

> We may avoid the function call if is_normal_map_pos is a
> macro. And with the RANGE_CHECK_0 from Ross there would also be no
> double evaluation.

Well, whatever.  Should I add RANGE_CHECK_0 just for this patch? 
Shouldn't that be its own patch?  Would you rather I propose that patch
first?  Should I make this into a macro?  What is the consensus on any
of these issues?

> >  /**************************************************************************
> > -  smooth_map should be viewed  as a  corrosion function on the map, it 
> > levels
> > -  out the differences in the heightmap.
> > +  smooth_map should be viewed  as a corrosion function on the map, it
> > +  levels out the differences in the heightmap.
> >  **************************************************************************/
> >  static void smooth_map(void)
> >  {
> > -  int mx,my,px,py;
> > -  int a;
> > -
> > +  /* This overwrites itself as it runs, creating an unpredictable
> > +     feedback that depends on the ordering of whole_map_iterate.  Is
> > +     this desired?  --JDS */
> 
> Mhhh since this obvious wrong: either iterate randomly or save the
> value in another array.

It's wrong but it doesn't really matter; it smooths things out just
fine.  If you really want I'll provide another patch that fixes the code
to move values to another array.

> > +  assert(is_real_tile(x, y));
> > +  normalize_map_pos(&x, &y);
> 
> is_real_tile is unnecessary.

You mean I should use

is_real = normalize_map_pos(&x, &y);
assert(is_real);

?

That means a new variable declaration for every function, which is (IMO)
not worth the time savings it gives.  Almost all existing code does it
my way.

jason