From dbaff855465a3a599ec7ab63766646e092fbb928 Mon Sep 17 00:00:00 2001 From: Louis Moureaux Date: Tue, 14 Nov 2023 00:41:53 +0100 Subject: [PATCH] Fix map sharing Sharing maps between players was fundamentally broken. The server records the last turn in which a player saw a tile. This information is only updated when the player stops seeing a tile, and not when it just keeps seeing it because the tile is within its vision range. The following map sharing bugs existed: * Tiles seen by the giving player were not sent if the receiving player had last viewed them after the giving player; * Cities were only updated when destroyed. Renamed or growing cities were not updated. This commit fixes both bugs. The core logic in really_give_tile_info_from_player_to_player was completely overhauled to make it easier to follow and more correct. In addition, player_tile.site was turned to a unique_ptr to facilitate bookkeeping. --- common/vision.cpp | 5 -- common/vision.h | 1 - server/citytools.cpp | 26 +++----- server/diplomats.cpp | 5 +- server/maphand.cpp | 120 +++++++++++++--------------------- server/maphand.h | 8 +-- server/savegame/savegame2.cpp | 4 +- server/savegame/savegame3.cpp | 6 +- server/srv_main.cpp | 2 +- server/unittools.cpp | 2 +- 10 files changed, 66 insertions(+), 113 deletions(-) diff --git a/common/vision.cpp b/common/vision.cpp index 20ef5d6a24..33add6c348 100644 --- a/common/vision.cpp +++ b/common/vision.cpp @@ -61,11 +61,6 @@ bool vision_reveal_tiles(struct vision *vision, bool reveal_tiles) return was; } -/** - Frees vision site structure. - */ -void vision_site_destroy(struct vision_site *psite) { delete[] psite; } - /** Returns the basic structure. */ diff --git a/common/vision.h b/common/vision.h index 56cfb676ad..62c692cb0a 100644 --- a/common/vision.h +++ b/common/vision.h @@ -130,7 +130,6 @@ struct vision_site { }; #define vision_site_owner(v) ((v)->owner) -void vision_site_destroy(struct vision_site *psite); struct vision_site *vision_site_new(int identity, struct tile *location, struct player *owner); struct vision_site *vision_site_new_from_city(const struct city *pcity); diff --git a/server/citytools.cpp b/server/citytools.cpp index 26706d1e50..b25dd5ff92 100644 --- a/server/citytools.cpp +++ b/server/citytools.cpp @@ -2128,7 +2128,7 @@ bool send_city_suppression(bool now) static void package_dumb_city(struct player *pplayer, struct tile *ptile, struct packet_city_short_info *packet) { - struct vision_site *pdcity = map_get_player_city(ptile, pplayer); + const vision_site *pdcity = map_get_player_city(ptile, pplayer); fc_assert_ret(pdcity != nullptr); packet->id = pdcity->identity; @@ -2693,18 +2693,15 @@ bool update_dumb_city(struct player *pplayer, struct city *pcity) */ void reality_check_city(struct player *pplayer, struct tile *ptile) { - struct vision_site *pdcity = map_get_player_city(ptile, pplayer); + auto playtile = map_get_player_tile(ptile, pplayer); - if (pdcity) { + if (playtile->site && playtile->site->location == ptile) { 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); - fc_assert_ret(playtile->site == pdcity); + if (!pcity || pcity->id != playtile->site->identity) { + dlsend_packet_city_remove(pplayer->connections, + playtile->site->identity); playtile->site = nullptr; - vision_site_destroy(pdcity); } } } @@ -2714,15 +2711,12 @@ void reality_check_city(struct player *pplayer, struct tile *ptile) */ void remove_dumb_city(struct player *pplayer, struct tile *ptile) { - struct vision_site *pdcity = map_get_player_city(ptile, pplayer); - - if (pdcity) { - struct player_tile *playtile = map_get_player_tile(ptile, pplayer); + auto playtile = map_get_player_tile(ptile, pplayer); - dlsend_packet_city_remove(pplayer->connections, pdcity->identity); - fc_assert_ret(playtile->site == pdcity); + if (playtile->site && playtile->site->location == ptile) { + dlsend_packet_city_remove(pplayer->connections, + playtile->site->identity); playtile->site = nullptr; - vision_site_destroy(pdcity); } } diff --git a/server/diplomats.cpp b/server/diplomats.cpp index d795ee0c63..5f660631a1 100644 --- a/server/diplomats.cpp +++ b/server/diplomats.cpp @@ -378,9 +378,8 @@ void spy_send_sabotage_list(struct connection *pc, struct unit *pdiplomat, improvement_iterate_end; } else { // Can't see hidden buildings. - struct vision_site *plrcity; - - plrcity = map_get_player_city(city_tile(pcity), unit_owner(pdiplomat)); + const vision_site *plrcity = + map_get_player_city(city_tile(pcity), unit_owner(pdiplomat)); if (!plrcity) { // Must know city to remember visible buildings. diff --git a/server/maphand.cpp b/server/maphand.cpp index 4daf475612..23b0d2d556 100644 --- a/server/maphand.cpp +++ b/server/maphand.cpp @@ -563,7 +563,7 @@ void send_tile_info(struct conn_list *dest, struct tile *ptile, send_packet_tile_info(pconn, &info); } else if (pplayer && map_is_known(ptile, pplayer)) { struct player_tile *plrtile = map_get_player_tile(ptile, pplayer); - struct vision_site *psite = map_get_player_site(ptile, pplayer); + const vision_site *psite = map_get_player_site(ptile, pplayer); info.known = TILE_KNOWN_UNSEEN; info.continent = tile_continent(ptile); @@ -1128,17 +1128,9 @@ static void map_change_own_seen(struct player *pplayer, struct tile *ptile, void change_playertile_site(struct player_tile *ptile, struct vision_site *new_site) { - if (ptile->site == new_site) { - // Do nothing. - return; - } - - if (ptile->site != nullptr) { - // Releasing old site from tile - vision_site_destroy(ptile->site); + if (ptile->site.get() != new_site) { + ptile->site.reset(new_site); } - - ptile->site = new_site; } /** @@ -1190,9 +1182,8 @@ void show_map_to_all() */ void player_map_init(struct player *pplayer) { - pplayer->server.private_map = static_cast( - fc_realloc(pplayer->server.private_map, - MAP_INDEX_SIZE * sizeof(*pplayer->server.private_map))); + delete[] pplayer->server.private_map; + pplayer->server.private_map = new player_tile[MAP_INDEX_SIZE]; whole_map_iterate(&(wld.map), ptile) { player_tile_init(ptile, pplayer); } whole_map_iterate_end; @@ -1212,7 +1203,7 @@ void player_map_free(struct player *pplayer) whole_map_iterate(&(wld.map), ptile) { player_tile_free(ptile, pplayer); } whole_map_iterate_end; - free(pplayer->server.private_map); + delete[] pplayer->server.private_map; pplayer->server.private_map = nullptr; pplayer->tile_known->clear(); } @@ -1323,11 +1314,7 @@ static void player_tile_init(struct tile *ptile, struct player *pplayer) */ static void player_tile_free(struct tile *ptile, struct player *pplayer) { - struct player_tile *plrtile = map_get_player_tile(ptile, pplayer); - - if (plrtile->site != nullptr) { - vision_site_destroy(plrtile->site); - } + map_get_player_tile(ptile, pplayer)->site = nullptr; } /** @@ -1349,7 +1336,7 @@ struct vision_site *map_get_player_city(const struct tile *ptile, struct vision_site *map_get_player_site(const struct tile *ptile, const struct player *pplayer) { - return map_get_player_tile(ptile, pplayer)->site; + return map_get_player_tile(ptile, pplayer)->site.get(); } /** @@ -1459,63 +1446,46 @@ static void really_give_tile_info_from_player_to_player(struct player *pfrom, struct player *pdest, struct tile *ptile) { - struct player_tile *from_tile, *dest_tile; - if (!map_is_known_and_seen(ptile, pdest, V_MAIN)) { - /* I can just hear people scream as they try to comprehend this if :). - * Let me try in words: - * 1) if the tile is seen by pfrom the info is sent to pdest - * OR - * 2) if the tile is known by pfrom AND (he has more recent info - * OR it is not known by pdest) - */ - if (map_is_known_and_seen(ptile, pfrom, V_MAIN) - || (map_is_known(ptile, pfrom) - && (((map_get_player_tile(ptile, pfrom)->last_updated - > map_get_player_tile(ptile, pdest)->last_updated)) - || !map_is_known(ptile, pdest)))) { - from_tile = map_get_player_tile(ptile, pfrom); - dest_tile = map_get_player_tile(ptile, pdest); - // Update and send tile knowledge - map_set_known(ptile, pdest); - dest_tile->terrain = from_tile->terrain; - dest_tile->extras = from_tile->extras; - dest_tile->resource = from_tile->resource; - dest_tile->owner = from_tile->owner; - dest_tile->extras_owner = from_tile->extras_owner; - dest_tile->last_updated = from_tile->last_updated; - send_tile_info(pdest->connections, ptile, false); - - // update and send city knowledge - // remove outdated cities - if (dest_tile->site) { - if (!from_tile->site) { - /* As the city was gone on the newer from_tile - it will be removed by this function */ - reality_check_city(pdest, ptile); - } else // We have a dest_city. update - if (from_tile->site->identity != dest_tile->site->identity) { - /* As the city was gone on the newer from_tile - it will be removed by this function */ - reality_check_city(pdest, ptile); - } - } + // No need to transfer if pdest can see the tile + if (map_is_known_and_seen(ptile, pdest, V_MAIN)) { + return; + } - // Set and send new city info - if (from_tile->site) { - if (!dest_tile->site) { - /* We cannot assign new vision site with change_playertile_site(), - * since location is not yet set up for new site */ - dest_tile->site = vision_site_new(0, ptile, nullptr); - *dest_tile->site = *from_tile->site; - } - /* Note that we don't care if receiver knows vision source city - * or not. */ - send_city_info_at_tile(pdest, pdest->connections, nullptr, ptile); - } + // Nothing to transfer if the pfrom doesn't know the tile + if (!map_is_known(ptile, pfrom)) { + return; + } - city_map_update_tile_frozen(ptile); - } + // If pdest knows the tile, check that pfrom has more recent info + if (map_is_known(ptile, pdest) + && !map_is_known_and_seen(ptile, pfrom, V_MAIN) + && map_get_player_tile(ptile, pfrom)->last_updated + <= map_get_player_tile(ptile, pdest)->last_updated) { + return; } + + // Transfer knowldege + auto from_tile = map_get_player_tile(ptile, pfrom); + auto dest_tile = map_get_player_tile(ptile, pdest); + + // Update and send tile knowledge + map_set_known(ptile, pdest); + dest_tile->terrain = from_tile->terrain; + dest_tile->extras = from_tile->extras; + dest_tile->resource = from_tile->resource; + dest_tile->owner = from_tile->owner; + dest_tile->extras_owner = from_tile->extras_owner; + dest_tile->last_updated = from_tile->last_updated; + send_tile_info(pdest->connections, ptile, false); + + // Set and send latest city info + if (from_tile->site) { + change_playertile_site(dest_tile, new vision_site(*from_tile->site)); + // Note that we don't care if receiver knows vision source city or not. + send_city_info_at_tile(pdest, pdest->connections, nullptr, ptile); + } + + city_map_update_tile_frozen(ptile); } /** diff --git a/server/maphand.h b/server/maphand.h index 580656686a..f74683b6fe 100644 --- a/server/maphand.h +++ b/server/maphand.h @@ -25,10 +25,10 @@ struct section_file; struct conn_list; struct player_tile { - struct vision_site *site; // nullptr for no vision site - struct extra_type *resource; // nullptr for no resource - struct terrain *terrain; // nullptr for unknown tiles - struct player *owner; // nullptr for unowned + std::unique_ptr site; // nullptr for no vision site + struct extra_type *resource; // nullptr for no resource + struct terrain *terrain; // nullptr for unknown tiles + struct player *owner; // nullptr for unowned struct player *extras_owner; bv_extras extras; diff --git a/server/savegame/savegame2.cpp b/server/savegame/savegame2.cpp index 82889f9412..ab7ef6c960 100644 --- a/server/savegame/savegame2.cpp +++ b/server/savegame/savegame2.cpp @@ -4635,9 +4635,7 @@ static void sg_load_player_vision(struct loaddata *loading, } else { // Error loading the data. log_sg("Skipping seen city %d for player %d.", i, plrno); - if (pdcity != nullptr) { - vision_site_destroy(pdcity); - } + delete pdcity; } } diff --git a/server/savegame/savegame3.cpp b/server/savegame/savegame3.cpp index cd60938083..c315d65afe 100644 --- a/server/savegame/savegame3.cpp +++ b/server/savegame/savegame3.cpp @@ -6728,9 +6728,7 @@ static void sg_load_player_vision(struct loaddata *loading, } else { // Error loading the data. log_sg("Skipping seen city %d for player %d.", i, plrno); - if (pdcity != nullptr) { - vision_site_destroy(pdcity); - } + delete pdcity; } } @@ -6972,7 +6970,7 @@ static void sg_save_player_vision(struct savedata *saving, i = 0; whole_map_iterate(&(wld.map), ptile) { - struct vision_site *pdcity = map_get_player_city(ptile, plr); + const vision_site *pdcity = map_get_player_city(ptile, plr); char impr_buf[B_LAST + 1]; char buf[32]; diff --git a/server/srv_main.cpp b/server/srv_main.cpp index a1b09915aa..8fc6078f91 100644 --- a/server/srv_main.cpp +++ b/server/srv_main.cpp @@ -3386,7 +3386,7 @@ player *mapimg_server_tile_city(const struct tile *ptile, } if (knowledge && pplayer) { - struct vision_site *pdcity = map_get_player_city(ptile, pplayer); + const vision_site *pdcity = map_get_player_city(ptile, pplayer); if (pdcity) { return pdcity->owner; diff --git a/server/unittools.cpp b/server/unittools.cpp index ec40bf9054..476f18981b 100644 --- a/server/unittools.cpp +++ b/server/unittools.cpp @@ -4336,7 +4336,7 @@ static bool maybe_cancel_patrol_due_to_enemy(struct unit *punit) { struct unit *penemy = is_non_allied_unit_tile(ptile, pplayer); - struct vision_site *pdcity = map_get_player_site(ptile, pplayer); + const vision_site *pdcity = map_get_player_site(ptile, pplayer); if ((penemy && can_player_see_unit(pplayer, penemy)) || (pdcity && !pplayers_allied(pplayer, vision_site_owner(pdcity))