[Freeciv-Dev] Re: (PR#7304) iso-map support for mapgen

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7304 >

1. This

+  for (yn = ymin; yn < ymax; yn++) {
+    for (xn = 0; xn < map.xsize; xn++) {
+      do_in_map_pos(x, y, xn, yn) {

is surely whole_map_iterate

2. Here

+    get_random_nat_position_from_state(&xn, &yn, pstate);
+    if (hnat(xn, yn) == 0 && (hnat(xn + 1, yn) != 0
+                             || hnat(xn - 1, yn) != 0
+                             || hnat(xn, yn + 1) != 0
+                             || hnat(xn, yn - 1) != 0)) {

you are doing adjacency in natural coordinates, a bad idea!
when fixing, make sure you hide away all arithmetic, say using
cartesian iterate.

If I am wrong, it deserves a comment.


3. Most uses of get_random_nat_position_from_state are like

-    get_random_map_position_from_state(&x, &y, pstate);
+    get_random_nat_position_from_state(&xn, &yn, pstate);
+    native_to_map_pos(&x, &y, xn, yn);

I think you should have left get_random_map_position_from_state
I know the state is in nat coords, but you might as well return map 
coordinates, and save on 2 conversions.


4. In general, wherever possible, try to get rid of cycles and do 
iterates.  For example all cycles of initworld can be done within one 
whole_world_iterate plus a few ifs.  This will make things more portable I 
feel.

G.