[Freeciv-Dev] (PR#6931) Returning GR_FAILED in execute_orders doesn't cancel unit orders

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [jdorje - Sun Jan 25 06:56:35 2004]:
> 
> > [ali - Sun Nov 23 16:25:51 2003]:
> > 
> > Bug: maybe_cancel_goto_due_to_enemy() should idle the unit,
> > like in maybe_cancel_patrol_due_to_enemy().
> > 
> > Otherwise, unit retains the goto state when a non-attackable unit
> > unexpectedly exists on a waypoint (only when using waypoint).
> 
> The goto (now orders) returns GR_FAILED.  If this happens then the goto
> should be canceled; the unit will be left in idle.  It looks like the
> goto/orders function expects the caller to do this, but of course the
> caller doesn't.
> 
> Execute_orders should just return a bool indicating whether the unit
> survives.  Any other handling should be done within the function. 
> Perhaps a wrapper function would be helpful (to handle the different
> return values).  If not then the GR_*** enumeration can just be dropped.

Obviously, the goto_result is used for server goto, so it must stay. 
But unit orders are not server goto and do not need any return result. 
The difference is that the calling code is not AI code that needs to
know what happened, but just normal server execution code.

The attached patch:

- Changes the return type of execute_orders from goto_result to bool.
- Changes all callers.
- Whenever GR_FAILED was returned, now we call free_unit_orders and send
the player a notification.

This fixes a whole host of bugs with orders.  These bugs may have been
present in the original goto code; I don't see how it differed from the
current orders code here.

Note that free_unit_orders may be called more than once; this is okay. 
Also note that a number of the failed orders reasons are actually
errors: like a unit trying to move off the map.  Some of these are
possible if the server and client are not in sync.  In any case they get
player notifications just like the common cases.

The notification strings could probably be improved.

jason

Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.285
diff -u -r1.285 unithand.c
--- server/unithand.c   2004/01/23 02:29:32     1.285
+++ server/unithand.c   2004/01/25 07:20:56
@@ -1551,7 +1551,7 @@
 #endif
 
   assign_units_to_transporter(punit, TRUE);
-  if (execute_orders(punit) != GR_DIED) {
+  if (execute_orders(punit)) {
     send_unit_info(NULL, punit);
   }
 }
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.271
diff -u -r1.271 unittools.c
--- server/unittools.c  2004/01/20 21:52:09     1.271
+++ server/unittools.c  2004/01/25 07:20:56
@@ -900,7 +900,8 @@
   }
 
   if (unit_has_orders(punit)) {
-    if (execute_orders(punit) == GR_DIED) {
+    if (!execute_orders(punit)) {
+      /* Unit died. */
       return;
     }
   }
@@ -3162,13 +3163,19 @@
   Executes a unit's orders stored in punit->orders.  The unit is put on idle
   if an action fails or if "patrol" is set and an enemy unit is encountered.
 
+  The return value will be TRUE if the unit lives, FALSE otherwise.  (This
+  function used to return a goto_result enumeration, declared in gotohand.h.
+  But this enumeration was never checked by the caller and just lead to
+  confusion.  All the caller really needs to know is if the unit lived or
+  died; everything else is handled internally within execute_orders.)
+
   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, even if it have moves left.
 
   A unit will attack under orders only on its final action.
 ****************************************************************************/
-enum goto_result execute_orders(struct unit *punit)
+bool execute_orders(struct unit *punit)
 {
   int dest_x, dest_y;
   bool res, last_order;
@@ -3181,28 +3188,36 @@
   freelog(LOG_DEBUG, "Executing orders for %s %d",
          unit_name(punit->type), punit->id);
 
+  /* Any time the orders are canceled we should give the player a message.
+   * In the future these should probably get their own event type. */
+
   while (TRUE) {
     if (punit->moves_left == 0) {
       /* FIXME: this check won't work when actions take 0 MP. */
       freelog(LOG_DEBUG, "  stopping because of no more move points");
-      return GR_OUT_OF_MOVEPOINTS;
+      return TRUE;
     }
 
     if (punit->done_moving) {
       freelog(LOG_DEBUG, "  stopping because we're done this turn");
-      return GR_WAITING;
+      return TRUE;
     }
 
     if (punit->orders.vigilant && maybe_cancel_patrol_due_to_enemy(punit)) {
       /* "Patrol" orders are stopped if an enemy is near. */
       freelog(LOG_DEBUG, "  stopping because of nearby enemy");
-      return GR_FAILED;
+      free_unit_orders(punit);
+      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                      _("Game: Orders for %s aborted as there "
+                        "are units nearby."),
+                      unit_name(punit->type));
+      return TRUE;
     }
 
     if (moves_made == punit->orders.length) {
       /* For repeating orders, don't repeat more than once per turn. */
       freelog(LOG_DEBUG, "  stopping because we ran a round");
-      return GR_ARRIVED;
+      return TRUE;
     }
 
     last_order = (!punit->orders.repeat
@@ -3219,13 +3234,23 @@
       if (!MAPSTEP(dest_x, dest_y, punit->x, punit->y,
                   punit->orders.list[punit->orders.index].dir)) {
        freelog(LOG_DEBUG, "  move order sent us to invalid location");
-       return GR_FAILED;
+       free_unit_orders(punit);
+       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                        _("Game: Orders for %s aborted since they "
+                          "give an invalid location."),
+                        unit_name(punit->type));
+       return TRUE;
       }
 
       if (!last_order
          && maybe_cancel_goto_due_to_enemy(punit, dest_x, dest_y)) {
        freelog(LOG_DEBUG, "  orders canceled because of enemy");
-       return GR_FAILED;
+       free_unit_orders(punit);
+       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                        _("Game: Orders for %s aborted as there "
+                          "are units in the way."),
+                        unit_name(punit->type));
+       return TRUE;
       }
 
       freelog(LOG_DEBUG, "  moving to %d,%d", dest_x, dest_y);
@@ -3233,19 +3258,25 @@
                                     FALSE, !last_order);
       if (!player_find_unit_by_id(pplayer, unitid)) {
        freelog(LOG_DEBUG, "  unit died while moving.");
-       return GR_DIED;
+       /* A player notification should already have been sent. */
+       return FALSE;
       }
 
       if (res && !same_pos(dest_x, dest_y, punit->x, punit->y)) {
        /* Movement succeeded but unit didn't move. */
        freelog(LOG_DEBUG, "  orders resulted in combat.");
-       return GR_FOUGHT;
+       return TRUE;
       }
 
       if (!res && punit->moves_left > 0) {
        /* Movement failed (ZOC, etc.) */
        freelog(LOG_DEBUG, "  attempt to move failed.");
-       return GR_FAILED;
+       free_unit_orders(punit);
+       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                        _("Game: Orders for %s aborted because of "
+                          "failed move."),
+                        unit_name(punit->type));
+       return TRUE;
       }
 
       if (last_order && punit->transported_by != -1) {
@@ -3256,10 +3287,11 @@
       break;
     case ORDER_LAST:
       freelog(LOG_DEBUG, "  client sent invalid order!");
+      free_unit_orders(punit);
       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
                       _("Game: Your %s has invalid orders."),
                       unit_name(punit->type));
-      return GR_FAILED;
+      return TRUE;
     }
 
     /* We succeeded in moving one step forward */
@@ -3270,7 +3302,7 @@
        free_unit_orders(punit);
        assert(punit->has_orders == FALSE);
        freelog(LOG_DEBUG, "  stopping because orders are complete");
-       return GR_ARRIVED;
+       return TRUE;
       } else {
        /* Start over. */
        freelog(LOG_DEBUG, "  repeating orders.");
Index: server/unittools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.h,v
retrieving revision 1.61
diff -u -r1.61 unittools.h
--- server/unittools.h  2004/01/20 21:52:09     1.61
+++ server/unittools.h  2004/01/25 07:20:56
@@ -82,6 +82,6 @@
 void assign_units_to_transporter(struct unit *ptrans, bool take_from_land);
 bool move_unit(struct unit *punit, int dest_x, int dest_y,
              bool transport_units, bool take_from_land, int move_cost);
-enum goto_result execute_orders(struct unit *punit);
+bool execute_orders(struct unit *punit);
 
 #endif  /* FC__UNITTOOLS_H */