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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 09, 2001 at 03:32:15AM -0400, Ross W. Wetmore wrote:
> 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.

Ok I understand and accept this. I will not change patches from
you. However this means more iterations. We may see how this evolves.

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

My guide line is that I understand any code which I apply. You will
agree that it is easier to understand bug fixes or purely cosmetic
changes. Bigger patches aren't a no-no but it will take
longer. Another option is that some people say "Raimar this is
ok". However even than I would require more documentation so that I
understand is easily.

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

It would be helpful if you can come up with a list of what kind of
things such a Magna Carta should contain.

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

I agree. There are IMHO too few RFC and proposals. You may look at
<http://python.sourceforge.net/peps/pep-0001.html>. However I think
PEPs may overkill for freeciv. But something similar. It may be
required that the maintainers explicitly tell the patch author what
should be done to the patch and what would be "nice".

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

Ross I don't time and space to track multiple trees. You should manage
your tree and submit stuff to the CVS tree.

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

Then send the fix to freeciv-dev.

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

Then send the fix to freeciv-dev.

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

It doesn't matter who submits a patch. A patch has to tackle one
problem, be bug free and I should think that it improves freeciv. Yes
the last requirement is a personal thing. But there is IMHO no other
way. You may another maintainer which sees the improvement of a
patch. And yes we need more maintainer.

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

It would be nice if you can give a reference (URL, msgid).

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

I have no problem of making myself a "fool". Atleast in this way. You
and Jason had a deeper knowledge of the method and I slowly started to
get an understanding. The current solution is IMHO optimal. Do you
disagree?

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

Resubmit it. Provide a reference to the old discussion.

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

I don't think Gregory said this.

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

I won't accept a reformat patch (white spaces, return-()) without a
style guide line. Make a style guide and submit a patch which
implement this guide.

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

Ross please do this. Make a list of changes you want to see in the
CVS. But please not in the form of a patch but in human readable form:
 - formatting changes
 - wrapping changes
 - ai changes
   - add docu
   - change behaviour
 - city iteration macro change
 ...

Not I have to do this but you.

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

Summary for me: we need more maintainers.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Of course, someone who knows more about this will correct me if I'm
  wrong, and someone who knows less will correct me if I'm right."
    -- David Palmer (palmer@xxxxxxxxxxxxxxxxxx)