[Freeciv-Dev] Re: Changing interface for generate_warmap (PR#1108)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

 --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> On Sun, Dec 16, 2001 at 11:20:08AM +0000, Gregory Berkolaiko wrote:
> >  --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> > > >
> > > > void cost_map_generate(struct move_cost_map *p, 
> > > >                        int x, int y, struct unit *punit);
> > > 
> > > What are the semantics of this? When is punit used, when x,y?
> > 
> > (x,y) give the starting point of the warmap
> > punit is used to deduce the move type characteristics like
> > 1. Sea / land
> > 2. Igter /normal
> > 3. Number of move-points.
> > punit doesn't have to be at (x,y).
> > punit == NULL  means "city"-type warmap will be generated (it is not
> > necessary to have a city at (x,y)).
> 
> What do you think about changing this to:
> 
> void cost_map_generate(struct move_cost_map *p,
>                        int x, int y, struct unit_type *ptype, 
>                        int move_left, 
>                        int fuel,struct player *owner);
> ?

There are pluses and minuses to it.
+: you don't have to have a unit, just know the type.
-: this situation is adequately handled by use of city warmap
-: there is no immediate necessity for such interface
-: this interface is overly heavy, user has to think about all these
variables to pass.  why not just pass unit *, it knows it all?
-: the flexibility is illusory: what if cost_map generation is changed
to, say, depend on units HPs (and it should)?

a compromise would be
void cost_map_generate(struct move_cost_map *p, 
                       int x, int y, struct unit *punit,
                       struct unit_type *ptype);

where you can either pass unit or unit's type (virtual unit) or neither
(city-type map)

[...]

> > > > To make reviewing easier I can split the patch into parts:
> > > > 1. Introduction of "access functions". 
> > > 
> > > In the header file only or do you want to change all "warmap" uses?
> > 
> > Change all warmap uses.  I can split it into ("introduction of access
> > function" and "changing all warmap uses" parts but that is a bit
> > artificial IMO).
> 
> I don't like this. You change every access and than will touch these
> lines a second time if you change them to use a non-global
> cost_map. This is unnecessary IMHO. Only change the accesses which you
> transform to the non-global case.

I am very strongly against it.  If we change accesses one-by-one, here is
how it will go:
* there will be two parallel ways to access default warmap
* changing from default to local warmap is a lot of work (although the
experiment you did will help _very_much_), and it will take ages (about a
year I would guess) due to sheer slowness of review/commit process.
* it is not unprobable that it will be never fully finished.  then we
will be left with parallel interfaces, one half-wrong and one
completely-wrong.

There is no tragedy in touching the same line twice, provided each touch
is a consistent improvement.  As I understand you, you want to rewrite
each line perfectly once and for all.  This is a noble aim, but it is not
within our reach.

G.


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com