[Freeciv-Dev] (PR#2521) effects

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=2521 >

I sent this once before, but foolishly attached the patch uncompressed. 
  I'm assuming freeciv-dev will not forward it, so...

-----


Mike Kaufman wrote:

> in find_source_building() why do you check for !is_wonder(*pbldg) ?
> I think this is wrong. I should look for non-wonder buildings first, but after
> that search the wonders. This shouldn't cost much.

Maybe.  The problem is this is really an AI function, it just has to be
in effects.c because it accesses static data.  I renamed it
ai_find_source_building (nobody else should use it!) and left it at that.

> "Returns the number buildings of a certain type in a continent"
> s/in/on/

Yes.

> rename targets_in_range()

I renamed it to count_targets_in_range.  But I don't think that's
exactly what it does?

> I can stand no longer. in eff_redundant():
> s/group that exists wins/group that exists takes precedence/
> same for the comments in the rulesets.

OK.  But I found no such comments in the rulesets.

> The comment for eff_reqs_active() should read:
> "Check the requirements for the effect to see if it is active."

OK.

> in the comment of eff_eventually_active() 
> s/deps/reqs/

OK.

> rename eff_eventually_active() to is_eff_active_for_target()?
> maybe, in any case 'eventually' needs to be removed as it communicates
> future time rather than lack of range check.

I renamed it as is_effect_useful().

> rename eff_active() -> is_eff_active()

OK.  I also booleanized some other boolean functions.

> in get_effect_value(), rename the parameter 'eff' to 'type' or something.

effect_type

> eff_redundant(), eff_reqs_active(), eff_eventually_active() and 
> eff_active() rename parameter 'id' to 'building' as you did in
> targets_in_range()

I also renamed "eff" as "peffect".  And added explanations of the
parameters.

> in aicity.c:adjust_building_want_by_effects(), 
>   cities[EFR_LAST] is cached in ai_data. Use that instead.
> 
> in aicity.c:108
> add a comment there saying something like: iterate over each
>           effect for the building type, etc
> 
> in the header to ai_manage_buildings() a TODO: RECALC_SPEED should be
>           configurable to ai difficulty
> 
> aicity.c:893
> -       && i != B_CITY /* selling city walls is really, really dumb -- Syela 
> */
> -       && (wonder_replacement(pcity, i) || 
> building_unwanted(city_owner(pcity), i))) {
> +       && !building_has_effect(i, EFT_LAND_DEFEND)
> +             /* selling city walls is really, really dumb -- Syela */
> +       && (building_replaced(pcity, i)
> +        || building_unwanted(city_owner(pcity), i))) {
> 
> don't like this. If it's obsolete, then it has no effect and never will.
> $$ is better. Are there other assumptions (or lack thereof) made here?

AI stuff: I didn't touch this.  But Per changed a lot of the AI code so
they may be obsolete.

> city.c:can_build_improvement() 
> The comment is wrong. We aren't talking about units here.

Fixed.

> can_build_improvement() and can_eventually_build_improvement() return FALSE
> rather than 0. Change comment

No doubt a holdover from before we had booleans.

> can_build_improvement_direct(), can_build_improvement(), 
> can_eventually_build_improvement() should be in improvement.c

Probably, but the effect patch doesn't need to move it.

> in improvement_upkeep()
> 
>   if (improvement_types[i].upkeep <= 
>       get_city_bonus(pcity, EFT_UPKEEP_FREE))
> 
> can go on one line.

No it can't.  It's one character too long ;-).

> city.c:563
> the government comment here makes less sense now.

Rewritten (it still mentions government).

> base_get_shields_tile():
>  get_city_tile_bonus(pcity, ptile, EFT_PROD_PER_TILE); is missing.

Yep.  I also moved EFT_FOOD_PER_TILE in get_city_food_bonus so it
follows the sames tructure as the other two (previously FOOD_PER_TILE
was applied earlier).

> city.c:set_pollution(). This is wrong.

How so?

If you have recycling center and hydro/hoover/nuclear, you only get /5
to your pollution instead of /6.  This is hard or impossible to fix, and
probably isn't a big deal.

I did make a minor fix in case EFT_POLLU_POP_PCT was > 100%.  We don't
want mod to be negative in that case.

> city.c:2516
>    pcity->luxury_bonus = 100;
>    pcity->tax_bonus = 100;
> +  pcity->luxury_bonus = 100;
>    pcity->science_bonus = 100;
> redundant

Yes.  There's another duplicate luxury calc below that I also removed.

> combat.c:305,306, indent properly

OK.

> game.c:372 initialize spaceshipparts in the declaration

This code has changed; the complaint is obsolete.

> improvement.c:130 this is dangerous. what happens if the building has 
> multiple effects. This must be noted in the documentation.

I wrote about it in README.effects. (In fact the space parts weren't
documented here at all since I changed them.)

> improvement.c:556
> s/N.B. Unlike effects, we do not have/N.B. We do not need/

Sure...

> improvement.h: why do you have stuff aboe the #includes?

Removed.

> player.c:407 README.effects needs to note that a "value" for this effect
> does nothing (or alter this to reflect what it _does_ say in README.effects
> which would be better)

Modified README.effects.  I agree that the value should be made a
percentage.  But I didn't do this.

> unit.c:57 line wrapping...

I couldn't find anything.

> rulesets: "eff" should be replaced by "name"

I agree.  Done.

> there needs to to commentary for the effect groups.

Vasco, can you make some?

> Also, what happens if I put a -2 instead of 4 for Size_Adj. Correct behavior?

I assume it reduces the max size of the city.  This is correct.  In SMAC
one of the nations has this effect.

> I'm not happy that you make *_Defend a multiplier rather than a percentage.
> That's too coarse-grained.

I agree.  Done (but I didn't edit the ai code).

> ditto for Growth Food

Also done.  This was just a little uglier.  Someone should check the logic.

> ditto for Pollu_Prod_Pct, which makes even less sense considering the name.
> review all Pollu_Prod_Pct.

I didn't change this one.  I think it needs to be renamed.

We can't make it a percentage.  Recycle reduces pollution by 66% and
clean power by 50%.  But these don't combine to 116% as they would if we
made it a "normal" percentage.

The way it is now recycle gets a value of 2 and clean power a value of
5.  Each of them works fine individually.  But if you combine them
you'll get /= 5 instead of /= 6.  Not too big a problem.

> The mass_transit number for Pollu_Pop_Pct is incongruous.

100%?  Why?

> is this intentional:
> -       "Unit_Vet_Combat", "Player", 100, "Land"
> +    { "eff", "range", "value"
> +      "Land_Veteran", "Player"
> +      "Land_Regen", "Player"
> +      "Land_Vet_Combat", "Player", 50

I removed Land_Regen from Sun Tzu's in history/default/civ2 rulesets.
It seems to be clearly wrong.

The logic for Land_Vet_Combat seems to be a bit backwards.  I think the
Land_Vet_Combat value is correct for the current rules.  But the value
in civ2 ruleset was wrong.

> citytools.c:642
>   pcity->improvements[game.palace_building] = I_NONE;
>                                            ^ ^

ok...

> cityturn.c:310 probably should be: 
> get_current_construction_bonus(pcity, EFT_GROWTH_FOOD) >= 100

The check is

        && get_current_construction_bonus(pcity, EFT_GROWTH_FOOD) > 0

which I think is correct.  If we are building granary (bonus=50) then we
(may) want to throttle.

> cityturn.c:443 should be:
>   } else if (find_source_building(powner, EFT_SIZE_ADJ)
>              || find_source_building(powner, EFT_SIZE_UNLIMIT)) {
>       notify_player_ex(powner, pcity->x, pcity->y, E_CITY_AQUEDUCT,
>                        _("Game: %s needs an improvement to grow any 
> further."),
>                        pcity->name);
>   } else {
>       notify_player_ex(powner, pcity->x, pcity->y, E_CITY_AQUEDUCT,
>                        _("Game: %s can grow no further."), pcity->name);
>   }

This code shouldn't use find_source_building.  It does need to be better
before we can have fully generalized rulesets.  But it should be okay
for now.  Note that [ai_]find_source_building doesn't return a boolean
so your code is quite wrong.

> and then cityturn.c:460,461 is wrong.

> same issue for cityturn.c:530 and 540

I have no idea what code you mean.  Line numbers change you know.

> cityturn.c:751 the variable 'id' really isn't needed. fold into notify.

OK.  I renamed id2 as upgrades_to

> cityturn.c:759 extra space before comma

Yah.

> overarching comment you use get_current_construction_bonus() in conditionals
> and the answer might be negative, which as I recall is not defined.

All checks now do a direct comparison > 0.

> cityturn.c:943 mod can share the line with .name

Picky, aren't we?

> diplomats.c:759 comment makes no sense anymore.

Is "subvertable" a word?

> diplomats.c:1034  I don't like this at all. We should be able to specify a
> percentage.

> diplomats.c:1122 ditto 

For EFT_SPY_RESISTANT, yes.  This bonus is given defensively in
diplomatic combat as well as to protect buildings.  The former should be
a different effect (EFT_SPY_DEFEND) but I didn't make that.

> gotohand.c:989 see above on multipliers vs. percentages.

Yep.

> plrhand.c:do_tech_parasite_effect() why aren't we naming the building?

Because we don't know what building it is.  We can find out but it will
take some work.

> plrhand.c:903 is this right?

I assume you mean this on line 935:

-  if (player_owns_active_govchange_wonder(pplayer)) {
+  if (get_player_bonus(pplayer, EFT_NO_ANARCHY) > 0) {

why wouldn't it be right?

> ruleset.c:load_ruleset_buildings() some lines wrapping issues.

Some fixed (there may be more).

> settlers.c:877 see EFT_GROWTH_FOOD above.

Yes.

> unittools.c:do_upgrade_effects()
> there are problems here as you assign an int to a bool.
> I see you choose to ignore the AMOUNT spec...
> you are missing the upgrade step effects.
> int candidate_to_upgrade=-1; spacing
> i=0 spacing, or initialize it in the declaration.
> for the ALL_LEAP, you need not go through the candidate selection process.

This is ugly.  But I solved it ;-).  I removed all four effects and
replaced them with one EFT_UNIT_UPGRADE.  The amount specifies the
number to be ugpraded.  Really, even having an amount here is
unnecessary for the current level of generalization.

> unittools.c:496 line wrap.

?

> doc/README.effects needs to be updated.

I fixed the problems I noticed (and the ones I changed above).  Someone
will have to review it eventually.

In addition to the above changes, Per made a bunch of improvements to
the AI code.  These are merged in with my patch.  They include changes
to the prototypes of city_pollution and city_gold_surplus.  The patch is
about 100k bigger now because of all the AI changes.  I will guarantee
there are bugs.

jason

effects18.diff.bz2
Description: application/bzip