[Freeciv-Dev] foggy problems

[Jeff Mallatt <jjm@xxxxxxxxxxxx>]

There seem to be some problems with the new full Fog of War code, one of
which is causing an infinite loop in the server.  They all seem to have
something to do with resolve_unit_stack() and its calling routines
(transfer_city*() and civil_war()).

I've uploaded a test save-file to:
    <ftp://ftp.freeciv.org/freeciv/incoming/res_stack_inf_loop.sav.bz2>
Just load, connect as jjm, and start.  The infinite loop happens in 1745.

One thing I noticed that may be a problem with the new stack conflict
resolution code is in civil_war(), where we see:

  /* Transfer city and units supported by this city to the new owner

     We do NOT resolve stack conflicts here, but rather later.
     Reason: if we have a transporter from one city which is carrying
     a unit from another city, and both cities join the rebellion. We
     resolved stack conflicts for each city we would teleport the first
     of the units we met since the other would have another owner */
  if(!(pnewcity = transfer_city(cplayer, pplayer, pcity, -1, 0, 1))){
    freelog(LOG_VERBOSE,
            "Transfer city returned no city - aborting civil war.");
    return;
  }

The comment says to *not* resolve stack conflicts, yet the final argument
to transfer_city() is '1', specifying that it *should* resolve stack
conflicts.  Either the comment or the argument is wrong (I believe the
argument should be '0' -- the comment is correct in stating that stack
conflicts are resolved later).

Also, there seems to be a bug (not introduced by the full Fog patch)
whereby if transfer_city() returns NULL, civil_war() returns immediately,
without resolving stack conflicts.  This might be okay, given certain
assumptions about the behavior of transfer_city(), but I think it may be
safer to add a stack conflict resolution loop:

  unit_list_iterate(pplayer->units, punit) 
    resolve_unit_stack(punit->x, punit->y, 0);
  unit_list_iterate_end;

just before the 'return;'.

Another thing I noticed is that the patch re-used the x and y variables in
transfer_city_units(), by setting them to the location of the vunit (in the
"unit_list_iterate(vcity->units_supported, vunit)" loop).  This breaks the
"kill_outside" test, which uses x and y as the location of the vcity.

However, none of the above seem to be the cause of the infinite loop.  The
infinite loop happens as a result of a call to handle_unit_enter_city().
Here's a partial backtrace:

#0  resolve_unit_stack (x=65, y=25, verbose=1) at unittools.c:759
#1  0x804d151 in transfer_city_units (pplayer=0x8210ac8,
    pvictim=0x8212908, pcity=0x82a7b28, vcity=0x82596c8,
    kill_outside=0, verbose=0, resolve_stack=1) at citytools.c:683
#2  0x804d7a9 in transfer_city (pplayer=0x8210ac8, cplayer=0x8212908,
    pcity=0x82596c8, kill_outside=0, transfer_unit_verbose=0,
    resolve_stack=1) at citytools.c:1027
#3  0x80740d6 in handle_unit_enter_city (pplayer=0x8210ac8,
    pcity=0x82596c8) at unithand.c:1240

While I was there, I noticed in handle_unit_enter_city() that the patch
changed the code to wait to do the civil war processing until the end of
the routine.  I think this introduces a bug whereby if the city is of size
one and is destroyed, handle_unit_enter_city() now returns without having
done the civil war.  (Aside:  Aren't short-circuit 'return's evil?)

Now, handle_unit_enter_city() calls down through transfer_city() into
transfer_city_units(), where the patch has added a new call to
resolve_unit_stack():

  if (resolve_stack) {
  /* Now make sure that if we just deleted a transporter the carried units are
     not left floating in the water */
    resolve_unit_stack(x,y,1);
  }

It's this call that is causing the infinite loop, because the x,y arguments
can be the location of a city (see below).  Basically, I believe we
shouldn't make this call whenever x,y is the location of a city.

Finally, here are my thoughts as to the direct cause of the infinite loop
bug:  When resolve_unit_stack() is called in this situation (from
transfer_city() and transfer_city_units()), it is possible for pcity to be
at the same location as punit.  This leads to punit not moving (because it
already is in the closest city), and (having not moved) being itself the
next unit in the list to try to move, which it never does... loop...
However, I don't think this needs to be changed.

As a test, I simply removed the resolve_unit_stack() call from the loop in
transfer_city_units(), and re-ran my res_stack_inf_loop.sav test.  The
infinite loop went away.  (Though a few "Get_def bugged" messages appeared
-- so the real fix is a little more complex.)

The attached patch does the following:

- Conditionalizes the resolve_unit_stack() call in the loop in
transfer_city_units() on x,y not being a city location.

- Minor clean-up of resolve_unit_stack().

- Removes re-use of x and y in transfer_city_units().

- Changes transfer_city() call in civil_war() to not ask for resolving
stack conflicts.

- Adds a stack conflict resolution loop just before the short-circuit
'return;' in civil_war().

- Adds "if (do_civil_war) civil_war(cplayer);" before the short-circuit
'return;' in handle_unit_enter_city().

All of this is quite convoluted, and I could easily have missed something.
(With the attached patch, there is no infinite loop, nor any "Get_def
bugged" messages.)

fog-fix-1.diff
Description: Text document

jjm