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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [i-freeciv-lists@xxxxxxxxxxxxx - Sat Jan 10 14:30:24 2004]:
> 
> 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.

Yes.

> > +  p.dest_x = punit->x;
> > +  p.dest_y = punit->y;
> > +  send_packet_unit_orders(&aconnection, &p);
> 
> What is the purpose of the dest_* fields?

Well, there's a slight ugliness since the movement orders don't 
include positions.  So it's not particularly easy to recalculate the 
ending position of the orders list.  These values simply provide the 
ending position to the server.  This is needed so the goto_dest may be 
set.

If we were going to calculate the ending position manually at the 
server, these values would still be useful as a safeguard.

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

COuld be, yes.  Is there any advantage to doing so?

> [various menu.c files]
> 
> Setting them to constant TRUE is useless. Menu-items are enabled by
> default.

OK.

> > +struct unit_order {
> > +  enum unit_orders order;
> 
> > +  enum direction8 dir;
> 
> Add "/* only valid for ORDERS_MOVE */"

OK.

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

Fine with me.

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

It corresponds to the "wait" command at the client.

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

No.  repeat and vigilant are sent to the client.  They are used to 
determine whether to show the P or G icon.

Eventually the whole orders list will probably be sent.


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

I believe so.  Although, I have played a fair amount with the orders 
patch and never actually tested it.

> > +  A unit will attack under orders only on its final action.
> 
> I don't understand and also don't see this in the code.

I will double-check.

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

Ugh; yes.  Other values may need to be checked as well.

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

Yes.

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

Yes.  But it won't be a part of this patch.

To do it would be simple: simply send the orders list back to the 
client.  Changes to the drawing code to draw the orders are a little 
more complicated.  Deciding how the user interface would work is 
probably the biggest problem.  I think clicking on a unit should not 
cancel the orders, but rather activate the unit and cause the existing 
orders to be displayed.

jason