[freeciv-ai] Re: patch/rfc: goto and move/attack test wrapping

[Gregory Berkolaiko <Gregory.Berkolaiko@xxxxxxxxxxxx>]

Well, I took some time to read it and it seems I like it not.

On Mon, 18 Nov 2002, Per I. Mathisen wrote:

> gothere is used for goto over several turns and goto with ferries, and is
> the one which should eventually be expanded into a replacement for
> military and settler gothere with associated ferrying capability. Before
> calling it, always set goto destination explicitly.
> 
> goto is for one-turn gotos, for example run off and attack a target of
> opportunity. It will _retain_ existing goto destination and role, ie it is
> non-destructive for existing missions.

The above separation is very good.  The implementation is somewhat lacking 
but I would like to think it's only temporary.  Should put the above 
explanations into the code and maybe into the AI_hacking_guide or whatever 
it is called.  Maybe more descriptive names, if you can come up with 
something (I cannot).

> can_move is a wrapper which enables you to test for movement _without
> going ACTIVITY_IDLE first_. This way, if you decide not to move after all,
> your fortify status remains unchanged.
> 
> can_attack is the same for attack tests.

See below.

> Index: ai/aitools.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
> retrieving revision 1.62
> diff -u -r1.62 aitools.c
> --- ai/aitools.c        2002/11/14 09:14:50     1.62
> +++ ai/aitools.c        2002/11/18 13:43:50
> @@ -112,18 +112,113 @@
>  }
> +
> +/**************************************************************************
> +  Go to specified destination but do not disturb existing role or activity
> +  and do not clear the role's destination. Return FALSE iff we died.
> +
> +  FIXME: add some logging functionality to replace GOTO_LOG()
> +**************************************************************************/
> +bool ai_unit_goto(struct unit *punit, int x, int y)
> +{
> +  enum goto_result result;
> +  int oldx = punit->goto_dest_x, oldy = punit->goto_dest_y;
> +  enum unit_activity activity = punit->activity;
> +
> +  /* log error on same_pos with punit->x|y */
> +  punit->goto_dest_x = x;
> +  punit->goto_dest_y = y;
> +  ai_unit_new_activity(punit, ACTIVITY_GOTO);
> +  result = do_unit_goto(punit, GOTO_MOVE_ANY, FALSE);
> +  ai_unit_new_activity(punit, activity);

You will segfault here if the unit died.  Or am I wrong in assuming that 
unit structure is destroyed upon death?

> +  punit->goto_dest_x = oldx;
> +  punit->goto_dest_y = oldy;
> +
> +  return (result != GR_DIED);
> +}
> +
> +/**************************************************************************
> +  Check if unit can move to (x, y) without lifting current activity.
> +**************************************************************************/
> +bool ai_unit_can_move(struct unit *punit, int x, int y)
> +{
> +  enum unit_activity activity = punit->activity;
> +  bool can_move;
> +  bool igzoc = unit_flag(punit, F_IGZOC);
> +
> +  punit->activity = ACTIVITY_IDLE;
> +  can_move = can_unit_move_to_tile(punit, x, y, igzoc);
> +  punit->activity = activity;

Direct setting of activity should not be done!!

Also, have a look at can_unit_move_to_tile (which is actually 
test_unit_move_to_tile).  The problem with activity is number 1.  It 
returns MR_BAD_ACTIVITY.  Now _no_code_ever_checks_for_ MR_BAD_ACTIVITY.
This is a somewhat redundant check.  Actually other checks look suspicious 
too, like check number 4 is probably conflicting with number 8.

Anyway, I think the right solution for the problem is to boldly remove 
check number 1 -- it makes little sense anyway.

> +/**************************************************************************
> +  Check if unit can attack to (x, y) without lifting current activity.
> +  This also involves checking if we can move there!
> +**************************************************************************/
> +bool ai_unit_can_attack(struct unit *patt, int x, int y)
> +{
> +  enum unit_activity activity = patt->activity;
> +  bool can_attack;
> +
> +  patt->activity = ACTIVITY_IDLE;
> +  can_attack = can_unit_attack_tile(patt, x, y);
> +  patt->activity = activity;

I read through can_unit_attack_tile and it's sub-functions and I see no 
place where activity is actually checked for.  This makes the function 
useless.

G.