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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

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

<snip>

> > I assume you going to get rid of unit_vulnerability_virtual. 
> 
> I don't know how to do this without behavior change.
>

I'm not clear on this. I did a search, and did not find any code in the patch
using unit_vulnerability_virtual. It all seems to be using
unit_vulnerability_virtual2.
 
<snip>

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

You've added a lot of checks to defence_multiplication for example. If
Randon function A calls defence_multiplication directly, your change has 
just caused a change in behavior. 

If no function ever calls defence_multi except through
get_virtual_defense_power, then we know your change is correct. I can sleep
the restful peace that only the truly evil can attain.


If on the other hand Random function A calls defence_multi expecting terrain
and fortress bonus only, it will be in for a rude surprise.

Correct? I'm asking if you have checked the code to see that for example that
defence_multiplication is only ever used by two functions . There are other
functions for which this might be a problem, but defence_multi is the big one.
It seems the only other direct caller is get_total_defense_power. There is no
behaviour change for get_total_defense_power. I'm just asking if these are the
only two functions that ever call defence_multiplication directly.

If they are, I can categorically state that defence_multi has caused no
behaviour changes.

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

Ben says this is an actual bug. Apparently only Civ 1 rules allow city walls to
defend against helis. So our AI is stupidly thinking his city wall will protect
him against a chopper.

TODO: Added.

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

Absolutely correct. I am suggesting their addition . If we're going to check
for the effects of the terrain of defensive bonus, we should do it well.

TODO: Added.

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

Interesting. How is 10 a random factor? Don't tell me you belong to the heathen
church of blasphemous idolators called "We can do random number generation with
software" ;-).



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