[Freeciv-Dev] Re: Safe paths for triremes etc. (PR#1007)

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

Hi G,

Some sections of your patch are quite good. You got rid of yet another magic
number 3. Usage of your goto results has improved readability. You should
probably break your patch into 4 smaller sub patches

1) Removing magic numbers and renaming constants(very easy to check) Everything
you've done here is good.

2) Warmap generation change Excellent.

3) Usage of goto results Good

4) Your corrected move cost functions( Hardest one to check) Scary, as it now
stands I have no idea if your functions actually work as advertised. In
replying to this post let me know what the rules of ZOC are. I'm not sure units
that ignore zoc will work well with this.

Excellent work! It looks like my dream of adding railroad movement costs like
SMAC, CTP may one day be realised.

List of Scary Code

find_the_shortest_path
Some of the corrected functions.
I'm whimpering after reading that. Can someone double check that code, I do not
feel I understood it. I did not review it at all. Someone else will have to
look it over with an electron microscope.

 c = map_get_tile(punit->x, punit->y)->move_cost[k];

Better name than c. Something like

current_move_cost would be good.
 


-  if (unit_profits_of_watchtower(punit)
-      && map_get_tile(punit->x, punit->y)->special & S_FORTRESS)
-    range =get_watchtower_vision(punit);

You removed all the code that checks for increased unit vision range. Your bad
or part of
an evil masterplan?

/* maxcost is maximum distnace of search * SINLGE_MOVE */

int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3 * SINGLE_MOVE;

I see no reason to care if the player is an ai player or not. Obviously you do,
go ahead and explain your reasoning.

if (gotores == GR_ARRIVED)
          return ai_manage_explorer(punit);
          
/* init_warmap(punit->x, punit->y, LAND_MOVING); */

I am pretty sure that init_warmap does not actually do anything on x86 boxes.
Does anyone know any reason to leave it in?

return -SINGLE_MOVE

No, no,no. Bad Berkolaiko, Bad! Do not return a negative single move cost, this
is part of the reason I used to believe there was a relation between single
move cost and the sea move cost.

return SEA_MOVE_COST would be better.

is_coast_seen2 What does this marvellous function do that is different enough
from is_coast_seen to justify its existence? It should be possible to roll
these two functions into one.

igter_move_unit(int x, int y, int dir, int x1, int y1)
+{
+  if (map_get_terrain(x1, y1) == T_OCEAN) {
+    if (ground_unit_transporter_capacity(x1, y1, unit_owner(warmap.warunit)) >
0)
+      return SINGLE_MOVE;
+    else
+      return MAXCOST;
+  } else {
+    return (map_get_tile(x, y)->move_cost[dir] ? MOVE_COST_ROAD : 0);
+  } 

I believe this implies that warmap generation no longer believes the move cost
for igter units is 3 instead of 1. I don't remember seeing this change. *Reads
further on* Yes, that change is a good enough reason to include this patch all
by itself.

What normal_move_city does ? No idea. Comment it out and watch what happens. I
can think of a function that calculates how to attack a unit within 1-2 moves
of the city. Obvious use to the AI.

maxcost= THRESHOLD * 6 + 2

Why add 2? 





__________________________________________________
Do You Yahoo!?
Make a great connection at Yahoo! Personals.
http://personals.yahoo.com