<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40092 >
In PR#40072, the second reported error has Teotitlan shown with the wrong owner. This patch does not fix the underlying issue, but is a step toward making the savegame playable. The savegame.c:2800 code tries to fix inconsistent games. This leads to reality_check_city() and update_dumb_city(). I've straightened the logic in update_dumb_city(), and added a bit more error checking. Found a bug in PR#39895, missing one crucial line: update_vision_site_from_city(). Comparing the current code with 2.1 and 2.0, and walk-through of PR#13718 changes, found a possible improvement through waiting to update the national borders (a few lines), matching older code in 2.0. Other minor cleanup.
Index: server/citytools.c =================================================================== --- server/citytools.c (revision 14391) +++ server/citytools.c (working copy) @@ -186,10 +186,9 @@ terrain_type_iterate(pterrain) { /* Now we do the same for every available terrain. */ - goodness - = is_terrain_near_tile(ptile, pterrain) - ? nc->terrain[terrain_index(pterrain)] - : -nc->terrain[terrain_index(pterrain)]; + goodness = is_terrain_near_tile(ptile, pterrain) + ? nc->terrain[terrain_index(pterrain)] + : -nc->terrain[terrain_index(pterrain)]; if (goodness > 0) { priority /= mult_factor; } else if (goodness < 0) { @@ -998,12 +997,11 @@ vision_reveal_tiles(pcity->server.vision, game.info.city_reveal_tiles); city_refresh_vision(pcity); - tile_set_city(ptile, pcity); + tile_set_city(ptile, pcity); /* partly redundant to server_set_tile_city() */ city_list_prepend(pplayer->cities, pcity); /* Claim the ground we stand on */ map_claim_ownership(ptile, pplayer, ptile); - map_claim_border(ptile, pplayer); if (terrain_control.may_road) { tile_set_special(ptile, S_ROAD); @@ -1031,7 +1029,7 @@ } } - /* Place a worker at the city center; this is the free-worked tile. + /* Place a worker at the is_city_center() is_free_worked_tile(). * This must be done before the city refresh (below) so that the city * is in a sane state. */ /* Ugly detail here is that if another city is currently working @@ -1042,6 +1040,10 @@ * ptile->worked does not point to it, it will give tile up. */ server_set_tile_city(pcity, CITY_MAP_SIZE/2, CITY_MAP_SIZE/2, C_TILE_WORKER); + /* Update the national borders. This affects the citymap tile status, + * so must be done after the above and before arranging workers. */ + map_claim_border(ptile, pplayer); + /* Refresh the city. First a city refresh is done (this shouldn't * send any packets to the client because the city has no supported units) * then rearrange the workers. Note that auto_arrange_workers does its @@ -1172,9 +1174,9 @@ /* idex_unregister_city() is called in game_remove_city() below */ - /* identity_number_release(pcity->id) is *NOT* done! The cities may still be - alive in the client, or in the player map. The number of removed - cities is small, so the loss is acceptable. + /* identity_number_release(pcity->id) is *NOT* done! The cities may + still be alive in the client, or in the player map. The number of + removed cities is small, so the loss is acceptable. */ old_vision = pcity->server.vision; @@ -1730,10 +1732,11 @@ bool update_dumb_city(struct player *pplayer, struct city *pcity) { bv_imprs improvements; - struct vision_site *pdcity = map_get_player_city(pcity->tile, pplayer); + struct tile *ptile = city_tile(pcity); + struct vision_site *pdcity = map_get_player_city(ptile, pplayer); /* pcity->occupied isn't used at the server, so we go straight to the * unit list to check the occupied status. */ - bool occupied = (unit_list_size(pcity->tile->units) > 0); + bool occupied = (unit_list_size(ptile->units) > 0); bool walls = city_got_citywalls(pcity); bool happy = city_happy(pcity); bool unhappy = city_unhappy(pcity); @@ -1747,27 +1750,32 @@ } improvement_iterate_end; if (NULL == pdcity) { + map_get_player_tile(ptile, pplayer)->site = pdcity = create_vision_site_from_city(pcity); - map_get_player_tile(pcity->tile, pplayer)->site = pdcity; - } else if (pdcity->identity == pcity->id - && vision_owner(pdcity) == city_owner(pcity) - && pdcity->size == pcity->size - && 0 == strcmp(pdcity->name, city_name(pcity)) - && pdcity->occupied == occupied + } else if (pdcity->location != ptile) { + freelog(LOG_ERROR, "Trying to update bad city (wrong location)" + " at %i,%i for player %s", + TILE_XY(pcity->tile), + player_name(pplayer)); + pdcity->location = ptile; /* ?? */ + } else if (pdcity->identity != pcity->id) { + freelog(LOG_ERROR, "Trying to update old city (wrong identity)" + " at %i,%i for player %s", + TILE_XY(pcity->tile), + player_name(pplayer)); + pdcity->identity = pcity->id; /* ?? */ + } else if (pdcity->occupied == occupied && pdcity->walls == walls && pdcity->happy == happy && pdcity->unhappy == unhappy - && BV_ARE_EQUAL(pdcity->improvements, improvements)) { + && BV_ARE_EQUAL(pdcity->improvements, improvements) + && pdcity->size == pcity->size + && vision_owner(pdcity) == city_owner(pcity) + && 0 == strcmp(pdcity->name, city_name(pcity))) { return FALSE; } - if (pdcity->identity != pcity->id) { - freelog(LOG_ERROR, "Trying to update old city (wrong ID)" - " at %i,%i for player %s", - TILE_XY(pcity->tile), - player_name(pplayer)); - pdcity->identity = pcity->id; /* ?? */ - } + update_vision_site_from_city(pdcity, pcity); pdcity->occupied = occupied; pdcity->walls = walls; pdcity->happy = happy; @@ -1782,12 +1790,14 @@ **************************************************************************/ void reality_check_city(struct player *pplayer,struct tile *ptile) { - struct city *pcity = tile_city(ptile); - struct player_tile *playtile = map_get_player_tile(ptile, pplayer); struct vision_site *pdcity = map_get_player_city(ptile, pplayer); if (pdcity) { + struct city *pcity = tile_city(ptile); + if (!pcity || pcity->id != pdcity->identity) { + struct player_tile *playtile = map_get_player_tile(ptile, pplayer); + dlsend_packet_city_remove(pplayer->connections, pdcity->identity); playtile->site = NULL; free(pdcity); Index: server/maphand.c =================================================================== --- server/maphand.c (revision 14391) +++ server/maphand.c (working copy) @@ -1742,6 +1742,26 @@ #endif /************************************************************************* + Change ownership of a single tile. + + This implementation is somewhat inefficient. By design, it's not + called often. Does not send updates to client. +*************************************************************************/ +static void map_change_ownership(struct tile *ptile, struct player *play) +{ +#ifdef CHANGE_CITY_SITE + struct vision_site *psite = map_get_player_site(ptile, play); + + assert(NULL != psite); + update_city_tile_status_map(tile_city(psite->location), ptile); +#else + city_list_iterate(play->cities, pcity) { + update_city_tile_status_map(pcity, ptile); + } city_list_iterate_end; +#endif +} + +/************************************************************************* Claim ownership of a single tile. *************************************************************************/ void map_claim_ownership(struct tile *ptile, struct player *powner, @@ -1761,7 +1781,7 @@ } } - if (NULL != powner /* assume && NULL != psource */) { + if (NULL != powner && NULL != psource) { struct city *pcity = tile_city(ptile); struct player_tile *playtile = map_get_player_tile(psource, powner); @@ -1769,37 +1789,35 @@ if (ptile != psource) { map_get_player_tile(ptile, powner)->site = playtile->site; } else if (NULL != pcity) { - playtile->site = update_vision_site_from_city(playtile->site, pcity); + update_vision_site_from_city(playtile->site, pcity); } else { /* has new owner */ playtile->site->owner = powner; } } else { assert(ptile == psource); + assert(NULL != pcity); /* FIXME: temporary IDENTITY_NUMBER_ZERO */ if (NULL != pcity) { playtile->site = create_vision_site_from_city(pcity); } else { - playtile->site = create_vision_site(-1/*FIXME*/, psource, powner); + playtile->site = create_vision_site(IDENTITY_NUMBER_ZERO, psource, powner); } } + } else { + assert(NULL == powner && NULL == psource); } tile_set_owner(ptile, powner); send_tile_info(NULL, ptile, FALSE); - /* This implementation is somewhat inefficient. By design, it's not - * called often. Does not send updates to client. - */ - if (NULL != ploser && ploser != powner) { - city_list_iterate(ploser->cities, pcity) { - update_city_tile_status_map(pcity, ptile); - } city_list_iterate_end; + if (ploser != powner) { + if (NULL != ploser) { + map_change_ownership(ptile, ploser); + } + if (NULL != powner) { + map_change_ownership(ptile, powner); + } } - if (NULL != powner && ploser != powner) { - city_list_iterate(powner->cities, pcity) { - update_city_tile_status_map(pcity, ptile); - } city_list_iterate_end; - } } /************************************************************************* Index: common/vision.c =================================================================== --- common/vision.c (revision 14391) +++ common/vision.c (working copy) @@ -99,8 +99,8 @@ ****************************************************************************/ struct vision_site *create_vision_site_from_city(const struct city *pcity) { - struct vision_site *psite = create_vision_site(pcity->id, pcity->tile, - city_owner(pcity)); + struct vision_site *psite = + create_vision_site(pcity->id, city_tile(pcity), city_owner(pcity)); psite->size = pcity->size; sz_strlcpy(psite->name, city_name(pcity)); @@ -110,12 +110,12 @@ /**************************************************************************** Returns the basic structure filled with current elements. ****************************************************************************/ -struct vision_site *update_vision_site_from_city(struct vision_site *psite, - const struct city *pcity) +void update_vision_site_from_city(struct vision_site *psite, + const struct city *pcity) { - psite->identity = pcity->id; - psite->owner = pcity->owner; + /* should be same identity and location */ + psite->owner = city_owner(pcity); + psite->size = pcity->size; sz_strlcpy(psite->name, city_name(pcity)); - return psite; } Index: common/vision.h =================================================================== --- common/vision.h (revision 14391) +++ common/vision.h (working copy) @@ -113,12 +113,12 @@ struct tile *location; /* Cannot be NULL */ struct player *owner; /* May be NULL, always check! */ - int identity; /* city/unit >= 100 */ + int identity; /* city > IDENTITY_NUMBER_ZERO */ + unsigned short size; bool occupied; bool walls; bool happy; bool unhappy; - unsigned short size; bv_imprs improvements; }; @@ -128,8 +128,8 @@ struct vision_site *create_vision_site(int identity, struct tile *location, struct player *owner); struct vision_site *create_vision_site_from_city(const struct city *pcity); -struct vision_site *update_vision_site_from_city(struct vision_site *psite, - const struct city *pcity); +void update_vision_site_from_city(struct vision_site *psite, + const struct city *pcity); /* get 'struct site_list' and related functions: */ #define SPECLIST_TAG site
_______________________________________________ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev