widelands

Merge lp:~widelands-dev/widelands/failed_skipped_10s into lp:widelands

  1. failed_skipped_10s
  2. Merge into trunk
Proposed by Toni Förster
Status: Merged
Merged at revision: 9133
Proposed branch: lp:~widelands-dev/widelands/failed_skipped_10s
Merge into: lp:widelands
Diff against target: 393 lines (+44/-43)
7 files modified
data/tribes/buildings/productionsites/barbarians/helmsmithy/init.lua (+3/-0)
data/tribes/buildings/productionsites/barbarians/metal_workshop/init.lua (+10/-10)
data/tribes/buildings/productionsites/empire/toolsmithy/init.lua (+12/-12)
src/logic/map_objects/tribes/productionsite.cc (+7/-9)
src/logic/map_objects/tribes/productionsite.h (+4/-4)
src/map_io/map_buildingdata_packet.cc (+6/-6)
test/maps/plain.wmf/scripting/test_casern.lua (+2/-2)
To merge this branch: bzr merge lp:~widelands-dev/widelands/failed_skipped_10s
Reviewer Review Type Date Requested Status
hessenfarmer review Approve
GunChleoc Approve
Review via email: mp+368014@code.launchpad.net

Commit message

add failed programs to the skipped stack

don't show % for trend

move sleep behind consume in metal_workshop & toolsmithy

added sound to helmsmithy

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 5079. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/538470132.
Appveyor build 4859. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_failed_skipped_10s-4859.

Revision history for this message
GunChleoc (gunchleoc) wrote :

LGTM :)

I removed the gettext function from the pure placeholder, because it does not need translation.

@bunnybot merge

review: Approve
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

looks good to me. However there might be some complaints about having the sound as some people (we all know) do find this annoying. However I like it.

review: Approve (review)
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Oops found some other buildings which need to be adopted to this scheme then

atlanteans crystal mine
empire marble mine, marble mine deep
frisians tailors shop, aqua farm, recycling center and eventually reindeer farm

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

Found some more. Barbarian's mines. Going to add the changes to this branch, since it won't be merged because the casern_test failed. I guess we need to adapt this test as well...

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

Hmm, there are many more. The question is. Shall we move the sleep time just behind consume, or remove sleep and add the time to the animation time, were applicable?

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

It basically affects all buildings. Most buildings have a sleep before skipped and/or consume. So we need to move them as well. But the question is, do we want to move them or add them to the animation cycle? Or at least move some seconds to the animation cycle?

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Barbarians mines are not affected I think.

Basically the change of order is definitely necessary for all buildings with multiple outputs and different inputs needed for the outputs (best example atlantean or frisian toolsmithy).

It can be adopted (which I prefer) for buildings with multiple outputs and high sleep times (e.g. crystal mine).
It is not necessary for buildings with leading sleep times of less then 10 secs as these would been masked by the code now.
Furthermore it is not necessary for buildings with only one output as the effect of wasted time if undersupplied would been averaged over time.

That was the basis for my suggestion above.

I'd like to keep the sleep times around at least of one third to one half of the total time as it looks more natural if a building isn't busy all the time.
Having only working animations would make the screen a bit unquiet I am afraid.

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

Okay, so basically only the buildings you listed above?

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Unless you find others for which the criteria apply, but we could change that anytime in a new branch as well.

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

Did some thinking :D

> It is not necessary for buildings with leading sleep times of less then 10
> secs as these would been masked by the code now.

> Furthermore it is not necessary for buildings with only one output as the
> effect of wasted time if undersupplied would been averaged over time.

But now in all programs that have a sleep before consume and/or skipped it will always be 10s plus sleep. E.g. the barbarian's baker has a sleep of 30 seconds before consume. Every fail would now result in 10s + 30s before for the next execution.

Even if the time would be 1 second it would now always be 10s + 1s, the first sleep never gets masked but added.

That's why I would like to have skipped the first, consume the second and sleep the third call in every program. Generally, having a sleep before consume would be a time penalty, regardless of the length.

Having every program like this would also have the benefit, that the program flow itself would be unified. A program skipped or failed means, it starts 10 second later before it would fail or skip again. Otherwise, it would be 10s plus whatever sleep time we have before consume/skip. The sleep times then would be part of a completed cycle and not a failed or skipped.

Does this sound reasonable?

> That was the basis for my suggestion above.
>
> I'd like to keep the sleep times around at least of one third to one half of
> the total time as it looks more natural if a building isn't busy all the time.
> Having only working animations would make the screen a bit unquiet I am
> afraid.

Sounds sensible.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

On a second thought this sounds very reasonable.

especially the unifying aspect is a position I alkready took in the forum discussion so if you want to unify them all I would have no objection. But we should playtest the changes before merging in this case.

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

Okay, I'm going to set a new branch up that contains the changes to the lua files. Base will be this branch.

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

This branch fails:

test/maps/plain.wmf/scripting/test_casern.lua

How do I run the test locally?

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Running regression tests locally:

./regression_test.py -b ./widelands

Revision history for this message
GunChleoc (gunchleoc) wrote :

This will run the whole test suite - he will only want to run the casern test when fixing.

https://wl.widelands.org/wiki/RegressionTests/

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

The 10s increase in the test fixed it. But we only need this as long as the unify_sleep_time isn't merged into branch. We can revert these changes there because sleeps aren't executed before a possible fail anymore.

@bunnybot merge

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 5095. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/538979803.

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

the inputqueues again

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/tribes/buildings/productionsites/barbarians/helmsmithy/init.lua'
2--- data/tribes/buildings/productionsites/barbarians/helmsmithy/init.lua 2019-05-19 11:25:28 +0000
3+++ data/tribes/buildings/productionsites/barbarians/helmsmithy/init.lua 2019-05-29 21:25:51 +0000
4@@ -81,6 +81,7 @@
5 "return=skipped unless economy needs helmet",
6 "consume=coal iron",
7 "sleep=32000",
8+ "playsound=sound/smiths/smith 192",
9 "animate=working 35000",
10 "produce=helmet"
11 }
12@@ -93,6 +94,7 @@
13 "return=skipped unless economy needs helmet_mask",
14 "consume=coal iron:2",
15 "sleep=32000",
16+ "playsound=sound/smiths/smith 192",
17 "animate=working 45000",
18 "produce=helmet_mask"
19 }
20@@ -105,6 +107,7 @@
21 "return=skipped unless economy needs helmet_warhelm",
22 "consume=coal gold iron:2",
23 "sleep=32000",
24+ "playsound=sound/smiths/smith 192",
25 "animate=working 55000",
26 "produce=helmet_warhelm"
27 }
28
29=== modified file 'data/tribes/buildings/productionsites/barbarians/metal_workshop/init.lua'
30--- data/tribes/buildings/productionsites/barbarians/metal_workshop/init.lua 2019-05-19 11:25:28 +0000
31+++ data/tribes/buildings/productionsites/barbarians/metal_workshop/init.lua 2019-05-29 21:25:51 +0000
32@@ -94,8 +94,8 @@
33 descname = _"making a bread paddle",
34 actions = {
35 "return=skipped unless economy needs bread_paddle",
36+ "consume=iron log",
37 "sleep=32000",
38- "consume=iron log",
39 "playsound=sound/smiths/toolsmith 192",
40 "animate=working 35000",
41 "produce=bread_paddle"
42@@ -106,8 +106,8 @@
43 descname = _"making a felling ax",
44 actions = {
45 "return=skipped unless economy needs felling_ax",
46+ "consume=iron log",
47 "sleep=32000",
48- "consume=iron log",
49 "playsound=sound/smiths/toolsmith 192",
50 "animate=working 35000",
51 "produce=felling_ax"
52@@ -118,8 +118,8 @@
53 descname = _"making fire tongs",
54 actions = {
55 "return=skipped unless economy needs fire_tongs",
56+ "consume=iron log",
57 "sleep=32000",
58- "consume=iron log",
59 "playsound=sound/smiths/toolsmith 192",
60 "animate=working 35000",
61 "produce=fire_tongs"
62@@ -130,8 +130,8 @@
63 descname = _"making a fishing rod",
64 actions = {
65 "return=skipped unless economy needs fishing_rod",
66+ "consume=iron log",
67 "sleep=32000",
68- "consume=iron log",
69 "playsound=sound/smiths/toolsmith 192",
70 "animate=working 35000",
71 "produce=fishing_rod"
72@@ -142,8 +142,8 @@
73 descname = _"making a hammer",
74 actions = {
75 "return=skipped unless economy needs hammer",
76+ "consume=iron log",
77 "sleep=32000",
78- "consume=iron log",
79 "playsound=sound/smiths/toolsmith 192",
80 "animate=working 35000",
81 "produce=hammer"
82@@ -154,8 +154,8 @@
83 descname = _"making a hunting spear",
84 actions = {
85 "return=skipped unless economy needs hunting_spear",
86+ "consume=iron log",
87 "sleep=32000",
88- "consume=iron log",
89 "playsound=sound/smiths/toolsmith 192",
90 "animate=working 35000",
91 "produce=hunting_spear"
92@@ -166,8 +166,8 @@
93 descname = _"making kitchen tools",
94 actions = {
95 "return=skipped unless economy needs kitchen_tools",
96+ "consume=iron log",
97 "sleep=32000",
98- "consume=iron log",
99 "playsound=sound/smiths/toolsmith 192",
100 "animate=working 35000",
101 "produce=kitchen_tools"
102@@ -178,8 +178,8 @@
103 descname = _"making a pick",
104 actions = {
105 "return=skipped unless economy needs pick",
106+ "consume=iron log",
107 "sleep=32000",
108- "consume=iron log",
109 "playsound=sound/smiths/toolsmith 192",
110 "animate=working 35000",
111 "produce=pick"
112@@ -190,8 +190,8 @@
113 descname = _"making a scythe",
114 actions = {
115 "return=skipped unless economy needs scythe",
116+ "consume=iron log",
117 "sleep=32000",
118- "consume=iron log",
119 "playsound=sound/smiths/toolsmith 192",
120 "animate=working 35000",
121 "produce=scythe"
122@@ -202,8 +202,8 @@
123 descname = _"making a shovel",
124 actions = {
125 "return=skipped unless economy needs shovel",
126+ "consume=iron log",
127 "sleep=32000",
128- "consume=iron log",
129 "playsound=sound/smiths/toolsmith 192",
130 "animate=working 35000",
131 "produce=shovel"
132
133=== modified file 'data/tribes/buildings/productionsites/empire/toolsmithy/init.lua'
134--- data/tribes/buildings/productionsites/empire/toolsmithy/init.lua 2019-03-17 07:20:58 +0000
135+++ data/tribes/buildings/productionsites/empire/toolsmithy/init.lua 2019-05-29 21:25:51 +0000
136@@ -83,8 +83,8 @@
137 descname = _"making a felling ax",
138 actions = {
139 "return=skipped unless economy needs felling_ax",
140+ "consume=iron log",
141 "sleep=32000",
142- "consume=iron log",
143 "playsound=sound/smiths/toolsmith 192",
144 "animate=working 35000",
145 "produce=felling_ax"
146@@ -95,8 +95,8 @@
147 descname = _"making a basket",
148 actions = {
149 "return=skipped unless economy needs basket",
150+ "consume=iron log",
151 "sleep=32000",
152- "consume=iron log",
153 "playsound=sound/smiths/toolsmith 192",
154 "animate=working 35000",
155 "produce=basket"
156@@ -107,8 +107,8 @@
157 descname = _"making a bread paddle",
158 actions = {
159 "return=skipped unless economy needs bread_paddle",
160+ "consume=iron log",
161 "sleep=32000",
162- "consume=iron log",
163 "playsound=sound/smiths/toolsmith 192",
164 "animate=working 35000",
165 "produce=bread_paddle"
166@@ -119,8 +119,8 @@
167 descname = _"making fire tongs",
168 actions = {
169 "return=skipped unless economy needs fire_tongs",
170+ "consume=iron log",
171 "sleep=32000",
172- "consume=iron log",
173 "playsound=sound/smiths/toolsmith 192",
174 "animate=working 35000",
175 "produce=fire_tongs"
176@@ -131,8 +131,8 @@
177 descname = _"making a fishing rod",
178 actions = {
179 "return=skipped unless economy needs fishing_rod",
180+ "consume=iron log",
181 "sleep=32000",
182- "consume=iron log",
183 "playsound=sound/smiths/toolsmith 192",
184 "animate=working 35000",
185 "produce=fishing_rod"
186@@ -143,8 +143,8 @@
187 descname = _"making a hammer",
188 actions = {
189 "return=skipped unless economy needs hammer",
190+ "consume=iron log",
191 "sleep=32000",
192- "consume=iron log",
193 "playsound=sound/smiths/toolsmith 192",
194 "animate=working 35000",
195 "produce=hammer"
196@@ -155,8 +155,8 @@
197 descname = _"making a hunting spear",
198 actions = {
199 "return=skipped unless economy needs hunting_spear",
200+ "consume=iron log",
201 "sleep=32000",
202- "consume=iron log",
203 "playsound=sound/smiths/toolsmith 192",
204 "animate=working 35000",
205 "produce=hunting_spear"
206@@ -167,8 +167,8 @@
207 descname = _"making kitchen tools",
208 actions = {
209 "return=skipped unless economy needs kitchen_tools",
210+ "consume=iron log",
211 "sleep=32000",
212- "consume=iron log",
213 "playsound=sound/smiths/toolsmith 192",
214 "animate=working 35000",
215 "produce=kitchen_tools"
216@@ -179,8 +179,8 @@
217 descname = _"making a pick",
218 actions = {
219 "return=skipped unless economy needs pick",
220+ "consume=iron log",
221 "sleep=32000",
222- "consume=iron log",
223 "playsound=sound/smiths/toolsmith 192",
224 "animate=working 35000",
225 "produce=pick"
226@@ -191,8 +191,8 @@
227 descname = _"making a saw",
228 actions = {
229 "return=skipped unless economy needs saw",
230+ "consume=iron log",
231 "sleep=32000",
232- "consume=iron log",
233 "playsound=sound/smiths/toolsmith 192",
234 "animate=working 35000",
235 "produce=saw"
236@@ -203,8 +203,8 @@
237 descname = _"making a scythe",
238 actions = {
239 "return=skipped unless economy needs scythe",
240+ "consume=iron log",
241 "sleep=32000",
242- "consume=iron log",
243 "playsound=sound/smiths/toolsmith 192",
244 "animate=working 35000",
245 "produce=scythe"
246@@ -215,8 +215,8 @@
247 descname = _"making a shovel",
248 actions = {
249 "return=skipped unless economy needs shovel",
250+ "consume=iron log",
251 "sleep=32000",
252- "consume=iron log",
253 "playsound=sound/smiths/toolsmith 192",
254 "animate=working 35000",
255 "produce=shovel"
256
257=== modified file 'src/logic/map_objects/tribes/productionsite.cc'
258--- src/logic/map_objects/tribes/productionsite.cc 2019-05-26 17:21:15 +0000
259+++ src/logic/map_objects/tribes/productionsite.cc 2019-05-29 21:25:51 +0000
260@@ -427,12 +427,9 @@
261 trend = "=";
262 }
263
264- const std::string trend_str =
265- g_gr->styles().color_tag((boost::format(_("%i%%")) % trend).str(), color);
266-
267 // TODO(GunChleoc): We might need to reverse the order here for RTL languages
268 statistics_string_on_changed_statistics_ =
269- (boost::format("%s\u2009%s") % perc_str % trend_str).str();
270+ (boost::format("%s\u2009%s") % perc_str % g_gr->styles().color_tag(trend, color)).str();
271 } else {
272 statistics_string_on_changed_statistics_ = perc_str;
273 }
274@@ -924,8 +921,8 @@
275
276 program_timer_ = true;
277 uint32_t tdelta = 10;
278- SkippedPrograms::const_iterator i = skipped_programs_.find(program_name);
279- if (i != skipped_programs_.end()) {
280+ FailedSkippedPrograms::const_iterator i = failed_skipped_programs_.find(program_name);
281+ if (i != failed_skipped_programs_.end()) {
282 uint32_t const gametime = game.get_gametime();
283 uint32_t const earliest_allowed_start_time = i->second + 10000;
284 if (gametime + tdelta < earliest_allowed_start_time)
285@@ -956,13 +953,14 @@
286
287 switch (result) {
288 case ProgramResult::kFailed:
289+ failed_skipped_programs_[program_name] = game.get_gametime();
290 statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
291 statistics_.push_back(false);
292 calc_statistics();
293 update_crude_statistics(current_duration, false);
294 break;
295 case ProgramResult::kCompleted:
296- skipped_programs_.erase(program_name);
297+ failed_skipped_programs_.erase(program_name);
298 statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
299 statistics_.push_back(true);
300 train_workers(game);
301@@ -970,11 +968,11 @@
302 calc_statistics();
303 break;
304 case ProgramResult::kSkipped:
305- skipped_programs_[program_name] = game.get_gametime();
306+ failed_skipped_programs_[program_name] = game.get_gametime();
307 update_crude_statistics(current_duration, false);
308 break;
309 case ProgramResult::kNone:
310- skipped_programs_.erase(program_name);
311+ failed_skipped_programs_.erase(program_name);
312 break;
313 }
314
315
316=== modified file 'src/logic/map_objects/tribes/productionsite.h'
317--- src/logic/map_objects/tribes/productionsite.h 2019-05-22 15:46:08 +0000
318+++ src/logic/map_objects/tribes/productionsite.h 2019-05-29 21:25:51 +0000
319@@ -307,13 +307,13 @@
320
321 int32_t fetchfromflag_; ///< Number of wares to fetch from flag
322
323- /// If a program has ended with the result Skipped, that program may not
324+ /// If a program has ended with the result Failed or Skipped, that program may not
325 /// start again until a certain time has passed. This is a map from program
326- /// name to game time. When a program ends with the result Skipped, its name
327+ /// name to game time. When a program ends with the result Failed or Skipped, its name
328 /// is added to this map, with the current game time. (When the program ends
329 /// with any other result, its name is removed from the map.)
330- using SkippedPrograms = std::map<std::string, Time>;
331- SkippedPrograms skipped_programs_;
332+ using FailedSkippedPrograms = std::map<std::string, Time>;
333+ FailedSkippedPrograms failed_skipped_programs_;
334
335 using Stack = std::vector<State>;
336 Stack stack_; ///< program stack
337
338=== modified file 'src/map_io/map_buildingdata_packet.cc'
339--- src/map_io/map_buildingdata_packet.cc 2019-05-18 11:58:43 +0000
340+++ src/map_io/map_buildingdata_packet.cc 2019-05-29 21:25:51 +0000
341@@ -646,13 +646,13 @@
342 if (pr_descr.programs().count(program_name)) {
343 uint32_t const skip_time = fr.unsigned_32();
344 if (gametime < skip_time)
345- throw GameDataError("program %s was skipped at time %u, but time is only "
346+ throw GameDataError("program %s failed/was skipped at time %u, but time is only "
347 "%u",
348 program_name, skip_time, gametime);
349- productionsite.skipped_programs_[program_name] = skip_time;
350+ productionsite.failed_skipped_programs_[program_name] = skip_time;
351 } else {
352 fr.unsigned_32(); // eat skip time
353- log("WARNING: productionsite has skipped program \"%s\", which "
354+ log("WARNING: productionsite has failed/skipped program \"%s\", which "
355 "does not exist\n",
356 program_name);
357 }
358@@ -1129,10 +1129,10 @@
359 fw.signed_32(productionsite.fetchfromflag_);
360
361 // skipped programs
362- assert(productionsite.skipped_programs_.size() <= std::numeric_limits<uint8_t>::max());
363- fw.unsigned_8(productionsite.skipped_programs_.size());
364+ assert(productionsite.failed_skipped_programs_.size() <= std::numeric_limits<uint8_t>::max());
365+ fw.unsigned_8(productionsite.failed_skipped_programs_.size());
366
367- for (const auto& temp_program : productionsite.skipped_programs_) {
368+ for (const auto& temp_program : productionsite.failed_skipped_programs_) {
369 fw.string(temp_program.first);
370 fw.unsigned_32(temp_program.second);
371 }
372
373=== modified file 'test/maps/plain.wmf/scripting/test_casern.lua'
374--- test/maps/plain.wmf/scripting/test_casern.lua 2016-12-03 14:11:35 +0000
375+++ test/maps/plain.wmf/scripting/test_casern.lua 2019-05-29 21:25:51 +0000
376@@ -34,7 +34,7 @@
377 assert_equal(nil, rv.meat)
378
379 -- Sleep long enough to train a soldier. But ax are missing so nothing should happen
380- sleep(35000)
381+ sleep(45000)
382 rv = br:get_inputs("all")
383 assert_equal(0, rv.ax)
384 assert_equal(3, rv.barbarians_recruit)
385@@ -44,7 +44,7 @@
386 rv = br:get_inputs("all")
387 assert_equal(3, rv.ax)
388 assert_equal(0, rv.barbarians_recruit)
389- sleep(35000)
390+ sleep(45000)
391 rv = br:get_inputs("all")
392 assert_equal(3, rv.ax)
393 assert_equal(0, rv.barbarians_recruit)

Subscribers

People subscribed via source and target branches

to status/vote changes: