Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-30 Thread GunChleoc 
I'm not a big Java thing, but I guess that's one thing they did better, that 
you have to declare explicitly which exceptions can be thrown by a function.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-29 Thread SirVer 
I hate exceptions. For everything. They add a  second control flow - but it is 
invisible. Your functions can return a value - or they might throw an 
exception. The first thing is visible in its signature, the second is not. At 
the call site you cannot tell which exceptions might come your way. 

> Am 29.01.2018 um 13:20 schrieb GunChleoc :
> 
> I guess the reason for disallowing Exceptions is to make sure that nobody 
> starts programming by exception - I have no issues with Exceptions as long as 
> they are used sensibly, i.e. for error handling rather than for code path 
> decisions.
> 
> https://en.wikipedia.org/wiki/Coding_by_exception
> 
> -- 
> https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
> You are subscribed to branch lp:widelands.


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-29 Thread GunChleoc 
I guess the reason for disallowing Exceptions is to make sure that nobody 
starts programming by exception - I have no issues with Exceptions as long as 
they are used sensibly, i.e. for error handling rather than for code path 
decisions.

https://en.wikipedia.org/wiki/Coding_by_exception

-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread SirVer 
you are right, we should probably codify our style somewhere - since we are 
only using parts of the google c++ style.

we have a long history of boost usage in Widelands, so we continue using it. We 
also have used exceptions a lot in the past, so we also continue using them.

Were we a greenfield project, I personally would follow the style guide closer.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread Teppo Mäenpää 
I will take note for future. Thanks.

I cannot resist asking: The Google C++ style guide disallows boost, with 
exceptions; lexical_cast is not mentioned in the list of allowed uses. How 
strictly should one follow the style guide?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread GunChleoc 
One more comment for the future: You could use boost::lexical_cast instead of 
"int ival = strtol(value.c_str(), nullptr, 10);"
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread noreply 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread Teppo Mäenpää 
Forgot the commit message. I guess bunnybot is not offended by repeated 
requests.

@bunnybot merge

-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Commit Message changed to:

Bug 1574379: Forester prefers good soil, and is thus more efficient.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Commit Message changed to:

Bug 1574379: Forester prefers good soil.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread Teppo Mäenpää 
I understood the Jan14th comments so that it is okay to merge this.

@bunnybot merge

Curious to see what happens.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-27 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3102. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/334024010.
Appveyor build 2909. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2909.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-26 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3092. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/333588431.
Appveyor build 2899. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2899.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-24 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3089. State: failed. Details: 
https://travis-ci.org/widelands/widelands/builds/332953711.
Appveyor build 2896. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2896.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-24 Thread Teppo Mäenpää 
intended to say "comment" in the above.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-23 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3086. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/332469214.
Appveyor build 2893. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2893.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-22 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3083. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/331668194.
Appveyor build 2890. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2890.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
Got the random error, again. I guess bunnybot will not merge now.

If we cannot alter the way the infra works, would it be possible to work around?

What do the scripts look like ? If the errors are truly random, one could try
command || (sleep 10 && command) || $(sleep 10 && command)

or, in this case, 
git clone $ADDR || (rm -Rf $repodir || true ; sleep 10 && git clone $ADDR )) 


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Commit Message changed to:



For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Status: Approved => Needs review

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Status: Needs review => Approved

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Commit Message changed to:

Forester prefers good soil for planting. Fixes bug #1574379.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
The proposal to merge lp:~widelands-dev/widelands/bug-1574379-forester-wit into 
lp:widelands has been updated.

Commit Message changed to:

Forester prefers good soil for planting. Fixes but #1574379.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread kaputtnik 
Thanks :-)

Travis has some problems fetching the packages. There is nothing we can do 
against this, afaik.

You should add the commit message, bunnybot uses it when merging into trunk.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
Side note: Yes, it is gone.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
I have been pretty unlucky with Travis and appveyor. A new commit just to merge 
trunk sounds scary already.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread kaputtnik 
> Side note: WuiPlotArea::register_plot_data leaks memory according to 
> valgrind, is this known?

That should be fixed with 
http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/revision/8557

Can you merge trunk and test again?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-21 Thread Teppo Mäenpää 
Played more. No problems. IMO, merge-quality again.

Side note: WuiPlotArea::register_plot_data leaks memory according to valgrind, 
is this known?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-20 Thread Teppo Mäenpää 
Tried to get the assert for ~2 hours, without success.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-19 Thread Teppo Mäenpää 
I currently mostly write code using language, where single equal sign is a 
comparison instead of assignment. A side-effect is that "return a = b" looks 
very boolean to my eyes..

About the cache_entry > kInvalidForesterEntry: You are right, this is more 
robust, even if it looks weird. I'll add a comment there.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-16 Thread Klaus Halfmann 
> return forester_cache[mi] = correct_val;
< forester_cache[mi] = correct_val;"
< return correct_val;

Well any compiler with some omptimization should optimize this away.
Maybe som bad habit from the old days of C programming :-)


> The comparison (cache_entry > kInvalidForesterEntry) looks weird. 

Yes I thought about this, too. It its more robust, in case one of the
calculactions later returns some negative value this might fix it?

Feel freee to change these aspects as you wish.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-14 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3053. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/328793965.
Appveyor build 2861. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2861.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-14 Thread Teppo Mäenpää 
Hello, Klaus,

Since you asked: Does the line like "return forester_cache[mi] = correct_val;" 
bring any performance benefits? I know it is valid and all, but the old way 
(separate assignment and returning) is more intuitive. Not everybody reading 
the source are professionals, especially C/C++ professionals. Apart from that, 
the changes improve. I have a bad habit of dropping magic numbers in.

About magical constants: The comparison (cache_entry > kInvalidForesterEntry) 
looks weird. It makes sense when one knows that the numeric value of the 
constant is -1. Would inequality comparison look better? No other negative 
values should be around.

Thanks for the review.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-14 Thread Klaus Halfmann 
Review: Approve rerview, compile debug, test

Hello Teppo, I did some small changes, I hope you agree.

I assume this will improve efiicency of tree planting quite 
a lot, thus chnageing the balance of existing maps.

To stick to the ideas of widelands a "tree planter" should become a "power 
planter" 
only after gaining some experience :-) 
So our mircomanaging friends get another toy to play with ;-)

Gun: as of the code this change can go in
As it changes gameplay I am not sure.

I will continue playing my current map till the end.
And then try some dester Maps or such ..


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1574379-forester-wit.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-14 Thread Klaus Halfmann 
You shoud make the -1 a constexpr, e.g.

> constexpr int16_t kInvalidForesterEntry = -1;

Now, after more thinking, forester_cache_ must be member of some gameobject.
Otherwise it may happen, that 

* a cache from some previous game is used for a different map.
* In case of a network game bothe side may have diffenter caches
  depending of the game they played before.

I checked out that code now and will think about a good location
for that cache and the functions. As I do not have much time 
dont expect me to come around with a solution soon.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3051. State: errored. Details: 
https://travis-ci.org/widelands/widelands/builds/328489684.
Appveyor build 2859. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1574379_forester_wit-2859.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread Teppo Mäenpää 
Klaus: I did not quite get the first sentence either: If you build on fertile 
land, there will be less fertile land for the forest. This has not changed. The 
fertile land is now used more effectively than previously, but that was the 
whole point of this ticket, I guess?

No messages were requested in the bug description.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread Teppo Mäenpää 
Hello, 

Thanks for the comments.

The key name "saplingsearches" is silly, and should be replaced by ... hmm, 
what? Any English-speaking people here?

The larger the saplingsearches-number is, the smaller the odds that the 
forester plants the tree to marginal land. This becomes important when the area 
where trees grow well is very small. If there is a borderline of two poor 
terrains, and the terrain property merging algorithm happens to make the 
borderline itself okay for something, you want many dozens of attempts to pick 
that line.

Should we go there? If a significant fraction of all available spots will often 
be inspected by the forester, then we could consider removing the 
already-instanced slots from the "list" of run_findspace in worker.cc. I do not 
think we want the forester to work this way.



I agree, that making the saplingsearches-numbers unequal can change balance 
somewhere. We do not have to use that possibility, or use only modest 
differences.

I am not sure if I understood your comment right. The cache must not survive 
from one game to other. Else there is a (small but still) possibility that the 
one player's cached values are from a different but same size map, and 
multiplayer game desyncs before it is catched. Therefore "game" seems like 
logical place, and it cannot be a static class member. If I make it extern and 
instantiate in dot-cc, effectively same problem appears. Where did I get it 
wrong?
Also, I did not understand the "should be const" remark. Values are added to 
the vector, so the vector itself is not constant. Integers are constant by 
definition, the value of one remains one.

Apart from that, I liked what you wrote.



If you feel like trying this out: Thanks for the feedback! No particular reason 
to hurry -- we are not close to any deadlines AFAIK.

-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread Klaus Halfmann 
This is basicaly a good idea, but it will change game balance.
e.g. before the change one had to place roads or buildings on the infertile
spots to avoid that waste. 

Will there we a warning from foresters now, too?
e.g. when productivity is below 25% meaning there 
is no place to plant a tree at all?
Code does not indicate that.

only tribes on the same computer will share the cache, 
but the shared random geneartor will make it update on all
games, ok.

Code LGTM expect for the forester_cache_, see inline comments.

I want to check soemthing else before I can testplay this.
I still think what the differences in saplingsearches for the tribes 
will mean in the end. I think I must do some lengthy testgames with 
all of them and on deserts maps and very tree friendly maps.

Diff comments:

> 
> === modified file 'src/logic/game.h'
> --- src/logic/game.h  2017-12-19 07:12:18 +
> +++ src/logic/game.h  2018-01-13 09:57:39 +
> @@ -251,6 +251,10 @@
>   void accept_trade(int trade_id);
>   void cancel_trade(int trade_id);
>  
> + // TODO(kxq): The lifetime of game-instance is okay for this, but is 
> this the right spot?
> + // TODO(kxq): I should find the place where LUA changes map, and clear 
> this whenever that happens.
> + std::vector forester_cache_;

shoud be extern and be deplaced in game.cc.
Qualitity of terrain for tree planting normalized to int16,
indexed by MapIndex
-1 meaning invalid, this should be const
shared between all tribes (on the same server)
will be cleared when diffrences are detected (which happes only some times).

> +
>  private:
>   void sync_reset();
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread Teppo Mäenpää 
This is quite simple implementation, and seems to work OK in most cases.

There are some things which I am not perfectly happy with:
- There are quite many nested loops. I worked around by a cache, but that 
spawns other issues:
-- If the map is changes during a game, via LUA scripting, the forester might 
not immediately realize this. It would be more civilized if the cache would be 
invalidated in those cases. This quirk does not harm anything now.
-- The cache is physically located in game.h. This is probably not the best 
place. What is?

If the cache was done right, it could also be benefitted while working on the 
forestability tooltip into editor. Not really in scope, but still.



Remark, should be actually added to comments. The forester considers six best 
suited trees while planting, but only the best suited three while searching for 
spots. This is approximately okay, but could in some situations cause the 
forester/ranger to prefer next-best locations.

Example.

There is terrain where the growth probabilities for the six best trees are:
a => 0.9, 1e-5, 1e-5, 1e-5, 1e-5, 1e-5
b => 1.0, 0.5,  0.5,  0.5,  0.5,  0.5

The goodness of spot a is 0.9 during search, and goodness of spot b is 1.0. 
Therefore the forester/ranger select option b.

While planting, in spot b there are non-negligible changes that the forester 
picks one of the fifty-percent trees (56% of the time). Average tree growth 
thus happens ~72% of the time.

If the forester would have picked slot a, he would practically always have 
chosen the best-suited tree, with ~90% growth success rate.

This is of course an exaggerated example, and IMO should be left like this. 
Feel free to disagree.

=

Now, the Atlantean forester considers most spots, Barbarian ranger least. This 
was requested in the comments of one of the duplicate bugs. The new constants 
(number of slots to consider per plantation) are not well thought-of. If you 
playtest, give feedback.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands

2018-01-13 Thread Teppo Mäenpää 
Teppo Mäenpää has proposed merging 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1574379 in widelands: "Forester plants trees where probability to grow 
is nearly zero"
  https://bugs.launchpad.net/widelands/+bug/1574379

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1574379-forester-wit/+merge/336068

Previously, forester picked a random free spot at his work area, and attempted 
to plant a tree there.

After the change, the forester picks n free slots (the number n can be selected 
separately for each tribe), and plants a tree to the spot which is best suited 
for the three that grows best at that spot.

In short: foresters favor areas which are suitable for forestation.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1574379-forester-wit into lp:widelands.
=== modified file 'data/tribes/workers/atlanteans/forester/init.lua'
--- data/tribes/workers/atlanteans/forester/init.lua	2017-02-12 09:10:57 +
+++ data/tribes/workers/atlanteans/forester/init.lua	2018-01-13 09:57:39 +
@@ -41,7 +41,7 @@
 
programs = {
   plant = {
- "findspace size:any radius:5 avoid:field",
+ "findspace size:any radius:5 avoid:field saplingsearches:12",
  "walk coords",
  "animation dig 2000", -- Play a planting animation
  "animation planting 1000", -- Play a planting animation

=== modified file 'data/tribes/workers/barbarians/ranger/init.lua'
--- data/tribes/workers/barbarians/ranger/init.lua	2017-02-12 09:10:57 +
+++ data/tribes/workers/barbarians/ranger/init.lua	2018-01-13 09:57:39 +
@@ -41,7 +41,7 @@
 
programs = {
   plant = {
- "findspace size:any radius:5 avoid:field",
+ "findspace size:any radius:5 avoid:field saplingsearches:5",
  "walk coords",
  "animation dig 2000",
  "animation planting 1000",

=== modified file 'data/tribes/workers/empire/forester/init.lua'
--- data/tribes/workers/empire/forester/init.lua	2017-02-12 09:10:57 +
+++ data/tribes/workers/empire/forester/init.lua	2018-01-13 09:57:39 +
@@ -41,7 +41,7 @@
 
programs = {
   plant = {
- "findspace size:any radius:5 avoid:field",
+ "findspace size:any radius:5 avoid:field saplingsearches:8",
  "walk coords",
  "animation dig 2000",
  "animation planting 1000",

=== modified file 'src/logic/game.h'
--- src/logic/game.h	2017-12-19 07:12:18 +
+++ src/logic/game.h	2018-01-13 09:57:39 +
@@ -251,6 +251,10 @@
 	void accept_trade(int trade_id);
 	void cancel_trade(int trade_id);
 
+	// TODO(kxq): The lifetime of game-instance is okay for this, but is this the right spot?
+	// TODO(kxq): I should find the place where LUA changes map, and clear this whenever that happens.
+	std::vector forester_cache_;
+
 private:
 	void sync_reset();
 

=== modified file 'src/logic/map_objects/tribes/worker.cc'
--- src/logic/map_objects/tribes/worker.cc	2017-11-12 08:59:45 +
+++ src/logic/map_objects/tribes/worker.cc	2018-01-13 09:57:39 +
@@ -419,6 +419,82 @@
 }
 
 /**
+ * findspace_helper_for_forester
+ *
+ * Making the run_findspace routine shorter, by putting one special case into its own function.
+ * This gets called many times each time the forester searches for a place for a sapling.
+ * Since this already contains three nested for-loops, I dedided to cache the values for a
+ * cpu/memory tradeoff (hint: Widelands is pretty heavy on my oldish PC).
+ * Since the implementation details of double could vary between platforms, I decided to
+ * quantize the terrain goodness into int16_t. This lowers the footprint of the caching,
+ * and also makes desyncs caused by different floats horribly unlikely.
+ *
+ * At the moment of writing, map changing is really infrequent (only in two scenarios)
+ * and even those do not affect this. However, since map changes are possible, this
+ * checks the reliability of the cached value with a small probability (~1%), If a
+ * disparency is found, the entire cache is nuked.
+ *
+ * If somebody in the future makes a scenario, where the land first is barren, and then
+ * spots of eden show up, the foresters will not immediately notice (because of the cache).
+ * They will eventually notice, and since the instance is shared between tribes,
+ * all foresters notice this at the same moment. I hope this is okay.
+ *
+ */
+
+int16_t Worker::findspace_helper_for_forester(const Coords& pos, const Map& map, Game& game) {
+
+	const MapIndex mi = map.get_index(pos, map.get_width());
+	const FCoords fpos = map.get_fcoords(pos);
+	const unsigned vecsize = 1 + unsigned(map.max_index());
+	// This if-sta