[freeciv-ai] Re: [Freeciv-Dev] Re: (PR#3646) freeciv-ai ai_military_findvictim

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

On Mon, 24 Mar 2003, Per I. Mathisen wrote:

> On Mon, 24 Mar 2003, Gregory Berkolaiko wrote:
> > Attached (if I won't forget) is a proof-of-concept patch which makes
> > ai_military_rampage look beyond adjacent tiles.
> 
> Very nice. And lots of juicy stuff in there to argue endlessly about.

Hehe :)

Thanks for comments btw!

> I like the removing the shall-we-really-do-it check from the rampage want
> function (which you cleverly renamed it to).
> 
> --2*MORT*TRADE_WEIGHTING
> -         means nothing found and punit causing unhappiness
> 
> Yes. Good riddance to this crap. Wrong place for this check anyway. Was it
> (a negative value) ever used?

Never.

> +      /* See description of kill_desire() about this variables. */
> 
> "these variables"

Yes, it's Pasky's mistake ;)

> +        /* FIXME? Why we don't use stack_size as victim_count? --pasky */
> 
> Yes, why don't we, Greg?

Indeed... (urgently trying to come up with reasons)...
One reason is that I believe that victim_count comes into the want 
equation in a wrong way.  But the real reason is that I didn't want to 
change it yesterday, I just wanted to have it working first and even that 
took 3 hours.

> +      if (map_has_special(x, y, S_HUT)
>            && !is_barbarian(unit_owner(punit))
>            && punit->ai.ai_role != AIUNIT_ESCORT
> -          && punit->ai.charge == BODYGUARD_NONE /* Above line doesn't seem 
> to work. :( */
> +          /* Above line doesn't seem to work. :( */
> +          && punit->ai.charge == BODYGUARD_NONE
>            && punit->ai.ai_role != AIUNIT_DEFEND_HOME) {
> 
> I think (no, I am confident) that the above comment is no longer valid.
> Checking for BODYGUARD_NONE here is wrong. We have it set to
> BODYGUARD_WANTED for a long time, during which we _do_ want to rampage.
> 
> Also, it might be that a unit which is "escorting" a city should rampage.
> So maybe we should do a stricter check here, that checks if we are indeed
> stacked with our charge unit.

My attitude to these checks is expressed by
        /* TODO: All these bodyguard checks don't belong here! */
below.  I don't know why I didn't remove them ;)

> But the intended game logic of the code is correct: A bodyguard should not
> break ranks to take a hut. This check _does_ belong here.

I disagree.  The calling code should decide on the conditions of breaking 
ranks and pass it as the threshold.  The ultimate form of ai_mil_rampage 
should be like this:

ai_mil_rampage(punit, thresh_no_move, thresh_move)

where thresh_no_move is the threshold for actions which will result in 
the unit ending up where it is (like attack neighbouring square) and 
thresh_move, well, is self-explanotary.

Then if unit is a bodyguard on duty, it should call
        ai_mil_rampage(punit, 100, 99999)
meaning "we will move _only_ to pick up a free city but we are happy to 
attack adjacent squares as long as they are worthy of it".

Also, I am planning symbolic names for RAMPAGE_FREE_CITY and RAMPAGE_HUT

> +/*************************************************************************
> +  Look for worthy targets within one-turn horizon.
> 
> Nitpick: "within a one-turn"

And this is my mistake :)

> +static int find_rampage_target(struct unit *punit, int *x, int *y,
> +                               struct pf_path **path)
> +{
> +  struct pf_map *map;
> +  struct pf_parameter parameter;
> 
> I hope we can use a unit path iterator here. If the current iterator does
> not expose enough pf innards, then the iterator should be made more
> flexible.

My iterator used different map setup (attack costs + ZOC).  I don't think 
it would be a wide-spread combination.

>  /*************************************************************************
> +  Put a path-executing function here
> +*************************************************************************/
> +static bool ai_unit_execute_path(struct unit *punit, struct pf_path *path)
> 
> Hmmmmm. This is a bad hack. We should aim to duplicate all of
> do_unit_goto() for new pf, and your name and placement for this new
> creature is a good one. It should _not_ go into gotohand.c.

This function was a last-minute hack.  And it should go to aitools 
eventually.

> But first we need to think hard about caching paths for later use. Can we
> indiscriminately use eg punit->ai.pf_path for this?

Why would we want to cache paths?  Please give me an example.

> +/*************************************************************************
> +  Find and kill anything reachable within this turn and worth more than
> +  the given threshold until we have run out of juicy targets or movement.
>    Wraps ai_military_findvictim().
> 
> You mean: "Wraps ai_rampage_want()."

Mmmh.  There should be a "-" in front of this line ;)

Other things that came to my attention:

1. There should be a version of "can_unit_attack_tile" function in common, 
summarising the rules of engagement.

2. It would be good to enforce "transitivity" of alliances: "if A allied 
to B and B allied to C, then A is allied to C".  Or, at least "if A allied
to B and B allied to C, then A cannot be at war with C".

G.