widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1191295 in widelands: "Seafaring: builder not listed in expedition list in port"
  https://bugs.launchpad.net/widelands/+bug/1191295

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1191295-no-seafaring-builder/+merge/315525

Added display of builder to expedition bootstrap. Seems to work fine so far, but I am not sure how to implement the saving/loading code (in the branch disabled). The current version of it does not contain a version number so changing it will probably break it. 

Possible hack:

Currently the first entry in the saved bootstrap class is the number of builders already waiting. Since this is always 0 or 1 this could be abused as a "version number" when loading the packet, using version number 2 for the new packet format. This should work for loading older save games with a new version of the game but would give some strange error/crash when loading a new save game with an old version of the game (I guess. Since the old game would not recognize the new format it would try to load two workers).
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1191295-no-seafaring-builder into lp:widelands.

=== modified file 'src/economy/expedition_bootstrap.cc'
--- src/economy/expedition_bootstrap.cc	2016-12-05 09:24:10 +0000
+++ src/economy/expedition_bootstrap.cc	2017-01-24 23:24:19 +0000
@@ -24,6 +24,8 @@
 #include "base/macros.h"
 #include "base/wexception.h"
 #include "economy/portdock.h"
+#include "economy/wares_queue.h"
+#include "economy/workers_queue.h"
 #include "io/filewrite.h"
 #include "logic/map_objects/tribes/warehouse.h"
 #include "logic/player.h"
@@ -33,35 +35,17 @@
 
 namespace Widelands {
 
-struct ExpeditionBootstrap::ExpeditionWorker {
-	// Ownership is taken.
-	ExpeditionWorker(Request* r) : request(r), worker(nullptr) {
-	}
-
-	std::unique_ptr<Request> request;
-
-	// we never own the worker. Either he is in the PortDock in which case it
-	// owns it or it is on the ship.
-	Worker* worker;
-};
-
 ExpeditionBootstrap::ExpeditionBootstrap(PortDock* const portdock)
    : portdock_(portdock), economy_(portdock->get_economy()) {
 }
 
 ExpeditionBootstrap::~ExpeditionBootstrap() {
-	assert(workers_.empty());
-	assert(wares_.empty());
+	assert(queues_.empty());
 }
 
 void ExpeditionBootstrap::is_ready(Game& game) {
-	for (std::unique_ptr<WaresQueue>& wq : wares_) {
-		if (wq->get_max_fill() != wq->get_filled())
-			return;
-	}
-
-	for (std::unique_ptr<ExpeditionWorker>& ew : workers_) {
-		if (ew->request)
+	for (std::unique_ptr<InputQueue>& iq : queues_) {
+		if (iq->get_max_fill() != iq->get_filled())
 			return;
 	}
 
@@ -70,48 +54,13 @@
 }
 
 // static
-void ExpeditionBootstrap::ware_callback(
-   Game& game, InputQueue*, DescriptionIndex, Worker*, void* const data) {
+void ExpeditionBootstrap::input_callback(Game& game, InputQueue*, DescriptionIndex, Worker*, void* data) {
 	ExpeditionBootstrap* eb = static_cast<ExpeditionBootstrap*>(data);
 	eb->is_ready(game);
 }
 
-// static
-void ExpeditionBootstrap::worker_callback(
-   Game& game, Request& r, DescriptionIndex, Worker* worker, PlayerImmovable& pi) {
-	Warehouse* warehouse = static_cast<Warehouse*>(&pi);
-
-	warehouse->get_portdock()->expedition_bootstrap()->handle_worker_callback(game, r, worker);
-}
-
-void ExpeditionBootstrap::handle_worker_callback(Game& game, Request& request, Worker* worker) {
-	for (std::unique_ptr<ExpeditionWorker>& ew : workers_) {
-		if (ew->request.get() == &request) {
-			ew->request.reset(nullptr);  // deletes &request.
-			ew->worker = worker;
-
-			// This is not really correct. All the wares are not in the portdock,
-			// so why should the worker be there. Well, it has to be somewhere
-			// (location != nullptr) and it should be in our economy so he is
-			// properly accounted for, so why not in the portdock. Alternatively
-			// we could kill him and recreate him as soon as we bord the ship -
-			// this should be no problem as workers are not gaining experience.
-			worker->set_location(portdock_);
-			worker->reset_tasks(game);
-			worker->start_task_idle(game, 0, -1);
-
-			is_ready(game);
-			return;
-		}
-	}
-	// Never here, otherwise we would have a callback for a request we know
-	// nothing about.
-	NEVER_HERE();
-}
-
 void ExpeditionBootstrap::start() {
-	assert(workers_.empty());
-	assert(wares_.empty());
+	assert(queues_.empty());
 
 	// Load the buildcosts for the port building + builder
 	Warehouse* const warehouse = portdock_->get_warehouse();
@@ -123,18 +72,17 @@
 	// TODO(sirver): could try to get some of these directly form the warehouse.
 	// But this is really a premature optimization and should probably be
 	// handled in the economy code.
-	wares_.resize(buildcost_size);
+	queues_.resize(buildcost_size + 1);
 	std::map<DescriptionIndex, uint8_t>::const_iterator it = buildcost.begin();
 	for (size_t i = 0; i < buildcost_size; ++i, ++it) {
 		WaresQueue* wq = new WaresQueue(*warehouse, it->first, it->second);
-		wq->set_callback(ware_callback, this);
-		wares_[i].reset(wq);
+		wq->set_callback(input_callback, this);
+		queues_[i].reset(wq);
 	}
 
 	// Issue the request for the workers (so far only a builder).
-	workers_.emplace_back(
-	   new ExpeditionWorker(new Request(*warehouse, warehouse->owner().tribe().builder(),
-	                                    ExpeditionBootstrap::worker_callback, wwWORKER)));
+	queues_[buildcost_size].reset(new WorkersQueue(*warehouse, warehouse->owner().tribe().builder(), 1));
+	queues_[buildcost_size]->set_callback(input_callback, this);
 
 	// Update the user interface
 	if (upcast(InteractiveGameBase, igb, warehouse->owner().egbase().get_ibase()))
@@ -144,19 +92,19 @@
 void ExpeditionBootstrap::cancel(Game& game) {
 	// Put all wares from the WaresQueues back into the warehouse
 	Warehouse* const warehouse = portdock_->get_warehouse();
-	for (std::unique_ptr<WaresQueue>& wq : wares_) {
-		warehouse->insert_wares(wq->get_index(), wq->get_filled());
-		wq->cleanup();
-	}
-	wares_.clear();
-
-	// Send all workers from the expedition list back inside the warehouse
-	for (std::unique_ptr<ExpeditionWorker>& ew : workers_) {
-		if (ew->worker) {
-			warehouse->incorporate_worker(game, ew->worker);
+	for (std::unique_ptr<InputQueue>& iq : queues_) {
+		if (iq->get_type() == wwWARE) {
+			warehouse->insert_wares(iq->get_index(), iq->get_filled());
+		} else {
+			assert(iq->get_type() == wwWORKER);
+			WorkersQueue *wq = dynamic_cast<WorkersQueue*>(iq.get());
+			while (iq->get_filled() > 0) {
+				warehouse->incorporate_worker(game, wq->extract_worker());
+			}
 		}
+		iq->cleanup();
 	}
-	workers_.clear();
+	queues_.clear();
 
 	// Update the user interface
 	if (upcast(InteractiveGameBase, igb, warehouse->owner().egbase().get_ibase())) {
@@ -166,29 +114,26 @@
 }
 
 void ExpeditionBootstrap::cleanup(EditorGameBase& /* egbase */) {
-	// This will delete all the requests. We do nothing with the workers as we
-	// do not own them.
-	workers_.clear();
 
-	for (std::unique_ptr<WaresQueue>& wq : wares_) {
-		wq->cleanup();
+	for (std::unique_ptr<InputQueue>& iq : queues_) {
+		iq->cleanup();
 	}
-	wares_.clear();
+	queues_.clear();
 }
 
-WaresQueue& ExpeditionBootstrap::waresqueue(DescriptionIndex index) const {
-	for (const std::unique_ptr<WaresQueue>& wq : wares_) {
-		if (wq->get_index() == index) {
-			return *wq.get();
+InputQueue& ExpeditionBootstrap::inputqueue(DescriptionIndex index, WareWorker type) const {
+	for (const std::unique_ptr<InputQueue>& iq : queues_) {
+		if (iq->get_index() == index && iq->get_type() == type) {
+			return *iq.get();
 		}
 	}
 	NEVER_HERE();
 }
 
-std::vector<WaresQueue*> ExpeditionBootstrap::wares() const {
-	std::vector<WaresQueue*> return_value;
-	for (const std::unique_ptr<WaresQueue>& wq : wares_) {
-		return_value.emplace_back(wq.get());
+std::vector<InputQueue*> ExpeditionBootstrap::queues() const {
+	std::vector<InputQueue*> return_value;
+	for (const std::unique_ptr<InputQueue>& iq : queues_) {
+		return_value.emplace_back(iq.get());
 	}
 	return return_value;
 }
@@ -197,21 +142,12 @@
 	if (new_economy == economy_)
 		return;
 
-	// Transfer the wares.
-	for (std::unique_ptr<WaresQueue>& wq : wares_) {
+	// Transfer the wares and workers.
+	for (std::unique_ptr<InputQueue>& iq : queues_) {
 		if (economy_)
-			wq->remove_from_economy(*economy_);
+			iq->remove_from_economy(*economy_);
 		if (new_economy)
-			wq->add_to_economy(*new_economy);
-	}
-
-	// Transfer the workers.
-	for (std::unique_ptr<ExpeditionWorker>& ew : workers_) {
-		if (ew->request) {
-			ew->request->set_economy(new_economy);
-		}
-		if (ew->worker)
-			ew->worker->set_economy(new_economy);
+			iq->add_to_economy(*new_economy);
 	}
 
 	economy_ = new_economy;
@@ -221,27 +157,37 @@
                                                         const TribeDescr& tribe,
                                                         std::vector<Worker*>* return_workers,
                                                         std::vector<WareInstance*>* return_wares) {
-	for (std::unique_ptr<WaresQueue>& wq : wares_) {
-		const DescriptionIndex ware_index = wq->get_index();
-		for (uint32_t j = 0; j < wq->get_filled(); ++j) {
-			WareInstance* temp = new WareInstance(ware_index, tribe.get_ware_descr(ware_index));
-			temp->init(game);
-			temp->set_location(game, portdock_);
-			return_wares->emplace_back(temp);
+	for (std::unique_ptr<InputQueue>& iq : queues_) {
+		if (iq->get_type() == wwWARE) {
+			const DescriptionIndex ware_index = iq->get_index();
+			for (uint32_t j = 0; j < iq->get_filled(); ++j) {
+				WareInstance* temp = new WareInstance(ware_index, tribe.get_ware_descr(ware_index));
+				temp->init(game);
+				temp->set_location(game, portdock_);
+				return_wares->emplace_back(temp);
+			}
+		} else {
+			assert(iq->get_type() == wwWORKER);
+			WorkersQueue *wq = dynamic_cast<WorkersQueue*>(iq.get());
+			while (iq->get_filled() > 0) {
+				return_workers->emplace_back(wq->extract_worker());
+			}
 		}
-		wq->set_filled(0);
-		wq->set_max_fill(0);
-	}
-
-	for (std::unique_ptr<ExpeditionWorker>& ew : workers_) {
-		assert(ew->worker != nullptr);
-		assert(!ew->request);
-		return_workers->emplace_back(ew->worker);
 	}
 
 	cleanup(game);
 }
 
+void ExpeditionBootstrap::save(FileWrite&, Game&, MapObjectSaver&) {
+}
+
+void ExpeditionBootstrap::load(Warehouse&,
+                               FileRead&,
+                               Game&,
+                               MapObjectLoader&) {
+}
+
+/*
 void ExpeditionBootstrap::save(FileWrite& fw, Game& game, MapObjectSaver& mos) {
 	// Expedition workers
 	fw.unsigned_8(workers_.size());
@@ -296,5 +242,6 @@
 		}
 	}
 }
+*/
 
 }  // namespace Widelands

=== modified file 'src/economy/expedition_bootstrap.h'
--- src/economy/expedition_bootstrap.h	2016-11-13 22:30:07 +0000
+++ src/economy/expedition_bootstrap.h	2017-01-24 23:24:19 +0000
@@ -24,7 +24,7 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "economy/wares_queue.h"
+#include "economy/input_queue.h"
 
 namespace Widelands {
 
@@ -36,7 +36,6 @@
 class Request;
 class WareInstance;
 class Warehouse;
-class WaresQueue;
 class Worker;
 
 // Handles the mustering of workers and wares in a port for one Expedition. This
@@ -62,30 +61,27 @@
 	                                   std::vector<Worker*>* return_workers,
 	                                   std::vector<WareInstance*>* return_wares);
 
-	// Returns the wares currently in stock.
-	std::vector<WaresQueue*> wares() const;
-
 	// Changes the economy for the wares that are already in store.
 	void set_economy(Economy* economy);
 
-	// Returns the waresqueue for this ware.
-	WaresQueue& waresqueue(DescriptionIndex index) const;
+	// Returns the wares and workers currently waiting for the expedition.
+	std::vector<InputQueue*> queues() const;
+
+	// Returns the matching input queue for the given index and type.
+	InputQueue& inputqueue(DescriptionIndex index, WareWorker type) const;
 
 	// Delete all wares we currently handle.
 	void cleanup(EditorGameBase& egbase);
 
 	// Save/Load this into a file. The actual data is stored in the buildingdata
 	// packet, and there in the warehouse data packet.
+	// NOCOM(Notabilis): Methods are currently empty, not quite sure how to implement them
 	void load(Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader& mol);
 	void save(FileWrite& fw, Game& game, MapObjectSaver& mos);
 
 private:
-	struct ExpeditionWorker;
-
 	// Handles arriving workers and wares.
-	static void worker_callback(Game&, Request& r, DescriptionIndex, Worker*, PlayerImmovable&);
-	static void ware_callback(Game& game, InputQueue*, DescriptionIndex, Worker*, void* const data);
-	void handle_worker_callback(Game&, Request&, Worker*);
+	static void input_callback(Game&, InputQueue*, DescriptionIndex, Worker*, void*);
 
 	// Tests if all wares for the expedition have arrived. If so, informs the portdock.
 	void is_ready(Game& game);
@@ -93,8 +89,7 @@
 	PortDock* const portdock_;  // not owned
 	Economy* economy_;
 
-	std::vector<std::unique_ptr<WaresQueue>> wares_;
-	std::vector<std::unique_ptr<ExpeditionWorker>> workers_;
+	std::vector<std::unique_ptr<InputQueue>> queues_;
 
 	DISALLOW_COPY_AND_ASSIGN(ExpeditionBootstrap);
 };

=== modified file 'src/economy/workers_queue.cc'
--- src/economy/workers_queue.cc	2016-12-05 19:32:06 +0000
+++ src/economy/workers_queue.cc	2017-01-24 23:24:19 +0000
@@ -159,6 +159,17 @@
 	}
 }
 
+Worker* WorkersQueue::extract_worker() {
+	assert(get_filled() > 0);
+	assert(!workers_.empty());
+
+	Worker* w = workers_.front();
+	// Don't remove from game
+	// Remove reference from list
+	workers_.erase(workers_.begin());
+	return w;
+}
+
 /**
  * Read and write
  */

=== modified file 'src/economy/workers_queue.h'
--- src/economy/workers_queue.h	2017-01-05 19:51:34 +0000
+++ src/economy/workers_queue.h	2017-01-24 23:24:19 +0000
@@ -61,6 +61,14 @@
 
 	void set_max_fill(Quantity q) override;
 
+	/**
+	 * Extracts the first worker from the queue and returns it
+	 * without removing it from the game.
+	 * Used by ExpeditionBootstrap.
+	 * @return The first worker in stored in this list.
+	 */
+	Worker* extract_worker();
+
 protected:
 	void read_child(FileRead&, Game&, MapObjectLoader&) override;
 	void write_child(FileWrite&, Game&, MapObjectSaver&) override;

=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc	2017-01-21 15:36:52 +0000
+++ src/logic/map_objects/tribes/warehouse.cc	2017-01-24 23:24:19 +0000
@@ -1274,9 +1274,8 @@
 InputQueue& Warehouse::inputqueue(DescriptionIndex index, WareWorker type) {
 	assert(portdock_ != nullptr);
 	assert(portdock_->expedition_bootstrap() != nullptr);
-	assert(type == wwWARE);
 
-	return portdock_->expedition_bootstrap()->waresqueue(index);
+	return portdock_->expedition_bootstrap()->inputqueue(index, type);
 }
 
 /*

=== modified file 'src/wui/portdockwaresdisplay.cc'
--- src/wui/portdockwaresdisplay.cc	2016-11-13 22:30:07 +0000
+++ src/wui/portdockwaresdisplay.cc	2017-01-24 23:24:19 +0000
@@ -25,11 +25,10 @@
 #include "economy/portdock.h"
 #include "logic/player.h"
 #include "wui/inputqueuedisplay.h"
-#include "wui/waresdisplay.h"
 
 using Widelands::PortDock;
 using Widelands::Warehouse;
-using Widelands::WaresQueue;
+using Widelands::InputQueue;
 
 namespace {
 
@@ -75,17 +74,10 @@
 create_portdock_expedition_display(UI::Panel* parent, Warehouse& wh, InteractiveGameBase& igb) {
 	UI::Box& box = *new UI::Box(parent, 0, 0, UI::Box::Vertical);
 
-	// Add the wares queues.
-	for (WaresQueue* wq : wh.get_portdock()->expedition_bootstrap()->wares()) {
+	// Add the input queues.
+	for (InputQueue* wq : wh.get_portdock()->expedition_bootstrap()->queues()) {
 		box.add(new InputQueueDisplay(&box, 0, 0, igb, wh, wq, true), UI::Align::kLeft);
 	}
 
-	// TODO(unknown): Implement UI for Builder + Soldiers
-	// UI::Box & workers = *new UI::Box(&box, 0, 0, UI::Box::Horizontal);
-	// box.add(&workers, UI::Align::kLeft);
-
-	// for (uint32_t i = 0; i < wh.get_expedition_workers().size(); ++i)
-	// workers.add(icon of worker, UI::Align::kLeft);
-
 	return &box;
 }