Merge into trunk : bug-1786613-10s-return-skipped : Code : widelands

Status:

Superseded

Proposed branch:

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

Merge into:

lp://staging/widelands

Diff against target:

33 lines (+9/-7)

1 file modified

src/logic/map_objects/tribes/productionsite.cc (+9/-7)

To merge this branch:

bzr merge lp://staging/~widelands-dev/widelands/bug-1786613-10s-return-skipped

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
hessenfarmer		2018-08-20	Pending
Review via email: mp+353437@code.staging.launchpad.net

This proposal supersedes a proposal from 2018-08-13.

This proposal has been superseded by a proposal from 2018-08-20.

Commit message

check for whether the program name is work or not.

Description of the change

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.

Revision history for this message

bunnybot (widelandsofficial) wrote on 2018-08-13: Posted in a previous version of this proposal

#

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.

Revision history for this message

hessenfarmer (stephan-lutz) wrote on 2018-08-13: Posted in a previous version of this proposal

#

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

Revision history for this message

hessenfarmer (stephan-lutz) wrote on 2018-08-14: Posted in a previous version of this proposal

#

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

review: Approve

Revision history for this message

Toni Förster (stonerl) wrote on 2018-08-14: Posted in a previous version of this proposal

#

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

Revision history for this message

ypopezios (ypopezios) wrote on 2018-08-14: Posted in a previous version of this proposal

#

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.

Revision history for this message

hessenfarmer (stephan-lutz) wrote on 2018-08-14: Posted in a previous version of this proposal

#

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

Revision history for this message

bunnybot (widelandsofficial) wrote on 2018-08-14: Posted in a previous version of this proposal

#

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.

Revision history for this message

bunnybot (widelandsofficial) wrote on 2018-08-20:

#

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.

widelands

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

Commit message

Description of the change

Preview Diff

Subscribers