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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

Overall, it's a pretty good patch. I am now confident that the functions are
correct. If Greg looks this over and can't spot any obvious problems, I vote
for inclusion.

--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 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?

The yes comments mean - is this the equivalent of get_virtual defense_power.
Yes, those 3 functions are the equivalent.

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

Original
(punit->hp * (punit->veteran ? 15 : 10)* unit_type(punit)->defense_strength));

Your patch
+           (punit->hp * base_get_defense_power(punit)));

int base_get_defense_power(struct unit *punit)
+{
+  int power = unit_type(punit)->defense_strength * POWER_FACTOR;
                                                    ^^^^^^^^^^^^^
You multiply by power factor, where the original did not.

  if (punit->veteran) {
    power = (power * 3) / 2;

  }
  return power;
}

DOH. I just realised that in fact this function does use power factor, cleverly
hidden in the vet check. I retract my complaint.
 
<snip>
> > unit_vulnerability_virtual is not used anywhere in this patch. Surplus to
> > requirements.

I assume you going to get rid of unit_vulnerability_virtual. 

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

You did not answer this.
 
> > 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.
>

That is precisely the problem. Are you willing to guarantee that functions like
defence_multiplication are never called directly? If so, then this patch is
fine. Otherwise, you have introduced massive behaviour change.
 
> > -      m *= unit_types[n].hp * unit_types[n].firepower; Yes, in
> > unit_vulnerability_virtual2. Not quite the same though. ???
> >

This was due to extra hp issue. Depending on your explanation, this might not
be an issue.
 
> > -      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 funcs are like the lines above. If these ones are clean, then your
> > patch is error free.
> 
> Clean?
>

Clean means that your function does exactly what the old code did here.
 
> > 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.

The original function only multiplied by unit_types.hp. I have no idea what
alternative hp is about. I've never even heard it mentioned. This is another
bugfix.

> 
> > +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)) {

I am sure I had a comment here asking why helicopters suffer against city
walls.
I just cannot picture the situation. A riflemen crouching behind yelling at the
helicopter crew, nyah nyah you can't get me. The helicopter crew fly over the
wall and shoots him up.

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

Defence_multiplication. All that's left is the vet and terrain checks. The
terrain checks seem incomplete. The original code had them missing as well,
but terrain checks for forest and mountain are missing. I think this is handled
elsewhere, but if you're going to check for terrain effects in one place, might
as well do them all.
 
> > 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".

It's an improvement on the original.

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

I know. I think it probably should be, and the other check for "Pearl Harbour"
should be removed. It's ok if you don't, it's just convenient to have one place
to check for situations like Pearl Harbour, SAM sites, pikemen etc.

value = get_virtual_defense_power(U_LAST, u_type, x1, y1, FALSE, FALSE);
     value *= 10;

I am sure I noted that either value should be replaced by power_factor or be 
removed.

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