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

Proposed by GunChleoc on 2017-02-18
Status: Merged
Merged at revision: 8296
Proposed branch: lp:~widelands-dev/widelands/bug-1664145-miners
Merge into: lp:widelands
Diff against target: 87 lines (+37/-8)
3 files modified
src/logic/map_objects/tribes/warehouse.cc (+7/-7)
src/logic/map_objects/tribes/warehouse.h (+1/-1)
test/maps/plain.wmf/scripting/test_upgraded_workers.lua (+29/-0)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1664145-miners
Reviewer Review Type Date Requested Status
TiborB 2017-02-18 Approve on 2017-02-19
Review via email: mp+317710@code.launchpad.net

Commit message

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

To post a comment you must log in.
8294. By GunChleoc on 2017-02-18

Fixed bug in the test.

bunnybot (widelandsofficial) wrote :

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.

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) wrote :

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

GunChleoc (gunchleoc) wrote :

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) wrote :

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) wrote :

Thanks for the review :)

You make a good point about the glitch.

@bunnybot merge

>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) wrote :

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) wrote :

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...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/map_objects/tribes/warehouse.cc'
2--- src/logic/map_objects/tribes/warehouse.cc 2017-02-12 09:10:57 +0000
3+++ src/logic/map_objects/tribes/warehouse.cc 2017-02-18 23:38:55 +0000
4@@ -742,17 +742,17 @@
5 * requirements.
6 */
7 Quantity Warehouse::count_workers(const Game& /* game */,
8- DescriptionIndex ware,
9+ DescriptionIndex worker_id,
10 const Requirements& req,
11 Match exact) {
12 Quantity sum = 0;
13
14 do {
15- sum += supply_->stock_workers(ware);
16+ sum += supply_->stock_workers(worker_id);
17
18 // NOTE: This code lies about the TrainingAttributes of non-instantiated workers.
19- if (incorporated_workers_.count(ware)) {
20- for (Worker* worker : incorporated_workers_[ware]) {
21+ if (incorporated_workers_.count(worker_id)) {
22+ for (Worker* worker : incorporated_workers_[worker_id]) {
23 if (!req.check(*worker)) {
24 // This is one of the workers in our sum.
25 // But he is too stupid for this job
26@@ -761,11 +761,11 @@
27 }
28 }
29 if (exact == Match::kCompatible) {
30- ware = owner().tribe().get_worker_descr(ware)->becomes();
31+ worker_id = owner().tribe().get_worker_descr(worker_id)->becomes();
32 } else {
33- ware = INVALID_INDEX;
34+ worker_id = INVALID_INDEX;
35 }
36- } while (owner().tribe().has_ware(ware));
37+ } while (owner().tribe().has_worker(worker_id));
38
39 return sum;
40 }
41
42=== modified file 'src/logic/map_objects/tribes/warehouse.h'
43--- src/logic/map_objects/tribes/warehouse.h 2017-01-25 18:55:59 +0000
44+++ src/logic/map_objects/tribes/warehouse.h 2017-02-18 23:38:55 +0000
45@@ -198,7 +198,7 @@
46
47 bool fetch_from_flag(Game&) override;
48
49- Quantity count_workers(const Game&, DescriptionIndex, const Requirements&, Match);
50+ Quantity count_workers(const Game&, DescriptionIndex worker, const Requirements&, Match);
51 Worker& launch_worker(Game&, DescriptionIndex worker, const Requirements&);
52
53 // Adds the worker to the inventory. Takes ownership and might delete
54
55=== added file 'test/maps/plain.wmf/scripting/test_upgraded_workers.lua'
56--- test/maps/plain.wmf/scripting/test_upgraded_workers.lua 1970-01-01 00:00:00 +0000
57+++ test/maps/plain.wmf/scripting/test_upgraded_workers.lua 2017-02-18 23:38:55 +0000
58@@ -0,0 +1,29 @@
59+run(function()
60+ sleep(5000)
61+
62+ assert_equal(1, #p1:get_buildings("barbarians_headquarters"))
63+
64+ local hq = p1:get_buildings("barbarians_headquarters")[1]
65+
66+ hq:set_workers("barbarians_miner_chief", 5)
67+ assert_equal(5, hq:get_workers("barbarians_miner_chief"))
68+
69+ -- This mine needs a miner, a chief miner and a master miner
70+ p1:place_road(p1:place_flag(map:get_field(25,22)), "bl", "bl", "bl", "bl")
71+ local mine = p1:place_building("barbarians_coalmine_deeper", wl.Game().map:get_field(24, 21), false, true)
72+
73+ sleep(1000)
74+
75+ -- 2 of the chief miners should have been requested by the mine now,
76+ -- one of them acting as a basic miner
77+ assert_equal(3, hq:get_workers("barbarians_miner_chief"))
78+
79+ hq:set_workers("barbarians_miner_master", 1)
80+ sleep(1000)
81+
82+ -- The master miner has filled the remaining slot
83+ assert_equal(0, hq:get_workers("barbarians_miner_master"))
84+
85+ print("# All Tests passed.")
86+ wl.ui.MapView():close()
87+end)