[Freeciv-Dev] Re: (PR#6260) server bug: city tile status
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Jason Short wrote:
> Here's another example of the type of problems we have here.
>
> create_city first initializes all values in the city_map to either EMPTY
> or UNAVAILABLE, depending on the return value of city_can_work_tile.
> Then it updates the border information for the city, then marks the city
> center as WORKER.
The last two steps at least should be reversed.
> There is no end to the problems this simple code has because of the
> current design.
>
> - Updating the border information updates the city tile status for this
> city, among others.
Proably a mistake. At most it should (re)move workers (to the best available
tile, entertainer status or whatever) if they are in enemy territory, but not
call auto-arrange_workers. The latter is evil big-brotherism.
> This should result in calling
> auto_arrange_workers(). But the city tile status for this city isn't in
> a sane state because the center tile is unworked. Only by chance is
> auto_arrange_workers not called in this case - update_city_tile_status
> only calles it when a worked tile becomes unavailable; when an
> unavailable tile becomes available it relies on the caller to do it.
This is no longer a problem if you step back and order things correctly.
> - Marking the center tile as worked can't be done before the border
> update, since city_can_work_tile() may not return true for this tile (if
> the city is in enemy territory).
Definitely a mistake. Borders is broken if a city center tile is not owned
by the city in question always and forever. There is a strong argument for
all adjacent city tiles to always be owned by the city with the only exception
being if two cities are right next to each other so adjacents overlap.
In Civ III, if the cultural strength is so strong as to overcome the basic
city center tile weight, then there is a revolution and the city transfers
to the dominating Civ.
> - Even initializing the city_map array makes little sense, since the
> border information hasn't been updated at this point. E.g., if we found
> a city in enemy territory we'll initialize the city center to UNAVAILABLE.
Really, really bad ...
> In this situation, as in most similar places in the code, the problem is
> that we're trying to do everything in one step.
Not really. You are trying to do things that don't make a lot of sense.
> The above code needs to
> use a borders update function that will not call auto_arrange_workers
> for the created city (but, in general, should call it for other cities).
I agree that borders update should not call auto-arrange_workers, but mamke
that true in all cases.
> update_city_tile_status should never call auto_arrange_workers. It
> should instead return a TRUE value if the caller needs to call it. This
> value may need to be propogated back upward (as in the above case) so
> that it's not called until the city map is in a sane state.
No, always make sure that one can create and initialize a city_map to a sane
state before calling any of these other functions, i.e. the city center and
adjacent tiles can alwyas be taken over and populated, even if the later
calls change this for adjacent.
> One option might be to not call auto_arrange_workers at all at this
> point, but rather to set the pcity->workers_need_arrangement flag. This
> flag can be checked when sync_cities is called. This would be good,
> since then auto_arrange_workers needn't be recursive.
Even worse ...
Just make sure that the order of operations is well-defined and *not*
overlapping with foolish things like not being able to initialize a city_map
without calling borders update.
Don't make life so complex trying to handle a broken implementation elsewhere
that one can never figure out where to start, or bow many levels of recursion
will be needed to unravel a simple basic operation. Find and solve the initial
mistake, don't compound it.
> jason
Cheers,
RossW
=====
|
|
