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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 05:44 PM 01/10/05 +0100, Gregory Berkolaiko wrote:
>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.

Not quite :-).  Allow me to restate it again for the record ...

I certainly do not view everything as a monolithic code unit and have no 
problem splitting things up into smaller pieces when it is time to do 
the final code review and patches to put things into CVS.

I don't think it is worthwhile when there is still so much wrangling going 
on especially over cosmetic or inconsequential aspects and no one actually
looks at or comments constructively on any real code changes :-).

When there is serious agreement on what core cleanups are desired and 
what parts are needed next, then I'll be happy to make the appropriate 
splits and patches. When I do, I expect when an agreed patch (or patch 
set for those that need things further subdivided to match some personal 
quirks) is prepared, tested and submitted, it will receive a final 
professional review, have final updates where required and either be 
applied or rejected as a whole without unprofessional last minute tinkering. 

I was *very* unhappy at the number of bugs that Raimar introduced
into the autogame patch, especially as he never contacted me about any
of the changes. As you may have noticed, I find a number of problems in 
Raimar's code, and there are a lot of unnecessary fixes following many of
his checkins. I do not want him introducing bugs into something with 
my name attached to it without at least asking my permission or review.
In short, I want him to act professionally in handling my patches.

And if you notice, Raimar doesn't like working with anything but the
smallest of bug fixes or purely cosmetic changes. With the level of
resistance and unnecessary wrangling for anything significant there is 
no real agreement on any worthwhile part of the larger set yet. 

I really don't see this changing without some major shakeup in the way 
Freeciv maintainers handle things. It needs to move from a despotic 
"Boss" mode to a state where there is a Magna Carta document or Code of 
Laws that sets out appropriate rules and responsibilities for both
submitters *and* maintainers. Trent pointed out a number of contributors
whose names have been blacklisted and scrubbed from Freeciv, or whose
code has been rejected only to reappear a little later in buggy mutated
form. This is *very* unprofessional, but typical of an immature system.

So my position is, one needs to look at things first, a decision made
on what to attack (in a reasonable == comprehensive area) and then we can 
move to the "final" stages of making patch submissions for CVS. 
Meanwhile to look at changes made in the corecleanup devpath you should 
run it in the corecleanup development environment, not the CVS environment. 
Pick the things to move over from there, then go to work on them for CVS.

The current maintainer system is very much broken. It needs changing to
handle things more professionally and efficiently.

>I agree with you in some parts and most loudly disagree in other parts.
>
>Comments:
>whether your patch is harmonic amalgamation or "bastard" amalgamation it
>is a huge amalgamation and will never be applied in full, I think.

The development update was never expected to be applied the CVS. This is 
the snapshot of the corecleanup development path and as such is in a 
single unit to make application as easy as possible.

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

There is no point developing code in a buggy environment when the fixes
are available :-).

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

There is no point developing code in a buggy environment when the fixes
are available :-).

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

Thanks.

Maybe you should have a go at convincing Raimar those you like are 
useful, so at least a few parts of this logjam get unblocked. 

>But I think your
>mistake is that instead of trying to get your good changes in, you pile
>up more and more changes.

There has been a lot of effort put into getting changes in. 

Raimar explicitly rejected most of the earlier corecleanup patches after
initially starting to apply them. He gave no reason for most other than
he refused to look at them.

If he refuses to look at or identify any further corecleanup elements he 
is willing to consider, this process is effectively blocked under the 
current system.

There was a two month discussion on "straightest_direction()" starting 
with Raimar removing it from the patch he was introducing to CVS. The
reasons at the time were it was ugly, and he didn't understand it. After
trying just about every other alternative until it was finally understood,
and he had made a complete fool of himself, his solution was to reject the 
whole routine in favour of something completely different rather than 
backup and apply the changes.

The bugfix to find_city_beach() is still not in in spite of coming up
for discussion twice and being a 2 character fix.

Jason's incremental corecleanup patches are not really faring much better
and they are of the same several month old vintage. One of the reasons is
all the new riders that tend to get tacked on everytime the patch cycles.

I think you "might" be under some misconceptions of the history, or at
least alternate views of it :-).

>[..]
>> 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.

Thanks again. Though this is what should be done for any patch initially.

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

So you are saying one of the first "splits" you would like to see is a
pure reformat? This is certainly a legitimate comment and request.

Do you think there is any chance of this actually being accepted as a 
CVS patch? If there is agreement that this is desired it is not difficult
to do.

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

This is what you should do first. AI code by itself is not particularly
understandable. It is almost impossible to predict what one local block 
will do. But it is possible to work backwards from a perceived behaviour
to see why it comes about. You need to do this to work into the code.

I'm sorry this was not made more clear ... I tried, really.

>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 

Sorry, the "your" is a bit of a misnomer. Once you kick the AI out of
despotic mode where it builds nothing but small cities and military units
you get these sorts of things. The capitalization stuff should probably be 
looked at to see why, and whatever effects introduced to rebalance things.

I suspect, that "tweaking" the AI to fix this might be one of the things
that would help the tweaker better understand the (original) code.

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

Yes, the current AI doesn't really have too many personality variables
or things that alter the hardcoded balance to make for flexible or more
varied play. 

>So I wouldn't like this patch to go in
>as a whole.  

No one is yet talking about putting this into CVS as a patch let alone a
whole patch ... unless you are setting up a strawman to beat on :-).

>Maybe there are parts which are very sensible (like wounded
>units going back), but it seems I will never find out.

Now this is where I think you are being quite wrongheaded. The goal of
development branches is to understand the system, find out what works 
and what doesn't and why.

Then you decide what needs doing and go do it. When you are done you
come back and start feeding changes to the mainline CVS.

Expecting a finished product at the first snapshot is a bit unreasonable.
And deciding that you won't look at it further because you didn't like 
some particular aspect that may not be particularly important or even
part of the work done is not what I expect from a mature analysis.

>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 

Now, you get my point about not really being able to work with the current
CVS. If you can't read the code, understanding becomes a lot more difficult.
And it really doesn't matter if you have a nice tiny diff of a few local
changes, you will still be lost.

You really needed to watch it before you could start to come up with any
insightful comments, right?

>(I think you must be very familiar with the code
>now if you manage to convert aggressive expansionist into civilised
>perfectionist). 

Actually not really. Once you block or rebias certain behaviours there
is no telling what you might get. It wasn't as if I planned to make a
civilized perfectionist, or even that it actually stays that way if you
tweak most any random thing, but the fact that the original code is 
pretty hardcoded to force it to republic probably pushes this. The other 
aspect is when enemy units aren't wandering around looking for things to 
kill, and then suiciding and your units have some stickiness to not wander 
off and leave their cities undefended, then a lot of the "danger" factors 
to force it into building tons of units are reduced, so it builds more 
improvements and falls into a different metastable state.

I have chased down and killed half a dozen bugs and behaviours now. The 
bugs turn up when you kick it into new code it doesn't normally run. But 
I find myself starting pretty much from square one each time :-).

The only trick is I have now touched a lot of code, so the "allowed" 
reformatting of things you touch is getting more complete along with
some understanding (at least limited understanding).

> I can do the first two tasks which are pretty basic and
>you do the third, how about it?

Sure. I'm game for doing anything to improve the code. And if there is 
someone actually willing to *do* things in a shared team approach it might
even make me wildly enthusiastic :-).

But you have to realize that this probably won't be work that gets into
CVS under the current system, and if you start this from CVS, then you
will be starting another development branch.

And actually, for the first little while it should stay out of CVS until
the shape of things is a little clearer.

>Then we need to develop and document a consistent structure for AI. 

I'm still back at the level of 1) hack it, 2) fix what breaks, 3) try
to generalize the code, and consolidate the "n" different places that you
found doing part of or all of the behaviour that was fixed.

I suspect this will go on for a while until 1) the code is much more
compact, free of bugs and the interfaces clearer, 2) a milestone is 
reached where it is possible to do some major advance like the MAP_WRAP
changes by recognizing the underlying structure and doing a significant
generalization of it, 3) the level of understanding reaches the point
where a real top-down structure can be designed and documented.

3) has almost no chance of getting into Freeciv, unless the code is
pretty much realigned in advance, or you get "maintainer" priveleges
and a modicum of acquiescence to proceed, i.e. become one of the "in"
group.

Stage 2) is actually where the MAP_WRAP stuff is sitting now, except
that CVS has not yet gone through stage 1). This might put things in
perspective.

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

Yes!!!

You really don't know just how much that comment means to me, or tells me 
how far you have come.

>Then we can set about really reworking the code.

Yes!!! We are in sync.

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

I think your optimism is a little unfounded here. This certainly is not 
the typical direction things usually go.

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

I know. I just wish I had as much luck in getting through to the core
maintainers as with some others :-).

But you should also realize it is the Freeciv core group that should want 
that.

For me it is just the additional effort of tracking CVS that provides
my incentive. I have the changes and can work at my own speed to add to
them without needing CVS. Eventually this will become too great and CVS
will fall sufficiently far behind that I will just have another splinter
variant and the ongoing interaction will cease. Some probably are wishing
for this day, rather than trying to take advantage of opportunities :-).

I could also wish for more patience and a better tolerance for the inane
or stupid. But I am into the "grumpy old men" stage of life, so there is
less incentive or flexibility to change my basic character all that much 
:-).

>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

Cheers,
RossW
=====