[Freeciv-Dev] AI - cleaning (Was: Re: [UPDATE] Corecleanup_08 patch)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

First a short summary of your position as I see it and then some comments
on different issues.

You feel that your work is an integral whole which should be considered
as such and you shall not bother to split it up so that it can be easily
reviewed and even applied because it is easier to review it as it is.

I agree with you in some parts and most loudly disagree in other parts.

Comments:

 --- "Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx> wrote: 
> I understand your hope that things might be easier to manage in smaller
> chunks, but I think that you are fooling yourself. The review is 
> probably actually easier working with a clean consistent version than
> something that is a bastard amalgamation of half-hearted and partial
> changes. 

whether your patch is harmonic amalgamation or "bastard" amalgamation it
is a huge amalgamation and will never be applied in full, I think.

> It is also easier to just look at an updated snapshot vs a current
> CVS in side-by-side views for those things you are interested in, as 
> opposed to reading diffs.

I did that.  See below.

[..]

> It is not productive to have to interpret, deal with or test each 
> artifical division of a consistent set of changes, with all the extra 
> hacks needed to make it work with broken code that is being fixed by 
> a whole series. Certainly not for review and basic understanding which 
> is where the process is usually blocked.

I think division of coordinate-related / AI related is not artificial.

> Whereas, in your case, if you try to play with just the AI code applied
> to a current CVS, then you are neither looking at it in the environment
> it was built and tested in, nor are you likely reviewing the changes
> but just rediscovering already found bugs and problems :-).

I do not think it would be that hard for you to change AI code in a
different evironment, that of a current CVS.  In how many places do you
use your new coordinate/iterate code?  3 or 4.  Was it really
unavoidable?

> And frankly, I really *am* tired of doing yet-another-split everytime 

I understand.  Maybe you can change your development policy an incy-wincy
bit: once you start working on a different area, start from current code,
not the code you have from your previous experiments.

Please understand me correctly.  If it were up to me, your new coordinate
system and especially your new coordinate macros would have been in long
time ago.  Few times I myself actually needed the macros that I saw in
your patch and that are not in the current code.  But I think your
mistake is that instead of trying to get your good changes in, you pile
up more and more changes.

[..]

> It is maybe time to *try* a slightly different approach :-).

I did.  I applied your patch and then compared it in emacs.  It is much
better than attempting to read .diff.  I applied your patch in full, so
it complied, then run the code and also looked at the changes in ai/
directory.

Changes: there are so many inconsequential formatting changes!!!
No, I am 100% for cleaning things up, but not when pure cleaning is
interspersed with some crucial changes.  There is a lot to be cleaned in
ai code: 
1. formatting
2. comments referring to the code which is not there for 3 years
3. whole chunks of code never used

It should be cleaned asap.
But it should not change the executing code!

Then your real changes.  I was lost in the sea of reformatting changes
and actually didn't see that many of them but I run the game in AI mode
and watched.

AI looks civilised.  It builds nice cities not too close, it builds roads
and then it switches all production to coinage and 100% to science and
when done with science it switches 100% to tax and buy a spaceship and
leaves.  I have not seen expansion into other continents, I have not seen
invasions, even small fighting (maybe I did not watch enough).  However I
have seen pirates taking out 1/3 of Canadian cities (selection of nations
somewhat affected by the authorship of the patch) and Canadians just
carrying on with their coinages.

Your make of AI is civilised perfectionist.  No aggression.  Very hard to
compete against (because they build their spaceship so fast) but not
terribly exciting, I would say.  So I wouldn't like this patch to go in
as a whole.  Maybe there are parts which are very sensible (like wounded
units going back), but it seems I will never find out.

May I make a suggestion?  How about mopping the code up first,
consistently reformatting all ai*.[ch] files, removing obsolete comments,
adding some new comments (I think you must be very familiar with the code
now if you manage to convert aggressive expansionist into civilised
perfectionist).  I can do the first two tasks which are pretty basic and
you do the third, how about it?

Then we need to develop and document a consistent structure for AI. 
Right now it's pretty horisontal, very few parameters which are changed
in too many places.  We need to parametrise the behaviour and build it
vertically.

Then we can set about really reworking the code.

And at all stages we will be doing something that is both useful and will
go into the CVS.

Otherwise it is a real pain for me to see so few of your great and clever
ideas to find their way into the code.

Best wishes,
G.

____________________________________________________________
Do You Yahoo!?
Get your free @yahoo.co.uk address at http://mail.yahoo.co.uk
or your free @yahoo.ie address at http://mail.yahoo.ie