Merge lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport into lp:widelands

[GunChleoc]

Fix saveloading of interactive player's viewport

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3819. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/418129123.
Appveyor build 3618. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1784122_singleplayer_viewport-3618.

[Notabilis (notabilis27)]

Code is looking good and working as intended.

That definitely is one nasty bug...

review: Approve

[Klaus Halfmann (klaus-halfmann)]

Now I am confused. That code should be semantically identical?

As peformance optimizer I see the the extra null-assignement is a waste :-)

Can you add a comment what compiler/enviroment causes this problem?

I tested this in bzr8791[trunk] and it was ok for me.

[GunChleoc (gunchleoc)]

> That code should be semantically identical?

That's what I thought too when I broke it. I really don't want to dig into the streamreader right now though - there is some subtle unexpected side-effect going on there.

@bunnybot merge

[Notabilis (notabilis27)]

That bug is really fun but the streamreader isn't to blame. ;-)
The problem is that the float_32() call not only returns the value but also modifies the internal state of the stream. As a consequence, the order of assignment to x and y matters (or more precisely, the order in which the respective float_32() calls are made).
It seems as if calling it twice as constructor parameters results in the *second* call being done first, resulting in x and y being swapped after the assignment. Doing only one call as parameter and assigning the other one later on is fine. Or just assign both later on as in this branch.
Took me quite some time of trial&error until I realized that...

I am using gcc version 8.2.0 with a release build. The order of the evaluation is undefined in the standard, so it might indeed be compiler dependent.

The null-assignment is required since the default constructor is deleted.

[Notabilis (notabilis27)]

*with a debug build, sorry.

=== modified file 'src/game_io/game_interactive_player_packet.cc'
--- src/game_io/game_interactive_player_packet.cc	2018-07-08 15:16:16 +0000
+++ src/game_io/game_interactive_player_packet.cc	2018-08-20 07:44:00 +0000
@@ -59,7 +59,11 @@
 				if (player_number > max)
 					throw GameDataError("The game has no players!");
 			}
-			Vector2f center_map_pixel(fr.float_32(), fr.float_32());
+
+			Vector2f center_map_pixel = Vector2f::zero();
+			center_map_pixel.x = fr.float_32();
+			center_map_pixel.y = fr.float_32();
+
 			uint32_t const display_flags = fr.unsigned_32();
 
 			if (InteractiveBase* const ibase = game.get_ibase()) {