Review: Approve
LGTM, 1 nit. Do you want to change the exception back to an assert?
Diff comments:
> === modified file 'src/ai/defaultai_warfare.cc'
> --- src/ai/defaultai_warfare.cc 2017-11-02 19:55:17 +
> +++ src/ai/defaultai_warfare.cc 2017-11-04 20:46:09 +
> @@ -576,14
For the assert, I'm calculating the biggest possible numerator. For the
return, I calculate the actual average.
Thanks for the review!
@bunnybot merge
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1716166-ai-strength-assert/+merge/333227
Your team Widelands Developers is
The proposal to merge
lp:~widelands-dev/widelands/bug-1716166-ai-strength-assert into lp:widelands
has been updated.
Status: Needs review => Merged
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1716166-ai-strength-assert/+merge/333227
--
Your team
The proposal to merge lp:~widelands-dev/widelands/bug-1729856 into lp:widelands
has been updated.
Commit Message changed to:
Added a check to the AI when considering the upgrade of trainingsites to make
sure we are not going to exceed the limit for weaker AIs.
For more details, see:
Review: Needs Fixing
I only looked at the commit-diffs, but the changes are mostly fine with me,
thanks.
However, there are two problems:
- Codecheck complains about wrong include order in logic/game.cc
- For me, it does not compile. Strangely, Travis seems to be happy with the
code. The
The NOCOM-bug is really a bug and has been fixed. I fear I introduced it myself
a few month ago.
Trimming is also added now. Space is removed in front and at the end of
messages. When this results in an empty message, it is dropped. This is also
done with the message-part of the @whisper, /motd
So If I understand correctly, get_allows_seafaring will be LUA-only, so not
usable by AI?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1718745-allows-seafaring/+merge/333233
Your team Widelands Developers is requested to review the proposed merge of
We're trying to keep the base directory as free as possible. How about putting
them in src/io, since it's all filesystem stuff? Not entirely happy with that
though, since it's all widelands-specific, and io is widelands-agnostic.
kStatisticsSampleTime could be moved to logic/game.h, since it's
OK, all working now :)
Just a small graphics nit: The arrow is very green - desaturate it a bit? I'd
also remove its background color and make that transparent. Shouldn't be a
problem if it's a png file.
--
Continuous integration builds have changed state:
Travis build 2765. State: passed. Details:
https://travis-ci.org/widelands/widelands/builds/297537925.
Appveyor build 2577. State: success. Details:
allows_seafaring is already usable by the AI. And now you don't even need a
mutable_map - a const map& will do.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1718745-allows-seafaring/+merge/333233
Your team Widelands Developers is requested to review the proposed merge of
OK, everything should be addressed now.
I renamed logic/constants.h to logic/filesystem_constants.h, because I couldn't
think of a better place to put them. I moved the statistics sample time
constant out into game.h.
--
Review: Approve commit diff and short test
Code looks good and compiling & starting games works. As far as I am concerned,
this branch can be merged.
I guess filesystem_constants is a good enough name for the file. After all,
that is what it contains.
--
Continuous integration builds have changed state:
Travis build 2766. State: passed. Details:
https://travis-ci.org/widelands/widelands/builds/297640330.
Appveyor build 2578. State: success. Details:
Continuous integration builds have changed state:
Travis build 2770. State: failed. Details:
https://travis-ci.org/widelands/widelands/builds/297682919.
Appveyor build 2582. State: success. Details:
If I interpret the Travis output right, it is complaining about the cmake
library liblogic_constants. Wasn't there something that header-only libraries
do not work? Might be the problem here, it only contains widelands.h.
--
GunChleoc has proposed merging
lp:~widelands-dev/widelands/bug-1718745-allows-seafaring into lp:widelands.
Commit message:
Split new function cleanup_portspaces() from allows_seafaring().
allows_seafaring() is now const. Exposed 3 new functions to LuaMap:
- get_allows_seafaring
-
Well, neither is good solution - throw gives details, but affects also
"production" release, so I opted for combined solution: log+assert, what do you
think?
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is subscribed to branch
18 matches
Mail list logo
