Map data transfer in need of thorough review

[jeltsch]

I think that the implementation of one-time transfer of map data and of shared vision should be thoroughly reviewed in its entirety. There seem to be numerous bugs. I’ve seen several in the past, from what I remember; in particular there was this one where with shared vision you were shown wrong assignments of territory to tiles (this particular one might or might not have been fixed meanwhile). Just now I saw another map sharing bug in action: you give your world map to someone, and the city data is from various earlier turns apparently (drastically too low city sizes, drastically different city sizes of cities built around the same time, wrong assignment of nations to cities).

How to reproduce these bugs? Well, play a lot of Freeciv with lots of transfers of world and sea maps as well as shared vision and look very carefully at the map data you get. That said, since there seem to be multiple bugs and these bugs seem to often have non-obvious reasons, it would be better, I think, to really thoroughly inspect the code that implements all this sharing of map data (one-shot and continuous sharing).

What’s the expected behavior? Well, that this whole thing works correctly. :slightly_smiling_face: In particular, the newest data available should always be shown. So don’t have old data fetched from another player and have it overwrite newer data from another player or newer data that you already possess.

Regarding platforms, I guess this is a platform-independent problem, but I can tell you that I use freeciv21-client 3.0.886883.1-patch via the Snap package on Ubuntu 20.04 with an Intel processor. The ruleset I’m currently using is LTX as used in LT79.

[lmoureaux]

The bug would be in this code:

freeciv21/server/maphand.cpp

Line 374 in eed1be4

void give_map_from_player_to_player(struct player *pfrom,

Digging a bit, it indeed doesn't look super simple.

[lmoureaux]

After more discussion, it appeared that a current issue is as follows:

• A conquers B
• A shares its map with C
• C sees former B territory as belonging to A, but still sees cities as belonging to B

[lmoureaux]

The city transfer algorithm is buggy when the players have conflicting info about a city (called site in the code). The city update code starts like this:

freeciv21/server/maphand.cpp

Lines 1488 to 1501 in eed1be4

	// 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);
	}
	}

This first if doesn't do anything when both players know the city and its id (identity) didn't change. Then we have:

freeciv21/server/maphand.cpp

Lines 1503 to 1514 in eed1be4

	// 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);
	}

This won't do anything but call send_city_info_at_tile.