Merge lp:~widelands-dev/widelands/bug-1786613-10s-return-skipped into lp:widelands

[Toni Förster]

check for whether the program name is work or not.

I went a different route now. I did some logging and I figured out that the program is only labeled "work" when it calls other programs. Checking for the program name would solve this. there is one catch though. The main work program in the lua files should call a sub program to make this work. this is the case for e.g. Farms, Shipyard, Metalworkshop, etc. There only the unconditioned "return=skipped" needs to be removed.

Productionssites that only have work as a program need to be modified. E.g. the barbarians bakery currently looks like this:

   programs = {
      work = {
         -- TRANSLATORS: Completed/Skipped/Did not start baking bread because ...
         descname = pgettext("barbarians_building", "baking pitta bread"),
         actions = {
            "sleep=20000",
            "return=skipped unless economy needs barbarians_bread",
            "consume=water:3 wheat:3",
            "animate=working 20000",
            "produce=barbarians_bread",
            "animate=working 20000",
            "produce=barbarians_bread"
         }
      },
   },

Should then look like this:

   programs = {
      work = {
         -- TRANSLATORS: Completed/Skipped/Did not start baking bread because ...
         descname = _"working",
         actions = {
            "call=bake_bread",
         }
      },
      bake_bread = {
         descname = pgettext("barbarians_building", "baking pitta bread"),
         actions = {
            "sleep=20000",
            "return=skipped unless economy needs barbarians_bread",
            "consume=water:3 wheat:3",
            "animate=working 20000",
            "produce=barbarians_bread",
            "animate=working 20000",
            "produce=barbarians_bread"
         }
      },
   },

This of course needs to be done for all other buildings as well. It would also unify the way production programs are called. Work then would always call the "sub-programs" for all buildings. Not only the ones that produce only one ware or have the working routine split for other reasons.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3782. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/415390227.
Appveyor build 3581. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786613_10s_return_skipped-3581.

[hessenfarmer (stephan-lutz)]

Ok I tested this with different timings.
My machine is a Core 2 Duo T7500 @ 2,2 Ghz with 4 GB RAM.
My setup was
map: the nile
8 AI players 2 of each tribe
Let the game run for 4:50 hours
saved and loaded with every config.

results were:
original timing: around 30% cpu usage in task manager, peaks up to 45%
10 ms timing: around 33% with more and higher peaks up to 60%
100 ms timing: around 32% less peaks than 10s up to 50%
500 ms timing: around 30 to 31 % peaks up to 50% but not much frequent.

So I would propose to have a value of 500 to 1000ms as compromise to ensure minimal effect on performance.

review: Needs Fixing

[hessenfarmer (stephan-lutz)]

Looks good from my side now.
Would be good though if somebody with an underpowered machine would confirm minimal effect on performance

review: Approve

[Toni Förster (stonerl)]

> Looks good from my side now.
> Would be good though if somebody with an underpowered machine would confirm
> minimal effect on performance

Do you think it gets much more underpowered. Your processor is basically the same as mine. Just a few MHz slower. The only difference is RAM.

[ypopezios (ypopezios)]

In principle, if I was given a range to compromise with, I would go the conservative route and pick the edge of the range which is closer to the previous value. In this case, the given range is 500 to 1000ms, the previous value is 10000ms, so I would go with 1000ms.

Having said that, in my eyes an arbitrary value gets replaced by another arbitrary value. More tests of that kind won't make that value any less arbitrary. Therefore, I would comment this code for redesign in a future build.

[hessenfarmer (stephan-lutz)]

@ypopezios: In principle you are right. It is somewhat arbitrary or you might say it is try and error. On the other hand I don't know how to handle this in a different way, cause more frequent calls of the working programs will always consume more processing power. so the value of 500 to 1000ms was chosen due to the consumed processing power on my machine while knowing the kind of measurement had some big uncertainties. Therefore I asked to test his issue on an even more underpowered system. For my understanding the more underpowered a testsystem would be the more visible would be the effect of this change.
@Toni Förster:
I was thinking to have this tested on a netbook or similar. As we support 1024x600 resolution we should support the processing power of those configs as well.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3791. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/415624736.
Appveyor build 3590. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786613_500ms_return_skipped-3590.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3782. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/415390227.
Appveyor build 3581. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786613_10s_return_skipped-3581.

=== modified file 'src/logic/map_objects/tribes/productionsite.cc'
--- src/logic/map_objects/tribes/productionsite.cc	2018-06-19 08:52:49 +0000
+++ src/logic/map_objects/tribes/productionsite.cc	2018-08-20 16:49:32 +0000
@@ -920,7 +920,7 @@
 	stack_.pop_back();
 	if (!stack_.empty())
 		top_state().phase = result;
-
+	
 	switch (result) {
 	case Failed:
 		statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
@@ -929,12 +929,14 @@
 		crude_percent_ = crude_percent_ * 8 / 10;
 		break;
 	case Completed:
-		skipped_programs_.erase(program_name);
-		statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
-		statistics_.push_back(true);
-		train_workers(game);
-		crude_percent_ = crude_percent_ * 8 / 10 + 1000000 * 2 / 10;
-		calc_statistics();
+		if (program_name.compare("work") != 0) {
+			skipped_programs_.erase(program_name);
+			statistics_.erase(statistics_.begin(), statistics_.begin() + 1);
+			statistics_.push_back(true);
+			train_workers(game);
+			crude_percent_ = crude_percent_ * 8 / 10 + 1000000 * 2 / 10;
+			calc_statistics();
+		}
 		break;
 	case Skipped:
 		skipped_programs_[program_name] = game.get_gametime();