[Freeciv-Dev] Re: CMA failed assertion caused by Freight (PR#1684)

[rf13@xxxxxxxxxxxxxxxxx]

On Thu, Sep 26, 2002 at 03:11:29AM -0700, Per I. Mathisen wrote:
> On Sat, 14 Sep 2002, Raimar Falke wrote:
> > > > This is a nasty one: the freight moves and makes a tile to Oulu
> > > > unavailable and later available again. The worker is removed the first
> > > > time and isn't put to work the second time. First problem: Oulu isn't
> > > > recalulated the second time. Second problem: the client isn't informed
> > > > about this. However if the unit arrives at Tampere the server also
> > > > recalculates Oulu which now have a worker less (than the current info
> > > > of the client) causing reduces trade which causes reduces trade from
> > > > trade route which causes a mismatch between server and client.
> > >
> > > > There is IMHO no easy fix and recalculating the cities at the server
> > > > (which will be part of the fix) will make the server slower. So since
> > > > it uncommon and not critical I would vote for post-release.
> > >
> > > And the patch. AI-people please comment on the use of
> > > auto_arrange_workers.
> 
> Index: server/citytools.c
> ===================================================================
> --- server/citytools.c        2002/09/02 02:19:54     1.189
> +++ server/citytools.c        2002/09/14 15:42:14
> @@ -2146,12 +2148,23 @@
>        server_set_tile_city(pcity, city_x, city_y, C_TILE_UNAVAILABLE);
>        add_adjust_workers(pcity); /* will place the displaced */
>        city_refresh(pcity);
> +      send_city_info(NULL, pcity);
>      }
>      break;
> 
>    case C_TILE_UNAVAILABLE:
>      if (is_available) {
> +      /* Was auto_arrange_workers called? This avoids recursion. */
> +      static bool aaw_called = FALSE;
> +
>        server_set_tile_city(pcity, city_x, city_y, C_TILE_EMPTY);
> +
> +      if (!aaw_called) {
> +     aaw_called = TRUE;
> +     auto_arrange_workers(pcity);
> +     aaw_called = FALSE;
> +     send_city_info(NULL, pcity);
> +      }
>      }
>      break;

> This looks very wrong. I don't like the way it is currently done,
> and your patch is another bad kludge on top of it. The use of of a
> static variable here screams of wrongness.

I agree that it isn't a good solution. But it is a solution.

> I don't understand why you cannot leave it at sending the new
> information to the client. The use of auto arrange workers here
> should be unnecessary for the CMA.

The citymap state and the city status has to be
consistent. server_set_tile_city doesn't update the city
status. auto_arrange_workers is just a bonus here.

> As I said earlier, auto arrange worker code should not be called this deep
> in the code. That also applies to current case C_TILE_WORKER. Instead, I
> suggest displaced workers are turned into elvises, and that a new city
> bitvector is used to tell if a tile has been previously displaced, so that
> if the tile becomes available again, we remove the last specialist and put
> a worker in the tile instead.

Sounds complicated if the server uses CM.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx