[Freeciv-Dev] Re: [Patch] Cleanup of defense power calculations

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:

A few comments on the patch. The defence power calcs require a lot more
auditing
than the attack power diff. This is only a partial audit.

m = get_virtual_defense_power(i, n, x, y); Yes, in unit_vulnerability_virtual2
and get_virtual_defense_power and defence_multiplication. You have added
multiplying by the POWER_FACTOR, which seems to be missing from the original.
????

Yes I agree with you, that if we multiply the attack calculations by 10 and
divide by 30, we should do the same here. It is the case though that the
original code did not do this.

unit_vulnerability_virtual is not used anywhere in this patch. Surplus to
requirements.

Raimar, you need to split this patch into cleanup and actual behaviour change.
I'm not sure if moving code from one function to another constitutes behaviour
change or not. It potentially might if the wrapper functions are not always
used. If they are always used, then obviously it doesn't.

This makes auditing code really hard. I get to play the fun game of trace which
function calls which other function.

I need two patches, one where you fix actual bugs, the other one where you just
apply cleanups. That way I can check a function to see where yours might be
different to the original.

-      m *= unit_types[n].hp * unit_types[n].firepower; Yes, in
unit_vulnerability_virtual2. Not quite the same though. ???

-      if (vet) m *= 1.5; Yes, in base_get_defense_power Clean.
-      m /= 30; I believe this to be clean. POWER_DIVIDER is 30, although I
could not find it in this patch.
-      m *= m; Yes, in unit_vulnerability_virtual2. Clean.

A lot of funs are like the lines above. If these ones are clean, then your
patch is error free.

Original

int get_virtual_defense_power(Unit_Type_id a_type, Unit_Type_id d_type, int x,
int y)
 {
 int defensepower=unit_types[d_type].defense_strength;
 enum unit_move_type m_type = unit_types[a_type].move_type;
 struct city *pcity = map_get_city(x, y);
 enum tile_terrain_type t = map_get_terrain(x, y);
 int db;
 if (unit_types[d_type].move_type == LAND_MOVING && t == T_OCEAN) return 0;
 /* I had this dorky bug where transports with mech inf aboard would go next
 to enemy ships thinking the mech inf would defend them adequately. -- Syela */
db = get_tile_type(t)->defense_bonus;
if (map_has_special(x, y, S_RIVER))
db += (db * terrain_control.river_defense_bonus) / 100;
defensepower *= db;
if (unit_type_flag(d_type, F_PIKEMEN) && unit_type_flag(a_type, F_HORSE)) 
defensepower*=2;
if (unit_type_flag(d_type, F_AEGIS) &&
(m_type == AIR_MOVING || m_type == HELI_MOVING)) defensepower*=5;
if (m_type == AIR_MOVING && pcity) {
if (city_got_building(pcity, B_SAM))
defensepower*=2;
if (city_got_building(pcity, B_SDI) && unit_type_flag(a_type, F_MISSILE))
defensepower*=2;
} else if (m_type == SEA_MOVING && pcity) {
if (city_got_building(pcity, B_COASTAL))
defensepower*=2;
}
if (!unit_type_flag(a_type, F_IGWALL)
&& (m_type == LAND_MOVING || m_type == HELI_MOVING
|| (improvement_variant(B_CITY)==1 && m_type == SEA_MOVING))
&& pcity && city_got_citywalls(pcity)) {
defensepower*=3;
}
if (map_has_special(x, y, S_FORTRESS) && !pcity)
defensepower+=(defensepower*terrain_control.fortress_defense_bonus)/100;
if (pcity && unit_types[d_type].move_type == LAND_MOVING)
defensepower*=1.5;
return defensepower;
}
==============================================================================

The Patch

int unit_vulnerability_virtual2(Unit_Type_id att_type, Unit_Type_id def_type,
                                int x, int y, bool fortified, bool veteran,
                                bool use_extra_hp, int extra_hp)
{
  int v = (get_virtual_defense_power(att_type, def_type, x, y, fortified,
                                     veteran) *
           (use_extra_hp ? extra_hp : unit_types[def_type].hp) *

New extra. I've no idea on what the extra hp means. A comment please. This is
not the same as the original function you've replaced.

           unit_types[def_type].firepower / POWER_DIVIDER);

  return v * v;
}

 int base_get_defense_power(struct unit *punit)
 {
   int power = unit_type(punit)->defense_strength * POWER_FACTOR;
   
   
   if (punit->veteran) {
     power = (power * 3) / 2;
   }
   return power;
}

This function is correct.

+int get_virtual_defense_power(Unit_Type_id att_type, Unit_Type_id def_type,
+                             int x, int y, bool fortified, bool veteran)
 {
-  int defensepower=unit_types[d_type].defense_strength;
-  enum unit_move_type m_type = unit_types[a_type].move_type;
-  struct city *pcity = map_get_city(x, y);
+  int defensepower = unit_types[def_type].defense_strength;
   enum tile_terrain_type t = map_get_terrain(x, y);
   int db;
 
-  if (unit_types[d_type].move_type == LAND_MOVING && t == T_OCEAN) return 0;
-/* I had this dorky bug where transports with mech inf aboard would go next
-to enemy ships thinking the mech inf would defend them adequately. -- Syela */
+  if (unit_types[def_type].move_type == LAND_MOVING && t == T_OCEAN) {
+    /* Ground units on ship doesn't defend. */
+    return 0;
+  }
 
   db = get_tile_type(t)->defense_bonus;
-  if (map_has_special(x, y, S_RIVER))
+  if (map_has_special(x, y, S_RIVER)) {
     db += (db * terrain_control.river_defense_bonus) / 100;
+  }
   defensepower *= db;
 
-  if (unit_type_flag(d_type, F_PIKEMEN) && unit_type_flag(a_type, F_HORSE)) 
-    defensepower*=2;
-  if (unit_type_flag(d_type, F_AEGIS) &&
-       (m_type == AIR_MOVING || m_type == HELI_MOVING)) defensepower*=5;
-  if (m_type == AIR_MOVING && pcity) {
-    if (city_got_building(pcity, B_SAM))
-      defensepower*=2;
-    if (city_got_building(pcity, B_SDI) && unit_type_flag(a_type, F_MISSILE))
-      defensepower*=2;
-  } else if (m_type == SEA_MOVING && pcity) {
-    if (city_got_building(pcity, B_COASTAL))
-      defensepower*=2;
-  }
-  if (!unit_type_flag(a_type, F_IGWALL)
-      && (m_type == LAND_MOVING || m_type == HELI_MOVING
-         || (improvement_variant(B_CITY)==1 && m_type == SEA_MOVING))
-      && pcity && city_got_citywalls(pcity)) {
-    defensepower*=3;
+  if (veteran) {
+    defensepower = (defensepower * 3) / 2;
   }
-  if (map_has_special(x, y, S_FORTRESS) && !pcity)
-    defensepower+=(defensepower*terrain_control.fortress_defense_bonus)/100;
-  if (pcity && unit_types[d_type].move_type == LAND_MOVING)
-    defensepower*=1.5;
 
-  return defensepower;
+  return defence_multiplication(att_type, def_type, x, y, defensepower,
+                               fortified);
 }
 
 
get_virtual has been gutted. It might have been better to make them one
function. Unless there are times defence_multiplication is called directly.
This function is correct.

+static int defence_multiplication(Unit_Type_id att_type,
+                                 Unit_Type_id def_type, int x, int y,
+                                 int defensepower, bool fortified)
 {
-  int defensepower=unit_types[d_type].defense_strength;
   struct city *pcity = map_get_city(x, y);
-  enum tile_terrain_type t = map_get_terrain(x, y);
-  int db;
 
-  if (unit_types[d_type].move_type == LAND_MOVING && t == T_OCEAN) return 0;
-/* I had this dorky bug where transports with mech inf aboard would go next
-to enemy ships thinking the mech inf would defend them adequately. -- Syela */
+  if (unit_type_exists(att_type)) {
+    enum unit_move_type att_m_type = unit_types[att_type].move_type;

I'd never thought I would suggest this. att_m_type is too brief. att_move_type,
attacker_move_type or something along those lines would be better. Hell must be
freezing over. I've suggested longer var names to Raimar ;).


-  db = get_tile_type(t)->defense_bonus;
-  if (map_has_special(x, y, S_RIVER))
-    db += (db * terrain_control.river_defense_bonus) / 100;
-  defensepower *= db;
+    if (unit_type_flag(def_type, F_PIKEMEN)
+       && unit_type_flag(att_type, F_HORSE)) {
+      defensepower *= 2;
+    }
 
-  if (map_has_special(x, y, S_FORTRESS) && !pcity)
-    defensepower+=(defensepower*terrain_control.fortress_defense_bonus)/100;
-  if (pcity && unit_types[d_type].move_type == LAND_MOVING)
-    defensepower*=1.5;
+    if (unit_type_flag(def_type, F_AEGIS) &&
+       (att_m_type == AIR_MOVING || att_m_type == HELI_MOVING)) {
+      defensepower *= 5;
+    }
+
+    if (att_m_type == AIR_MOVING && pcity) {
+      if (city_got_building(pcity, B_SAM)) {
+       defensepower *= 2;
+      }
+      if (city_got_building(pcity, B_SDI)
+         && unit_type_flag(att_type, F_MISSILE)) {
+       defensepower *= 2;
+      }
+    } else if (att_m_type == SEA_MOVING && pcity) {
+      if (city_got_building(pcity, B_COASTAL)) {
+       defensepower *= 2;
+      }
+    }
+    if (!unit_type_flag(att_type, F_IGWALL)
+       && (att_m_type == LAND_MOVING || att_m_type == HELI_MOVING

Not a fault with the patch. I've just never seen a city wall that could stop a
flying unit. Bizarre that Civ 2 & Freeciv give a defensive bonus against helis
based on city walls.

+           || (improvement_variant(B_CITY) == 1
+               && att_m_type == SEA_MOVING)) && pcity
+       && city_got_citywalls(pcity)) {
+      defensepower *= 3;
+    }
+  }
+
+  if (map_has_special(x, y, S_FORTRESS) && !pcity) {
+    defensepower +=
+       (defensepower * terrain_control.fortress_defense_bonus) / 100;
+  }
+
+  if ((pcity || fortified) && unit_types[def_type].move_type == LAND_MOVING) {
+    defensepower = (defensepower * 3) / 2;
+  }
 
   return defensepower;
 }
 
This function is ok. I agree with moving all the multiplication bonuses to
defence_multiplication. It does however make the wrapper function a bit
pointless.

There is also a negative defence bonus. The "Pearl Harbour" of ships caught in
port being penalised is not in defence_multiplication.
 
> base_get_defense_power, get_defense_power and get_total_defense_power
> multiply defense_strength with POWER_FACTOR. get_virtual_defense_power
> doesn't.
> 
> You may have a better name for unit_vulnerability_virtual2.
> 

No I really don't. unit_vulnerability_complete_squared is the only thing that
comes to mind.
 
> From the belligerence functions only unit_belligerence returns a
> squared value. From the vulnerability functions only
> unit_vulnerability_basic returns a _non_-squared value.
> 
> get_simple_defense_power has been removed.
> 
> The patch is on top of attack_power2.diff.
> 
> Nothing should be changed except this one (which I consider a bug):
> -  if (unit_on_fortress(defender) &&
> -      !map_get_city(defender->x, defender->y))
> -    defensepower*=2;
> 
> +  if (map_has_special(x, y, S_FORTRESS) && !pcity) {
> +    defensepower +=
> +       (defensepower * terrain_control.fortress_defense_bonus) / 100;
> +  }
> 
>       Raimar

Yes it is. 

__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com