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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 28, 2002 at 03:56:51AM -0800, Raahul Kumar wrote:
> 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.
> 
> > > unit_vulnerability_virtual is not used anywhere in this patch. Surplus to
> > > requirements.
> 
> I assume you going to get rid of unit_vulnerability_virtual. 

I don't know how to do this without behavior change.

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

The patch wasn't intended to relly change behavior but reorders code.

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

This makes sense if s/never called directly/never changed/. But I
don't understand the original question.

> If so, then this patch is fine.

> Otherwise, you have introduced massive behaviour change.

How?

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

No it isn't. It is missing docu.

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

No behavior change. Put it on the todo.

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

  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;

is in get_virtual_defense_power and in get_defense_power. There are no
forest or mountain checks.

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

This isn't POWER_FACTOR. Just a random factor which is local to
find_a_good_partisan_spot.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 A life? Cool! Where can I download one?