[Freeciv-Dev] Re: [UPDATE] Corecleanup_08 patch to cvs-Sep28

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 09, 2001 at 02:45:33AM -0400, Ross W. Wetmore wrote:
> At 09:38 AM 01/10/05 +0200, Raimar Falke wrote:
> >On Thu, Oct 04, 2001 at 10:10:19PM -0400, Ross W. Wetmore wrote:
> [...]
> >
> >Short comment on this: the patch includes reformatting like:
> > -  return i / 2;
> > +
> > +  return i/2;
> >which are useless without a strict coding style (which we still lack).
> 
> Actually, these are often the original CVS where I haven't picked up 
> all the patch bits that added spaces everywhere. In patches for CVS I
> have usually made a sanity run to remove redundant cosmetic stuff unless 
> it is too endemic and/or I really think CVS should be "improved".
> 
> There may be some in the real patches, but it will be minimal.
> 

> Can we stop Gaute from doing this unnecessarily, too :-?

He is stopped ;)

> >It includes pieces like:
> > -  map_city_radius_iterate(src_x, src_y, x1, y1) {
> > +  city_map_checked_iterate(src_x, src_y, cx, cy, x1, y1) {
> >      struct city *pcity = map_get_city(x1, y1);
> >      if (pcity) {
> >        update_city_tile_status_map(pcity, src_x, src_y);
> >      }
> > -  } map_city_radius_iterate_end;
> > +  } city_map_checked_iterate_end;
> >which are ok and I would like to get as a seperate patch.
> 
> There are I think 3 places in unittools. And it was part of the cleanup
> to get rid of all the duplicate flavours of city_map iteration along
> with the cases where the checked flavour was useful.
> 
> If you want, I can do the city.h consolidation and anything left in CVS 
> that doesn't conform to the final set. Look at city.h and let me know
> whether you want to get this all out of the way.

No I don't want different implemenations of the iterate macros. We had
the dicussion about city_map_iterate_outwards_indices before and I
still think it is ugly and unnecessary. The
s/map_city_radius_iterate/city_map_checked_iterate/ is ok. So is the
s/5/CITY_MAP_SIZE/ change.

> The simple renames were in corecleanup_07d, it was corecleanup_07h,i
> that did the final parts of the job if you want a more concise handle
> on the set of things to look at (of course understanding that not
> everything may be quite the same now, but the stuff in corecleanup_08
> is current). 
> 
> If you have any major changes to this let me know. Or if you are
> happy enough with them, I'll package up the set for a proper review
> cycle. The total will probably just over 500 lines of diff.
> 
> >It includes pieces like:
> > -  punit->x = map_adjust_x(x); /* was = x, caused segfaults -- Syela */
> > -  punit->y=y;
> > -  if (y < 0 || y >= map.ysize) {
> > +  punit->x = x;
> > +  punit->y = y;
> > +  if( !normalize_map_pos(&punit->x, &punit->y) ) {
> >which are also ok.
> >
> >It includes code as:
> > -          ACTIVITY_FORTIFIED and the unit has no chance of moving anyway */
> > -       handle_unit_activity_request(bodyguard, ACTIVITY_IDLE);
> > +          ACTIVITY_FORTIFIED and the unit has no chance of moving anyway
> > +       handle_unit_activity_request(bodyguard, ACTIVITY_IDLE); */
> >and
> > +    pplayer->ai.expand +=
> > +        (myrand(pplayer->ai.expand)-myrand(pplayer->ai.expand))/5;
> >which will not be applied without a statement of motivation.
> 
> This is part of the AI stuff. The first is the response to the FIXME a
> line or so above if you look at patched code.
> 
> The second tries to use the only real "personality" variable in the whole
> server AI to provide per civ difference in the weight they give to 
> procreation. Instead of 100% (no variation), it sets per civ random
> value from 80 to 120. Smaller values kick one out of smallpox mode to 
> vertical growth faster.
> 
> As a general I wouldn't recommend taking any of the AI stuff just yet. 
> But if you want to try the expand one it doesn't really hurt.
> 
> >And there is also code like:
> >        *canvas_x = map_x-map_view_x0;
> > -    else if(map_x < map_adjust_x(map_view_x0+map_canvas_store_twidth))
> > +    else if(map_x < map_wrap_x(map_view_x0+map_canvas_store_twidth))
> >        *canvas_x = map_x+map.xsize-map_view_x0;
> >
> >which deals with the wrapping types (which is ok but such changes are
> >buried under the other changes)
> 
> This is part of removing map_adjust. In some places the code really does
> want to clip (i.e nearest_map_pos), but note it is pretty much confined to
> client/GUI code, and should not be confused with doing bad things to
> gaming coordinates as broken fixes for not understanding normalization, 
> hence the function name is more apt.
> 

> If you and/or Jason agree, I can do all the map_adjust removal as well.

There aren't this many left in the non-client parts:

$ grep -Ir map_adjust ai common/ server/
common/map.h:#define map_adjust_x(X)            \
common/map.h:#define map_adjust_y(Y) \
server/mapgen.c:#define hmap(x,y) (height_map[(y) * map.xsize + 
map_adjust_x(x)])
server/mapgen.c:          map_set_terrain(map_adjust_x(x), y, T_HILLS);
server/mapgen.c:    my = map_adjust_y(y - 1);
server/mapgen.c:    py = map_adjust_y(y + 1);
server/mapgen.c:    mx = map_adjust_x(x - 1);
server/mapgen.c:    px = map_adjust_x(x + 1);
server/maphand.c:  int offset = map_adjust_x(x)+map_adjust_y(y)*map.xsize;
server/settlers.c:  x=map_adjust_x(x);
server/settlers.c:  y=map_adjust_y(y);
server/unittools.c:  punit->x = map_adjust_x(x); /* was = x, caused segfaults 
-- Syela */

The mapgen parts will go away shortly. This leaves the client
directory. We have here to seperate "normal" cases where map_adjust_*
can be changed easily to and cases normalize_map_pos. It would be nice
if you can give a patch for the first cases. I don't know how large
these two sets are.

> There are also a fair number of places where (x+1,y-1) and such are used
> that would complete the corecleanups for most of the things needed to
> have it all go through normalize_map_pos, mapstep or iterate. I think
> I have all the +/-1, but there may be a +/-3 or something odd that I
> haven't picked up via grep or triggered in testing yet.

I would be happy about a patch.

> This can be part of this or done as a separate patch if you want. Note
> there is a fair bit of this in tilespec.c if you want to see what the
> basic flavour is all about.

-  for (dir = 0; dir < 8; dir++) {
-    ttype_near[dir] = T_UNKNOWN;
-    tspecial_near[dir] = S_NO_SPECIAL;
-  }
-
-  adjc_dir_iterate(x, y, x1, y1, dir) {
-    ttype_near[dir] = map_get_terrain(x1, y1);
-    tspecial_near[dir] = map_get_special(x1, y1);
-
-    /* hacking away the river here... */
-    if (ttype_near[dir] == T_RIVER) {
-      ttype_near[dir] = T_GRASSLAND;
-      tspecial_near[dir] |= S_RIVER;
+  for (dir=0; dir<8; dir++) {
+    int x1 = x + DIR_DX[dir];
+    int y1 = y + DIR_DY[dir];
+    if (normalize_map_pos(&x1, &y1)) {
+      ttype_near[dir] = map_get_terrain(x1, y1);
+      tspecial_near[dir] = map_get_special(x1, y1);
+
+      /* hacking away the river here... */
+      if (ttype_near[dir] == T_RIVER) {
+       ttype_near[dir] = T_GRASSLAND;
+       tspecial_near[dir] |= S_RIVER;
+      }
+    } else {
+      ttype_near[dir] = T_UNKNOWN;

Aehhmmm I have changed the for to the for and the adjc_dir_iterate
just some short time ago. As to the other changes to tilespec: I would
really like to get rid of these huge amount of variables:
   int ttype_north_east, ttype_south_east, ttype_south_west, ttype_north_west;
   int tspecial, tspecial_north, tspecial_south, tspecial_east, tspecial_west;
   int tspecial_north_east, tspecial_south_east, tspecial_south_west, 
tspecial_north_west;

One or two arrays and indexing with DIR8_* is the way to go. Any patch
which does this or just replaces -1/+1 is ok. Note: I will not accept
a patch which contains DIR_DX.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "Windows is the one true OS. MS invented the GUI. MS invented 
   the 32 bit OS. MS is open and standard. MS loves you. We have 
   always been at war with Oceana."