Merge lp:~widelands-dev/widelands/productionsite-bugs into lp:widelands

[Benedikt Straub]

– Switch building to unoccupied animation when worker is removed
– Allow basic tasks like carrying out superfluous wares even if not all workers are present yet

[Toni Förster (stonerl)]

Changing animations works. But the linked bug #1643170. I tested with a wood hardener. Working slot is shown as vacant and no lumberjack is available, but the wood hardener is filled with wares.

review: Needs Fixing

[Toni Förster (stonerl)]

I think I need to explain better, sorry. The changing animations are working, also a building gets emptied when only one worker is available (tested with the enhanced brewery). But a building that has all worker slot vacant gets filled with wares. Since no worker will enter the building the wares can't be removed either and are kind of "lost".

[Benedikt Straub (nordfriese)]

The bug is only about wares not being carried out if some slots are still vacant although one is occupied. I consider the behaviour which you mean desired: Most of the time, you want to start transporting wares to the building at once (you don´t usually build buildings if you expect them to remain vacant for a long time, right?) so it can start working at once when the worker(s) arrive(s). If you really want the inputqueues to remain empty until a worker comes, you should have to micromanage this by just setting the capacities to 0 when construction is completed IMHO.

[Toni Förster (stonerl)]

Hmm. The problem I see is that usually no reminder/notification or any other indicator is present when a building has been constructed. So you might end up with a stocked building before you're even able to set it to 0.

[Benedikt Straub (nordfriese)]

I usually keep the construction window open in a corner of the screen if I want to decrease wares directly after completion. We have an open wishlist bug about setting the settings for the later building in the constructionsite window, that will fix this.

[Toni Förster (stonerl)]

Okay, it was just because the bug report I referred to, was explicitly mentioning what I described. But If this will be taken care in another branch then I'll have no reason for complaining :D

review: Approve

[Benedikt Straub (nordfriese)]

Since it´s related to this feature, I´ll take care of bug 1597310 soon then :)

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4936. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/531118419.
Appveyor build 4717. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_productionsite_bugs-4717.

[GunChleoc (gunchleoc)]

1 nit for the saveloading, not tested yet.

We should also remember to add TODO comments when adding savegame compatibility code - this will make it easier to find and remove after Build 21.

[GunChleoc (gunchleoc)]

This does not quite work yet.

1. Build a Wood Hardener
2. Wait until it is full
3. Set input queue to 0 logs
4. Evict the worker

When the worker returns, he won't remove the wares from the building

review: Needs Fixing

[lp:~widelands-dev/widelands/productionsite-bugs]

9099. By Nordfriese on 2019-05-12

Mixed up two indices

[Benedikt Straub (nordfriese)]

Bug fixed

[lp:~widelands-dev/widelands/productionsite-bugs]

9100. By GunChleoc on 2019-05-12

Added todo comment for savegae compatibility.

[GunChleoc (gunchleoc)]

Confirmed. Dome more testing and is working fine :)

@bunnybot merge

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4954. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/531379175.
Appveyor build 4735. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_productionsite_bugs-4735.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4954. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/531379175.

[Benedikt Straub (nordfriese)]

Transient travis failure

@bunnybot merge force

=== modified file 'src/logic/map_objects/tribes/building.h'
--- src/logic/map_objects/tribes/building.h	2019-04-26 05:52:49 +0000
+++ src/logic/map_objects/tribes/building.h	2019-05-12 10:53:28 +0000
@@ -326,14 +326,14 @@
 	                  uint32_t throttle_time = 0,
 	                  uint32_t throttle_radius = 0);
 
+	void start_animation(EditorGameBase&, uint32_t anim);
+
 protected:
 	// Updates 'statistics_string' with the string that should be displayed for
 	// this building right now. Overwritten by child classes.
 	virtual void update_statistics_string(std::string*) {
 	}
 
-	void start_animation(EditorGameBase&, uint32_t anim);
-
 	bool init(EditorGameBase&) override;
 	void cleanup(EditorGameBase&) override;
 	void act(Game&, uint32_t data) override;

=== modified file 'src/logic/map_objects/tribes/production_program.cc'
--- src/logic/map_objects/tribes/production_program.cc	2019-05-05 14:05:07 +0000
+++ src/logic/map_objects/tribes/production_program.cc	2019-05-12 10:53:28 +0000
@@ -699,7 +699,7 @@
 
 void ProductionProgram::ActCallWorker::execute(Game& game, ProductionSite& ps) const {
 	// Always main worker is doing stuff
-	ps.working_positions_[0].worker->update_task_buildingwork(game);
+	ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
 }
 
 bool ProductionProgram::ActCallWorker::get_building_work(Game& game,
@@ -985,7 +985,7 @@
 void ProductionProgram::ActProduce::execute(Game& game, ProductionSite& ps) const {
 	assert(ps.produced_wares_.empty());
 	ps.produced_wares_ = produced_wares_;
-	ps.working_positions_[0].worker->update_task_buildingwork(game);
+	ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
 
 	const TribeDescr& tribe = ps.owner().tribe();
 	assert(produced_wares_.size());
@@ -1071,7 +1071,7 @@
 void ProductionProgram::ActRecruit::execute(Game& game, ProductionSite& ps) const {
 	assert(ps.recruited_workers_.empty());
 	ps.recruited_workers_ = recruited_workers_;
-	ps.working_positions_[0].worker->update_task_buildingwork(game);
+	ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
 
 	const TribeDescr& tribe = ps.owner().tribe();
 	assert(recruited_workers_.size());
@@ -1523,7 +1523,7 @@
 	if (map.find_reachable_immovables(area, &immovables, cstep, FindImmovableByDescr(descr))) {
 		state.objvar = immovables[0].object;
 
-		psite.working_positions_[0].worker->update_task_buildingwork(game);
+		psite.working_positions_[psite.main_worker_].worker->update_task_buildingwork(game);
 		return;
 	}
 
@@ -1559,7 +1559,7 @@
 
 		state.coord = best_coords;
 
-		psite.working_positions_[0].worker->update_task_buildingwork(game);
+		psite.working_positions_[psite.main_worker_].worker->update_task_buildingwork(game);
 		return;
 	}
 

=== modified file 'src/logic/map_objects/tribes/productionsite.cc'
--- src/logic/map_objects/tribes/productionsite.cc	2019-05-05 14:05:07 +0000
+++ src/logic/map_objects/tribes/productionsite.cc	2019-05-12 10:53:28 +0000
@@ -270,7 +270,8 @@
      last_stat_percent_(0),
      crude_percent_(0),
      is_stopped_(false),
-     default_anim_("idle") {
+     default_anim_("idle"),
+     main_worker_(-1) {
 	calc_statistics();
 }
 
@@ -565,16 +566,20 @@
 void ProductionSite::remove_worker(Worker& w) {
 	molog("%s leaving\n", w.descr().name().c_str());
 	WorkingPosition* wp = working_positions_;
+	int32_t wp_index = 0;
 
 	for (const auto& temp_wp : descr().working_positions()) {
 		DescriptionIndex const worker_index = temp_wp.first;
-		for (uint32_t j = temp_wp.second; j; --j, ++wp) {
+		for (uint32_t j = temp_wp.second; j; --j, ++wp, ++wp_index) {
 			Worker* const worker = wp->worker;
 			if (worker && worker == &w) {
 				// do not request the type of worker that is currently assigned - maybe a trained worker
 				// was
 				// evicted to make place for a level 0 worker.
 				// Therefore we again request the worker from the WorkingPosition of descr()
+				if (main_worker_ == wp_index) {
+					main_worker_ = -1;
+				}
 				*wp = WorkingPosition(&request_worker(worker_index), nullptr);
 				Building::remove_worker(w);
 				return;
@@ -685,11 +690,14 @@
 		program_timer_ = false;
 
 		if (!can_start_working()) {
-			while (!stack_.empty())
+			start_animation(game, descr().get_unoccupied_animation());
+			while (!stack_.empty()) {
 				program_end(game, ProgramResult::kFailed);
+			}
 		} else {
+			assert(main_worker_ >= 0);
 			if (stack_.empty()) {
-				working_positions_[0].worker->update_task_buildingwork(game);
+				working_positions_[main_worker_].worker->update_task_buildingwork(game);
 				return;
 			}
 
@@ -737,8 +745,10 @@
 bool ProductionSite::fetch_from_flag(Game& game) {
 	++fetchfromflag_;
 
-	if (can_start_working())
-		working_positions_[0].worker->update_task_buildingwork(game);
+	if (main_worker_ >= 0) {
+		assert(working_positions_[main_worker_].worker);
+		working_positions_[main_worker_].worker->update_task_buildingwork(game);
+	}
 
 	return true;
 }
@@ -767,10 +777,19 @@
 }
 
 void ProductionSite::try_start_working(Game& game) {
-	if (can_start_working() && descr().working_positions().size()) {
-		Worker& main_worker = *working_positions_[0].worker;
-		main_worker.reset_tasks(game);
-		main_worker.start_task_buildingwork(game);
+	if (main_worker_ >= 0) {
+		return;
+	}
+	const size_t nr_workers = descr().working_positions().size();
+	for (uint32_t i = 0; i < nr_workers; ++i) {
+		if (Worker* worker = working_positions_[i].worker) {
+			// We may start even if can_start_working() returns false, because basic actions
+			// like unloading extra wares should take place anyway
+			main_worker_ = i;
+			worker->reset_tasks(game);
+			worker->start_task_buildingwork(game);
+			return;
+		}
 	}
 }
 
@@ -781,7 +800,8 @@
  */
 bool ProductionSite::get_building_work(Game& game, Worker& worker, bool const success) {
 	assert(descr().working_positions().size());
-	assert(&worker == working_positions_[0].worker);
+	assert(main_worker_ >= 0);
+	assert(&worker == working_positions_[main_worker_].worker);
 
 	// If unsuccessful: Check if we need to abort current program
 	if (!success) {
@@ -860,8 +880,11 @@
 	}
 
 	// Check if all workers are there
-	if (!can_start_working())
-		return false;
+	if (!can_start_working()) {
+		// Try again a bit later
+		worker.start_task_idle(game, 0, 3000);
+		return true;
+	}
 
 	// Start program if we haven't already done so
 	State* state = get_state();

=== modified file 'src/logic/map_objects/tribes/productionsite.h'
--- src/logic/map_objects/tribes/productionsite.h	2019-05-05 14:05:07 +0000
+++ src/logic/map_objects/tribes/productionsite.h	2019-05-12 10:53:28 +0000
@@ -333,6 +333,8 @@
 	std::string statistics_string_on_changed_statistics_;
 	std::string production_result_;  // hover tooltip text
 
+	int32_t main_worker_;
+
 	DISALLOW_COPY_AND_ASSIGN(ProductionSite);
 };
 

=== modified file 'src/logic/map_objects/tribes/worker.cc'
--- src/logic/map_objects/tribes/worker.cc	2019-05-04 10:47:44 +0000
+++ src/logic/map_objects/tribes/worker.cc	2019-05-12 10:53:28 +0000
@@ -1639,7 +1639,15 @@
 	// Reset any signals that are not related to location
 	std::string signal = get_signal();
 	signal_handled();
+
+	upcast(Building, building, get_location(game));
+
 	if (signal == "evict") {
+		if (building) {
+			// If the building was working, we do not tell it to cancel – it'll notice by itself soon –
+			// but we already change the animation so it won't look strange
+			building->start_animation(game, building->descr().get_unoccupied_animation());
+		}
 		return pop_task(game);
 	}
 
@@ -1647,7 +1655,6 @@
 		state.ivar1 = (signal == "fail") * 2;
 
 	// Return to building, if necessary
-	upcast(Building, building, get_location(game));
 	if (!building)
 		return pop_task(game);
 

=== modified file 'src/map_io/map_buildingdata_packet.cc'
--- src/map_io/map_buildingdata_packet.cc	2019-04-09 17:16:11 +0000
+++ src/map_io/map_buildingdata_packet.cc	2019-05-12 10:53:28 +0000
@@ -65,7 +65,7 @@
 // Responsible for warehouses and expedition bootstraps
 constexpr uint16_t kCurrentPacketVersionWarehouse = 7;
 constexpr uint16_t kCurrentPacketVersionMilitarysite = 6;
-constexpr uint16_t kCurrentPacketVersionProductionsite = 6;
+constexpr uint16_t kCurrentPacketVersionProductionsite = 7;
 constexpr uint16_t kCurrentPacketVersionTrainingsite = 5;
 
 void MapBuildingdataPacket::read(FileSystem& fs,
@@ -552,6 +552,7 @@
                                                 MapObjectLoader& mol) {
 	try {
 		uint16_t const packet_version = fr.unsigned_16();
+		// TODO(GunChleoc): Savegame compatibility, remove after Build 21.
 		if (packet_version >= 5 && packet_version <= kCurrentPacketVersionProductionsite) {
 			ProductionSite::WorkingPosition& wp_begin = *productionsite.working_positions_;
 			const ProductionSiteDescr& pr_descr = productionsite.descr();
@@ -710,6 +711,13 @@
 				productionsite.statistics_[i] = fr.unsigned_8();
 			productionsite.statistics_string_on_changed_statistics_ = fr.c_string();
 			productionsite.production_result_ = fr.c_string();
+
+			// TODO(GunChleoc): Savegame compatibility, remove after Build 21.
+			if (kCurrentPacketVersionProductionsite >= 7) {
+				productionsite.main_worker_ = fr.signed_32();
+			} else {
+				productionsite.main_worker_ = productionsite.working_positions_[0].worker ? 0 : -1;
+			}
 		} else {
 			throw UnhandledVersionError("MapBuildingdataPacket - Productionsite", packet_version,
 			                            kCurrentPacketVersionProductionsite);
@@ -1173,6 +1181,8 @@
 		fw.unsigned_8(productionsite.statistics_[i]);
 	fw.string(productionsite.statistics_string_on_changed_statistics_);
 	fw.string(productionsite.production_result());
+
+	fw.signed_32(productionsite.main_worker_);
 }
 
 /*