[Freeciv-Dev] Re: Ruleset Development - improvements

[Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx>]

(heavily trimmed for clarity)

Re: generalized improvements...

On Tue, 4 Dec 2001, Raimar Falke wrote:

> AFAIK there wasn't a submission till I got maintainer.

Well yes, it's been around for some time now. But I should refer you to
http://arch.freeciv.org/freeciv-dev-200110/msg00444.html

> Also note this:
> 
> \begin{quote}
> 
> This is an automated notification of a change to freeciv cvs,
> on Wed Aug 22 14:40:06 PDT 2001 = Wed Aug 22 21:40:06 2001 (GMT)
> by Sebastian Bauer <sebauer@xxxxxxxxxxx>
> 
> ---- Files affected:
> 
> freeciv/client packhand.c
> freeciv/common city.c city.h game.c game.h improvement.c improvement.h 
> player.c player.h
> freeciv/data/civ1 buildings.ruleset
> freeciv/data/civ2 buildings.ruleset
> freeciv/data/default buildings.ruleset
> freeciv/server citytools.c cityturn.c diplhand.c diplomats.c gamehand.c 
> maphand.c savegame.c
> 
> ---- Log message:
> 
> Generalized the construction of the improvements by using the things defined 
> in the buildings.ruleset.
> Changed the differnt buildings.ruleset so that they are compatible to the old 
> hardcoded code.
> 
> Patch by Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx> and me
> 
> \end{quote}
> 
> So Sebastian does know more about this.

        As far as I can tell by looking at Freeciv-CVS and the original 
impr-gen patch, this submission would appear to be part of my impr-gen 
patch, with some of the function names changed and a few comments added, 
so I disagree. Besides, Sebastian has been very quiet recently anyway.

> From my todo list:
>  - effect of building (client only)
> 
> Consider something like this:
>  - save city state
>  - for each improvement which can be build:
>    - virtually build the improvement
>    - recalculate the city
>    - calculate and save the difference (effect) to the starting point
>    - restore saved state
>  - for client: display the effect
>  - for AI: weighten effects and select one

        The problem with this approach is that a building can have a 
global effect (e.g. a wonder) so to be completely general you have to 
consider all cities in the game (yours and other players') when you 
recalculate the city. This is what causes the problem that you refer to in 
the URLs below.

> > > - Client and server have different ideas of the world (e.g. islands can 
> > > be 
> > >   different in the client, due to unexplored territory) and this can give 
> > >   the client incorrect effects
> > > - Handling of population size limits (e.g. Aqueduct, Sewer System)
> > > - Code optimisation; many algorithms are still O(N**2) in number of 
> > >   cities, and we iterate over effects too frequently for my liking
> > Anyway, this isn't something so fatal IMHO and can be adjusted on the fly.
> 
> http://arch.freeciv.org/freeciv-dev-200109/msg00019.html

        I count this as "code optimisation". The problem here is that the 
client is recalculating all effects whenever a new city packet is 
received, and the savegame referred to at that URL has loads of cities in 
it. The server seems to be slinging packets at the client like there's no 
tomorrow (surely you only need the same number of packets as there are 
cities?) so update_all_effects is being called far too often on the 
client. It's possible to optimise this (e.g. only update effects when a 
city_info packet changes the improvements) but I think perhaps making the 
client more "dumb", with no understanding of improvement effects, is a 
better way to go (although I'm not sure if this is actually doable). I'll 
have to get back to you on that.

> And from http://arch.freeciv.org/freeciv-dev-200109/msg00051.html:
> 
> > I think much of update_all_effects() and the other calls are really
> > unneccessary, but I wanted to wait for the next step of the
> > generalizaton where time needed will further increases (this is the
> > price).

        Well, that's not actually true. It's time-consuming because it's 
O(N**2) in number of cities. The full patch actually, er, updates all 
effects (the patch in CVS just decides which buildings are replaced by 
wonders) so including this extra code makes very little difference to the 
runtime.

> Well the next step didn't managed to get included. And the tree is
> left with code which uses 40% of the runtime.
> 
> So I'm a bit critical.

        Hey, I never submitted this patch to CVS; I only advertised it 
and asked for feedback. The clientside part of the code wasn't really 
ready for CVS inclusion then, and it's not now either (although that "40% 
runtime" figure is dependent on how many cities you have, of course - in 
games I play the overhead is negligible). The serverside code (e.g. the 
landlocked improvements stuff at the URL I mentioned) on the other hand, 
largely is.

        As I said, I'll try to split the patch (currently rather large) 
into some more manageable chunks, and then sling them towards the bug 
tracking system. Hopefully then everybody will be happy. ;)

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"So maybe you're a 4 horse engine with a power drive"