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

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

On Sat, 23 Nov 2002, Ross W. Wetmore wrote:
> >/**************************************************************************
> >+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.

The way I'm heading, goto destination is set at the same time as you set
your mission by calling ai_unit_new_role(punit, role, x, y). I used to
have whether we have goto or not decided by a macro called valid_goto(),
but I think this should be dependent on our role instead. So certain roles
always have a valid goto, and others don't.

> >+**************************************************************************/
> >+bool ai_unit_gothere(struct unit *punit)
>
> Return a valid move status, bool hides too much.

That is where I started out. I turned out, however, that the code never
used this extra information, and when I tried to make it use it, the
benefits were negligible and the resulting code ugly, since all the checks
had to be made while the result variable was still guaranteed to be valid
and in the right scope. Instead, explicit checks like punit->moves_left >
0 or same_pos(x1, y1, x2, y2) were much better and allow better code flow.

> >+{
> >+assert(valid_goto(punit));
>
> Assert is bad.

This shouldn't be there.

> >+/* log error on same_pos with punit->x|y */
>
> is the error log missing???

Not really. I don't want to add it before some pathological cases of it
are removed, so that users aren't spammed with lots of stupid LOG_ERROR
messages.

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

I don't understand. No state is destroyed. It is just that
can_unit_move_to_tile() checks if we are idle, since we must be idle to
move. But since we want to know if we _could_ move _if_ we were idle, we
fake it.

  - Per