[Freeciv-Dev] Re: (PR#7131) client orders to replace client goto

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7131 >

On Fri, Jan 09, 2004 at 10:48:04AM -0800, Jason Short wrote:
> +    if (punit->has_orders) {

> +       && !unit_has_orders(punit_focus)

Please use only one version.

> +  p.dest_x = punit->x;
> +  p.dest_y = punit->y;
> +  send_packet_unit_orders(&aconnection, &p);

What is the purpose of the dest_* fields?

> +  punit->has_orders = packet->has_orders;
> +  punit->orders.repeat = packet->repeat;
> +  punit->orders.vigilant = packet->vigilant;

The has_orders field could be moved into the orders struct.

[various menu.c files]

Setting them to constant TRUE is useless. Menu-items are enabled by
default.

> +struct unit_order {
> +  enum unit_orders order;

> +  enum direction8 dir;

Add "/* only valid for ORDERS_MOVE */"

> +/* If this enum is changed add a manditory capability. */
> +enum unit_orders {
> +  ORDERS_MOVE, ORDERS_WAIT,
> +};

I'm preferring singular here: enum unit_order and ORDER_MOVE

I would also rename ORDER_WAIT to ORDER_FINISH_TURN. "Wait" is too
general.

> +  bool has_orders;
> +  struct {
> +    int length, index;       /* server only */
> +    bool repeat;     /* The path is to be repeated on completion. */
> +    bool vigilant;   /* Orders should be cleared if an enemy is met. */
> +    struct unit_order *list; /* server only */
> +  } orders;

Isn't the whole "orders" struct server-only?

[snip a lot of lines for savegame.c]

Note to self: think about a generator for savegame.c.

> +    /* Set activity to sentry if boarding a ship.  It's safe to do this even
> +     * if the unit has orders. */

Really?

> +/*************************************************************************
> +  Executes a unit's orders stored in punit->orders.  The unit is put on idle
> +  if an action fails or if "patrol" is set an an enemy unit is encountered.

anD an

> +
> +  If the orders are repeating the loop starts over at the beginning once it
> +  completes.  To avoid infinite loops on railroad we stop for this

> +  turn when the unit is back where it started, eben if it have moves left.

eVen

> +  A unit will attack under orders only on its final action.

I don't understand and also don't see this in the code.

> +****************************************************************************/

> +    case ORDERS_MOVE:
> +      /* Move unit */
> +      if (!MAPSTEP(dest_x, dest_y, punit->x, punit->y,
> +                punit->orders.list[punit->orders.index].dir)) {
> +     return GR_FAILED;
> +      }

You have to check the "dir" somewhere. Otherwise you get a nice core
dump. Either at the time the packet is received or here.

All in all is the patch "only" a conversion of the client-side goto
routes to client-side orders. No new functionality.

Something which the old goto routes lacked and also this patches lacks
is a client feedback. I.e. the client doesn't know the orders which
has been sent 3 turns ago. Without this you can't show the orders on
the map. I think this is a useful feature.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Are you saying that you actually used the Classpath Java AWT classes in 
  addition to the GTK peers and got them to display something?
  Wow.  That's way better than I did and I wrote the code!"
    -- Aaron M. Renn in the classpath mailing list