Merge lp:~widelands-dev/widelands/bug-1559729-lost_portspace into lp:widelands

[Miroslav Remák]

If:
 1. The second field to the SE of a possible port space is owned by the player,
 2. AND the player hasn't expanded far enough to own the water fields necessary for portdocks or the border is blocking them (i.e. the blue portspace icon is not visible yet),

then when the player saves the game, the saving process incorrectly cleans up this port space, considering it invalid.

[TiborB (tiborb95)]

I see in fact two issues here, but I am not that far working on that:

a) is_port_space(f) - it seems there is something broken with this, it says false, even though should say true

b) portdock - the issue here is that if a port is build there and is not able to establish a portdock, it will crash the game. Unless the port expands a territory a bit...

I thinks we should fix "a)" item

There is a code:

if ((buildsize == BaseImmovable::BIG) && is_port_space(f) && !find_portdock(f).empty())

and in this case is_port_space(f) should return true and !find_portdock(f).empty() should return false, until the player conquers some sea area

My opinion

[TiborB (tiborb95)]

Or we can just consider new placeholder, indicating that a port might be built here, but not all conditions are met.... f.e. shape of building placeholder (SMALL/MEDIUM/BIG), but of blue color. I know there has been some suggestions in this regard before....

[TiborB (tiborb95)]

BTW where is the code that reinsert port coordinates into port_spaces?

[Miroslav Remák (miroslavr256)]

I don't understand, are these issues consequences of my changes?

a) I can't reproduce it. When does this happen? Maybe include a savegame?
b) Again, I don't know when or how this can happen. You shouldn't be allowed to build on such a spot... But maybe I am missing something. I'm not too familiar with the seafaring code.

> BTW where is the code that reinsert port coordinates into port_spaces?
What do you mean by reinsert? If you mean where they are loaded, then that would be MapPortSpacesPacket.

[TiborB (tiborb95)]

Few months (weeks?) ago I fixed a bug in find_portdock, when it did not test existence of another portdock on a field nor ownership of a field. Now you are partially reverting the change.

I had some printfs in the code that showed how are is_port_space(f) and !find_portdock(f).empty() evaluated and it really showed that is_port_space(f) returned "false" without good reason. Only reason was that somewhere under the hood the field was not listed in port_spaces.

So I think the problem is in port_spaces concept. Should they be completely static - created only in editor? Or is there a problem with pushing a field that re-gained port capabilities back to port_spaces?

I believe the game should behave this way: If a port cannot be built there, because there is no room for portdock - PORT placeholder should NOT be be shown. As soon as some nearby water is conquered, PORT placeholder should be shown.

So in summary, the problem is not where you are fixing it. IMHO

Gun should also say her opinion, it is her code.

[GunChleoc (gunchleoc)]

Actually, it's not my code - you are much more familiar with it than I am, Tibor. We somehow have to remember the original port spaces that were there during map creation or maybe have been added later through a scenario script, so we can reenable them once the space gets freed up again.

[TiborB (tiborb95)]

I made alternative branch, that allows realtime adding/removing portspaces, see: lp:~widelands-dev/widelands/bug-1559729-2

But the result is that it enables portspaces not intended by map creator, see screenshot:

http://postimg.org/image/4nlzfgfm5/

This is probably something we dont want, so I will try something else...

[TiborB (tiborb95)]

My idea is to modify this part of code:
http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1559729-lost_portspace/view/head:/src/logic/map.cc#L2104

if in editor, it should delete port spaces, but if in game - do not delete them, coordinates should be preserved until obstacles for port space are gone.

I understand this is called whenever you click the save map/game. But I dont know how to find out if it is the game or editor, so I am not able to test it right now....

At least this is how I understand the logic...

[Miroslav Remák (miroslavr256)]

I still don't understand where you're going with all this.

> Should they be completely static - created only in editor?

I think port_spaces_ should ALWAYS hold coordinates of all the port spaces defined in the map and we shouldn't ever need to call set_port_space during normal gameplay (except maybe via scripting). It also works like that right now and I don't see a problem with that.

> Only reason was that somewhere under the hood the field was not listed in port_spaces.

That's an entirely valid reason. It's not a port space because it wasn't defined by the map author as such.

> I believe the game should behave this way: If a port cannot be built there, because there is no room for portdock - PORT placeholder should NOT be be shown. As soon as some nearby water is conquered, PORT placeholder should be shown.

That's exactly how the game behaves right now...

> I understand this is called whenever you click the save map/game. But I dont know how to find out if it is the game or editor, so I am not able to test it right now....

That only applies to saving maps in the editor. The idea is to remove port spaces that the map maker invalidates e.g. by changing the height of the port space field.

[TiborB (tiborb95)]

I am sure Map::allows_seafaring() is called even during normal game and it can delete a port space. Just put there simple printf to see that it is called. Or use whatever other mean you like. This is the core of problem.

The consequence is that if you dont save/load the game, port space can re-appear. After save/load it can be lost.

Please test it

[Miroslav Remák (miroslavr256)]

No, it's not called during normal game. Just search for any "allows_seafaring" occurrences in the code. I've tested it as well just to make sure.

[TiborB (tiborb95)]

Well, I cannot reproduce it. I really believed I saw it. I will look at your solution once more..

[TiborB (tiborb95)]

You say, with your change, you are able to load the savegame from bug, dismantle shipyard and portspace re-appears?

[Miroslav Remák (miroslavr256)]

No, I've never said that. The port space has been removed from the savegame permanently due to a saving bug in MapPortSpacesPacket::write that this merge request fixes. You will have to start the game anew.

[Miroslav Remák (miroslavr256)]

The conditions for this bug to happen (see the description of my merge request) were already met when the bug reporter made that savegame.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 878. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/117424299.
Appveyor build 712. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1559729_lost_portspace-712.

[TiborB (tiborb95)]

Oops, i did not your description carefully

But at the end it really seems that during the game the code can remove some port_spaces if currently not valid. Impossible portdock is one cause, but it can be caused by buildings, trees as well - I am right? I think MapPortSpacesPacket::write should only write content of port_spaces_ without any tests. Or more specifically, it can test them if caled from editor, but not in the game... However, allows_seafaring should take care of testing them in editor instead.

[Miroslav Remák (miroslavr256)]

Yep, you're right. We should get rid of these tests in MapPortSpacesPacket::write altogether.

[Miroslav Remák (miroslavr256)]

Btw, do you happen to know why find_portdock doesn't check ownership against the owner of the given field itself, but rather the second field to the SE of it?

 const FCoords start = br_n(br_n(get_fcoords(c)));
 const Widelands::PlayerNumber owner = start.field->get_owned_by();

[TiborB (tiborb95)]

Well there is no good reason. Usually both fields has the same owner. You can take owner from starting field (and change the code if you thik), but portock is to be searched form expected or current flag position.

[TiborB (tiborb95)]

Looks good to me, I am glad we figured it out (and I apologize for misleading), it is really great this was hunted down and fixed. And I hope this will not backfire somewhere :)

review: Approve

[GunChleoc (gunchleoc)]

Glad you found the bug!

I think we can differentiate in the write function whether we are in a game or in the editor:

    if (!is_a(Game, &egbase)) {
        // cleanup
    }

This way, we would still have the cleanup for the editor.

[TiborB (tiborb95)]

I am not 100% sure, but I believe allows_seafaring() takes care of this (removal of invalid portspaces) and thus no differentiation is needed.

[Miroslav Remák (miroslavr256)]

Yep, I can confirm that allows_seafaring() indeed does take care of that.

[kaputtnik (franku)]

It is really annoying that in the situation of the screenshot no portspace is shown: https://bugs.launchpad.net/widelands/+bug/1559729/+attachment/4607287/+files/shot0000.png

I have to build another blockhouse to expand the territory. But this blockhouse prevent building a port because there is not enough space. So one has to dismantle the blockhouse again...

Finding a possible port space is luck here. And i don't understand why water has to be discovered because ships do not care about the borders.

[TiborB (tiborb95)]

Ships do not care, but portdock (1 or 2 fields in water) needs owned territory. Even if it is not visible it is regular immovable and thus cannot be placed outside of player's territory.

[GunChleoc (gunchleoc)]

I think this is ready for merging now :)

It would still be nice to have the blue building plot symbols around a possible port space, but that's another bug.

@bunnybot merge

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2016-03-06 21:59:16 +0000
+++ src/logic/map.cc	2016-03-22 01:13:29 +0000
@@ -1353,12 +1353,12 @@
 		}
 
 		// If starting point is owned we make sure this field has the same owner
-		if (is_good_water && owner > 0 && f.field->get_owned_by() != owner) {
+		if (is_good_water && owner != neutral() && f.field->get_owned_by() != owner) {
 			is_good_water = false;
 		}
 
 		// ... and is not on a border
-		if (is_good_water && owner > 0 && f.field->is_border()) {
+		if (is_good_water && owner != neutral() && f.field->is_border()) {
 			is_good_water = false;
 		}
 

=== modified file 'src/map_io/map_port_spaces_packet.cc'
--- src/map_io/map_port_spaces_packet.cc	2015-10-24 15:42:37 +0000
+++ src/map_io/map_port_spaces_packet.cc	2016-03-22 01:13:29 +0000
@@ -71,34 +71,13 @@
 	Section & s1 = prof.create_section("global");
 	s1.set_int("packet_version", kCurrentPacketVersion);
 
-
-	// Clean up before saving: Delete port build spaces that are defined for a
-	// FCoord, that can in no way be a building of size big.
-	//
-	// This clean up might interfer with scenarios that alter the terrain or the
-	// height of the map. However those types of scenarios can be seen to be a
-	// rare case in which the port spaces can be handled by rewriting port
-	// spaces via a LUA script once the terrain is changed.
-	Map::PortSpacesSet port_spaces;
 	Map& map = egbase.map();
-	for (const Coords& c : map.get_port_spaces()) {
-		FCoords fc = map.get_fcoords(c);
-		if
-			((map.get_max_nodecaps(egbase.world(), fc) & BUILDCAPS_SIZEMASK) != BUILDCAPS_BIG
-			 ||
-			 map.find_portdock(fc).empty())
-		{
-			continue;
-		}
-		port_spaces.insert(c);
-	}
-
-	const uint16_t num = port_spaces.size();
+	const uint16_t num = map.get_port_spaces().size();
 	s1.set_int("number_of_port_spaces", num);
 
 	Section & s2 = prof.create_section("port_spaces");
 	int i = 0;
-	for (const Coords& c : port_spaces) {
+	for (const Coords& c : map.get_port_spaces()) {
 		set_coords(std::to_string(i++), c, &s2);
 	}
 	prof.write("port_spaces", false, fs);