[Freeciv-Dev] Re: [PATCH] Improvement effects speedup (PR#1094)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jan 14, 2002 at 05:54:56PM -0800, Vasco Alexandre Da Silva Costa wrote:
> On Mon, 14 Jan 2002, Ben Webb wrote:
> > Actually, there are two distinct problems:
> > 
> > 1. update_all_effects() is currently called whenever a city_info, 
> >    short_city or game_info packet is received. In reality, updates are 
> >    only required if these packets add or remove improvements/Wonders.
> > 2. On loading a savegame, or during a turn update, many cities may build 
> >    or sell improvements. Since update_all_effects() currently updates, er, 
> >    "all" effects, it makes sense to call it only once, rather than for 
> >    each city which has changed its list of improvements.
> > 
> >     I believe the "city report dialog speedup" issue is the same as 
> > (2) here, but (1) is a large contributor to the problem, and should, 
> 
> Yes it is the same as (2).
> 
> > IMHO, be applied ASAP. This can be resolved by applying the patch minus 
> > the "do_deferred_effect_updates" portion, such that the try_update_effects 
> > function reads
> 
> In effect-init and effect-implement you did add tests for that in many
> places. I didn't notice you also added some in this patch.
> 
> > static void try_update_effects(int need_effect_update)
> > {
> >   if (need_effect_update) {
> >     update_all_effects();
> >   }
> > }
> > 
> > If desired, I can split this patch and post it.
> 
> Yes this would be a good idea. However wouldn't it be even better for
> you to add that function inside common/ and replace all instances of
> update_all_effects() with tests added in effect-init and effect-implement
> for try_update_effects?
> Or maybe rename update_all_effects() to try_update_effects() and add that
> test there?
> 
> > P.S. I set out to use the START_TURN packet to signal "all deferred effect 
> >      updates should now be performed" for (2) since this seemed to be the 
> >      purpose of the packet. However, I found I had to add extra handling 
> >      to handle_game_state() and handle_new_year(), since the START_TURN 
> >      packet does not seem to get sent under all circumstances (e.g. 
> >      restarting from a savegame, reconnecting to a running game, etc.) Can 
> >      the START_TURN packet be sent under these circumstances too, or would 
> >      this break agents? If the latter, perhaps we could add a suitable "do 
> >      all updates at start of turn" packet.
> 
> Well Raimar is the knowledgeable person in this case. What do you think
> Raimar?

Short answer: it is currently a mess.

Long answer: I had similar problems for the the auto-connect and the
agents. Either we add a hack to send the START_TURN every time or we
take a step back and make it the right way. This would include a
documentation which packet are sent in which order for which type of
game (complete new game, reconnect, savegame). And than making these
changes.

I'm for the latter because it provides a better cost/benefit ratio in
the long run.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Transported to a surreal landscape, a young girl kills the first woman
  she meets and then teams up with three complete strangers to kill again."
    -- TV listing for the Wizard of Oz in the Marin Independent Journal