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

[TiborB]

This fixes special situation, when two ships (both in expedition mode) of two players are on same field in the time of game saving.

See the also the bug report. A savefile attached to the bug is functional now and I run regression tests as well.

[GunChleoc (gunchleoc)]

You found it :)

How about only writing the ships that the player owns as well? We still need the test when loading for older savegames, but not writing it in the first place would be a good idea I think.

Around line 137:

if (ship->get_economy() == temp_economy) {

We could have:

if (ship->get_economy() == temp_economy && ship->get_owner() == player) {

I also spotted a typo in a comment: curent -> current. It would also be good to have a blank space between // and We, makes it easier to read. :)

[TiborB (tiborb95)]

@GunChleoc

I was considering exactly this, but this is of no benefit because if economy of ship is the same as current economy (we are saving now) than it is obvious that the ship is ours.

[GunChleoc (gunchleoc)]

Makes sense to me. Go ahead and merge :)

review: Approve

[Hans Joachim Desserud (hjd)]

Hi, just a post-review comment:

Please write more descriptive commit messages when merging to trunk. This makes it easier to tell what was changed, both when reading through the commit log, but also when someone wants to know why a block of code was added six months later.

Including the bug number is great, but it should say a bit more about how the code has changed from the previous commit (and depending on the situation something about the issue which was fixed.) The description you have at the beginning of this merge proposal is a good starting point.

As a hypothetical example; if you're looking up the history/changes of a piece of code you're working on; "Fixed bug 123" doesn't say a lot unless you remember all the bug numbers. You can of course look it up in a browser and skim through it, but then the next code line was added to fix another bug.

If it instead said "Ports now check if it has a ship available before starting expeditions. Used to attempt to load wares onto nothing and crashed. (Fixes bug 123)" it immediately gives you a context on why the code was added and what problem it solves. And when moving on to the next line, you can see that it was added to "Fixed warning. Doesn't need to assign value to unused variable".

Ideally, the commit log should tell a story about how the code has evolved over time.

Other than that, keep up the good work. :)

[TiborB (tiborb95)]

Oh, I did not realize that this might be important, I will be more elaborate next time

=== modified file 'src/game_io/game_player_economies_packet.cc'
--- src/game_io/game_player_economies_packet.cc	2014-09-20 09:37:47 +0000
+++ src/game_io/game_player_economies_packet.cc	2015-05-04 18:24:59 +0000
@@ -65,10 +65,15 @@
 							Bob* bob = map[read_map_index_32(&fr, max_index)].get_first_bob();
 							while (bob) {
 								if (upcast(Ship const, ship, bob)) {
-									assert(ship->get_economy());
-									EconomyDataPacket d(ship->get_economy());
-									d.read(fr);
-									read_this_economy = true;
+
+									// We are interested only in current player's ships
+									if (ship->get_owner() == player) {
+										assert(ship->get_economy());
+										EconomyDataPacket d(ship->get_economy());
+										d.read(fr);
+										read_this_economy = true;
+										break;
+									}
 								}
 								bob = bob->get_next_bob();
 							}
@@ -133,10 +138,10 @@
 								// TODO(sirver): the 0xffffffff is ugly and fragile.
 								fw.unsigned_32(0xffffffff); // Sentinel value.
 								fw.unsigned_32(field - &field_0);
-
 								EconomyDataPacket d(ship->get_economy());
 								d.write(fw);
 								wrote_this_economy = true;
+								break;
 							}
 						}
 						bob = bob->get_next_bob();