[Freeciv-Dev] Re: At endyear map in not displayed (PR#1883)

[Raimar Falke <rf13@xxxxxxxxxxxxxxxxx>]

On Wed, Aug 28, 2002 at 10:06:10AM +0200, Davide Pagnin wrote:
> On Tue, 2002-08-27 at 18:59, Raimar Falke wrote:
> > On Mon, Aug 05, 2002 at 09:08:35AM -0700, nightmare@xxxxxxxxxx wrote:
> > > As well known, 1.13.0 release and further development code (1.13.1-devel)
> > > doesn't display the full world map at the end of a game.
> > > 
> > > This misbehaviour as been introduced on 3rd July, when the 
> > > CLIENT_GAME_OVER_STATE has been introduced in the code.
> > 
> > This and the no-chatline-during-nation-selection bug is fixed by the
> > attached second version of the POPS/POPF patch. The problem is the
> > same in the two cases: we used some more or less arbitrary packets to
> > group packets for the agents (which was latter adopted by the reports
> > (the chatline is a report in this context)). In two cases a packet
> > which can be used as a border-packet wasn't available. So we added
> > PACKET_START_TURN and CLIENT_GAME_OVER_STATE to recognize the end of
> > the pregame (started by PACKET_JOIN_GAME_REPLY) and the end of a game
> > in general (started by PACKET_BEFORE_NEW_YEAR).
> > 
> > As pointed out in the first version of the POPS patch I think that
> > adding these two packets were a mistake. The patch fixes this. We
> > could remove these two packets. But as usual other things use the
> > packets. At least the PACKET_START_TURN.
> 
> Wouldn't be nice, if we take this occasion to write the purpose and side
> effects of every type of packet we use?

Where? And how? How much documention do you need for this?

> > A second patch (an extended version of Davide's) should be applied on
> > top of this which will let the client take full advantage of the
> > information of CLIENT_GAME_OVER_STATE. Till then the state is ignored.
> > 
> > I know that the patch is only a band-aid for the problem is was
> > created for. To make it clear that I don't intend to use the patch for
> > this I removed this part:
> > 
> > @@ -565,15 +565,18 @@
> >                   err = gettimeofday(&start, &tz);
> >                   assert(!err);
> >  #endif
> > -               connection_do_buffer(pconn);
> > +               conn_list_do_buffer(&game.all_connections);
> >                 start_processing_request(pconn,
> >                                          pconn->server.
> >                                          last_request_id_seen);
> > +               prepare_for_sending_POPS(pconn);
> > +
> >                 command_ok = handle_packet_input(pconn, packet, type);
> >                 packet = NULL;
> > 
> > +               send_POPF();
> >                 finish_processing_request(pconn);
> > -               connection_do_unbuffer(pconn);
> > +               conn_list_do_unbuffer(&game.all_connections);
> >                 if (!command_ok) {
> >                   close_connection(pconn);
> >                 }
> > 
> > So the name of the packets should be changed from
> > PACKET_OTHER_PROCESSING_STARTED and PACKET_OTHER_PROCESSING_FINISHED
> > to PACKET_FREEZE_HINT and PACKET_THAW_HINT. The emphasis is on
> > HINT. These packets mark a set of other packets which are send out
> > within a small timeframe and may contain inconsistent data.
> 
> I don't want to annoy you, but I found the proposed name more
> insightful, and thus I vote for the name change.

Ok.

> > diff -Nurd -X clean/diff_ignore -x *.sav* clean/client/agents/agents.c 
> > work/client/agents/agents.c
> > --- clean/client/agents/agents.c    Sat Aug 24 15:38:51 2002
> > +++ work/client/agents/agents.c     Tue Aug 27 16:38:06 2002
> > @@ -352,10 +352,27 @@
> >  /***********************************************************************
> >   Called from client/packhand.c.
> >  ***********************************************************************/
> > +void agents_other_processing_started(void)
> > +{
> > +  freelog(META_CALLBACKS_LOGLEVEL, "agents_other_processing_started()");
> > +  freeze();
> > +}
> 
> Just a question, all the cma stuff, is polluted with this define
> (META_CALLBACKS_LOGLEVEL) that I found very interesting for debug
> purposes, is there any chance that a similar define can found his way in
> the cvs and 

Per tries something like this for the AI. If I added them for
debugging during development I leave them in like in the turn-done
patch.

> a freelog call can be put at beginning of any function call?

This would be overkill IMHO.

> A second thing, all those 'agent_*' function are mostly wrappers for
> a freelog call and otherwise void or contain a single function call
> (freeze or thaw), perhaps they contribute to auto-comment code, but
> aren't them overkill?

There were used in the past. Some of them can be dispatched to
individual agents if the need arise.

> > diff -Nurd -X clean/diff_ignore -x *.sav* clean/client/agents/cma_core.c 
> > work/client/agents/cma_core.c
> > --- clean/client/agents/cma_core.c  Wed Aug  7 14:38:11 2002
> > +++ work/client/agents/cma_core.c   Sun Aug 25 19:59:56 2002
> > @@ -1719,6 +1719,8 @@
> >    bool handled;
> >    int i;
> >  
> > +  assert(find_city_by_id(pcity->id) == pcity);
> > +
> 
> What this assert is for? (I don't get how this can cope with proposed
> goal of this patch).

The current agent code is wrong in the sense that the city_callbacks
and unit_callbacks of struct agent get a "struct city|unit *". The
unit/city can be removed at the time the callback is called. So the
code has to be changed to only take a "int city_id" and the agent has
to do a lookup to see if the city is stil alive. This is fixed in the
unit_agents patch (which will only be integrated if we have the path
finding applied).

> > diff -Nurd -X clean/diff_ignore -x *.sav* clean/po/Makefile.in.in 
> > work/po/Makefile.in.in
> > --- clean/po/Makefile.in.in Tue Aug 27 17:27:31 2002
> > +++ work/po/Makefile.in.in  Tue Aug 27 18:31:17 2002
> > @@ -65,7 +65,7 @@
> >     $(MSGMERGE) $< $(srcdir)/$(PACKAGE).pot -o $*.pox
> >  
> >  .po.mo:
> > -   $(MSGFMT) -c -o $@ $<
> > +   $(MSGFMT) -o $@ $<
> 
> hmm... I have my own idea for this... but definitely I don't think that
> such code "$(MSGFMT) -c -o $@ $<" is in the CVS, or perhaps I'm wrong??
> 
> Otherwise it would be nice to have the -c parameter in CVS! :-)

Uhhh. This shouldn't be in the patch. Sorry.

> P.S. You introduced 2 new packets, I suspect that a capability string is
> needed for this to cope with old server/client.

Yes. But this is easy since we also need a mandatory capability for
the new year2turn patch (prepatch for the new calendar) and for
Thomas's new tax calculation.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Despite all the medical advances of the 20th century, the mortality 
  rate remains unchanged at 1 death per person."