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

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

Some quick minor nits or suggestions.

Cheers,
RossW
=====

At 01:59 PM 02/11/18 +0000, Per I. Mathisen wrote:
>
>Taken from massive ai. Unless I get disagreeing comments, I will commit
>these wrappers and the associated changes in the rest of the code.
>
>This is an important API decision so please consider it carefully.
>
>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.
>
>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.
>
>(The two latter are necessary but not sufficient to avoid the AI's
>always-break fortification syndrome.)
>
>  - Per
>
>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 @@
> }
>
>
>/**************************************************************************
>+  This will eventually become the ferry-enabled goto. For now, it just
>+  wraps ai_unit_goto()

I'm tempted to think you got it reversed.
The wrapper should check and assign the dest_xy values.
This should be the execute with minimal dest_xy sanity check.

>+**************************************************************************/
>+bool ai_unit_gothere(struct unit *punit)

Return a valid move status, bool hides too much. 

>+{
>+  assert(valid_goto(punit));

Assert is bad. Verify 
0) the location is valid, if not log error and return GR_FAILED
1) punit is not at the location, return appropriate GR_ARRIVED
2) try and the return status.

One and 2 can be merged if you don't want a special longterm move
GR_status to be returned, and the short move is not an error.

>+  if (ai_unit_goto(punit, punit->goto_dest_x, punit->goto_dest_y)) {
>+    return TRUE; /* ... and survived */
>+  } else {
>+    return FALSE; /* we died */
>+  }
>+}
>+
>+/**************************************************************************
>+  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)

bool is still bad

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

is the error log missing???

>+  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);
>+  punit->goto_dest_x = oldx;
>+  punit->goto_dest_y = oldy;
>+
>+  return (result != GR_DIED);

Very bad. there is no point in describing return conditions if you
always hide them. Make wrapper functions that check collective result
conditions if it becomes too much to do the correct thing at each call.

The wrappers *just* do the special-case if-check massaging as in,
  if (wrapper_cond(ai_unit_goto(punit)))
    do_wrapper_ops;

Do not hardwire functionality together when it can be easily modularized
and there is *any* reason for wanting to do the sub-operations in an
independent manner, or at least access the intermediate results.

>+}
>+
>+/**************************************************************************
>+  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;

There is something wrong with this. The lower level function should
*never* destroy state. Push the wrapper consitions down to the 
appropriate level.

>+
>+  return can_move;
>+}
>+
>+/**************************************************************************
>+  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;

Ditto with level of wrapper.

>+  return can_attack;
>+}