Merge lp:~widelands-dev/widelands/bug-1526903 into lp:widelands

[TiborB]

This is very simple change - prohibiting save of allowed_building_types in editor (=creating particular file). Should not cause problems, but must be tested

[TiborB (tiborb95)]

The diff looks bit complex, but really only thing that was added is:
if (is_a(Game, &m_egbase)) { .... }

[GunChleoc (gunchleoc)]

Could you go through the map saver and have a look at other packets that we don't need?

[kaputtnik (franku)]

We should check the player directory... but i am unsure if it is needed for scritping. Maybe wl_zocker could tell us.

I will test this branch...

[kaputtnik (franku)]

I couldn't trigger any of the errors described in the related bug reports anymore :-)

The file "allowed_building_types" is never been created. So for this it seems to be fixed and i give approve for testing.

But there is another issue with the number of players: Open editor, open "set players", set number of players to two but place only one player to the map. Save the map and after reload the map the "set player" window will always show two players. If you try to start a game with this map, a error occurs:

Fatal exception: Widelands could not start the game, because player 2 has no starting position.
You can manually add a starting position with the Widelands Editor to fix this problem.

I will make another bug report for this issue.

review: Approve

[TiborB (tiborb95)]

> Could you go through the map saver and have a look at other packets that we don't need?

There is couple of sections we can discuss, or perhaps can be ommited for saving in editor:

log("Writing Flag Data ... ");
log("Writing Road Data ... ");
log("Writing Map Objects ... "); - this is probably needed in editor, or isn't it?
// DATA PACKETS 3 sections here
log("Writing Node Ownership Data ... ");
log("Writing Exploration Data ... ");
log("Writing Players Unseen Data ... ");
log("Writing Scripting Data ... ");
log("Writing Objective Data ... ");

What is your opinion? Or is here somebody willing to test what can be skipped?

I see kaputtnik approved this. So can I merge to trunk or will we go on with discussion what can be ommited as well?

[TiborB (tiborb95)]

>Fatal exception: Widelands could not start the game, because player 2 has no starting position.
You can manually add a starting position with the Widelands Editor to fix this problem.

For me this is "unfinished map" not a "bug".

[bunnybot (widelandsofficial)]

Hi, I am bunnybot (https://github.com/widelands/bunnybot).

I am keeping the source branch lp:~widelands-dev/widelands/bug-1526903 mirrored to
  https://github.com/widelands/widelands/tree/_widelands_dev_widelands_bug_1526903

The latest continuous integration build can always be found here:
  https://travis-ci.org/widelands/widelands/branches
Please do not merge without making sure that it passes.

You can give me commands by starting a line with @bunnybot <command>. I understand:
 merge: Merges the source branch into the target branch, closing the pull request.

[bunnybot (widelandsofficial)]

Travis build 101 has changed state to: passed. Details: https://travis-ci.org/widelands/widelands/builds/99640935.

[bunnybot (widelandsofficial)]

Travis build 101 has changed state to: passed. Details: https://travis-ci.org/widelands/widelands/builds/99640935.

[GunChleoc (gunchleoc)]

> But there is another issue with the number of players: Open editor, open "set
> players", set number of players to two but place only one player to the map.
> Save the map and after reload the map the "set player" window will always show
> two players. If you try to start a game with this map, a error occurs:
>
> Fatal exception: Widelands could not start the game, because player 2 has no
> starting position.
> You can manually add a starting position with the Widelands Editor to fix this
> problem.
>
> I will make another bug report for this issue.

Yes, this is unrelated.

> There is couple of sections we can discuss, or perhaps can be ommited for saving in editor:

I think maybe we should discuss this in a new bug, so it won't get lost after this branch is merged?

[TiborB (tiborb95)]

I believe that I can merge it

@bunnybot merge

[GunChleoc (gunchleoc)]

I have created a bug for the packages that we should look at: https://bugs.launchpad.net/widelands/+bug/1531463

=== modified file 'src/map_io/map_saver.cc'
--- src/map_io/map_saver.cc	2015-11-11 09:53:54 +0000
+++ src/map_io/map_saver.cc	2015-12-29 22:26:03 +0000
@@ -139,18 +139,22 @@
 	{MapAllowedWorkerTypesPacket p; p.write(m_fs, m_egbase, *m_mos);}
 	log("took %ums\n ", timer.ms_since_last_query());
 
- //  allowed building types
-	iterate_players_existing_const(plnum, nr_players, m_egbase, player) {
-		for (DescriptionIndex i = 0; i < m_egbase.tribes().nrbuildings(); ++i) {
-			if (!player->is_building_type_allowed(i)) {
-				log("Writing Allowed Building Types Data ... ");
-				MapAllowedBuildingTypesPacket p;
-				p                                  .write(m_fs, m_egbase, *m_mos);
-				log("took %ums\n ", timer.ms_since_last_query());
-				goto end_find_a_forbidden_building_type_loop;
+    //  allowed building types
+    // Not saving this when in editor - it causes issues when changing tribe
+    // after next load in editor
+	if (is_a(Game, &m_egbase)) {
+		iterate_players_existing_const(plnum, nr_players, m_egbase, player) {
+			for (DescriptionIndex i = 0; i < m_egbase.tribes().nrbuildings(); ++i) {
+				if (!player->is_building_type_allowed(i)) {
+					log("Writing Allowed Building Types Data ... ");
+					MapAllowedBuildingTypesPacket p;
+					p                                  .write(m_fs, m_egbase, *m_mos);
+					log("took %ums\n ", timer.ms_since_last_query());
+					goto end_find_a_forbidden_building_type_loop;
+				}
 			}
-		}
-	} end_find_a_forbidden_building_type_loop:;
+		} end_find_a_forbidden_building_type_loop:;
+	}
 
 	// !!!!!!!!!! NOTE
 	// This packet must be before any building or road packet. So do not