Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1729856 into lp:widelands
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 +576,24 @@
> TrainingSiteObserver& tso = trainingsites.front();
>
> // Make sure we are not above ai type limit
> - assert(tso.bo->total_count() <= tso.bo->cnt_limit_by_aimode);
> + if (tso.bo->total_count() > tso.bo->cnt_limit_by_aimode) {
> + throw wexception("%d AI count of %s exceeds an AI limit %d:
> actual count: %d\n",
> + player_number(), tso.bo->name,
> tso.bo->cnt_limit_by_aimode,
> + tso.bo->total_count());
> + }
>
> const DescriptionIndex enhancement = ts->descr().enhancement();
>
> if (enhancement != INVALID_INDEX && ts_without_trainers_ == 0 &&
> mines_.size() > 3 &&
> ts_finished_count_ > 1 && ts_in_const_count_ == 0) {
>
> - if (player_->is_building_type_allowed(enhancement)) {
> + // Make sure that"
// Make sure that" -> // Make sure that
> + // 1. Building is allowed
> + // 2. AI limit for weaker AI is not to be exceeded
> + BuildingObserver& en_bo =
> +
> get_building_observer(tribe_->get_building_descr(enhancement)->name().c_str());
> + if (player_->is_building_type_allowed(enhancement) &&
> + en_bo.aimode_limit_status() ==
> AiModeBuildings::kAnotherAllowed) {
> game().send_player_enhance_building(*tso.site,
> enhancement);
> }
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1729856.
___
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-1716166-ai-strength-assert into lp:widelands
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 subscribed to branch lp:~widelands-dev/widelands/bug-1716166-ai-strength-assert. ___ 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-1716166-ai-strength-assert into lp:widelands
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 Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1716166-ai-strength-assert. ___ 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-1729856 into lp:widelands
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: https://code.launchpad.net/~widelands-dev/widelands/bug-1729856/+merge/333230 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1729856. ___ 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/savegame-menu into lp:widelands
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 problem is the undefined variable mso in ai/defaultai_warfare.cc. It has been removed in commit 8154, is there a reason for it? After re-adding the line it compiles fine. A quick test with my locally fixed version did not crash any longer in the menus and the issues from the review are done or have become TODOs. What is a bit strange: The preview-map of savegames and the additional game information are gone. Is this intentional? Oh, and I am not able to start or load a game. It loads fine but the game crashes as soon as the game is displayed. I found the source in AiDnaHandler::fetch_dna() in logic/ai_dna_handler.cc. The path to the file is created by concatenating char pointers since a std::string() cast is missing due to the new char* constants. I haven't checked the other uses of the constants, maybe its happening there, too. Is it possible to make the constants std::strings to avoid this (possible/future) bug? Regarding constants.h: Currently the file is in src/logic/ but also contains constants from, e.g. the ai. Maybe place it directly in src/ ? Related: in src/network/ there is also a constants.h containing the used network port numbers. Shall I move them to our global(?) constants file (I have to do this myself)? -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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/net-uuid into lp:widelands
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 and /announcement commands. As far as I am concerned, this can be merged as soon as the metaserver is ready. -- https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid. ___ 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-1718745-allows-seafaring into lp:widelands
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 lp:~widelands-dev/widelands/bug-1718745-allows-seafaring 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/savegame-menu into lp:widelands
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 used both by logic and wui and has nothing to do with the I/O. -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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-website/searching_with_haystack_whoosh into lp:widelands-website
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. -- https://code.launchpad.net/~widelands-dev/widelands-website/searching_with_haystack_whoosh/+merge/331605 Your team Widelands Developers is subscribed to branch lp:widelands-website. ___ 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/net-relay into lp:widelands
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: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_relay-2577. -- https://code.launchpad.net/~widelands-dev/widelands/net-relay/+merge/332386 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/net-relay 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-1718745-allows-seafaring into lp:widelands
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 lp:~widelands-dev/widelands/bug-1718745-allows-seafaring 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/savegame-menu into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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/savegame-menu into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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-1718745-allows-seafaring into lp:widelands
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: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1718745_allows_seafaring-2578. -- 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 lp:~widelands-dev/widelands/bug-1718745-allows-seafaring 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/savegame-menu into lp:widelands
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: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_savegame_menu-2582. -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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/savegame-menu into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/savegame-menu/+merge/322924 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/savegame-menu. ___ 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-1718745-allows-seafaring into lp:widelands
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
- get_number_of_port_spaces
- set_port_space
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1718745 in widelands: "Please expose "allows_seafaring" in Lua interface"
https://bugs.launchpad.net/widelands/+bug/1718745
For more details, see:
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
lp:~widelands-dev/widelands/bug-1718745-allows-seafaring into lp:widelands.
=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc 2017-08-20 17:45:42 +
+++ src/editor/ui_menus/main_menu_save_map.cc 2017-11-05 17:26:05 +
@@ -279,6 +279,7 @@
g_fs->create_sub_file_system(tmp_name, binary ? FileSystem::ZIP : FileSystem::DIR));
// Recompute seafaring tag
+ map->cleanup_portspaces();
if (map->allows_seafaring()) {
map->add_tag("seafaring");
} else {
=== modified file 'src/logic/map.cc'
--- src/logic/map.cc 2017-09-13 07:27:00 +
+++ src/logic/map.cc 2017-11-05 17:26:05 +
@@ -1949,32 +1949,15 @@
check_neighbour_heights(n[i], area);
}
-/*
-===
-Map::allows_seafaring()
-
-This function checks if there are two ports that are reachable
-for each other - then the map is seafaring.
-=
-*/
-bool Map::allows_seafaring() {
- Map::PortSpacesSet port_spaces = get_port_spaces();
- std::vector portdocks;
+bool Map::allows_seafaring() const {
std::set swim_coords;
- for (const Coords& c : port_spaces) {
+ for (const Coords& c : get_port_spaces()) {
std::queue q_positions;
std::set visited_positions;
FCoords fc = get_fcoords(c);
- portdocks = find_portdock(fc);
-
- /* remove the port space if it is not longer valid port space */
- if ((fc.field->get_caps() & BUILDCAPS_SIZEMASK) != BUILDCAPS_BIG || portdocks.empty()) {
- set_port_space(c, false);
- continue;
- }
-
- for (const Coords& portdock : portdocks) {
+
+ for (const Coords& portdock : find_portdock(fc)) {
visited_positions.insert(portdock);
q_positions.push(portdock);
}
@@ -2003,6 +1986,16 @@
return false;
}
+void Map::cleanup_portspaces() {
+ for (const Coords& c : get_port_spaces()) {
+ const FCoords fc = get_fcoords(c);
+ if ((fc.field->get_caps() & BUILDCAPS_SIZEMASK) != BUILDCAPS_BIG || find_portdock(fc).empty()) {
+ set_port_space(c, false);
+ continue;
+ }
+ }
+}
+
bool Map::has_artifacts() {
for (MapIndex i = 0; i < max_index(); ++i) {
if (upcast(Immovable, immovable, fields_[i].get_immovable())) {
=== modified file 'src/logic/map.h'
--- src/logic/map.h 2017-09-01 15:45:59 +
+++ src/logic/map.h 2017-11-05 17:26:05 +
@@ -451,7 +451,11 @@
return port_spaces_;
}
std::vector find_portdock(const Widelands::Coords& c) const;
- bool allows_seafaring();
+
+ /// Check whether there are at least 2 port spaces that can be reached from each other by water
+ bool allows_seafaring() const;
+ /// Remove all port spaces that are not valid (Buildcap < big or not enough space for a portdock)
+ void cleanup_portspaces();
/// Checks whether there are any artifacts on the map
bool has_artifacts();
=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc 2017-10-04 00:27:47 +
+++ src/scripting/lua_map.cc 2017-11-05 17:26:05 +
@@ -1138,7 +1138,7 @@
.. class:: Map
- Access to the map and it's objects. You cannot instantiate this directly,
+ Access to the map and its objects. You cannot instantiate this directly,
instead access it via :attr:`wl.Game.map`.
*/
const char LuaMap::className[] = "Map";
@@ -1146,9 +1146,12 @@
METHOD(LuaMap, place_immovable),
METHOD(LuaMap, get_field),
METHOD(LuaMap, recalculate),
+ METHOD(LuaMap, set_port_space),
{nullptr, nullptr},
};
const PropertyType LuaMap::Properties[] = {
+ PROP_RO(LuaMap, allows_seafaring),
+ PROP_RO(LuaMap, number_of_port_spaces),
PROP_RO(LuaMap, width),
PROP_RO(LuaMap, height),
PROP_RO(LuaMap, player_slots),
@@ -1167,6 +1170,31 @@
==
*/
/* RST
+ .. attribute:: allows_seafaring
+
+ (RO) Whether the map currently allows seafaring. This will calculate a path between port spaces,
+ so it's more accurate but less efficient than :any:`number_of_port_spaces`.
+
+ :returns: True if there are at least two port spaces that can be reached from each other.
+*/
+int LuaMap::get_allows_seafaring(lua_State* L) {
+ lua_pushboolean(L, get_egbase(L).map().allows_seafaring());
+ return 1;
+}
+/* RST
+ .. attribute:: number_of_port_spaces
+
+ (RO) The amount of port spaces on the
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1729856 into lp:widelands
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 lp:~widelands-dev/widelands/bug-1729856. ___ 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
