[Freeciv-Dev] Re: gen-impr stuff

[Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, 8 Jan 2002, andi payn wrote:

[re: my concerns about impr-gen]
> With all those strikes against it, I'm thankful that anyone bothered
> to look at it. (I wasn't even going to upload it originally, until a
> couple of people asked me to.)

        Well, I'm lazy. ;) If someone else has written the code, I'm going 
to look at their patch before I write it myself (particularly concerning 
the AI).

> Also, I replaced all of the iterations (both iter and pointer) to use 
> the macros I wrote (to be more in keeping with the rest of FreeCiv, and 
> so I could change the way those iterations worked without having to make 
> changes all over the place).

        I noticed. I'm improving the iteration code at present anyway 
(largely because the existing functions are only really suitable for 
looking at city effects - they don't work very well for looking at unit 
effects) and will probably pinch your #defines at the same time. ;)

> >3. ... your handling of "happy Wonders" (e.g. Shakespeare's
> >Theatre) is wrong.
> I just took the code straight out of your old patch. If you've fixed
> it since then, that's the reason.

        The solution is fairly straightforward - the addition of 
"Force_Content" and "Force_Happy" effects. These affect happiness _after_ 
the martial law/aggressive units check, while the old effects work before.

> >4. You overuse is_effect_activated() horrifically.
> I wasn't sure about this. I first wrote it to use is_under_effect all
> over the place, and found some serious AI bugs.

        The effects only become activated properly after 
update_all_effects is called (and this should be called when you build or 
remove improvements). If you're considering the effects of buildings that 
aren't built yet, the effects won't be activated.

> However, I didn't see the exploding CPU usage. I've been running a
> server with multiple AIs with one local Gtk client and two remote
> clients, and played some pretty big games, and my P200 kept up no
> problem. And a quick profiling shows not much time at all spent in
> that function. That doesn't mean the extra calls shouldn't be
> eliminated, but I think the concerns are overstated.

        Perhaps, but if the function is unnecessary you shouldn't be 
calling it. ;) The problem is most profound if you use the cond_eff field 
liberally (in the default ruleset this is only used for city walls) as 
this requires an effect iteration.

        The major CPU hog with impr-gen at present is the 
update_all_effects function, as it scales as O(N**2) with the number of 
cities. See http://arch.freeciv.org/freeciv-dev-200109/msg00019.html. (You 
might want to try your code with the savegame referenced there, as that'll 
really test it.) The clientside CPU usage should be largely resolved by my 
patch PR#1141 (in the case referenced, it should cut the CPU used by 
impr-gen by three orders of magnitude) but bizarrely this patch has not 
been applied to Freeciv CVS. (It is, however, in impr-gen-combined.)

> >5. As far as I can tell, you do not honour the "survives" field.
> I didn't look at this at all. If your older patch didn't handle it,
> then my code doesn't either.

        Thought so. You just confused me when you said you'd "added 
support for all remaining effects". As you say, destroyed Wonders don't 
feature in many games, but I'm currently working on supporting them 
properly with impr-gen.

> Neither do I. First of all, Enable_Nuke has a typo, so it's
> world-range only. Enable_Space works either world-range or
> player-range. But they should both be available at the city
> level.

        The latest impr-gen patch (20020106) from 
http://freecivac.sourceforge.net/ begins to add city-range support for 
these effects (see effect-global-city-v2.patch).

> However, while the problem should definitely be fixed, the CPU usage
> can't be too bad, as again, my P200 had no problem keeping up while
> handling lots of other things at the same time.

        You should try that 1000-city savegame above. ;) My Athlon 800 has 
enough trouble with that, so it'll tax your P200...

> >9. You're absolutely right about the pollution modifiers.
> Phew! I thought I was just missing something obvious!
> 
> >... I'm more inclined to add them all and subtract from 100...
> 
> That doesn't work. First of all, if you have a Recycling Center, you
> get 34, subtract from 100, and it's only reducing production pollution
> by 34%, rather than the 66% it's supposed to.

Well yes, you'd have to change the ruleset to say 66 rather than 34 there.

> If we want these to work like other bonuses, there is a right way to
> do it: They're negative bonuses. Recycling Center has a -66 amount,
> Hydro Plant has a -50 amount, the compensating factor for Hydro Plant
> with Recycling Center has a 50 amount.

Yes, that's what I meant (just without the negation).

> However, I don't think this really is what any modpack designer will
> want. If three 50% bonuses together make a 150% bonus (meaning 250%
> adjustment, so 50 science points becomes 125 science points), that's
> fine. If three -50% bonuses together make a -150% bonus (meaning -50%
> adjustment, so 50 pollution points becomes -25 pollution), that's more
> than a little weird.

        You may be right, but in that case you'd want to change the other 
bonuses (science, tax, etc.) to be multiplicative too, otherwise it just 
gets too confusing. I _think_ this is what the original impr-gen author 
meant the various _Pct effects to do (i.e. all Luxury_Bonus effects are 
summed, while Luxury_Pct is multiplied, and then Luxury_Pct is applied 
after Luxury_Bonus). An additional problem with the multiplication 
approach is that you introduce rounding error, because you can't represent 
(for example) 1/3 or 2/3 exactly as a percentage. Consider the Marketplace 
and the Bank. With addition, they both have a 50% bonus (so both gives 
100%). With multiplication, the Marketplace would have a 50% bonus, while 
the Bank would have a 33% bonus (as 1.5 * 1.33 = 2.0). But actually you'd 
get a 1.995 multiplier, as 1.33 is an inexact representation of 4/3.

        Disallowing a total negative pollution is trivial, but perhaps it 
could have an application anyway. After all, negative gold and production 
are valid concepts in Freeciv, so why not pollution?

> Think about it as a modpack designer: You want to make two pollution
> adjustment buildings that can be used together. You want the first one
> to reduce pollution for a city by two thirds, just like a Recycling
> Center, and you want the other to reduce pollution for an island by
> half.

OK, so here's my proposal.

Luxury_Bonus   - increases base luxury production by AMOUNT percent (all 
                 bonuses are summed before being applied)
Luxury_Pct     - multiplies total luxury (after Luxury_Bonus) by AMOUNT
                 percent (multiple effects are multiplicative)
Pollu_Prod_Adj - increases base pollution from production by AMOUNT 
                 percent (all bonuses are summed before being applied)
Pollu_Prod_Pct - multiplies total pollution from production (after 
                 Pollu_Prod_Adj) by AMOUNT percent (multiple effects are 
                 multiplicative)

        This then makes the various effects consistent. The Bank and 
Marketplace would then continue to work exactly as they do now, while the 
Recycling Center etc. would apply a negative Pollu_Prod_Adj of -66 (after 
all, it's entirely possible that you could introduce buildings which 
_increase_ pollution, so I think a negative modifier makes sense). For 
your modpack example, you could use Pollu_Prod_Pct values of 33 and 50, 
resulting in a total of 16% (0.33*0.50 = 0.16), i.e. one-sixth, albeit 
with the rounding error caveat. Mass Transit would use a Pollu_Pop_Pct 
value of zero (replacing the old Pollu_Set_Pop effect, which, as you 
said, makes little sense which any value other than zero). This covers all 
eventualities, doesn't it?

> Here's my current plan: Rather than trying to untangle the massive
> patch I created, I'm going to start over.

        From looking at your patch, I suspect this'd be the easiest way 
of doing things. ;)

> 1. Pull today's CVS
> 2. Rebuild each of the easy patches (vet-levels, diplomacy battles,
>    etc.) from scratch and upload
> 3. Rebuild the calendar patches as I explained in my other messages
>    and upload them
> 4. Apply all of the other patches I use on a regular basis
> 5. Rebuild all of my simple patches that depend on other patches and
>    upload them
> 6. Pull your latest impr-gen-combined patch
> 7. Rebuild all of the foundation-level changes (like the new
>    iteration macros) from scratch and upload that as a patch
>    dependent on yours

        I suggest you get my impr-gen patch from the FreecivAC pages, as 
this is more up-to-date than that on the Freeciv FTP server. Also, let me 
know when you get to the impr-gen stuff, so I can coordinate my changes 
with yours.

> For the impr-gen-related patches, I'll
> also post about it on freecivac-devel (do I have to be subscribed to
> the lists to post? Hotmail can't handle the traffic...).

        No, you don't need to be subscribed; just email to 
freecivac-devel@xxxxxxxxxxxxxxxxxxxxx. However, the list isn't nearly as
high-traffic as freeciv-dev; it averages at about one message a day, so I
suspect even Hotmail could cope with that!

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"God does not play dice with the universe."
        - Albert Einstein