production times are 10s longer when return=skipped

[Toni Förster]

The logic for production sites contains a function that starts the programs. See here:

https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/logic/map_objects/tribes/productionsite.cc#L888

Interesting is this part:

 SkippedPrograms::const_iterator i = skipped_programs_.find(program_name);
 if (i != skipped_programs_.end()) {
  uint32_t const gametime = game.get_gametime();
  uint32_t const earliest_allowed_start_time = i->second + 10000;
  if (gametime + tdelta < earliest_allowed_start_time)
   tdelta = earliest_allowed_start_time - gametime;
 }

This is used for cases like this:

        "return=skipped unless economy needs barbarians_bread"

This means 10 seconds are added before the action gets executed again.

The problem is that unconditional "return=skipped" are used to avoid wrong productivity statistics. This is because the skipped case does not calculate stats as seen in the program end function.

https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/logic/map_objects/tribes/productionsite.cc#L915

But 10 seconds are added to the production time every time an unconditional "return=skipped is called.

Possible solutions would be to add a 4th case that doesn't trigger the above mentioned "SkippedPrograms:" function.

Another solution would be, to remove the above mentioned "SkippedPrograms:" function and add a

sleep=10000

before every conditional return=skipped where a sleep missing and remove these 10s from other sleep times in the production. Doing this would keep the production times length the same but also have the 10s that are added before production continues if a ware wasn't needed.

Some production sites already have a sleep before a conditional return=skipped. Have these extra 10s been taken into consideration when adding these? Here for example the Barbarians bakery:

   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"
         }
      },

If no bread is needed the sleep before the return is called is 20s + 10s because these 10 seconds have been added since bread wasn't needed.

This was discovered in the forum. Starting from this post: https://wl.widelands.org/forum/post/25595/

[GunChleoc (gunchleoc)]

I am against a manual invocation of sleep - it makes things to complicated for tribe development. It would be better to amend the C++ code to add no time when there was no condition in the program.

[Toni Förster (stonerl)]

Yes, you're completely right, this should be handled in C++.

[hessenfarmer (stephan-lutz)]

from my perspective it is also a problem for conditionally skipped programs. Especially when you think of the toolsmithies if you really need one type of weapon and others are not needed you need to wait 10s multiplied with the not needed tools in the economy. That adds up very quickly as each tribe has at least 10 tools to produce. I was always asking me why the hell the production of tools is so slow if you just need exactly one of them.
The only problem in completly skipping this burden could be performance but this should be tested. Perhaps a short step could be to have this penalty reduced to 1 or 2 sec if performance is an issue.

By the way if we completely can skip this I am against adding them to the lua files. as the effect would be beneficial for all tribes.

[Toni Förster (stonerl)]

Now that you mentioned it. I wonder why these 10s are there in the first place, besides maybe performance. It doesn't make much sense to punish an economy just because wares are not needed. Not only the toolsmithies are punished, also the armorsmithy, weaponsmithy, ax_workshop, helmsmithy, marble/crystal mines and inns. Basically all production sites that produce more then one ware.

[Toni Förster (stonerl)]

I did a test build were I removed this entire part:

 SkippedPrograms::const_iterator i = skipped_programs_.find(program_name);
 if (i != skipped_programs_.end()) {
  uint32_t const gametime = game.get_gametime();
  uint32_t const earliest_allowed_start_time = i->second + 10000;
  if (gametime + tdelta < earliest_allowed_start_time)
   tdelta = earliest_allowed_start_time - gametime;
 }

Performance-wise I couldn't see any difference. But wares got produced much faster. As an example: when only shovels are needed, it would take 60 seconds until the smith started producing the shovel because the 10 other tools that usually get produced weren't needed. So it took 130.6 for a new shovel being produced. Without "SkippedPrograms" it now takes only 70.6.

The only "downside" is that the "skipped making..." hover-message isn't shown for 10 seconds. Because it gets replaced by the next "skipped making..."-message. But even with the current 10s waiting time not all "skipped making..."-messages were shown. In case of the metal_workshop, the bread_paddle, hunting_spear & pick's "skipped making..." messages were on screen while the others were omitted. So to be honest I don't think this is a downside at all.

I'd say it would be for the better if this got removed.

[GunChleoc (gunchleoc)]

This is the documentation for SkippedPrograms:

 /// If a program has ended with the result Skipped, that program may not
 /// start again until a certain time has passed. This is a map from program
 /// name to game time. When a program ends with the result Skipped, its name
 /// is added to this map, with the current game time. (When the program ends
 /// with any other result, its name is removed from the map.)

So, it controls how often the *same* program gets skipped, so I'm not sure of the implications here. Maybe go with:

    uint32_t const earliest_allowed_start_time = i->second + 100;

Or something similar?

[hessenfarmer (stephan-lutz)]

I did not test this yet. However after GunChleocs explanation and carefully reading the code I think that the conditional skipped programs should not be a problem, cause as long as one of the programs (f.e. shovel) is working the time of this program lasts longer as the 10 seconds delay for the skipped programs (f.e. pick). Furthermore the 10 secs for the unconditional skip will be enough to eat up any skip delay in the called programs. So for my current understanding the whole thing is only about the 10 secs delay caused by the unconditional skip at the end of the work program. All other instances of return=skipped should not add an additional delay to the cycle. Therefore the reduction of the value to 100 ms to one second should not result in a major performance increase.
I will compile Toni Försters solution and do some tests though.
@GunChleoc: Do we / you have a dedicated performance scenario to test this?

[Toni Förster (stonerl)]

I just tested it once again just to be sure. I build only woodcutter huts, so the metal workshop had only felling axes to produce, without the patch. I took 80,6 seconds for a new ax to be produced. With the patch 70,6.

So it's is indeed not a +10s for every skipped tool. But taking the whole economy into consideration this adds up. In the metal workshops case: after producing 7 tools it could have produced 8 instead.

The metal workshop isn't affected as much as others, though. Because it produces 10 tools in total. Which results in 10 seconds on top of total duration of 700 seconds (if all 10 tools were produced) which result in an 1.4% time increase.

But if only one were produced, as seen above, the 10s are an increase of 14.2%. This hits those buildings that produce only one or two types really hard. Regardless whether the skip is conditional or unconditional.

[hessenfarmer (stephan-lutz)]

Fully agreed. I just wanted to make clear that we (I) have understood the topic in total.
For me the worst case is a site with a work ={} program that does call 2 subroutines where one of them would be needed the other not.
If you have just one ware produced and the cycle is skipped due to the ware not needed that doesn't much harm.
However all programs calling at least 2 worker actions are severely affected. These are amongst others all farmers.
A said above I will test this on my machine which is far less powered than yours if I can see any performance deviation

[GunChleoc (gunchleoc)]

Fixed in build20-rc1