[Freeciv-Dev] Re: PATCH: remove map_adjuxt_[xy] invocations

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> I would not clutter map_inx(), which was supposed to be changed to
> map_index(), with a lot of extra checks and stuff.

OK, fine by me.

Raimar suggested map_pos_to_index for this macro (with a corresponding
index_to_map_pos for the reverse).  How do you feel about this
nomenclature?

> Make a map_index_checked() for this purpose, and put it where it
> is needed.
> 
> Some minor nits below.

No doubt you've seen my later patch that addressed some of this.  I'll
reply anyway...I just got this mail from you, but maybe it was sent some
time ago.  Who knows.

> At 01:03 AM 01/10/03 -0400, Jason Dorje Short wrote:

<snip all the redundancy with is_real_tile>

Damn, I didn't think to check for that.  I'll review it all.

> >     for(x=punit->x-2; x<punit->x+3; ++x) {
> >       unit_list_iterate(map_get_tile(x, y)->units, pu)
> >       if(unit_flag(pu, F_PARTIAL_INVIS)) {
> >-        refresh_tile_mapcanvas(map_adjust_x(pu->x), y, 1);
> >+        /* Why not just use (x, y) instead of (pu->x, pu->y)?  --JDS */
> >+        assert(is_normal_map_pos(pu->x, pu->y));
> >+        refresh_tile_mapcanvas(pu->x, pu->y, 1);
> >       }
> >       unit_list_iterate_end
> >     }
> >   }

I don't think the assertion is "incorrect".  It is unneeded since the
unit should by definition have normal map positions - if a check is to
be done it should be in refresh_tile_mapcanvas.  Thus I removed it in
the later patch.

> >
> >Index: server/mapgen.c
> >===================================================================
> >RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
> >retrieving revision 1.72
> >diff -u -u -5 -r1.72 mapgen.c
> >--- server/mapgen.c    2001/09/30 21:55:34     1.72
> >+++ server/mapgen.c    2001/10/02 15:45:03
> >@@ -1336,33 +1338,23 @@
> >   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.  Is this desired?  --JDS */
> >   whole_map_iterate(x, y) {
> >-    my = map_adjust_y(y - 1);
> >-    py = map_adjust_y(y + 1);
> >-    mx = map_adjust_x(x - 1);
> >-    px = map_adjust_x(x + 1);
> >     a = hmap(x, y) * 2;
> >-
> >-    a += hmap(px, my);
> >-    a += hmap(mx, my);
> >-    a += hmap(mx, py);
> >-    a += hmap(px, py);
> >
> >-    a += hmap(x, my);
> >-    a += hmap(mx, y);
> >+    adjc_iterate(x, y, x2, y2) {
> >+      a += hmap(x2, y2);
> >+    } adjc_iterate_end;
> >
> >-    a += hmap(x, py);
> >-    a += hmap(px, y);
> >+    a += myrand(60) - 30;
> >
> >-    a += myrand(60);
> >-    a -= 30;
> >     if (a < 0)
> >       a = 0;
> >     hmap(x, y) = a / 10;
> 
> Your use of adjacent_iterate means the /10 is incorrect. You should
> probably accumulate the count of tiles actually collected.

Correct.  The newer patch uses nearest_real_tile to achieve the
identical behavior to current code.  I do think current code is
incorrect, but the correctness of the cleanup patch is much easier to
verify if it leaves the behavior identical.

The biggest problem I have with current code is not that it does the
feedback thing (the smoothing is a pretty flexible process anyway), but
that the feedback is dependent on whole_map_iterate.  This violates the
idea behind whole_map_iterate's use IMO.  But, I'm not prepared to fix
it at this time.

jason