Merge lp:~widelands-dev/widelands/expedition_portspace_indicator into lp:widelands

[Benedikt Straub]

Show a port icon on the field where an expedition ship can build a port

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4835. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/525916562.
Appveyor build 4616. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_expedition_portspace_indicator-4616.

[GunChleoc (gunchleoc)]

We are trying to get rid of references to InteractiveBase from the logic library.

Would you like to try your hand at adding new actions to NoteShip and subscribing to those in InteractivePlayer?

After placing a port construction site, a big building plot icon is shown on top of the construction site.

review: Needs Fixing

[Benedikt Straub (nordfriese)]

OK, now InteractiveBase receives a notification instead when a portspace is found. Since there are fairly many reasons why a port space can become unavailable again (ship sails on, port is constructed, ship is sunk…), the IBase takes care of cleaning up the set itself now when think()ing. This fixes the bug that the indicator won´t go away on portconstruction.
Also added saving support for the indicators so they are also shown for ships that already are in PortSpaceFound state when the game is loaded.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

I am still wondering if we can get more performance out of this thing.

* Ship sails on and ship is sunk could be taken care of by the ShipNote
* Port being built can be taken care of by buildcaps, just like we do for the building spaces overlays in general

Any other reasons?

+1 for the saving support :)

[Benedikt Straub (nordfriese)]

> Port being built can be taken care of by buildcaps
IMHO we should remove the portspaces either only in response to Notes or only autonomously. Mixing both feels messy.
To use notes, we´d need subscriptions (in addition to what we already have) to
– NoteExpeditionCanceled
– FieldPossession (another possible reason why a portspace might become unavailable is that some other player conquers that land, which is not even necessarily reflected in the buildcaps)
– FieldTerrainChanged (if the port space is removed by a terrain-changing script)
– A tree grows over the port space (or is this forbidden somehow?) → is there a notification for that?
– A script unsets a port space directly → is there a notification for that?

That´s quite a lot for a pretty small feature IMHO; and I don´t think the autonomous cleanup of displayed portspaces is performance-critical – I can´t imagine a player will have so many expeditions at a time that it really matters.

[GunChleoc (gunchleoc)]

You have convinced me.

I have done more testing - all working now :)

@bunnybot merge

[GunChleoc (gunchleoc)]

review: Approve

=== modified file 'src/game_io/game_interactive_player_packet.cc'
--- src/game_io/game_interactive_player_packet.cc	2019-02-23 11:00:49 +0000
+++ src/game_io/game_interactive_player_packet.cc	2019-05-03 09:15:36 +0000
@@ -25,6 +25,8 @@
 #include "logic/game_data_error.h"
 #include "logic/map_objects/tribes/tribe_descr.h"
 #include "logic/player.h"
+#include "map_io/map_object_loader.h"
+#include "map_io/map_object_saver.h"
 #include "wui/interactive_player.h"
 #include "wui/mapview.h"
 
@@ -32,16 +34,16 @@
 
 namespace {
 
-constexpr uint16_t kCurrentPacketVersion = 4;
+constexpr uint16_t kCurrentPacketVersion = 5;
 
 }  // namespace
 
-void GameInteractivePlayerPacket::read(FileSystem& fs, Game& game, MapObjectLoader*) {
+void GameInteractivePlayerPacket::read(FileSystem& fs, Game& game, MapObjectLoader* mol) {
 	try {
 		FileRead fr;
 		fr.open(fs, "binary/interactive_player");
 		uint16_t const packet_version = fr.unsigned_16();
-		if (packet_version == kCurrentPacketVersion) {
+		if (packet_version <= kCurrentPacketVersion && packet_version >= 4) {
 			PlayerNumber player_number = fr.unsigned_8();
 			if (!(0 < player_number && player_number <= game.map().get_nrplayers())) {
 				throw GameDataError("Invalid player number: %i.", player_number);
@@ -92,6 +94,17 @@
 						ibase->set_landmark(i, view);
 					}
 				}
+
+				if (packet_version == kCurrentPacketVersion) {
+					size_t nr_port_spaces = fr.unsigned_32();
+					for (size_t i = 0; i < nr_port_spaces; ++i) {
+						uint32_t serial = fr.unsigned_32();
+						int16_t x = fr.signed_16();
+						int16_t y = fr.signed_16();
+						ibase->get_expedition_port_spaces().emplace(&mol->get<Widelands::Ship>(serial),
+								Widelands::Coords(x, y));
+					}
+				}
 			}
 		} else {
 			throw UnhandledVersionError(
@@ -105,7 +118,7 @@
 /*
  * Write Function
  */
-void GameInteractivePlayerPacket::write(FileSystem& fs, Game& game, MapObjectSaver* const) {
+void GameInteractivePlayerPacket::write(FileSystem& fs, Game& game, MapObjectSaver* const mos) {
 	FileWrite fw;
 
 	fw.unsigned_16(kCurrentPacketVersion);
@@ -138,6 +151,13 @@
 			fw.float_32(landmark.view.viewpoint.y);
 			fw.float_32(landmark.view.zoom);
 		}
+
+		fw.unsigned_32(ibase->get_expedition_port_spaces().size());
+		for (const auto& pair : ibase->get_expedition_port_spaces()) {
+			fw.unsigned_32(mos->get_object_file_index(*pair.first));
+			fw.signed_16(pair.second.x);
+			fw.signed_16(pair.second.y);
+		}
 	}
 
 	fw.write(fs, "binary/interactive_player");
 
=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2019-04-25 06:31:33 +0000
+++ src/wui/interactive_base.cc	2019-05-03 09:15:36 +0000
@@ -152,6 +152,12 @@
 	   });
 	sound_subscriber_ = Notifications::subscribe<NoteSound>(
 	   [this](const NoteSound& note) { play_sound_effect(note); });
+	shipnotes_subscriber_ = Notifications::subscribe<Widelands::NoteShip>([this](const Widelands::NoteShip& note) {
+		if (note.action == Widelands::NoteShip::Action::kWaitingForCommand &&
+				note.ship->get_ship_state() == Widelands::Ship::ShipStates::kExpeditionPortspaceFound) {
+			expedition_port_spaces_.emplace(note.ship, note.ship->exp_port_spaces().front());
+		}
+	});
 
 	toolbar_.set_layout_toplevel(true);
 	map_view_.changeview.connect([this] { mainview_move(); });
@@ -304,6 +310,15 @@
 void InteractiveBase::on_buildhelp_changed(bool /* value */) {
 }
 
+bool InteractiveBase::has_expedition_port_space(const Widelands::Coords& coords) const {
+	for (const auto& pair : expedition_port_spaces_) {
+		if (pair.second == coords) {
+			return true;
+		}
+	}
+	return false;
+}
+
 // Show the given workareas at the given coords and returns the overlay job id associated
 void InteractiveBase::show_workarea(const WorkareaInfo& workarea_info, Widelands::Coords coords) {
 	workarea_previews_[coords] = &workarea_info;
@@ -457,6 +472,16 @@
 	}
 	egbase().think();  // Call game logic here. The game advances.
 
+	// Cleanup found port spaces if the ship sailed on or was destroyed
+	for (auto it = expedition_port_spaces_.begin(); it != expedition_port_spaces_.end(); ++it) {
+		if (!egbase().objects().object_still_available(it->first) ||
+				it->first->get_ship_state() != Widelands::Ship::ShipStates::kExpeditionPortspaceFound) {
+			expedition_port_spaces_.erase(it);
+			// If another port space also needs removing, we'll take care of it in the next frame
+			return;
+		}
+	}
+
 	UI::Panel::think();
 }
 
 
=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2019-04-25 06:31:33 +0000
+++ src/wui/interactive_base.h	2019-05-03 09:15:36 +0000
@@ -86,6 +86,11 @@
 	void show_workarea(const WorkareaInfo& workarea_info, Widelands::Coords coords);
 	void hide_workarea(const Widelands::Coords& coords);
 
+	bool has_expedition_port_space(const Widelands::Coords&) const;
+	std::map<Widelands::Ship*, Widelands::Coords>& get_expedition_port_spaces() {
+		return expedition_port_spaces_;
+	}
+
 	//  point of view for drawing
 	virtual Widelands::Player* get_player() const = 0;
 
@@ -291,11 +296,14 @@
 	// coordinate that the building that shows the work area is positioned.
 	std::map<Widelands::Coords, const WorkareaInfo*> workarea_previews_;
 
+	std::map<Widelands::Ship*, Widelands::Coords> expedition_port_spaces_;
+
 	RoadBuildingOverlays road_building_overlays_;
 
 	std::unique_ptr<Notifications::Subscriber<GraphicResolutionChanged>>
 	   graphic_resolution_changed_subscriber_;
 	std::unique_ptr<Notifications::Subscriber<NoteSound>> sound_subscriber_;
+	std::unique_ptr<Notifications::Subscriber<Widelands::NoteShip>> shipnotes_subscriber_;
 	Widelands::EditorGameBase& egbase_;
 	uint32_t display_flags_;
 	uint32_t lastframe_;        //  system time (milliseconds)
 
=== modified file 'src/wui/interactive_player.cc'
--- src/wui/interactive_player.cc	2019-04-25 06:31:33 +0000
+++ src/wui/interactive_player.cc	2019-05-03 09:15:36 +0000
@@ -339,8 +339,10 @@
 
 		if (f->vision > 0) {
 			// Draw build help.
-			if (buildhelp()) {
-				const auto* overlay = get_buildhelp_overlay(plr.get_buildcaps(f->fcoords));
+			bool show_port_space = has_expedition_port_space(f->fcoords);
+			if (show_port_space || buildhelp()) {
+				const auto* overlay = get_buildhelp_overlay(show_port_space ?
+						f->fcoords.field->maxcaps() : plr.get_buildcaps(f->fcoords));
 				if (overlay != nullptr) {
 					blit_field_overlay(dst, *f, overlay->pic, overlay->hotspot, scale);
 				}