[Freeciv-Dev] Re: GTK client crash (assertion failed: too much luxury) (PR#1409)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Apr 28, 2002 at 01:22:15PM -0700, Alex Wilkins wrote:
> On Sun, 28 Apr 2002, Raimar Falke wrote:
> 
> >On Thu, Apr 25, 2002 at 07:54:12PM -0700, awilkins@xxxxxxxxxxxx wrote:
> >> Full_Name: Alexander Wilkins
> >> Version: April 23 CVS
> >> Distribution: Built from source
> >> Client: Gtk+
> >> OS: Redhat Linux 7.2 (kernel 2.4.9-31)
> >> Submission from: (NULL) (128.12.194.114)
> >
> >Alexander: can you please try the attached patch?
> >
> >Mike: I have also added some docu to agents.c and the cma.
> >
> >     Raimar
> 
> When I try it, it again crashes when I try to open a city with too much 
> luxury.  It appears that this assertion fails:
> 
>   assert(luxury <= cache2.allocated_luxury);
> 
> (At the time of the crash, luxury was 252 and cache2.allocated_luxury 
> was 183.)  Some rudimentary analysis with gdb seems to indicate that the 
> estimate on luxury in ensure_invalid_cache2 is too low.  The 
> problem is that if all of a city's trade is used for luxury, the 
> total amount of luxury scales as total_trade * 2.5.  
> 
> One suggestion would be to use as a bound
> 
>     (pcity->tax_bonus * total_trade / 100) * pct_luxury + 
>       (pcity->size - 1) * pcity->tax_bonus / 50
> 
> where pct_luxury is the percentage of the nation's trade allocated for 
> luxury.  

You are of course right. pcity->tax_bonus has to be taken into
account.

> However, since pcity->tax_bonus doesn't appear to be correct in 
> update_cache2, set_city_tax_bonus from city.c (or an equivalent 
> function) would have to be called first.

I have change the stuff in city.c so that we can access the tax_bonus
as needed.

> I'm not sure if pct_luxury is actually accessible from inside 
> update_cache2; if not

It is accessible. However you have to fight with rounding errors. See
set_tax_income for the details. So I just assume the worst case: that
the player set the luxury rate to 100%.

> , it can probably be estimated from the relative 
> sizes of pcity->trade_prod and pcity->luxury_total.  Now that I think 
> of it, a better bound might be 
> 
>     pcity->luxury_total * total_trade / pcity->trade_prod + 
>       pcity->size * 5
> 
> which would use only the information that is already available.  
> Testing this latter bound, it seems to work O.K., although it is a 
> little loose (by 105 at most, though).  Since it seems that 
> ensure_invalid_cache2 is called every time a worker is changed to an 
> elvis, you might be able to get away with 
> 
>     pcity->luxury_total * total_trade / pcity->trade_prod +
>         pcity->ppl_elvis * 5
> 
> instead.
> 
> At the minimum, though, ensure_invalid_cache2 should probably check to 
> make sure that cache2.allocated_luxury > pcity->total_luxury.

Changes:
 - make set_city_{shield,tax,science}_bonus public
 - rename them to get_city_..._bonus and change semantics slightly
 - add {}s
 - change get_city_shield_bonus to be consistent with the other two
 - cleanup add_buildings_effect
 - move test for G_REDUCED_RESEARCH from add_buildings_effect to
 get_city_science_bonus

Alex: please test. If it fails again please send me a savegame.

Others: please review the changes made on city.[ch]

Bottom line: and another patch which mutated to something bigger

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "I do feel kind of sorry for Microsoft. Their attornies and marketing
  force must have tons of ulcers trying to figure out how to beat (not
  just co-exist with) a product that has no clearly defined (read
  suable) human owner, and that changes on an hourly basis like the
  sea changes the layout of the sand on a beach. Severely tough to
  fight something like that."
    -- David D.W. Downey at linux-kernel

dyn_cma_docu2.diff.gz
Description: GNU Zip compressed data