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.

[hessenfarmer (stephan-lutz)]

This solution doesn't work for the atlantean Crystal mine and the empire marblemine in their new design as defined in "mines-worldsavior" branch.
In principle there may be a misunderstanding about the "work" program. This is the master program of each productionsite. Meaning each of them must at least have a program called "work" from this and every other program other programs could be called. Only condition is that the called programs need to be sorted in the correct order and sorting criteria is the name of the program. Therefore it doesn't work for the named mines as they have a subprogram calling sub-subprograms.
Therefore I disapprove this solution as it would limit the current flexible program design of productionsites.
Furthermore I really doubt that the desired effect is worth the pain of changing all programs of all productionsite buildings.

review: Disapprove

[Toni Förster (stonerl)]

New revision. We check whether the program is "work" or end with "work". This would work with the marble mines as well. Now if a program shall not calculate stats, only append _work at the end.

Here is the aforementioned marble-mine:

      work = {
         -- TRANSLATORS: Completed/Skipped/Did not start working because ...
         descname = _"working",
         actions = {
            "call=mine_granite",
            "call=mine_marble",
            "return=skipped"
         }

This would become:

      work = {
         -- TRANSLATORS: Completed/Skipped/Did not start working because ...
         descname = _"working",
         actions = {
            "call=mine_marble_work",
            "call=mine_granite_work",
         }

Still the others need to be changed. But it is unified. Being "work" or ending with "_work" would result in no stats be calculated for this program.

>Furthermore I really doubt that the desired effect is worth the pain of changing all programs of all productionsite buildings.

It's worth it, IMHO. The use of unconditional "return=skipped" also "overloads" the skipped_programs stack. Usually when a program completes successfully it gets removed from the skipped_programs stack, but with the unconditional ones it never will.

Take the the marble-mine for example:

work, mine_granite and mine_marble have a return=skipped. So each marble mine adds 3 programs to the stack the will never be removed.

Every single crystal-mine will add 4 programs to the stack that will not removed. And the list goes on for all others that use this method of avoiding stats being calculated. So in a singleplayer with just one tribe. You would end up with some hundred programs cloaking the stack for no good reason.

Also it makes the use of conditional return=skipped useless. Here of the crystal-mine:

         descname = _"mining diamonds",
         actions = {
            "return=skipped unless economy needs diamond",
            "sleep=40000",
            "consume=smoked_fish,smoked_meat:2 atlanteans_bread:2",
            "call=a_mine_produce_diamond",
            "call=a_mine_produce_granite",
            "call=a_mine_produce_diamond",
            "return=skipped"
         }

The only thing it does is not calling the sub-programs. But the way it is intended to work is: Skip the program, because we don't need the ware. Next time we need the ware the program gets executed and returns a completed. This then removes it from the stack. But since it ends with "return=skipped" it always stays in the stack.

I know that it might be some tedious labor, but productions-sites that already have sub-programs just need the return=skipped removed and a _work appended to the sub_program if stats are not needed. The others need the sub-program added, but I would willingly do this.

Now that I spend some time looking into this, I'm 100% sure, unconditional skips are the wrong way. Since they do more harm (+10s; and the increasing stack) and skips were never intended for this usage.

[hessenfarmer (stephan-lutz)]

Ok I see your point about the stack. However your solution would need a lot of changes I the programs and additionally we would need to document the rule to implicitly avoid stats calculation by adding_work to the end of the program.
So I was thinking why not introduce a way to explicitly state that no stats update is required / foreseen for a work program. This could probably be done by defining a new return state. Alternatively I'm asking myself if we should not try to return=None or return= to avoid stats calculation without having a skipped state.

=== modified file 'src/logic/map_objects/tribes/production_program.cc'
--- src/logic/map_objects/tribes/production_program.cc	2018-07-12 16:53:21 +0000
+++ src/logic/map_objects/tribes/production_program.cc	2018-08-21 16:01:49 +0000
@@ -467,9 +467,11 @@
 			result_ = Completed;
 		else if (match(parameters, "skipped"))
 			result_ = Skipped;
+		else if (match(parameters, "no_stats"))
+			result_ = None;
 		else
 			throw GameDataError(
-			   "expected %s but found \"%s\"", "{\"failed\"|\"completed\"|\"skipped\"}", parameters);
+			   "expected %s but found \"%s\"", "{\"failed\"|\"completed\"|\"skipped\"|\"no_stats\"}", parameters);
 
 		if (skip(parameters)) {
 			if (match_force_skip(parameters, "when")) {

=== modified file 'src/logic/map_objects/tribes/production_program.h'
--- src/logic/map_objects/tribes/production_program.h	2018-07-12 16:53:21 +0000
+++ src/logic/map_objects/tribes/production_program.h	2018-08-21 16:01:49 +0000
@@ -107,10 +107,11 @@
 	///
 	/// Parameter syntax:
 	///    parameters         ::= return_value [condition_part]
-	///    return_value       ::= Failed | Completed | Skipped
+	///    return_value       ::= Failed | Completed | Skipped | None
 	///    Failed             ::= "failed"
 	///    Completed          ::= "completed"
 	///    Skipped            ::= "skipped"
+	///    None               ::= "no_stats"
 	///    condition_part     ::= when_condition | unless_conition
 	///    when_condition     ::= "when" condition {"and" condition}
 	///    unless_condition   ::= "unless" condition {"or" condition}
@@ -126,8 +127,9 @@
 	/// Parameter semantics:
 	///    return_value:
 	///       If return_value is Failed or Completed, the productionsite's
-	///       statistics is updated accordingly. If return_value is Skipped, the
-	///       statistics are not affected.
+	///       statistics is updated accordingly. If return_value is Skipped or
+	///       None, the statistics are not affected. But Skipped adds a 10s delay
+	///       before the program is executed again.
 	///    condition:
 	///       A boolean condition that can be evaluated to true or false.
 	///    condition_part:
@@ -235,11 +237,12 @@
 	/// Parameter syntax:
 	///    parameters         ::= program {handling_directive}
 	///    handling_directive ::= "on" Result handling_method
-	///    Result             ::= "failure" | "completion" | "skip"
+	///    Result             ::= "failure" | "completion" | "skip" | "no_stats"
 	///    handling_method    ::= Fail | Complete | Skip | Repeat
 	///    Fail               ::= "fail"
 	///    Ignore             ::= "ignore"
 	///    Repeat             ::= "repeat"
+	///    None               ::= "no_stats"
 	/// Parameter semantics:
 	///    program:
 	///       The name of a program defined in the productionsite.
@@ -260,6 +263,9 @@
 	///       * If handling_method is Skip, the command skips the calling
 	///         program (with the same effect as executing "return=skipped").
 	///       * If handling_method is "repeat", the command is repeated.
+	///       * If handling_method is None the called program continues normal,
+	///         but no statistics are calculated (with the same effect as
+	///         executing "return=no_stats")
 	struct ActCall : public Action {
 		ActCall(char* parameters, const ProductionSiteDescr&);
 		void execute(Game&, ProductionSite&) const override;

=== 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-21 16:01:49 +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);
@@ -941,6 +941,7 @@
 		crude_percent_ = crude_percent_ * 98 / 100;
 		break;
 	case None:
+		skipped_programs_.erase(program_name);
 		break;
 	}