[Freeciv-Dev] Re: (PR#2370) Path finding

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

Quoting Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>:

> On Fri, Feb 21, 2003 at 08:31:10PM +0000, Gregory Berkolaiko wrote:
> > Quoting Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>:
> > > > + *  FORMULAE:  
> > > > + *  - calculating total_MC
> > > > + *  in TM_NONE
> > > > + *    total_MC = sum of MC
> > > > + *  in TM_CAPPED
> > > > + *    total_MC = sum of MIN(MC, move_rate)
> > > > + *  in TM_*_TIME
> > > > + *     total_MC = ((turn + 1) * move_rate - moves_left)
> > > 
> > > I don't know what "in TM" means here.
> > 
> 
> > TM was defined earlier and a reference to a more detailed
> > explanation was given.
> 
> It looks still odd. Can't this be:
> 
> FORMULAE:  
> - calculating total_MC
>    * TM_NONE: total_MC = sum of MC
>    * TM_CAPPED: total_MC = sum of MIN(MC, move_rate)
>    * TM_*_TIME: total_MC = ((turn + 1) * move_rate - moves_left)

Even better.

> > > > +  enum direction8 dir_to_next_pos;     /* Unsed only in struct_path */
> > > 
> > > > +  enum direction8 dir_to_here; /* Where did we come from */
> > > 
> > > This wasn't there the last time. It introduces redundancy. But it can
> > > stay.
> 
> > You are mistaken, there is no redundancy.
> 
> In the path there is redundancy: we have dir_to_next_pos, dir_to_here
> and (x,y). If you have (x_prev_step, y_prev_step) one of the three is
> enough to calculate the others.
> 
> If you only have a single position there is obviously no redundancy.

Yes in the path there is redundancy.  I thought about having just one dir and
then putting different things in it, depending on what is the caller, but
decided that it's not good.

> > > > +   */
> > > > +  int (*get_MC) (int from_x, int from_y, enum direction8 dir,
> > > > +                int to_x, int to_y, void *);
> > > 
> > > We should also supply "known".
> > 
> > When any user code needs it, it will be added.
> 
> Based on this we would have to remove this parameter also from the
> other callbacks. It should be added here and also to is_pos_dangerous.

Definitely add to is_pos_dangerous, I am surprised it's not there yet.
Definitely not here.  I am not having it just for uniformity's sake.  
This is a back-bone of the whole module and +- one byte makes a difference. 
Give me a problem that requires "known" here and cannot be solved by existing
call-backs and then we can negotiate.

> > > Why did you remove get_known? It is required to handicap handling.
> > 
> > Please explain.
> 
> We want to limit the user knowledge of the map via this callback.

But there are standard callbacks?  Why should we pretend it's "known" when it
isn't and vice versa?

> > > > +static int single_seamove(int x, int y, enum direction8 dir,
> > > > +                         int x1, int y1, void *data)
> > > > +{
> > > > +  /* MOVE_COST_FOR_VALID_SEA_STEP means ships can move between */
> > > > +  if (map_get_tile(x, y)->move_cost[dir]
> > > > +      == MOVE_COST_FOR_VALID_SEA_STEP) {
> > > > +    return SINGLE_MOVE;
> > > > +  } else {
> > > > +    return PF_MAXCOST;
> > > > +  }
> > > > +}
> > > 
> > > Now I see why you need PF_MAXCOST. This should be changed. get_MC
> > > should look like this:
> > > 
> > >   bool (*get_MC) (int *dest_MC, int from_x, int from_y, enum direction8
> > > dir,
> > >             int to_x, int to_y, void *);
> > 
> > This is ugly.  PF_MAXCOST is a well-defined special value and it should
> stay.
> > I will have no cruft in the function signature, it's big enough as it is.
> 
> PF_MAXCOST is ugly. Comment from others?

While waiting for comments, I can suggest an alternative: return a negative 
number.

> > > > +  Must put owner into *data.
> > > 
> > > What about also giving the parameters to all callbacks?
> > 
> > They can be supplied through *data fields if needed.
> 
> Yes but the above starts to make exceptions. What about leaving the
> *_data fields and passing the param struct * into the callback instead
> of the _data pointers. This way the callback have both the "normal"
> fields of the parameter and also the special _data fields.

Makes good sense.  Shall we cut down number of *data fields?  If needed *data
can point to a struct with more data.

> > > > +  Alternatively,we can change the flow to
> > > > +  if (ptile->terrain == T_OCEAN) {
> > > > +    move_cost = PF_MAXCOST;
> > > > +  } else if (terrain1 == T_OCEAN) {
> > > > +    move_cost = SINGLE_MOVE;
> > > > +  } else {
> > > > +    move_cost = ptile->move_cost[dir];
> > > > +  }
> > > > +  which will achieve thesame without call-back.
> > > 
> > > Why didn't you done this?
> > 
> > For educational purposes.
> 
> ??

I wanted to have an example of each call-back.  So people have some idea of what
it's about.  Consider it a practical comment.

> > > > +/************************************************************ 
> > > > +  Reversed LAND_MOVE cost function for a unit.
> > > 
> > > > +  Will be used. DO NOT REMOVE you pricks.
> > > 
> > > When will it be used?
> > 
> > After path-finding gets committed.
> 
> Obviously. But why not earlier?

Because you cannot use something which hasn't been committed.

Speaking seriously, the demand will come pretty soon in the guise of
danger-estimating code.  If you really want to be absolute purist, you canremove
it now and then it will be put in later.

> > I have no opinion on that.  The name must not exceed 77 characters in
> total,
> > however.
> 
> There is another limit before this ;)
> 
> 5.2.4.1 Translation limits 
> 
> The implementation shall be able to translate and execute at least one
> program that contains at least one instance of every one of the
> following limits:
> ...
> 
>  - 63 significant initial characters in an internal identifier or a
>  macro name (each universal character name or extended source
>  character is considered a single character)

yeah, I guess they want to leave sufficient space to put at least one variable
type and name... ;)

G.