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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raahul Kumar <raahul_da_man@xxxxxxxxx> wrote: 

Thanks a lot for bothering to look at my patch.

First, I would like to announce that all trouble bit were inherited by my
patch from previous code.  Well, many of trouble bits...

[..]

> number 3. Usage of your goto results has improved readability. You
> should
> probably break your patch into 4 smaller sub patches

I will do it, maybe in a slightly different way, but I will.

[..]

> List of Scary Code
> 
> find_the_shortest_path
> Some of the corrected functions.

sorry
All I did is to cut-n-paste relevant pieces of find_the_shortest_path
into these corrected cost functions.  I shall add some comments.  Now I
know why each component is there, although I am not 100% sure that all
the needed components are there yet.  I added some checks for FoW
(is_known_and_seen) but that's it.

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

oops
that's cut-n-paste for sure.
ah, actually I didn't touch this part at all (apart from fixing the
majik)
I will probably fix it as a first patch, together with other majik
things.

> -  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?

both :0
actually the use of range in this function is to estimate how far the
unit _will_ see when it come to some location (x,y).  So the presence of
a watchtower at the initial location should not affect this variable.  At
most we can recalculate range for each final location (x,y) but it is
costly and sometimes the tile (x,y) is still unknown...

> /* 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.

actually this is another "inherited" bit.  I just changed it a little :)
I think if the human player sees a hut and wants to collect it, he should
send the explorer directly to it.  In some cases you do not want to try
the hut for the fear of unleashing barbarians too close to your
undefended cities.

>         
> /* 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?

this particular init_warmap doesn't do anything, it seems.
I asked the same question, nobody answered...

> 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.

well, this is similar to the use of SEA_MOVE_COST but in actual fact it
is completely opposite.  returning negative movecost is to signal that
routes must not pass through the tile and, at the same time, provide the
movecost
for _ending_ the route here.

> 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.

it answers the question: can player be sure (from what he sees) that the
given location is at most distance 2 from the coast?
is_coast_seen answers the same question but for distance 1.
if you can suggest a way to do it in a more elegant way, I will be very
grateful.

> 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.

I didn't see any reason for igter move to cost 3 and it is 1 (as I think
it should be) in client/goto.c and nobody complains -- so I changed it.

> 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.

yes, something like this.

the cost function normal_move_city is actually never used (yet).

> maxcost= THRESHOLD * 6 + 2
> 
> Why add 2? 

you ask me?
I got it from really_generate_warmap so my warmap would be compatible.

Thanks again for the comments.  I am sure that with your help we can
improve the patch and eventually get it through.

Best,
G.



____________________________________________________________
Do You Yahoo!?
Get your free @yahoo.co.uk address at http://mail.yahoo.co.uk
or your free @yahoo.ie address at http://mail.yahoo.ie