Merge lp:~widelands-dev/widelands/bug-1664145-miners into lp:widelands

[GunChleoc]

Fixed a bug where higher-level workers wouldn't occupy lower-level workers' working position slots in productionsites. Added a test.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1965. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/203049468.
Appveyor build 1800. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1664145_miners-1800.

[toptopple (7010622-q-deactivatedaccount)]

Tested today and appears to work correctly now. There was a tiny glitch in that 2 Master Miners (out of 7) remained at home despite new Miner posts requested; instead new Miners were created. We cannot say at this point where exactly this error lies, assuming it is an error. However, I would suggest that clearing out all Miners, Chief Miners and Master Miners should be target before new Miners are created from Pickaxes.

[TiborB (tiborb95)]

Gun, I can not see where in the code/diff you had changed anything..

[GunChleoc (gunchleoc)]

I marked up the line with the fix.

The tiny glitch is probably the result of a complex economy and a lot harder to track down. Can you reproduce it reliably? If so, we can add the info to the bug and keep the bug open.

[TiborB (tiborb95)]

Oh, I overlooked

As for the mentioned glitch - it is questionable if the fetching higher worker should be always preferred to creation of a new worker. I think mines right now are not able to switch higher worker for a spare lower worker to make the higher one available in case of need elsewhere.
But for now this should go as is

review: Approve

[GunChleoc (gunchleoc)]

Thanks for the review :)

You make a good point about the glitch.

@bunnybot merge

[toptopple (7010622-q-deactivatedaccount)]

>I think mines right now are not able to switch higher worker for a spare lower worker to make the higher one available in case of need elsewhere.

Not a good point in my view! I think there should be still something to do for the players of the game to perform. What is certainly correct is that AI-players must be in the position to perform miner shifting (not the mines themselves!), but this request must be up already.

[GunChleoc (gunchleoc)]

We have 3 open bugs regarding workers in productionsites:

https://bugs.launchpad.net/widelands/+bug/1235928
https://bugs.launchpad.net/widelands/+bug/1299011
https://bugs.launchpad.net/widelands/+bug/1619981

None of them describe your point, so please feel free to open a new bug for this :)

[TiborB (tiborb95)]

I think current situation is not that bad, game will try:
a) fetch miner of the same level
b) create new one (applicable for lowest miner only, of course)
c) fetch higher miner if available

switching options b<->c would not be significant difference I believe. But I dont mind such change...

=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
--- src/logic/map_objects/tribes/warehouse.cc	2017-02-12 09:10:57 +0000
+++ src/logic/map_objects/tribes/warehouse.cc	2017-02-18 23:38:55 +0000
@@ -742,17 +742,17 @@
  * requirements.
  */
 Quantity Warehouse::count_workers(const Game& /* game */,
-                                  DescriptionIndex ware,
+                                  DescriptionIndex worker_id,
                                   const Requirements& req,
                                   Match exact) {
 	Quantity sum = 0;
 
 	do {
-		sum += supply_->stock_workers(ware);
+		sum += supply_->stock_workers(worker_id);
 
 		// NOTE: This code lies about the TrainingAttributes of non-instantiated workers.
-		if (incorporated_workers_.count(ware)) {
-			for (Worker* worker : incorporated_workers_[ware]) {
+		if (incorporated_workers_.count(worker_id)) {
+			for (Worker* worker : incorporated_workers_[worker_id]) {
 				if (!req.check(*worker)) {
 					//  This is one of the workers in our sum.
 					//  But he is too stupid for this job
@@ -761,11 +761,11 @@
 			}
 		}
 		if (exact == Match::kCompatible) {
-			ware = owner().tribe().get_worker_descr(ware)->becomes();
+			worker_id = owner().tribe().get_worker_descr(worker_id)->becomes();
 		} else {
-			ware = INVALID_INDEX;
+			worker_id = INVALID_INDEX;
 		}
-	} while (owner().tribe().has_ware(ware));
+	} while (owner().tribe().has_worker(worker_id));
 
 	return sum;
 }
 
=== modified file 'src/logic/map_objects/tribes/warehouse.h'
--- src/logic/map_objects/tribes/warehouse.h	2017-01-25 18:55:59 +0000
+++ src/logic/map_objects/tribes/warehouse.h	2017-02-18 23:38:55 +0000
@@ -198,7 +198,7 @@
 
 	bool fetch_from_flag(Game&) override;
 
-	Quantity count_workers(const Game&, DescriptionIndex, const Requirements&, Match);
+	Quantity count_workers(const Game&, DescriptionIndex worker, const Requirements&, Match);
 	Worker& launch_worker(Game&, DescriptionIndex worker, const Requirements&);
 
 	// Adds the worker to the inventory. Takes ownership and might delete
 
=== added file 'test/maps/plain.wmf/scripting/test_upgraded_workers.lua'
--- test/maps/plain.wmf/scripting/test_upgraded_workers.lua	1970-01-01 00:00:00 +0000
+++ test/maps/plain.wmf/scripting/test_upgraded_workers.lua	2017-02-18 23:38:55 +0000
@@ -0,0 +1,29 @@
+run(function()
+   sleep(5000)
+
+   assert_equal(1, #p1:get_buildings("barbarians_headquarters"))
+
+   local hq = p1:get_buildings("barbarians_headquarters")[1]
+
+   hq:set_workers("barbarians_miner_chief", 5)
+   assert_equal(5, hq:get_workers("barbarians_miner_chief"))
+
+   -- This mine needs a miner, a chief miner and a master miner
+   p1:place_road(p1:place_flag(map:get_field(25,22)), "bl", "bl", "bl", "bl")
+   local mine = p1:place_building("barbarians_coalmine_deeper", wl.Game().map:get_field(24, 21), false, true)
+
+   sleep(1000)
+
+   -- 2 of the chief miners should have been requested by the mine now,
+   -- one of them acting as a basic miner
+   assert_equal(3, hq:get_workers("barbarians_miner_chief"))
+
+   hq:set_workers("barbarians_miner_master", 1)
+   sleep(1000)
+
+   -- The master miner has filled the remaining slot
+   assert_equal(0, hq:get_workers("barbarians_miner_master"))
+
+   print("# All Tests passed.")
+   wl.ui.MapView():close()
+end)