[Freeciv-Dev] (PR#7616) decrease_unit_hp_smooth is broken

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [jdorje - Sat Mar 06 07:19:02 2004]:

> decrease_unit_hp_smooth is broken.  There's nothing really wrong with 
> the implementation, but it's called at the wrong time!  By the time it's 
> called punit0 and punit1 have already had their HP values updated.  Thus 
> punit0->hp == hp0 and punit1->hp == hp1.

The problem is this:

In the server, in handle_unit_attack_request, unit_versus_unit is called
to do the combat.  This function knocks down the attacker and defender's HP.

Later the attacker and defender have their info sent to the client. 
This is needed because the client (all clients seeing the combat) need
to know about the attacker and defender even if they can't otherwise see
them (e.g., units in cities).  This packet tells the client the new HP
of the units (one of them will be 0).

After this the combat info is sent to the client.  This tells the client
*again* the HP of the units and expects the client to draw the
animation.  But the client can't do any animation because the HP have
already been dropped.

There are several possible solutions.  One such solution is attached. 
In this solution unit_versus_unit does not adjust the units' HP directly
but is given two additional paramaters to store the new HP in.  The
combat packet is given the new HP but the unit info packets will use the
original HP.  The units' HP are not updated until after the combat
packets are sent out.

This may not be the best solution.  Another solution would be to split
up the sending of the unit packets, so the unit info packets would be
sent before unit_versus_unit is called.  I think this would make the
logic more confusing, though.  No doubt other solutions are possible as
well.

Note that the current code is broken, so *some* fix is needed.

jason

Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.290
diff -u -r1.290 unithand.c
--- server/unithand.c   8 Mar 2004 03:00:22 -0000       1.290
+++ server/unithand.c   26 Mar 2004 20:57:22 -0000
@@ -609,6 +609,7 @@
   struct player *pplayer = unit_owner(punit);
   struct packet_unit_combat_info combat;
   struct unit *plooser, *pwinner;
+  int attacker_new_hp, defender_new_hp;
   struct city *pcity;
   int moves_used, def_moves_used; 
   int def_x = pdefender->x, def_y = pdefender->y;
@@ -655,7 +656,7 @@
 
   old_unit_vet = punit->veteran;
   old_defender_vet = pdefender->veteran;
-  unit_versus_unit(punit, pdefender);
+  unit_versus_unit(punit, pdefender, &attacker_new_hp, &defender_new_hp);
 
   /* Adjust attackers moves_left _after_ unit_versus_unit() so that
    * the movement attack modifier is correct! --dwp
@@ -675,7 +676,7 @@
     pdefender->moves_left = 0;
   }
 
-  if (punit->hp &&
+  if (attacker_new_hp > 0 &&
       (pcity=map_get_city(def_x, def_y)) &&
       pcity->size>1 &&
       !city_got_citywalls(pcity) &&
@@ -686,16 +687,16 @@
   }
   if (unit_flag(punit, F_ONEATTACK)) 
     punit->moves_left = 0;
-  pwinner = (punit->hp > 0) ? punit : pdefender;
-  plooser = (pdefender->hp > 0) ? punit : pdefender;
+  pwinner = (attacker_new_hp > 0) ? punit : pdefender;
+  plooser = (defender_new_hp > 0) ? punit : pdefender;
 
-  vet = (pwinner->veteran == ((punit->hp > 0) ? old_unit_vet :
+  vet = (pwinner->veteran == ((attacker_new_hp > 0) ? old_unit_vet :
        old_defender_vet)) ? 0 : 1;
 
   combat.attacker_unit_id=punit->id;
   combat.defender_unit_id=pdefender->id;
-  combat.attacker_hp=punit->hp;
-  combat.defender_hp=pdefender->hp;
+  combat.attacker_hp = attacker_new_hp;
+  combat.defender_hp = defender_new_hp;
   combat.make_winner_veteran=vet;
 
   package_unit(punit, &unit_att_full_packet, FALSE);
@@ -748,6 +749,12 @@
     }
   } players_iterate_end;
 
+  /* Now knock down the unit's HP. */
+  punit->hp = attacker_new_hp;
+  pdefender->hp = defender_new_hp;
+
+  package_unit(punit, &unit_att_full_packet, FALSE);
+  package_unit(pdefender, &unit_def_full_packet, FALSE);
   conn_list_iterate(game.game_connections, pconn) {
     if (!pconn->player && pconn->observer) {
       send_packet_unit_info(pconn, &unit_att_full_packet);
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.286
diff -u -r1.286 unittools.c
--- server/unittools.c  15 Mar 2004 21:57:34 -0000      1.286
+++ server/unittools.c  26 Mar 2004 20:57:23 -0000
@@ -144,7 +144,8 @@
   3) the aftermath, the looser (and potentially the stack which is below it)
      is wiped, and the winner gets a chance of gaining veteran status
 **************************************************************************/
-void unit_versus_unit(struct unit *attacker, struct unit *defender)
+void unit_versus_unit(struct unit *attacker, struct unit *defender,
+                     int *attacker_new_hp, int *defender_new_hp)
 {
   int attackpower = get_total_attack_power(attacker,defender);
   int defensepower = get_total_defense_power(attacker,defender);
@@ -153,27 +154,32 @@
   get_modified_firepower(attacker, defender,
                         &attack_firepower, &defense_firepower);
 
+  *attacker_new_hp = attacker->hp;
+  *defender_new_hp = defender->hp;
+
   freelog(LOG_VERBOSE, "attack:%d, defense:%d, attack firepower:%d, defense 
firepower:%d",
          attackpower, defensepower, attack_firepower, defense_firepower);
   if (attackpower == 0) {
-      attacker->hp=0; 
+    *attacker_new_hp = 0;
   } else if (defensepower == 0) {
-      defender->hp=0;
+    *defender_new_hp = 0;
   }
-  while (attacker->hp>0 && defender->hp>0) {
+  while (*attacker_new_hp > 0 && *defender_new_hp > 0) {
     if (myrand(attackpower+defensepower) >= defensepower) {
-      defender->hp -= attack_firepower;
+      *defender_new_hp -= attack_firepower;
     } else {
-      attacker->hp -= defense_firepower;
+      *attacker_new_hp -= defense_firepower;
     }
   }
-  if (attacker->hp<0) attacker->hp = 0;
-  if (defender->hp<0) defender->hp = 0;
+  *attacker_new_hp = MAX(*attacker_new_hp, 0);
+  *defender_new_hp = MAX(*defender_new_hp, 0);
 
-  if (attacker->hp > 0)
-    maybe_make_veteran(attacker); 
-  else if (defender->hp > 0)
+  if (*attacker_new_hp > 0) {
+    maybe_make_veteran(attacker);
+  }
+  if (*defender_new_hp > 0) {
     maybe_make_veteran(defender);
+  }
 }
 
 /***************************************************************************
Index: server/unittools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.h,v
retrieving revision 1.65
diff -u -r1.65 unittools.h
--- server/unittools.h  12 Mar 2004 06:20:32 -0000      1.65
+++ server/unittools.h  26 Mar 2004 20:57:24 -0000
@@ -24,7 +24,8 @@
 /* battle related */
 int find_a_unit_type(int role, int role_tech);
 bool maybe_make_veteran(struct unit *punit);
-void unit_versus_unit(struct unit *attacker, struct unit *defender);
+void unit_versus_unit(struct unit *attacker, struct unit *defender,
+                     int *attacker_new_hp, int *defender_new_hp);
 
 /* move check related */
 bool is_airunit_refuel_point(int x, int y, struct player *pplayer,