Merge lp:~widelands-dev/widelands/bug-1674243-crash_with_save_game into lp:widelands

[GunChleoc]

Assign owners to neighbours in Player::rediscover_node. The lack of owners was causing crashes with the road program, which no longer knew which texture to pick.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2212. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/233925310.
Appveyor build 2047. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1674243_crash_with_save_game-2047.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2214. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/234038024.
Appveyor build 2049. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1674243_crash_with_save_game-2049.

[SirVer (sirver)]

I think this line is correct and should not be removed. This is the idea: to draw a road, you need to know the owner of the road to pull the road textures from its tribe. So f.owner must be set.

In the changed function, the following is checked: this field was seen before by our current interactive player (pf.vision == 1), but it is no longer seen (otherwise pf.vision would be > 1).

const Player::Field& pf = player->fields()[map.get_index(f.fcoords, map.get_width())];

pulls out the cached information about what the player last saw there and of course, if we last saw this field owned by player 2 (empire) we need to draw with empire textures - even though in reality the field might be owned by atlanteans by now (we do not know, we do not currently see the field).

So the logic you remove seems correct to me. The more interesting question to me is why pf.owner == 0 though we apparently saw a road there before - this seems like it is not correct set on discovery of the node when we see it.

[GunChleoc (gunchleoc)]

In Player::rediscover_node, I found the following code:

 { // discover everything (above the ground) in this field
  field.terrains = f.field->get_terrains();
  field.roads = f.field->get_roads();
  field.owner = f.field->get_owned_by();

OK, owner set. Later on, we get to the neighbouring fields:

 { // discover the D triangle and the SW edge of the top right neighbour
  FCoords tr = map.tr_n(f);
  Field& tr_field = fields_[tr.field - &first_map_field];
  if (tr_field.vision <= 1) {
   tr_field.terrains.d = tr.field->terrain_d();
   tr_field.roads &= ~(RoadType::kMask << RoadType::kSouthWest);
   tr_field.roads |= RoadType::kMask << RoadType::kSouthWest & tr.field->get_roads();
  }
 }

No mention of the owner there - might that be the bug?

[SirVer (sirver)]

Yes, could be. I am traveling and cannot check right now. But if roads is set, owner should be too - there can't be a field with roads but without owners.

> Am 21.05.2017 um 11:39 schrieb GunChleoc <email address hidden>:
>
> In Player::rediscover_node, I found the following code:
>
> { // discover everything (above the ground) in this field
> field.terrains = f.field->get_terrains();
> field.roads = f.field->get_roads();
> field.owner = f.field->get_owned_by();
>
> OK, owner set. Later on, we get to the neighbouring fields:
>
>
> { // discover the D triangle and the SW edge of the top right neighbour
> FCoords tr = map.tr_n(f);
> Field& tr_field = fields_[tr.field - &first_map_field];
> if (tr_field.vision <= 1) {
> tr_field.terrains.d = tr.field->terrain_d();
> tr_field.roads &= ~(RoadType::kMask << RoadType::kSouthWest);
> tr_field.roads |= RoadType::kMask << RoadType::kSouthWest & tr.field->get_roads();
> }
> }
>
> No mention of the owner there - might that be the bug?
>
> --
> https://code.launchpad.net/~widelands-dev/widelands/bug-1674243-crash_with_save_game/+merge/324301
> Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1674243-crash_with_save_game into lp:widelands.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~widelands-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~widelands-dev
> More help : https://help.launchpad.net/ListHelp

[GunChleoc (gunchleoc)]

I have now implemented my new suggestion, because I think it makes more sense. It's hard to test for though, since this won't fix the crash for savegames that are already messed up.

review: Needs Resubmitting

[Klaus Halfmann (klaus-halfmann)]

Mhh, ao the savegames caanot be fixed, as they already have the broken status.
So when or why is Player::rediscover_node called?

As of the code this is for the NoteFieldTerrainChanged Event raised
by Map::change_terrain, this should happen by the gameeditor or some
luascript only -> should happen in scenarios only?

And when the a fields vision changes from 0 to 1 (read the field becomes visible)
from see_node/see_area, but this is called in a regular manner.

please add a comment to that function.

[Klaus Halfmann (klaus-halfmann)]

So to test this we would need some game / scenarion where the borders two between players
toggle a lot an then build roads along the newly seen parts of the map, correct?

[GunChleoc (gunchleoc)]

I guess that sort of situation could help with a bit of testing, yes.

Nodes are rediscovered if a worker or ship passes by them, or if a building is built nearby.

It can also be triggered by Lua code as in https://code.launchpad.net/~widelands-dev/widelands/bug-1687100-reveal_fields/+merge/323721, but that's the rarer case.

Lua change terrain is used in the Trident of Fire multiplayer scenario, and in the Atlantean campaign scenario.

[GunChleoc (gunchleoc)]

Well, let's get this in for some more testing exposure - I think it's important to get this into trunk because the crash doesn't happen that often.

@bunnybot merge

=== modified file 'src/graphic/game_renderer.cc'
--- src/graphic/game_renderer.cc	2017-01-25 18:55:59 +0000
+++ src/graphic/game_renderer.cc	2017-05-25 16:30:31 +0000
@@ -128,7 +128,7 @@
 	}
 }
 
-void draw_objets_for_formerly_visible_field(const FieldsToDraw::Field& field,
+void draw_objects_for_formerly_visible_field(const FieldsToDraw::Field& field,
                                             const Player::Field& player_field,
                                             const float zoom,
                                             RenderTarget* dst) {
@@ -246,7 +246,7 @@
 			const Map& map = egbase.map();
 			const Player::Field& player_field =
 			   player->fields()[map.get_index(field.fcoords, map.get_width())];
-			draw_objets_for_formerly_visible_field(field, player_field, zoom, dst);
+			draw_objects_for_formerly_visible_field(field, player_field, zoom, dst);
 		}
 
 		const FieldOverlayManager& overlay_manager = egbase.get_ibase()->field_overlay_manager();

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2017-05-21 22:18:02 +0000
+++ src/logic/player.cc	2017-05-25 16:30:31 +0000
@@ -969,6 +969,7 @@
 			tr_field.terrains.d = tr.field->terrain_d();
 			tr_field.roads &= ~(RoadType::kMask << RoadType::kSouthWest);
 			tr_field.roads |= RoadType::kMask << RoadType::kSouthWest & tr.field->get_roads();
+			tr_field.owner = tr.field->get_owned_by();
 		}
 	}
 	{  //  discover both triangles and the SE edge of the top left  neighbour
@@ -978,6 +979,7 @@
 			tl_field.terrains = tl.field->get_terrains();
 			tl_field.roads &= ~(RoadType::kMask << RoadType::kSouthEast);
 			tl_field.roads |= RoadType::kMask << RoadType::kSouthEast & tl.field->get_roads();
+			tl_field.owner = tl.field->get_owned_by();
 		}
 	}
 	{  //  discover the R triangle and the  E edge of the     left  neighbour
@@ -987,6 +989,7 @@
 			l_field.terrains.r = l.field->terrain_r();
 			l_field.roads &= ~(RoadType::kMask << RoadType::kEast);
 			l_field.roads |= RoadType::kMask << RoadType::kEast & l.field->get_roads();
+			l_field.owner = l.field->get_owned_by();
 		}
 	}
 }