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

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

On Sat, 13 Apr 2002, Raimar Falke wrote:

> On Fri, Apr 12, 2002 at 03:17:08PM +0100, Gregory Berkolaiko wrote:
> > On Thu, 11 Apr 2002, Raimar Falke wrote:
> >  struct pf_parameter {
> > @@ -65,10 +65,11 @@
> >       * Definition of the state of a virtual unit unit at the start.
> >       */
> >      int start_x, start_y;
> > -    enum unit_move_type move_type;
> > -    /* only F_IGZOC and F_IGTER are used */
> > -    enum unit_flag_id flags;
> > -    int moves_left;
> 
> > +  /* Indicates which built-in BMC-function should be used */
> > +  enum pf_move_mode {
> > +    LAND_MODE, SEA_MODE, CITY_MODE, IGTER_MODE, CARDINAL_MOVE
> > +  } move_mode;
> 
> I'm against this. Why should we add another enum when the same can be
> expressed with enum unit_move_type, enum unit_flag_id and enum
> goto_move_restriction which are used throughout the code?

To use a clear-cut unified switch inside the built-in costfn instead of 
if else if else if not.  But it isn't critical.

> >      /*
> > -     * Callback which can be used to provide extra costs depending on
> > -     * the tile and the moves left of the unit. Can be NULL. Parameter
> > -     * are x, y, known, moves left and user_data2. An implementation
> > -     * of path_finding.h may or may not cache this value.
> > +   * Another extra_cost callback, see above for comments.
> >       */
> 
> Why have you removed the comment? What is the extra int?

It's identical with the previous one.  What extra int?  Oops.  I see.  My 
apologies.

> > -      /*
> > -       * x, y, remaining_move_points and turn describe a position in
> > -       * space and time.
> > -       */
> > -    int x, y, remaining_move_points;
> > -    int turn[NUM_TURNS];
> > -    enum direction8 direction_for_next_position;
> > -  } positions[MAX_PATH_LENGTH + 1];
> > +  struct pf_position positions[MAX_PATH_LENGTH + 1];
> 
> No you misunderstood. pf_position isn't an opaque type. Where should
> the caller else gets these informations? There is probably another

From wrappers and access functions?

> internal type which is used to fill the pf_positions. The internal
> type for example will also have an extra field to avoid rail road
> loops.

> >  /*
> >   * Takes a "struct pf_path *" and returns the last position as a
> >   * "struct pf_position *".
> > + * Should not be needed -- GB.
> > + */
> > +struct pf_position *last_position_on_path(struct pf_path *);
> 
> Macro -> function: ok. The comment: no. Trust me this saves some writing.

okay.


> > +/*
> > + * Returns coordinates of the position in the supplied variables.
> > + */
> > +void pf_get_coords_of_position(int *x, int *y, struct pf_position *);
> 
> Obsolete if pf_position is public.

yes.

> > +/*
> > + * Returns number of turns used and number of moves left after reaching
> > + * the position according to the turn_mode.  
> > + * NB: No ORing of turn_mode allowed!
> > + * map should be provided for it contains the parameters.
> >   */
> > -#define LAST_POSITION(m_p) (&((m_p)->positions[(m_p)->positions_used-1]))
> > +void pf_get_turn_info_of_position(int *turns, int *moves_left, 
> > +                                  enum num_turn turn_mode,
> > +                                  pf_map_t map,
> > +                                  struct pf_position *);
> 
> Obsolete if pf_position is public.

I think we should store total_moves_expended or equivalent, then this is a 
useful function (does the division).

> >  /*
> > - * The next best path is written in the given struct pf_path. Caller
> > - * should test path->found_a_valid. Behavior is undefined after
> > + * The "non-destructive" version of the previous function.  Will not 
> > perform
> > + * any computations but if the path is ready, it will return it.
> > + * The path will be written in the given struct
> > + * pf_path. The caller should test path->found_valid.
> > + */
> > +void pf_get_path_from_map_if_ready(pf_map_t map, int dest_x, int dest_y,
> > +                                   struct pf_path *path);
> 
> As I pointed out we don't need these two versions. And even if we do
> we want to add a bool arg instead of another function.

Bool is fine.

> > +/*======================================================================*/
> > +/*===================== Hidden structures ==============================*/
> > +/*======================================================================*/
> > +
> > +struct pf_position {
> > +  int x, y;
> > +  int moves_used[NUM_TURNS];
> > +  enum direction8 came_from;
> 
> > +  enum direction8 went_to;
> 
> You can't do this.

Why not?  But I probably don't need this...

> > +};
> > +
> > +/*
> > + * Anything else goes here?
> > + */
> > +struct pf_map {
> > +  struct pf_position *the_map;
> > +  struct pf_parameter *parameters;
> > +  struct pf_queue *queue;
> > +}
> > +
> > +/*
> > + * Need to agree upon
> 
> No. All of this section should get removed.

Excuse me, this should go somewhere.  It shouldn't go into path_finding.h 
but probably path_finding.c  So why don't we discuss it now?  Imagine that 
everything below 
/*==============================================*/
is getting to be stowed away in some other file.

G.