[Freeciv-Dev] Re: [RFC] Move cost map interface

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

I have no major complaints.  However I expect big arguments when it comes 
to implementation.  Also, I don't think I can spend much time on it until 
late May.

On Wed, 24 Apr 2002, Raimar Falke wrote:

> On Tue, Apr 16, 2002 at 09:57:39AM +0200, Raimar Falke wrote:
> > Attached a version which have the requested changes. You would want to
> > wrap an int in the void *.
> 
> And now version 8. Changes:
>  - added move_backward flag

right.
I still think that it's better to have one map_type enum instead of three
different flags, but it's not important.

>  - added move_expended array

The information given by 
        moves_expended, initial_moves
and by 
        turn, move_left
is equivalent.  So I think it is excessive to have both of them.
If I were you, I would convert moves_left to an array though (however it 
only makes sense for MINIMAL and MAXIMAL modes).
Again, this is not an area of my immediate interest.

I also have many problems reading this bit of code:
=============================================================
/*
 * Construct the whole path to the given position and write it into
 * the given path. The position has to be obtained from
 * pf_get_next_position.
 */
void pf_construct_path(pf_map_t map, const struct pf_position *position,
                       const pf_location_t location);

/*
 * The last position of the next best path is written in the given
 * struct pf_position. Only the fields int x, y and
 * remaining_move_points are set. Returns FALSE if no more positions
 * are available in this map. Behavior is undefined after pf_get_path
 * was called.
 */
bool pf_get_next_position(pf_map_t map, pf_location_t *location);
===================================================================

1. In construct_path the second argument is probably struct pf_path *, not 
position.
2. The comment of get_next_position is wrong too.  It's obsolete.
3. The name of get_next_position is obsolete too.  Should be 
get_next_location IMO.
4. If you do 3, change the comment to construct_path.
5. I can foresee a lot of confusion with position / location names.  Thus 
I reccommend to change pf_position to pf_node (as in node of the route).
6. I hope that you do not consider this interface as wrought in stone.  
Circumastances might change, new tasks might demand new capabilities like 
user-suppliable BMC functons.

Best wishes,
G.