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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 27, 2002 at 08:48:36PM -0800, Raahul Kumar wrote:
> 
> --- 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.

Yes what?

> You have added multiplying by the POWER_FACTOR, which seems to be
> missing from the original.  ????

Where have I added multiplying? The patch shouldn't do this.

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

This patch wasn't intended to fix bugs (except the fortress bug). It
should just reorder code.

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

Clean?

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

Yes this is an error. It should be renamed to use_alternative_hp and
should be documented.

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

One with?

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

;) ok. Btw is was previously just "m_type".

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

It wasn't in the original code. see get_modified_firepower.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "These download files are in Microsoft Word 6.0 format. After
  unzipping, these files can be viewed in any text editor, including
  all versions of Microsoft Word, WordPad, and Microsoft Word Viewer."
    -- http://www.microsoft.com/hwdev/pc99.htm