Merge lp:~widelands-dev/widelands/bug-1687100-reveal_fields into lp:widelands

[GunChleoc]

When the hide_completely option is not selected, LuaPlayer::hide_fields no longer hides fields that the player's buildings see. Vision information for Unexplored fields is now kept in a new container in the Player object.

Also, removed unused bool view_changed_ from player.h and removed compatibility code for border_file in the player vision packet.

This was actually a long-standing bug - the function wasn't doing what the documentation described in the first place.

Can be tested by hacking the seafaring tutorial:

=== modified file 'data/campaigns/tutorial03_seafaring.wmf/scripting/mission_thread.lua'
--- data/campaigns/tutorial03_seafaring.wmf/scripting/mission_thread.lua 2016-10-23 11:31:25 +0000
+++ data/campaigns/tutorial03_seafaring.wmf/scripting/mission_thread.lua 2017-05-07 07:51:04 +0000
@@ -5,6 +5,14 @@
 function introduction()
    additional_port_space.terr = "summer_water" -- disable the port space
    sleep(1000)
+
+plr:reveal_fields(wl.Game().map:get_field(41,43):region(60))
+plr:hide_fields(wl.Game().map:get_field(41,43):region(40),true)
+
+plr:hide_fields(wl.Game().map:get_field(43,52):region(20),true)
+plr:reveal_fields(wl.Game().map:get_field(43,52):region(20))
+sleep(600000)
+
    message_box_objective(plr, intro_south)
    sleep(300)
    message_box_objective(plr, intro_north)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2154. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/229623909.
Appveyor build 1989. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-1989.

[kaputtnik (franku)]

Doesn't work for me... add just

   plr:hide_fields(wl.Game().map:get_field(41,43):region(40),true)
   sleep(6000000)

to the mission_thread.lua and you see that every field is visible.

review: Needs Fixing

[kaputtnik (franku)]

Maybe a better branch for testing is the branch where i have added already some reveal/hide animations to the first tutorials. This is also the branch where i had encountered this bug.

https://code.launchpad.net/~widelands-dev/widelands/reveal_hide_animations

[GunChleoc (gunchleoc)]

Next try.

review: Needs Resubmitting

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2221. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/234277729.
Appveyor build 2056. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2056.

[kaputtnik (franku)]

Looks good now :-)

One small nit: Since hide_completely and reveal_completely are optional it should be clarified which boolean (true/false) will take an effect at all.

I want to make some more tests, before approval of testing. In the meantime someone else could look at the logic...

[GunChleoc (gunchleoc)]

How is the documentation for you now?

[kaputtnik (franku)]

> How is the documentation for you now?

Perfect :-) Thanks

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2230. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/234503590.
Appveyor build 2065. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2065.

[kaputtnik (franku)]

Hmmm... this do not work. From my understanding the function is_seeing() restricts the seen fields to MapObjectType::WAREHOUSE;

So all nodes (fields) which are not in the area a Warehouse sees, behaves as before: E.g. the census and statistics disappear for a short time.

You could see this when playing this branch with the datadir of the reveal_hide_animations branch and playing the seafaring tutorial. Enable census and statistics and watch the region of the foresters at the southern islands (increase game speed).

I think at least the military buildings must be added to set the seen fields.

Another approach is maybe to make the function sees_field() (https://wl.widelands.org/docs/wl/autogen_wl_game/#wl.game.Player.sees_field) read- and settable. So a modder could read and set it by himself.

review: Needs Fixing

[kaputtnik (franku)]

Oh i didn't understood the code... is_seen() is only used to hide fields. But partly not working is revealing fields.

I am not familiar with the code, but i am asking myself why the function hide_fields() is now much complicated than the one in trunk ( http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/scripting/lua_game.cc#L628 ). Just asking because the one in trunk works fine imho.

[kaputtnik (franku)]

I have just seen that the mentioned behavior (disappearing census and statistics) do also happen in current trunk when playing the seafaring tutorial.

I check this in build 19... (must first compile from scratch)

[kaputtnik (franku)]

Sorry, wrong alert :S

I accidentally edited the seafaring tutorial in trunk ... So trunk does NOT show this behavior.

Seems i must limit the things i am working on... Sorry for the noise.

[GunChleoc (gunchleoc)]

I have dug some more into the vision stuff and completely reimplemented the thing. The new implementation should be safe for multiplayer too.

Could you please test again, and if still it doesn't work as expected, copy over some of your failing scripting code so I will have some real data to test with?

It would also be interesting to see if we can't just always hide completely, this would make the code a bit simpler.

I still need to implement saveloading, but I'll wait with that until it works.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2262. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/236985137.
Appveyor build 2097. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2097.

[GunChleoc (gunchleoc)]

I guess the bug in the military scenario is that the soldiers see where they are going - do you want that switched off?

[kaputtnik (franku)]

> do you want that switched off?

Lets say: If it doesn't make much work it would be fine to have the possibility to switch that off :-) Otherwise i could live with the current state.

The bug with the disappearing census and statistics is gone now.

Thank you very much for working on this.

You can always test your code if you run widelands with the --datadir= option pointing the data folder of my branch. Doesn't that work for you?

[GunChleoc (gunchleoc)]

Well, it turns out that it makes insanely complicated work, so worker vision will have to stay as it is.

Duh, I never thought of the datadir option!

BTW you don't need the boolean now when revealing fields, as the necessary steps are detected automatically. We still need the 2 modes for hiding though, e.g. in the Barbarian campaign scenario 2 when the tracks are discovered, we don't want to black this out.

Now on to saveloading...

[kaputtnik (franku)]

Sorry Gun,

with your last changes the prior problem appear again: The census and statistics overlay for some buildings suddenly disappear in the seafaring tutorial.

I have reverted to revision 8358 and then it works: The census and statistics overlays keep alive.

[GunChleoc (gunchleoc)]

Thanks for testing again. I thought the new changes would make the code more correct, but I was obviously wrong, since the test suite also blew up.

[GunChleoc (gunchleoc)]

I found the error in the test suite: we need to sleep after calls now, because the playercommands are causing delays.

I deleted the commit that broke things for you from the branch's history, so you will need to pull the branch with the option --overwrite.

[kaputtnik (franku)]

I have a fresh branch and compiled from scratch.

Works again :-)

review: Approve (testing)

[GunChleoc (gunchleoc)]

Saveloading is implemented, so this is ready for review again.

review: Needs Resubmitting

[kaputtnik (franku)]

Sorry, isn't working. Now it is the economy tutorial which does not work. Please run the branch with the --datadir option pointing to my branch and open first the seafaring tutorial, and afterwards the economy tutorial. Within the seafaring tutorial you see the 'reveal_randomly' code is working, whereas in the economy tutorial the same code isn't working (most areas get not hidden prior revealing). Maybe i have made a mistake in my code, so i would be glad if you can take a look at it.

review: Needs Fixing

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2284. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/240085373.
Appveyor build 2119. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2119.

[GunChleoc (gunchleoc)]

The difference is the headquarters in the economy tutorial - seems that it is creating vision no matter what we do.

[kaputtnik (franku)]

Hm.. that's strange, because this works also for the first tutorial, where a headquarter is used. The first campaign "A Place to Call Home" works also nicely with a headquarter.

[GunChleoc (gunchleoc)]

Yep, really strange. It very obviously is the vision or conquer radius of the headquarters though. Your code doesn't look any different to the Seafaring tutorial code, so I don't think that's it either.

[GunChleoc (gunchleoc)]

I have found the culprit - some of the roads are messing this up. So, I have moved them into a separate function and add them after the land has been revealed.

We can surely still pretty this up, but I need to create a debug build first so that I can see the coordinates.

[GunChleoc (gunchleoc)]

OK, done with my work on the other branch. The problem happens when we connect mines and constructionsites to the road network - there is nothing in the vision code that makes mines special as far as I can see, so I haven't the foggiest what's causing this. I'd say let's leave it as it is and live with the workaround that I coded for the Economy tutorial.

[kaputtnik (franku)]

Wow, thanks a lot :-)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2287. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/242180501.
Appveyor build 2122. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2122.

[kaputtnik (franku)]

I think this is working now :-)

review: Approve (testing)

[Klaus Halfmann (klaus-halfmann)]

Testing hint:

use this datadir this will use the functions
https://code.launchpad.net/~widelands-dev/widelands/reveal_hide_animations,

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2504. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/261130470.
Appveyor build 2328. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2328.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2528. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/263771923.
Appveyor build 2352. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2352.

[SirVer (sirver)]

I starting looking through the code and there is a lot to digest here. I have a few questions:

- Why do we require the player command? It seems that we only want to mess with the normal rules of vision through scripting which has to run in parallel on each machine anyways, i.e. should not require a player command. Using a player command will be more fragile, as for example the scripting code has no guarantee when the see/unsee actually happens (can be seconds if the network connection is bad).
- The semantis of hidden_fields_ is unclear to me. What happens in this case: Field f is seen by two buildings and a worker, i.e. f.vision = 4 (1 (has been seen before) + 3 viewers). It is now hidden through scripting. The worker will walk away eventually, so 'f.vision = 3'. What happens if the field is now 'revealed'?

[GunChleoc (gunchleoc)]

> I starting looking through the code and there is a lot to digest here. I have a few questions:
>
> - Why do we require the player command? It seems that we only want to mess with the normal rules of vision through scripting which has to run in parallel on each machine anyways, i.e. should not require a player command. Using a player command will be more fragile, as for example the scripting code has no guarantee when the see/unsee actually happens (can be seconds if the network connection is bad).

I wasn't sure whether we needed one or not and decided to be rather safe
than sorry. I don't have a problem with removing it.

> - The semantis of hidden_fields_ is unclear to me.

Join the club. I arrived at the solution pretty much through trial and
error. Since I wrote this a long time ago, I'd need to completely
reanalzye the code myself - let me know if you need me to do so and I'll
do that next month.

[SirVer (sirver)]

> I don't have a problem with removing it.

please do and ping me for another round of reviews here.

[GunChleoc (gunchleoc)]

I have removed the playercommand now.

Here's what I remember of the vision code:

Vision state of a field is an integer. Everything in sight of a building with vision is always visible. Every time a worker walks past an area, the vision on the field is incremented. It then slowly gets decremented until the player loses vision. Then the fields are darkened and not updated any longer, but never blacked out.

What we needed in the hide/reveal code were 2 versions of hiding fields from the player:

1. Just the darkened state
2. A permanent black state that can be undone by scripting, field by field

Buildings and workers will interfere with the hiding state, so implementing this became a bit tricky and also needed some extra data in the savegame.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2731. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/295910371.
Appveyor build 2543. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2543.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2778. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/298423953.
Appveyor build 2590. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1687100_reveal_fields-2590.

[GunChleoc (gunchleoc)]

This branch has been sitting here well tested for half a year now, so let's have it before we get more code duplication in from the next campaign scenario.

@bunnybot merge

=== modified file 'src/logic/CMakeLists.txt'
--- src/logic/CMakeLists.txt	2017-11-20 13:50:51 +0000
+++ src/logic/CMakeLists.txt	2017-12-19 07:13:00 +0000
@@ -164,6 +164,7 @@
     roadtype.h
     save_handler.cc
     save_handler.h
+    see_unsee_node.h
     trade_agreement.h
     widelands_geometry_io.cc
     widelands_geometry_io.h

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2017-12-02 12:43:38 +0000
+++ src/logic/player.cc	2017-12-19 07:13:00 +0000
@@ -173,7 +173,7 @@
 	field_terrain_changed_subscriber_ = Notifications::subscribe<NoteFieldTerrainChanged>(
 	   [this](const NoteFieldTerrainChanged& note) {
 		   if (vision(note.map_index) > 1) {
-			   rediscover_node(egbase().map(), egbase().map()[0], note.fc);
+			   rediscover_node(egbase().map(), note.fc);
 		   }
 		});
 
@@ -923,16 +923,15 @@
 	}
 }
 
-void Player::rediscover_node(const Map& map,
-                             const Widelands::Field& first_map_field,
-                             const FCoords& f) {
+void Player::rediscover_node(const Map& map, const FCoords& f) {
 
 	assert(0 <= f.x);
 	assert(f.x < map.get_width());
 	assert(0 <= f.y);
 	assert(f.y < map.get_height());
-	assert(&map[0] <= f.field);
-	assert(f.field < &map[0] + map.max_index());
+	const Widelands::Field& first_map_field = map[0];
+	assert(&first_map_field <= f.field);
+	assert(f.field < &first_map_field + map.max_index());
 
 	Field& field = fields_[f.field - &first_map_field];
 
@@ -1031,8 +1030,8 @@
 	}
 }
 
-void Player::see_node(const Map& map,
-                      const Widelands::Field& first_map_field,
+/// Returns the resulting vision.
+Vision Player::see_node(const Map& map,
                       const FCoords& f,
                       Time const gametime,
                       bool const forward) {
@@ -1040,7 +1039,8 @@
 	assert(f.x < map.get_width());
 	assert(0 <= f.y);
 	assert(f.y < map.get_height());
-	assert(&map[0] <= f.field);
+	const Widelands::Field& first_map_field = map[0];
+	assert(&first_map_field <= f.field);
 	assert(f.field < &first_map_field + map.max_index());
 
 	//  If this is not already a forwarded call, we should inform allied players
@@ -1049,31 +1049,34 @@
 		update_team_players();
 	if (!forward && !team_player_.empty()) {
 		for (uint8_t j = 0; j < team_player_.size(); ++j)
-			team_player_[j]->see_node(map, first_map_field, f, gametime, true);
+			team_player_[j]->see_node(map, f, gametime, true);
 	}
 
 	Field& field = fields_[f.field - &first_map_field];
 	assert(fields_ <= &field);
 	assert(&field < fields_ + map.max_index());
-	Vision fvision = field.vision;
-	if (fvision == 0)
-		fvision = 1;
-	if (fvision == 1)
-		rediscover_node(map, first_map_field, f);
-	++fvision;
-	field.vision = fvision;
+
+	if (field.vision == 0) {
+		field.vision = 1;
+	}
+	if (field.vision == 1) {
+		rediscover_node(map, f);
+	}
+	return ++field.vision;
 }
 
 /// If 'mode' = UnseeMode::kUnexplore, fields will be marked as unexplored. Else, player no longer
-/// sees what's currently going on.
-void Player::unsee_node(MapIndex const i,
+/// sees what's currently going on. Returns the vision that this node had before it was hidden.
+Vision Player::unsee_node(MapIndex const i,
                         Time const gametime,
-                        const UnseeNodeMode mode,
+                        const SeeUnseeNode mode,
                         bool const forward) {
 	Field& field = fields_[i];
-	if ((mode == UnseeNodeMode::kUnsee && field.vision <= 1) ||
+	if ((mode == SeeUnseeNode::kUnsee && field.vision <= 1) ||
 	    field.vision < 1)  //  Already does not see this
-		return;
+		return field.vision;
+
+	const Vision original_vision = field.vision;
 
 	//  If this is not already a forwarded call, we should inform allied players
 	//  as well of this change.
@@ -1084,7 +1087,7 @@
 			team_player_[j]->unsee_node(i, gametime, mode, true);
 	}
 
-	if (mode == UnseeNodeMode::kUnexplore) {
+	if (mode == SeeUnseeNode::kUnexplore) {
 		field.vision = 0;
 	} else {
 		--field.vision;
@@ -1093,6 +1096,38 @@
 	if (field.vision < 2) {
 		field.time_node_last_unseen = gametime;
 	}
+	return original_vision;
+}
+
+void Player::hide_or_reveal_field(const uint32_t gametime, const Coords& coords, SeeUnseeNode mode) {
+	const Map& map = egbase().map();
+	FCoords fcoords = map.get_fcoords(coords);
+	const Widelands::MapIndex index = fcoords.field - &map[0];
+
+	switch (mode) {
+	// Reveal field
+	case SeeUnseeNode::kReveal: {
+		Widelands::Vision new_vision = see_node(map, fcoords, gametime);
+		// If the field was manually hidden, restore the original vision
+		if (hidden_fields_.count(index) == 1) {
+			auto iter = hidden_fields_.find(index);
+			Vision original_vision = iter->second;
+			while (new_vision < original_vision) {
+				new_vision = see_node(map, fcoords, gametime);
+			}
+			hidden_fields_.erase(iter);
+		}
+	} break;
+	// Hide field
+	case SeeUnseeNode::kUnsee:
+	case SeeUnseeNode::kUnexplore: {
+		const Widelands::Vision new_vision = unsee_node(index, gametime, mode);
+		// Remember the original vision so that we can unhide the fields again
+		if (hidden_fields_.count(index) != 1) {
+			hidden_fields_.insert(std::make_pair(index, new_vision));
+		}
+	} break;
+	}
 }
 
 /**

=== modified file 'src/logic/player.h'
--- src/logic/player.h	2017-11-20 07:54:19 +0000
+++ src/logic/player.h	2017-12-19 07:13:00 +0000
@@ -34,6 +34,7 @@
 #include "logic/map_objects/tribes/warehouse.h"
 #include "logic/mapregion.h"
 #include "logic/message_queue.h"
+#include "logic/see_unsee_node.h"
 #include "logic/widelands.h"
 
 class Node;
@@ -146,7 +147,6 @@
 	// For cheating
 	void set_see_all(bool const t) {
 		see_all_ = t;
-		view_changed_ = true;
 	}
 	bool see_all() const {
 		return see_all_;
@@ -433,37 +433,28 @@
 		return (see_all_ ? 2 : 0) + fields_[i].vision;
 	}
 
-	bool has_view_changed() {
-		bool t = view_changed_;
-		view_changed_ = false;
-		return t;
-	}
-
 	/**
 	 * Update this player's information about this node and the surrounding
 	 * triangles and edges.
 	 */
-	void see_node(const Map&,
-	              const Widelands::Field& first_map_field,
+	Vision see_node(const Map&,
 	              const FCoords&,
 	              const Time,
 	              const bool forward = false);
 
 	/// Decrement this player's vision for a node.
-	enum class UnseeNodeMode { kUnsee, kUnexplore };
-	void
-	unsee_node(MapIndex, Time, UnseeNodeMode mode = UnseeNodeMode::kUnsee, bool forward = false);
+
+	Vision
+	unsee_node(MapIndex, Time, SeeUnseeNode mode = SeeUnseeNode::kUnsee, bool forward = false);
 
 	/// Call see_node for each node in the area.
 	void see_area(const Area<FCoords>& area) {
 		const Time gametime = egbase().get_gametime();
 		const Map& map = egbase().map();
-		const Widelands::Field& first_map_field = map[0];
 		MapRegion<Area<FCoords>> mr(map, area);
 		do {
-			see_node(map, first_map_field, mr.location(), gametime);
+			see_node(map, mr.location(), gametime);
 		} while (mr.advance(map));
-		view_changed_ = true;
 	}
 
 	/// Decrement this player's vision for each node in an area.
@@ -475,9 +466,15 @@
 		do
 			unsee_node(mr.location().field - &first_map_field, gametime);
 		while (mr.advance(map));
-		view_changed_ = true;
 	}
 
+	/// Explicitly hide or reveal the field at 'c'. The modes are as follows:
+	/// - kUnsee:     Decrement the field's vision
+	/// - kUnexplore: Set the field's vision to 0
+	/// - kReveal:    If the field was hidden previously, restore the vision to the value it had
+	///               at the time of hiding. Otherwise, increment the vision.
+	void hide_or_reveal_field(const uint32_t gametime, const Coords& c, SeeUnseeNode mode);
+
 	MilitaryInfluence military_influence(MapIndex const i) const {
 		return fields_[i].military_influence;
 	}
@@ -618,7 +615,7 @@
 	// Called when a node becomes seen or has changed.  Discovers the node and
 	// those of the 6 surrounding edges/triangles that are not seen from another
 	// node.
-	void rediscover_node(const Map&, const Widelands::Field&, const FCoords&);
+	void rediscover_node(const Map&, const FCoords&);
 
 	std::unique_ptr<Notifications::Subscriber<NoteImmovable>> immovable_subscriber_;
 	std::unique_ptr<Notifications::Subscriber<NoteFieldTerrainChanged>>
@@ -634,7 +631,6 @@
 	std::vector<Player*> team_player_;
 	bool team_player_uptodate_;
 	bool see_all_;
-	bool view_changed_;
 	const PlayerNumber player_number_;
 	const TribeDescr& tribe_;  // buildings, wares, workers, sciences
 	uint32_t casualties_, kills_;
@@ -649,6 +645,9 @@
 	std::string name_;  // Player name
 	std::string ai_;    /**< Name of preferred AI implementation */
 
+	// Fields that were explicitly hidden, with their vision at the time of hiding
+	std::map<MapIndex, Widelands::Vision> hidden_fields_;
+
 	/**
 	 * Wares produced (by ware id) since the last call to @ref sample_statistics
 	 */

=== added file 'src/logic/see_unsee_node.h'
--- src/logic/see_unsee_node.h	1970-01-01 00:00:00 +0000
+++ src/logic/see_unsee_node.h	2017-12-19 07:13:00 +0000
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2017 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef WL_LOGIC_SEE_UNSEE_NODE_H
+#define WL_LOGIC_SEE_UNSEE_NODE_H
+
+namespace Widelands {
+	enum class SeeUnseeNode { kUnsee, kUnexplore, kReveal };
+}
+#endif  // end of include guard: WL_LOGIC_SEE_UNSEE_NODE_H

=== modified file 'src/map_io/map_players_view_packet.cc'
--- src/map_io/map_players_view_packet.cc	2017-09-03 11:19:21 +0000
+++ src/map_io/map_players_view_packet.cc	2017-12-19 07:13:00 +0000
@@ -72,6 +72,9 @@
 constexpr uint8_t kCurrentPacketVersionSurveyTimes = 1;
 #define SURVEY_TIMES_FILENAME_TEMPLATE DIRNAME_TEMPLATE "/survey_times_%u"
 
+constexpr uint8_t kCurrentPacketVersionHidden = 1;
+#define HIDDEN_FILENAME_TEMPLATE DIRNAME_TEMPLATE "/hidden_%u"
+
 constexpr uint8_t kCurrentPacketVersionVision = 1;
 #define VISION_FILENAME_TEMPLATE DIRNAME_TEMPLATE "/vision_%u"
 
@@ -292,7 +295,7 @@
 	const uint16_t mapheight = map.get_height();
 	Field& first_field = map[0];
 	const PlayerNumber nr_players = map.get_nrplayers();
-	iterate_players_existing_const(plnum, nr_players, egbase, player) {
+	iterate_players_existing(plnum, nr_players, egbase, player) {
 		Player::Field* const player_fields = player->fields_;
 		uint32_t const gametime = egbase.get_gametime();
 
@@ -466,10 +469,14 @@
 		OPEN_INPUT_FILE(FileRead, survey_times_file, survey_times_filename,
 		                SURVEY_TIMES_FILENAME_TEMPLATE, kCurrentPacketVersionSurveyTimes);
 
-		OPEN_INPUT_FILE_NEW_VERSION_SILENT(FileRead, border_file, border_filename,
+		OPEN_INPUT_FILE_NEW_VERSION(FileRead, border_file, border_filename,
 		                                   border_file_version, BORDER_FILENAME_TEMPLATE,
 		                                   kCurrentPacketVersionBorder);
 
+		OPEN_INPUT_FILE_NEW_VERSION_SILENT(FileRead, hidden_file, hidden_filename,
+		                                   hidden_file_version, HIDDEN_FILENAME_TEMPLATE,
+		                                   kCurrentPacketVersionHidden);
+
 		for (FCoords first_in_row(Coords(0, 0), &first_field); first_in_row.y < mapheight;
 		     ++first_in_row.y, first_in_row.field += mapwidth) {
 			FCoords r = first_in_row, br = map.bl_n(r);
@@ -777,6 +784,21 @@
 				}
 			} while (r.x);
 		}
+
+		// Read the number of explicitly hidden fields and then loop through them
+		if (hidden_file_version == kCurrentPacketVersionHidden) {
+			const uint32_t no_of_hidden_fields = hidden_file.unsigned_32();
+			for (uint32_t i = 0; i < no_of_hidden_fields; ++i) {
+				player->hidden_fields_.insert(std::make_pair(hidden_file.unsigned_32(), hidden_file.unsigned_16()));
+			}
+		} else if (hidden_file_version < 0) {
+			// TODO(GunChleoc): Savegame compatibility - remove after Build 20
+			log("MapPlayersViewPacket - No hidden fields to read for Player %d - probably an old save file\n", plnum);
+		} else {
+			throw UnhandledVersionError("MapPlayersViewPacket - Hidden fields file",
+			                            hidden_file_version, kCurrentPacketVersionHidden);
+		}
+
 		CHECK_TRAILING_BYTES(unseen_times_file, unseen_times_filename);
 		CHECK_TRAILING_BYTES(node_immovable_kinds_file, node_immovable_kinds_filename);
 		CHECK_TRAILING_BYTES(node_immovables_file, node_immovables_filename);
@@ -788,6 +810,8 @@
 		CHECK_TRAILING_BYTES(surveys_file, surveys_filename);
 		CHECK_TRAILING_BYTES(survey_amounts_file, survey_amounts_filename);
 		CHECK_TRAILING_BYTES(survey_times_file, survey_times_filename);
+		CHECK_TRAILING_BYTES(border_file, border_filename);
+		CHECK_TRAILING_BYTES(hidden_file, hidden_filename);
 	}
 }
 
@@ -864,6 +888,7 @@
 		FileWrite surveys_file;
 		FileWrite survey_amounts_file;
 		FileWrite survey_times_file;
+		FileWrite hidden_file;
 		FileWrite vision_file;
 		FileWrite border_file;
 		for (FCoords first_in_row(Coords(0, 0), &first_field); first_in_row.y < mapheight;
@@ -970,6 +995,12 @@
 				}
 			} while (r.x);
 		}
+		// Write the number of explicitly hidden fields and then loop through them
+		hidden_file.unsigned_32(player->hidden_fields_.size());
+		for (const auto& hidden : player->hidden_fields_) {
+			hidden_file.unsigned_32(hidden.first);
+			hidden_file.unsigned_16(hidden.second);
+		}
 
 		char filename[FILENAME_SIZE];
 
@@ -1005,6 +1036,8 @@
 
 		WRITE(survey_times_file, SURVEY_TIMES_FILENAME_TEMPLATE, kCurrentPacketVersionSurveyTimes);
 
+		WRITE(hidden_file, HIDDEN_FILENAME_TEMPLATE, kCurrentPacketVersionHidden);
+
 		WRITE(vision_file, VISION_FILENAME_TEMPLATE, kCurrentPacketVersionVision);
 
 		WRITE(border_file, BORDER_FILENAME_TEMPLATE, kCurrentPacketVersionBorder);

=== modified file 'src/scripting/lua_game.cc'
--- src/scripting/lua_game.cc	2017-12-17 03:57:22 +0000
+++ src/scripting/lua_game.cc	2017-12-19 07:13:00 +0000
@@ -564,25 +564,23 @@
 /* RST
    .. method:: reveal_fields(fields)
 
-      Make these fields visible for the current player. The fields will remain
-      visible until they are hidden again.
+      Make these fields visible for the current player. If the fields were previously hidden via
+      `hide_fields`, restores the exact vision that they had before that was done.
 
-      :arg fields: The fields to show
+      :arg fields: The fields to reveal
       :type fields: :class:`array` of :class:`wl.map.Fields`
 
       :returns: :const:`nil`
 */
 int LuaPlayer::reveal_fields(lua_State* L) {
-	EditorGameBase& egbase = get_egbase(L);
-	Player& p = get(L, egbase);
-	const Map& map = egbase.map();
+	Game& game = get_game(L);
+	Player& p = get(L, game);
 
 	luaL_checktype(L, 2, LUA_TTABLE);
 
 	lua_pushnil(L); /* first key */
 	while (lua_next(L, 2) != 0) {
-		p.see_node(
-		   map, map[0], (*get_user_class<LuaField>(L, -1))->fcoords(L), egbase.get_gametime());
+		p.hide_or_reveal_field(game.get_gametime(), (*get_user_class<LuaField>(L, -1))->coords(), SeeUnseeNode::kReveal);
 		lua_pop(L, 1);
 	}
 
@@ -592,29 +590,28 @@
 /* RST
    .. method:: hide_fields(fields)
 
-      Make these fields hidden for the current player if they are not
-      seen by a military building.
+      Make these fields hidden for the current player.
 
       :arg fields: The fields to hide
       :type fields: :class:`array` of :class:`wl.map.Fields`
 
-      :arg hide_completely: *Optional*. The fields will be marked as unexplored.
-      :type hide_completely: :class:`boolean`
+      :arg unexplore: *Optional*. If  `true`, the fields will be marked as completely unexplored.
+      :type unexplore: :class:`boolean`
 
       :returns: :const:`nil`
 */
 int LuaPlayer::hide_fields(lua_State* L) {
-	EditorGameBase& egbase = get_egbase(L);
-	Player& p = get(L, egbase);
+	Game& game = get_game(L);
+	Player& p = get(L, game);
 
 	luaL_checktype(L, 2, LUA_TTABLE);
-	const bool mode = !lua_isnone(L, 3) && luaL_checkboolean(L, 3);
+	const SeeUnseeNode mode = (!lua_isnone(L, 3) && luaL_checkboolean(L, 3)) ?
+	                                      SeeUnseeNode::kUnexplore :
+	                                      SeeUnseeNode::kUnsee;
 
 	lua_pushnil(L); /* first key */
 	while (lua_next(L, 2) != 0) {
-		p.unsee_node((*get_user_class<LuaField>(L, -1))->fcoords(L).field - &egbase.map()[0],
-		             egbase.get_gametime(),
-		             mode ? Player::UnseeNodeMode::kUnexplore : Player::UnseeNodeMode::kUnsee);
+		p.hide_or_reveal_field(game.get_gametime(), (*get_user_class<LuaField>(L, -1))->coords(), mode);
 		lua_pop(L, 1);
 	}
 

=== modified file 'test/maps/lua_testsuite.wmf/scripting/gplayer.lua'
--- test/maps/lua_testsuite.wmf/scripting/gplayer.lua	2017-03-24 08:31:08 +0000
+++ test/maps/lua_testsuite.wmf/scripting/gplayer.lua	2017-12-19 07:13:00 +0000
@@ -8,82 +8,6 @@
    assert_equal("Awesome Atlantean", egbase.players[3].name)
 end
 
--- -- =======================================================================
--- --                          See Fields/Hide Fields
--- -- =======================================================================
-player_vision_tests = lunit.TestCase("Player vision tests")
-function player_vision_tests:setup()
-   self.f = map:get_field(50, 20)
-   player1.see_all = false
-end
-function player_vision_tests:teardown()
-   player1:hide_fields(self.f:region(1))
-   player1.see_all = false
-end
--- This test must appear as the very first
-function player_vision_tests:test_seen_field()
-   assert_equal(false, player1:sees_field(self.f))
-   assert_equal(false, player1:seen_field(self.f))
-   player1:reveal_fields(self.f:region(1))
-   player1:hide_fields(self.f:region(1))
-   assert_equal(false, player1:sees_field(self.f))
-   assert_equal(true, player1:seen_field(self.f))
-end
-
-function player_vision_tests:test_sees_field()
-   assert_equal(false, player1:sees_field(self.f))
-   player1:reveal_fields(self.f:region(1))
-   assert_equal(true, player1:sees_field(self.f))
-   player1:hide_fields(self.f:region(1))
-   assert_equal(false, player1:sees_field(self.f))
-end
-function player_vision_tests:test_see_all()
-   assert_equal(false, player1:sees_field(self.f))
-   player1.see_all = true
-   assert_equal(true, player1.see_all)
-   assert_equal(true, player1:sees_field(self.f))
-   player1.see_all = false
-   assert_equal(false, player1:sees_field(self.f))
-end
-function player_vision_tests:test_sees_field_see_all_hide()
-   player1.see_all = true
-   assert_equal(true, player1:sees_field(self.f))
-   player1:hide_fields(self.f:region(1))
-   assert_equal(true, player1:sees_field(self.f))
-   player1.see_all = false
-   assert_equal(false, player1:sees_field(self.f))
-
-   player1:reveal_fields(self.f:region(1))
-   assert_equal(true, player1:sees_field(self.f))
-   player1.see_all = false
-   assert_equal(true, player1:sees_field(self.f))
-end
-
--- This test must go last, because we change the state of player1:seen_field.
-function player_vision_tests:test_hide_completely()
-   player1.see_all = true
-   assert_equal(true, player1:sees_field(self.f))
-   player1.see_all = false
-   assert_equal(false, player1:sees_field(self.f))
-
-   player1:hide_fields(self.f:region(1), false)
-   game.desired_speed = 0;
-   assert_equal(false, player1:sees_field(self.f))
-   assert_equal(true, player1:seen_field(self.f))
-
-   player1.see_all = true
-   assert_equal(true, player1:sees_field(self.f))
-   player1.see_all = false
-   assert_equal(false, player1:sees_field(self.f))
-
-   player1:hide_fields(self.f:region(1), true)
-   game.desired_speed = 0;
-   assert_equal(false, player1:sees_field(self.f))
-   assert_equal(false, player1:seen_field(self.f))
-   player1:reveal_fields(self.f:region(1))
-end
-
-
 -- =========================
 -- Forbid & Allow buildings
 -- =========================

=== added file 'test/maps/plain.wmf/scripting/test_player_vision.lua'
--- test/maps/plain.wmf/scripting/test_player_vision.lua	1970-01-01 00:00:00 +0000
+++ test/maps/plain.wmf/scripting/test_player_vision.lua	2017-12-19 07:13:00 +0000
@@ -0,0 +1,85 @@
+-- -- =======================================================================
+-- --                          See Fields/Hide Fields
+-- -- =======================================================================
+
+run(function()
+   function cleanup(field)
+      p1:hide_fields(field:region(1))
+      p1.see_all = false
+   end
+
+   sleep(5000)
+   local field = wl.Game().map:get_field(50, 20)
+   cleanup(field)
+
+   -- Seen field
+   assert_equal(false, p1:sees_field(field))
+   assert_equal(false, p1:seen_field(field))
+   p1:reveal_fields(field:region(1))
+   sleep(1000)
+   p1:hide_fields(field:region(1))
+   sleep(1000)
+   assert_equal(false, p1:sees_field(field))
+   assert_equal(true, p1:seen_field(field))
+   cleanup(field)
+
+   -- Sees field
+   assert_equal(false, p1:sees_field(field))
+   p1:reveal_fields(field:region(1))
+   sleep(1000)
+   assert_equal(true, p1:sees_field(field))
+   p1:hide_fields(field:region(1))
+   sleep(1000)
+   assert_equal(false, p1:sees_field(field))
+   cleanup(field)
+
+   -- See all
+   assert_equal(false, p1:sees_field(field))
+   p1.see_all = true
+   assert_equal(true, p1.see_all)
+   assert_equal(true, p1:sees_field(field))
+   p1.see_all = false
+   assert_equal(false, p1:sees_field(field))
+   cleanup(field)
+
+   -- Sees field see all hide
+   p1.see_all = true
+   assert_equal(true, p1:sees_field(field))
+   p1:hide_fields(field:region(1))
+   sleep(1000)
+   assert_equal(true, p1:sees_field(field))
+   p1.see_all = false
+   assert_equal(false, p1:sees_field(field))
+   cleanup(field)
+
+   p1:reveal_fields(field:region(1))
+   sleep(1000)
+   assert_equal(true, p1:sees_field(field))
+   p1.see_all = false
+   assert_equal(true, p1:sees_field(field))
+   cleanup(field)
+
+   -- Unexplore
+   p1:hide_fields(field:region(1), false)
+   sleep(1000)
+   assert_equal(false, p1:sees_field(field))
+   assert_equal(true, p1:seen_field(field))
+
+   p1.see_all = true
+   assert_equal(true, p1:sees_field(field))
+   p1.see_all = false
+   assert_equal(false, p1:sees_field(field))
+
+   p1:hide_fields(field:region(1), true)
+   sleep(1000)
+   assert_equal(false, p1:sees_field(field))
+   assert_equal(false, p1:seen_field(field))
+   p1:reveal_fields(field:region(1))
+   sleep(1000)
+   assert_equal(true, p1:sees_field(field))
+   assert_equal(true, p1:seen_field(field))
+   cleanup(field)
+
+   print("# All Tests passed.")
+   wl.ui.MapView():close()
+end)