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

Proposed by Benedikt Straub
Status: Merged
Merged at revision: 9107
Proposed branch: lp:~widelands-dev/widelands/productionsite-bugs
Merge into: lp:widelands
Diff against target: 268 lines (+64/-22)
6 files modified
src/logic/map_objects/tribes/building.h (+2/-2)
src/logic/map_objects/tribes/production_program.cc (+5/-5)
src/logic/map_objects/tribes/productionsite.cc (+36/-13)
src/logic/map_objects/tribes/productionsite.h (+2/-0)
src/logic/map_objects/tribes/worker.cc (+8/-1)
src/map_io/map_buildingdata_packet.cc (+11/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/productionsite-bugs
Reviewer Review Type Date Requested Status
GunChleoc Approve
Toni Förster Approve
Review via email: mp+367306@code.launchpad.net

Commit message

– 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

To post a comment you must log in.
Revision history for this message
Toni Förster (stonerl) wrote :

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
Revision history for this message
Toni Förster (stonerl) wrote :

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

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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.

Revision history for this message
Toni Förster (stonerl) wrote :

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.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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.

Revision history for this message
Toni Förster (stonerl) wrote :

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
Revision history for this message
Benedikt Straub (nordfriese) wrote :

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

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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
9099. By Nordfriese

Mixed up two indices

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Bug fixed

9100. By GunChleoc

Added todo comment for savegae compatibility.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Confirmed. Dome more testing and is working fine :)

@bunnybot merge

review: Approve
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

Transient travis failure

@bunnybot merge force

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/building.h'
2--- src/logic/map_objects/tribes/building.h 2019-04-26 05:52:49 +0000
3+++ src/logic/map_objects/tribes/building.h 2019-05-12 10:53:28 +0000
4@@ -326,14 +326,14 @@
5 uint32_t throttle_time = 0,
6 uint32_t throttle_radius = 0);
7
8+ void start_animation(EditorGameBase&, uint32_t anim);
9+
10 protected:
11 // Updates 'statistics_string' with the string that should be displayed for
12 // this building right now. Overwritten by child classes.
13 virtual void update_statistics_string(std::string*) {
14 }
15
16- void start_animation(EditorGameBase&, uint32_t anim);
17-
18 bool init(EditorGameBase&) override;
19 void cleanup(EditorGameBase&) override;
20 void act(Game&, uint32_t data) override;
21
22=== modified file 'src/logic/map_objects/tribes/production_program.cc'
23--- src/logic/map_objects/tribes/production_program.cc 2019-05-05 14:05:07 +0000
24+++ src/logic/map_objects/tribes/production_program.cc 2019-05-12 10:53:28 +0000
25@@ -699,7 +699,7 @@
26
27 void ProductionProgram::ActCallWorker::execute(Game& game, ProductionSite& ps) const {
28 // Always main worker is doing stuff
29- ps.working_positions_[0].worker->update_task_buildingwork(game);
30+ ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
31 }
32
33 bool ProductionProgram::ActCallWorker::get_building_work(Game& game,
34@@ -985,7 +985,7 @@
35 void ProductionProgram::ActProduce::execute(Game& game, ProductionSite& ps) const {
36 assert(ps.produced_wares_.empty());
37 ps.produced_wares_ = produced_wares_;
38- ps.working_positions_[0].worker->update_task_buildingwork(game);
39+ ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
40
41 const TribeDescr& tribe = ps.owner().tribe();
42 assert(produced_wares_.size());
43@@ -1071,7 +1071,7 @@
44 void ProductionProgram::ActRecruit::execute(Game& game, ProductionSite& ps) const {
45 assert(ps.recruited_workers_.empty());
46 ps.recruited_workers_ = recruited_workers_;
47- ps.working_positions_[0].worker->update_task_buildingwork(game);
48+ ps.working_positions_[ps.main_worker_].worker->update_task_buildingwork(game);
49
50 const TribeDescr& tribe = ps.owner().tribe();
51 assert(recruited_workers_.size());
52@@ -1523,7 +1523,7 @@
53 if (map.find_reachable_immovables(area, &immovables, cstep, FindImmovableByDescr(descr))) {
54 state.objvar = immovables[0].object;
55
56- psite.working_positions_[0].worker->update_task_buildingwork(game);
57+ psite.working_positions_[psite.main_worker_].worker->update_task_buildingwork(game);
58 return;
59 }
60
61@@ -1559,7 +1559,7 @@
62
63 state.coord = best_coords;
64
65- psite.working_positions_[0].worker->update_task_buildingwork(game);
66+ psite.working_positions_[psite.main_worker_].worker->update_task_buildingwork(game);
67 return;
68 }
69
70
71=== modified file 'src/logic/map_objects/tribes/productionsite.cc'
72--- src/logic/map_objects/tribes/productionsite.cc 2019-05-05 14:05:07 +0000
73+++ src/logic/map_objects/tribes/productionsite.cc 2019-05-12 10:53:28 +0000
74@@ -270,7 +270,8 @@
75 last_stat_percent_(0),
76 crude_percent_(0),
77 is_stopped_(false),
78- default_anim_("idle") {
79+ default_anim_("idle"),
80+ main_worker_(-1) {
81 calc_statistics();
82 }
83
84@@ -565,16 +566,20 @@
85 void ProductionSite::remove_worker(Worker& w) {
86 molog("%s leaving\n", w.descr().name().c_str());
87 WorkingPosition* wp = working_positions_;
88+ int32_t wp_index = 0;
89
90 for (const auto& temp_wp : descr().working_positions()) {
91 DescriptionIndex const worker_index = temp_wp.first;
92- for (uint32_t j = temp_wp.second; j; --j, ++wp) {
93+ for (uint32_t j = temp_wp.second; j; --j, ++wp, ++wp_index) {
94 Worker* const worker = wp->worker;
95 if (worker && worker == &w) {
96 // do not request the type of worker that is currently assigned - maybe a trained worker
97 // was
98 // evicted to make place for a level 0 worker.
99 // Therefore we again request the worker from the WorkingPosition of descr()
100+ if (main_worker_ == wp_index) {
101+ main_worker_ = -1;
102+ }
103 *wp = WorkingPosition(&request_worker(worker_index), nullptr);
104 Building::remove_worker(w);
105 return;
106@@ -685,11 +690,14 @@
107 program_timer_ = false;
108
109 if (!can_start_working()) {
110- while (!stack_.empty())
111+ start_animation(game, descr().get_unoccupied_animation());
112+ while (!stack_.empty()) {
113 program_end(game, ProgramResult::kFailed);
114+ }
115 } else {
116+ assert(main_worker_ >= 0);
117 if (stack_.empty()) {
118- working_positions_[0].worker->update_task_buildingwork(game);
119+ working_positions_[main_worker_].worker->update_task_buildingwork(game);
120 return;
121 }
122
123@@ -737,8 +745,10 @@
124 bool ProductionSite::fetch_from_flag(Game& game) {
125 ++fetchfromflag_;
126
127- if (can_start_working())
128- working_positions_[0].worker->update_task_buildingwork(game);
129+ if (main_worker_ >= 0) {
130+ assert(working_positions_[main_worker_].worker);
131+ working_positions_[main_worker_].worker->update_task_buildingwork(game);
132+ }
133
134 return true;
135 }
136@@ -767,10 +777,19 @@
137 }
138
139 void ProductionSite::try_start_working(Game& game) {
140- if (can_start_working() && descr().working_positions().size()) {
141- Worker& main_worker = *working_positions_[0].worker;
142- main_worker.reset_tasks(game);
143- main_worker.start_task_buildingwork(game);
144+ if (main_worker_ >= 0) {
145+ return;
146+ }
147+ const size_t nr_workers = descr().working_positions().size();
148+ for (uint32_t i = 0; i < nr_workers; ++i) {
149+ if (Worker* worker = working_positions_[i].worker) {
150+ // We may start even if can_start_working() returns false, because basic actions
151+ // like unloading extra wares should take place anyway
152+ main_worker_ = i;
153+ worker->reset_tasks(game);
154+ worker->start_task_buildingwork(game);
155+ return;
156+ }
157 }
158 }
159
160@@ -781,7 +800,8 @@
161 */
162 bool ProductionSite::get_building_work(Game& game, Worker& worker, bool const success) {
163 assert(descr().working_positions().size());
164- assert(&worker == working_positions_[0].worker);
165+ assert(main_worker_ >= 0);
166+ assert(&worker == working_positions_[main_worker_].worker);
167
168 // If unsuccessful: Check if we need to abort current program
169 if (!success) {
170@@ -860,8 +880,11 @@
171 }
172
173 // Check if all workers are there
174- if (!can_start_working())
175- return false;
176+ if (!can_start_working()) {
177+ // Try again a bit later
178+ worker.start_task_idle(game, 0, 3000);
179+ return true;
180+ }
181
182 // Start program if we haven't already done so
183 State* state = get_state();
184
185=== modified file 'src/logic/map_objects/tribes/productionsite.h'
186--- src/logic/map_objects/tribes/productionsite.h 2019-05-05 14:05:07 +0000
187+++ src/logic/map_objects/tribes/productionsite.h 2019-05-12 10:53:28 +0000
188@@ -333,6 +333,8 @@
189 std::string statistics_string_on_changed_statistics_;
190 std::string production_result_; // hover tooltip text
191
192+ int32_t main_worker_;
193+
194 DISALLOW_COPY_AND_ASSIGN(ProductionSite);
195 };
196
197
198=== modified file 'src/logic/map_objects/tribes/worker.cc'
199--- src/logic/map_objects/tribes/worker.cc 2019-05-04 10:47:44 +0000
200+++ src/logic/map_objects/tribes/worker.cc 2019-05-12 10:53:28 +0000
201@@ -1639,7 +1639,15 @@
202 // Reset any signals that are not related to location
203 std::string signal = get_signal();
204 signal_handled();
205+
206+ upcast(Building, building, get_location(game));
207+
208 if (signal == "evict") {
209+ if (building) {
210+ // If the building was working, we do not tell it to cancel – it'll notice by itself soon –
211+ // but we already change the animation so it won't look strange
212+ building->start_animation(game, building->descr().get_unoccupied_animation());
213+ }
214 return pop_task(game);
215 }
216
217@@ -1647,7 +1655,6 @@
218 state.ivar1 = (signal == "fail") * 2;
219
220 // Return to building, if necessary
221- upcast(Building, building, get_location(game));
222 if (!building)
223 return pop_task(game);
224
225
226=== modified file 'src/map_io/map_buildingdata_packet.cc'
227--- src/map_io/map_buildingdata_packet.cc 2019-04-09 17:16:11 +0000
228+++ src/map_io/map_buildingdata_packet.cc 2019-05-12 10:53:28 +0000
229@@ -65,7 +65,7 @@
230 // Responsible for warehouses and expedition bootstraps
231 constexpr uint16_t kCurrentPacketVersionWarehouse = 7;
232 constexpr uint16_t kCurrentPacketVersionMilitarysite = 6;
233-constexpr uint16_t kCurrentPacketVersionProductionsite = 6;
234+constexpr uint16_t kCurrentPacketVersionProductionsite = 7;
235 constexpr uint16_t kCurrentPacketVersionTrainingsite = 5;
236
237 void MapBuildingdataPacket::read(FileSystem& fs,
238@@ -552,6 +552,7 @@
239 MapObjectLoader& mol) {
240 try {
241 uint16_t const packet_version = fr.unsigned_16();
242+ // TODO(GunChleoc): Savegame compatibility, remove after Build 21.
243 if (packet_version >= 5 && packet_version <= kCurrentPacketVersionProductionsite) {
244 ProductionSite::WorkingPosition& wp_begin = *productionsite.working_positions_;
245 const ProductionSiteDescr& pr_descr = productionsite.descr();
246@@ -710,6 +711,13 @@
247 productionsite.statistics_[i] = fr.unsigned_8();
248 productionsite.statistics_string_on_changed_statistics_ = fr.c_string();
249 productionsite.production_result_ = fr.c_string();
250+
251+ // TODO(GunChleoc): Savegame compatibility, remove after Build 21.
252+ if (kCurrentPacketVersionProductionsite >= 7) {
253+ productionsite.main_worker_ = fr.signed_32();
254+ } else {
255+ productionsite.main_worker_ = productionsite.working_positions_[0].worker ? 0 : -1;
256+ }
257 } else {
258 throw UnhandledVersionError("MapBuildingdataPacket - Productionsite", packet_version,
259 kCurrentPacketVersionProductionsite);
260@@ -1173,6 +1181,8 @@
261 fw.unsigned_8(productionsite.statistics_[i]);
262 fw.string(productionsite.statistics_string_on_changed_statistics_);
263 fw.string(productionsite.production_result());
264+
265+ fw.signed_32(productionsite.main_worker_);
266 }
267
268 /*

Subscribers

People subscribed via source and target branches

to status/vote changes: