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

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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.

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?

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

"these variables"

+        /* FIXME? Why we don't use stack_size as victim_count? --pasky */

Yes, why don't we, Greg?

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

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.

+/*************************************************************************
+  Look for worthy targets within one-turn horizon.

Nitpick: "within a one-turn"

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

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

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

+/*************************************************************************
+  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()."

  - Per