Merge lp:~widelands-dev/widelands/bug-1559729-lost_portspace into lp:widelands
- bug-1559729-lost_portspace
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 7906 | ||||
Proposed branch: | lp:~widelands-dev/widelands/bug-1559729-lost_portspace | ||||
Merge into: | lp:widelands | ||||
Diff against target: |
59 lines (+4/-25) 2 files modified
src/logic/map.cc (+2/-2) src/map_io/map_port_spaces_packet.cc (+2/-23) |
||||
To merge this branch: | bzr merge lp:~widelands-dev/widelands/bug-1559729-lost_portspace | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
TiborB | Approve | ||
Review via email: mp+289626@code.launchpad.net |
Commit message
Description of the change
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) wrote : | # |
TiborB (tiborb95) wrote : | # |
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) wrote : | # |
BTW where is the code that reinsert port coordinates into port_spaces?
Miroslav Remák (miroslavr256) wrote : | # |
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 MapPortSpacesPa
TiborB (tiborb95) wrote : | # |
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(
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) wrote : | # |
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) wrote : | # |
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://
This is probably something we dont want, so I will try something else...
TiborB (tiborb95) wrote : | # |
My idea is to modify this part of code:
http://
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) wrote : | # |
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) wrote : | # |
I am sure Map::allows_
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) wrote : | # |
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) wrote : | # |
Well, I cannot reproduce it. I really believed I saw it. I will look at your solution once more..
TiborB (tiborb95) wrote : | # |
You say, with your change, you are able to load the savegame from bug, dismantle shipyard and portspace re-appears?
Miroslav Remák (miroslavr256) wrote : | # |
No, I've never said that. The port space has been removed from the savegame permanently due to a saving bug in MapPortSpacesPa
Miroslav Remák (miroslavr256) wrote : | # |
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) wrote : | # |
Continuous integration builds have changed state:
Travis build 878. State: passed. Details: https:/
Appveyor build 712. State: success. Details: https:/
TiborB (tiborb95) wrote : | # |
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 MapPortSpacesPa
Miroslav Remák (miroslavr256) wrote : | # |
Yep, you're right. We should get rid of these tests in MapPortSpacesPa
Miroslav Remák (miroslavr256) wrote : | # |
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_
const Widelands:
TiborB (tiborb95) wrote : | # |
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) wrote : | # |
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 :)
GunChleoc (gunchleoc) wrote : | # |
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) wrote : | # |
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) wrote : | # |
Yep, I can confirm that allows_seafaring() indeed does take care of that.
kaputtnik (franku) wrote : | # |
It is really annoying that in the situation of the screenshot no portspace is shown: https:/
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) wrote : | # |
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) wrote : | # |
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
Preview Diff
1 | === modified file 'src/logic/map.cc' | |||
2 | --- src/logic/map.cc 2016-03-06 21:59:16 +0000 | |||
3 | +++ src/logic/map.cc 2016-03-22 01:13:29 +0000 | |||
4 | @@ -1353,12 +1353,12 @@ | |||
5 | 1353 | } | 1353 | } |
6 | 1354 | 1354 | ||
7 | 1355 | // If starting point is owned we make sure this field has the same owner | 1355 | // If starting point is owned we make sure this field has the same owner |
9 | 1356 | if (is_good_water && owner > 0 && f.field->get_owned_by() != owner) { | 1356 | if (is_good_water && owner != neutral() && f.field->get_owned_by() != owner) { |
10 | 1357 | is_good_water = false; | 1357 | is_good_water = false; |
11 | 1358 | } | 1358 | } |
12 | 1359 | 1359 | ||
13 | 1360 | // ... and is not on a border | 1360 | // ... and is not on a border |
15 | 1361 | if (is_good_water && owner > 0 && f.field->is_border()) { | 1361 | if (is_good_water && owner != neutral() && f.field->is_border()) { |
16 | 1362 | is_good_water = false; | 1362 | is_good_water = false; |
17 | 1363 | } | 1363 | } |
18 | 1364 | 1364 | ||
19 | 1365 | 1365 | ||
20 | === modified file 'src/map_io/map_port_spaces_packet.cc' | |||
21 | --- src/map_io/map_port_spaces_packet.cc 2015-10-24 15:42:37 +0000 | |||
22 | +++ src/map_io/map_port_spaces_packet.cc 2016-03-22 01:13:29 +0000 | |||
23 | @@ -71,34 +71,13 @@ | |||
24 | 71 | Section & s1 = prof.create_section("global"); | 71 | Section & s1 = prof.create_section("global"); |
25 | 72 | s1.set_int("packet_version", kCurrentPacketVersion); | 72 | s1.set_int("packet_version", kCurrentPacketVersion); |
26 | 73 | 73 | ||
27 | 74 | |||
28 | 75 | // Clean up before saving: Delete port build spaces that are defined for a | ||
29 | 76 | // FCoord, that can in no way be a building of size big. | ||
30 | 77 | // | ||
31 | 78 | // This clean up might interfer with scenarios that alter the terrain or the | ||
32 | 79 | // height of the map. However those types of scenarios can be seen to be a | ||
33 | 80 | // rare case in which the port spaces can be handled by rewriting port | ||
34 | 81 | // spaces via a LUA script once the terrain is changed. | ||
35 | 82 | Map::PortSpacesSet port_spaces; | ||
36 | 83 | Map& map = egbase.map(); | 74 | Map& map = egbase.map(); |
50 | 84 | for (const Coords& c : map.get_port_spaces()) { | 75 | const uint16_t num = map.get_port_spaces().size(); |
38 | 85 | FCoords fc = map.get_fcoords(c); | ||
39 | 86 | if | ||
40 | 87 | ((map.get_max_nodecaps(egbase.world(), fc) & BUILDCAPS_SIZEMASK) != BUILDCAPS_BIG | ||
41 | 88 | || | ||
42 | 89 | map.find_portdock(fc).empty()) | ||
43 | 90 | { | ||
44 | 91 | continue; | ||
45 | 92 | } | ||
46 | 93 | port_spaces.insert(c); | ||
47 | 94 | } | ||
48 | 95 | |||
49 | 96 | const uint16_t num = port_spaces.size(); | ||
51 | 97 | s1.set_int("number_of_port_spaces", num); | 76 | s1.set_int("number_of_port_spaces", num); |
52 | 98 | 77 | ||
53 | 99 | Section & s2 = prof.create_section("port_spaces"); | 78 | Section & s2 = prof.create_section("port_spaces"); |
54 | 100 | int i = 0; | 79 | int i = 0; |
56 | 101 | for (const Coords& c : port_spaces) { | 80 | for (const Coords& c : map.get_port_spaces()) { |
57 | 102 | set_coords(std::to_string(i++), c, &s2); | 81 | set_coords(std::to_string(i++), c, &s2); |
58 | 103 | } | 82 | } |
59 | 104 | prof.write("port_spaces", false, fs); | 83 | prof.write("port_spaces", false, fs); |
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