[Freeciv-Dev] Re: (PR#8624) New clima function to best handle terrain place, used to place poles.

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

1.  There's no need for is_singular_map_pos to be inline.  Just make it 
a regular function in map.c.  (Did I say differently before?  I hope not...)

2.  Clima isn't an english word.  I guess it's French or Flemish?  What 
can we call this function in english?  Or maybe we can invent a new term 
(at least that way it's unique), but in that case we have to make sure 
it's well defined.

3.  This code:

+    x_2 = NATURAL_WIDTH / 2 - 1 + NATURAL_WIDTH % 2;

isn't it the same as (NATURAL_WIDTH - 1) / 2?  Also x_2 is not a good 
name.  And, don't you want x_2 = (NATURAL_WIDTH + 1) / 2 so that when 
you divide by x_2 later you never get a value over 1?

4.  Be careful with your use of float values.  For instance in this code:

+  if (x + y > 1) {

I think it's better to have (x + y > 1.0) to make sure (and clear to the 
reader) that everything is treated as a float.

5.

+  return 150 * (x * x * y + x * y * y) - 50 * (x * x * x + y * y * y) +
+      150 * (x * x + y * y);

This needs some explanation.  Does this come from the Pierce projection?

6.  rand_clima_map_pos should use rand_map_pos_filtered.

7.  In near_singulararity, where does the value 2 come from?  Should 
this be MAP_CITY_RADIUS?

8.  In this code:

-    if (has_poles && nat_y > map.ysize * 42 / 100
-       && nat_y < map.ysize * 58 / 100 && myrand(100) > 50) {
+    if (T > 42         && T <  58  && myrand(100) > 50) {

I think it is wrong.  T == 100 at the equator, right?  So the check you 
want is T > 84.  I think there's a similar error in the creation of forests.

9.  In this code:

-    if (adjacent_river_tiles4(x, y) != 0||
-       adjacent_ocean_tiles4(x, y) != 0) {
+    if (adjacent_river_tiles4(x, y) != 0 ||
+       adjacent_ocean_tiles4(x, y) != 0 ||
+        (map_get_terrain(x, y) == T_ARCTIC
+         && map_clima(x, y) < 8)) { /*rivers end at poles */

why?  And can you avoid the reference to T_ARCTIC here?

10.  assign_poles_numbers is really not good!  It's not clear at all 
that this doesn't enter an infinite loop.

11.  I'm not sure that separatepoles is handled rigorously.  If you have 
a sufficiently small map I suspect there is some chance that the 
separation will not be sufficient.  But maybe this is acceptable or 
unavoidable.

12.  For flat-earth maps I don't think it's good to assume poles on just 
one end.  IMO it would be better to just assume there are no climate 
differences in the map.  Perhaps this is a question for the design board.

13.  The poles are very large.  Particularly in the standard case it's 
just wasteful to put lots of arctic tiles up there.  I'm really not sure 
what to do about this.

Finally, make sure you test it out with all generators.

There may be more but I think this is okay for now :-).  Your design is 
good but there's still a lot of work to be done.  But this is really 
true of any code that tries to change mapgen.  The current mapgen code 
is so non-extensible it's hard to add anything to it without rewriting a 
lot of it.

jason