[Freeciv-Dev] Re: Cleaning up gotohand.c (PR#1068)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Nov 30, 2001 at 04:22:41PM +0000, Gregory Berkolaiko wrote:
>  --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> > > -  /* FIXME: Should this apply only to F_CITIES units? -- jjm */
> > > +  /* FIXME: Should this apply only to F_CITIES units? -- jjm 
> > > +   * I don't think so, why should engineers go too far? -- GB */
> > >    if (punit
> > >        && unit_flag(punit, F_SETTLERS)
> > 
> > There is a comment refering to F_CITIES. You change it. But the code
> > two lines below referer to F_SETTLERS.
> 
> JJM says: I think the below should only apply to F_CITIES units, not to
> F_SETTLERS.
> I say: I think it should apply to F_SETTLERS
> Now I think it should apply to both F_SETTLERS and F_CITIES (I studied
> the distinction a bit more).
> 
> I can remove my comment if you want

The section in question:  
  /* FIXME: Should this apply only to F_CITIES units? -- jjm */
  if (punit
      && unit_flag(punit, F_SETTLERS)
      && unit_type(punit)->move_rate==3)
    maxcost /= 2;
  /* (?) was punit->type == U_SETTLERS -- dwp */

So why is the range be restricted for units which can do terrain
transformations? Digging in the cvs reveals that this code was there
in the first version (1.7) of the "new" warmap. So what is the reason
for this?

> > > -   /* NOT c = 1 (Syela) [why not? - Thue] */
> > > -   move_cost = (ptile->move_cost[dir] ? SINGLE_MOVE : 0);
> > > +   /* NOT move_cost = MOVE_COST_ROAD (Syela) [why not? - Thue] 
> > > +    * possibly because if an igter unit reuses warmap of a city 
> > > +    * (see generate_warmap), it won't be THAT much different 
> > > +    * from the correct warmap.  For consistency we should have 
> > > +    * SINGLE_MOVE for disembarking igter unit too -- GB */
> > 
> > No. Please don't write novels in comments. Especially if they start
> > with "possibly".
> 
> This is a question ("why not?") to which no one so far could give _any_
> answer.  I formulate a hypothesis which I think should be mentioned
> somewhere.  If not code than where?
> 
> As for "possibly", I didn't write the code, so I don't know for sure.
> Sorry.

IMHO such a comment doesn't help. If the code is complex and the
author has to document it. If this doesn't happen either another
person understand the code and writes the comment or the code is
itself is most accurate description.

> If you have a better idea as to why an IGTER move costs SINGLE_MOVE and
> not MOVE_COST_ROAD on the warmap, please enlighten us.

I have no answer. This and some other places where I only could say
"this is an example of a way to not do this" let me conclude that I
should write my own goto code.

People (all of you) take a step back. Do you really think that you
transform this (primary gotohand.c) into something which will be a
pleasure to read? Isn't your effort better spend in encapsulation the
interface of gotohand.c and write a second implementation learning
from the first version.

But it is your time (and to some smaller degree mine).

> > >  static int goto_zoc_ok(struct unit *punit, int src_x, int src_y,
> > > -                int dest_x, int dest_y)
> > > +                int dest_x, int dest_y, unsigned char came_from)
> > 
> > Directions are int.
> 
> this is not a direction.
> here you plug an element from
> unsigned char *vector[MAP_MAX_WIDTH];
> from struct move_cost_map.

And what does such an element describe?

> why are directions int?

Ok they aren't really ints. But ints are used throughout the code to
represent ints. I would also like a seperate type but the
effort/benefit ratio is too bad to do such a change.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "With a PC, I always felt limited by the software available.
   On Unix, I am limited by my knowledge."
    -- Peter J. Schoenster <pschon@xxxxxxxxxxxxxxxxx>