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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 12, 2001 at 12:52:39PM -0700, Gregory Berkolaiko wrote:
> See warmap_split7.readme for the info on the patch.
> 
> It is big but at least the changes are localised, more or less.
> 
> Comments are most welcome.

> The aims:
> 
> Make gotohand.c produce safe routes for triremes.
> As a result, make auto-explorer mode be safe for triremes.
> 
> 
> The tools: 
> 
> Since warmap generation is already full of various cases and exceptions,
> adding another check for F_TRIREME is not the best way to proceed.  
> Instead warmap generation is "functionalized": we first decide what type
> of move it is (sea+trireme / sea+regular / land+igter / land+regular /
> etc) and then pass the relevant move_cost calculation to the generic
> costmap generator as a parameter (pointer to a function).
> 
> Same is done to find_the_shortest_path.  The main difference is that the
> move_cost functions are now more refined, they take into account FoW and
> ZoC.  Also the generic costmap generator now records the route into
> special array (generate_warmap only records the distances, not the route
> taken).
> 
> Also, the functionalized warmap generation is opened up: now one can use
> it in the form:
> 
> warmap_initialize(NULL, punit, maxcost);
> while(warmap_next_location(&x1,&y1)){
>   if (that's what we are searching_for(x1, y1)) {
>       punit->goto_dest_x= x1;
>       punit->goto_dest_y= y1;
>       break;
>   }      
>   warmap_process_location(x1,y1);
> }
> 
> this allows us to terminate the costly calculation as soon as we find the
> location that we want.  warmap_next_location gives the locations in the
> sequence of increasing distance from the initial position.
> 
> Examples of use of the new warmap_ methods are given in
> ai_manage_explorer.
> 
> NB: with the right choice of the cost functions the warmap_ methods
> produce identical warmap to generate_warmap routine.  

> However the new methods are slower when making a full warmap
> (i.e. no termination condition).

Why? Is this the call overhead?

> This is why the old one is left intact.  Performance of
> find_the_shortest_path is not so crucial and, in my opinion, the
> functionalized form is easier to understand, maintain, and certainly
> extend.
> 
> 
> Affected files:
> 
>   server/gotohand.c  -- 90% of the changes
>   server/gotohand.h
>   ai/aiunit.c        -- 10% of the changes (all in ai_manage_explorer)
>   server/maphand.[ch]
> 
> 
> Also changed:
> 
> 1. goto_zoc_ok is made to use the true direction that we came from, not
> the guessed direction.  As a consequence dir_ok is made redundant (sorry).

Ohh we had soo much fun with dir_ok ;)

> 2. general cleanup in find_the_shortest_path
>   a. removed cargo dependence: it was ineffectual
>      I need to understand what was required of it to write 
>      a proper replacement
>   b. FoW is now respected (where I think it should be)
>      that fixes PR#727 part 1 (same as PR#958)
>   c. some other stuff probably
> 3. added some comments
> 
> 
> Mode d'emploi:
> 
> Apply to CVS straight away ;)
> jes joking
> The patch is pretty big but I still would like people to look at it.
> The best would probably be if one or two reviewers will come forward.
> Knowledge of costmap generation is desirable but not compulsory.

Comments from anybody else?

I will not go the whole patch since the patch is large.

> +/* SINGLE_MOVE cost function */
> +int single_move(int x, int y, int dir, int x1, int y1)
> +{
> +  return SINGLE_MOVE;
> +}

There should be "const(ant)" in the name of this method.

> +/* NB: SEA_MOVE cost functions sometimes return negative movecost
> + * it is to be interpreted as "can go to, but can't pass through" */

Why is this needed at all. I don't see a problem if a ship "goes
through" a city. This will be eliminated with the extra costs.

> +int healthy_trireme_seamove_strict(int x, int y, int dir, int x1, int y1)
> +int sick_trireme_seamove_strict(int x, int y, int dir, int x1, int y1)

These are weird names. 

> +/* LAND_MOVE cost function for a city ?? */
> +int normal_move_city(void *args, int x, int y, int dir, int x1, int y1)
> +{
> +  /* have no idea what code below is for */
> +  if (map_get_terrain(x1, y1) == T_OCEAN)
> +    return MAXCOST;
> +  else {
> +    struct tile *ptile = map_get_tile(x, y);
> +    int tmp = map_get_tile(x1, y1)->move_cost[DIR_REVERSE(dir)];
> +    return (ptile->move_cost[dir] + tmp +
> +         (ptile->move_cost[dir] > tmp ? 1 : 0))/2;
> +  }
> +}

Is there still no idea what warmaps for cities are for?

> +int cor_land_move(COSTFN *basic_cost, int x, int y, int dir, int x1, int y1)

cor? core?

> +    /* Don't go into the unknown. 5*SINGLE_MOVE is an arbitrary deterrent. */
> +    return (warmap.restriction == GOTO_MOVE_STRAIGHTEST) ?
> +      SINGLE_MOVE : 5*SINGLE_MOVE;

This 5 can be extracted. Something like:
#define UNKNOWN_MOVE_COST       5

This reminds me that there is still no distinction between "move cost"
and "turn based move cost". In the above example SINGLE_MOVE and
5*SINGLE_MOVE are "move costs" and the 5 is the "turn based move
cost". What are the offical names for these entities?

> +/* SEA MOVE correction */
> +int cor_sea_move(COSTFN *basic_cost, int x, int y, int dir, int x1, int y1)

cor == correction? Crrection for what? You may think about writing a
longer description of this warmap/movecost/dynamic movecost
code. *reading a bit further* Ahh if the basic move cost (depending on
the unittype and terrain) is equal the code decides upon the secondary
cost (which takes the environment into account). Correct?

Overall: nice ideas. The current code is hard to understand and extend
so almost anything can improve it. You may think about splitting your
patch because it may be hard to get such a big patch into CVS.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  reality.sys corrupt. Reboot Universe? (y,n,q)