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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Dec 16, 2001 at 01:22:42PM +0000, Gregory Berkolaiko wrote:
>  --- 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)?

I disagree here. This shouldn't be added here. To be honest I would
really like to move all the stuff in find_a_direction out of
gotohand.c. IMHO it doesn't belong there.

> 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)

This is worse that the two other ways. Ok let struct unit * be the the
parameter and document all the accessed fields. So that the interface
is documented and the implementation can be exchanged.

> > > > > 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

No. The new way can't access the global 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.

I haven't abandon the hope that there will be an extra maintainer some
day.

> * 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.

Why is one half 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.

If you create something new you have to get an understanding of the
problem domain. This will take some learning, experimenting and
prototypes. In this gotohand.c case however is nothing new. The code
is there and works. You don't need prototypes to recapture the problem
domain. You need time, pencil and paper to trace each warmap
generation (like the id patch of me has made it, but this was a
statistical sampling) and make the passing explicit in the code. I
agree with Ross here that code shouldn't be changed if it isn't
understood. Such understanding could produce for example something
like "there 15 calls of generate_warmap. 4 of these calls are made to
get the nearest ferryboat, 5 are made because....". This would give
the information we need to design the interface. Without such
information I would for example turn down an interface which allows
the overriding of a warmap.

There are 15 calls to generate_warmap which are outside gotohand.c. Do
I ask for too much if I ask you to understand them and give the
information I mentioned above? Also needed are information like "what
function will get a cost_map pointer passed in?". The analysis of
evaluate_city_building was very good IMHO.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The very concept of PNP is a lovely dream that simply does not translate to
  reality. The confusion of manually doing stuff is nothing compared to the
  confusion of computers trying to do stuff and getting it wrong, which they
  gleefully do with great enthusiasm." 
    -- Jinx Tigr in the SDM