Merge lp:~widelands-dev/widelands/bug-1823467-trainingsites-dependencies into lp:widelands

[GunChleoc]

Filter wares' producers and consumers for the tribe in the Lua interface.

Stop Frisian building from showing up in Barbarian help and vice versa.

This could also have been solved by 2 extra "if" statements in Lua, but I decided to go the C++ route to make the code safer against future bugs.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4675. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/516790028.
Appveyor build 4461. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1823467_trainingsites_dependencies-4461.

[hessenfarmer (stephan-lutz)]

code looks good.
Have tested the appveyor build. works for the reported bug as weel as for the frisian counterpart.

@bunnybot merge

review: Approve (test and review)

[GunChleoc (gunchleoc)]

Thanks for the review and testing :)

=== modified file 'data/tribes/scripting/help/building_help.lua'
--- data/tribes/scripting/help/building_help.lua	2018-09-26 07:08:39 +0000
+++ data/tribes/scripting/help/building_help.lua	2019-04-07 07:50:32 +0000
@@ -116,14 +116,15 @@
 --    Creates a dependencies line for any number of weapons.
 --
 --    :arg weapons: an array of weapon names
+--    :arg tribename: the name of the tribe for filtering the buildings
 --    :returns: a list weapons images with the producing and receiving building
 --
-function dependencies_training_weapons(weapons)
+function dependencies_training_weapons(weapons, tribename)
    local result = "";
    local producers = {};
    for count, weaponname in pairs(weapons) do
       local weapon_description = wl.Game():get_ware_description(weaponname)
-      for i, producer in ipairs(weapon_description.producers) do
+      for i, producer in ipairs(weapon_description:producers(tribename)) do
          if (producers[producer.name] == nil) then
             producers[producer.name] = {}
          end
@@ -270,13 +271,11 @@
 
    for i, ware_description in ipairs(building_description.inputs) do
     hasinput = true
-      for j, producer in ipairs(ware_description.producers) do
-         if (tribe:has_building(producer.name)) then
-            result = result .. dependencies(
-               {producer, ware_description},
-               _"%1$s from: %2$s":bformat(ware_description.descname, producer.descname)
-            )
-         end
+      for j, producer in ipairs(ware_description:producers(tribe.name)) do
+         result = result .. dependencies(
+            {producer, ware_description},
+            _"%1$s from: %2$s":bformat(ware_description.descname, producer.descname)
+         )
       end
    end
    if (hasinput) then
@@ -341,7 +340,7 @@
       end
 
       -- Normal buildings
-      for j, consumer in ipairs(ware_description.consumers) do
+      for j, consumer in ipairs(ware_description:consumers(tribe.name)) do
          if (tribe:has_building(consumer.name)) then
             outgoing = outgoing .. dependencies({ware_description, consumer}, consumer.descname)
          end
@@ -388,7 +387,7 @@
             building_description.icon_name,
             "tribes/workers/" .. tribe.name .. "/soldier/health_level" .. (building_description.max_health + 1) ..".png"})
       result = result .. dependencies_training_food(building_description.food_health)
-      result = result .. dependencies_training_weapons(building_description.weapons_health)
+      result = result .. dependencies_training_weapons(building_description.weapons_health, tribe.name)
    end
    if (building_description.max_attack and building_description.min_attack) then
       result = result .. h2(_"Attack Training")
@@ -401,7 +400,7 @@
             building_description.icon_name,
             "tribes/workers/" .. tribe.name .. "/soldier/attack_level" .. (building_description.max_attack + 1) ..".png"})
       result = result .. dependencies_training_food(building_description.food_attack)
-      result = result .. dependencies_training_weapons(building_description.weapons_attack)
+      result = result .. dependencies_training_weapons(building_description.weapons_attack, tribe.name)
    end
    if (building_description.max_defense and building_description.min_defense) then
       result = result .. h2(_"Defense Training")
@@ -414,7 +413,7 @@
             building_description.icon_name,
             "tribes/workers/" .. tribe.name .. "/soldier/defense_level" .. (building_description.max_defense + 1) ..".png"})
       result = result .. dependencies_training_food(building_description.food_defense)
-      result = result .. dependencies_training_weapons(building_description.weapons_defense)
+      result = result .. dependencies_training_weapons(building_description.weapons_defense, tribe.name)
    end
    if (building_description.max_evade and building_description.min_evade) then
       result = result .. h2(_"Evade Training")
@@ -427,7 +426,7 @@
             building_description.icon_name,
             "tribes/workers/" .. tribe.name .. "/soldier/evade_level" .. (building_description.max_evade + 1) ..".png"})
       result = result .. dependencies_training_food(building_description.food_evade)
-      result = result .. dependencies_training_weapons(building_description.weapons_evade)
+      result = result .. dependencies_training_weapons(building_description.weapons_evade, tribe.name)
    end
    return result
 end

=== modified file 'data/tribes/scripting/help/ware_help.lua'
--- data/tribes/scripting/help/ware_help.lua	2018-09-11 16:54:54 +0000
+++ data/tribes/scripting/help/ware_help.lua	2019-04-07 07:50:32 +0000
@@ -48,7 +48,7 @@
 --
 function ware_help_producers_string(tribe, ware_description)
    local result = ""
-   for i, building in ipairs(ware_description.producers) do
+   for i, building in ipairs(ware_description:producers(tribe.name)) do
       if (tribe:has_building(building.name)) then
          -- TRANSLATORS: Ware Encyclopedia: A building producing a ware
          result = result .. h2(_"Producer")
@@ -130,7 +130,7 @@
    local consumers_string = ""
    local consumers_amount = 0
 
-   for i, building in ipairs(ware_description.consumers) do
+   for i, building in ipairs(ware_description:consumers(tribe.name)) do
       if (tribe:has_building(building.name)) then
          consumers_string = consumers_string .. dependencies({ware_description, building}, building.descname)
          consumers_amount = consumers_amount + 1

=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2019-03-24 13:13:34 +0000
+++ src/scripting/lua_map.cc	2019-04-07 07:50:32 +0000
@@ -680,6 +680,14 @@
 	return result;
 }
 
+const Widelands::TribeDescr& get_tribe_descr(lua_State* L, const std::string& tribename) {
+	if (!Widelands::tribe_exists(tribename)) {
+		report_error(L, "Tribe '%s' does not exist", tribename.c_str());
+	}
+	const Tribes& tribes = get_egbase(L).tribes();
+	return *tribes.get_tribe_descr(tribes.tribe_index(tribename));
+}
+
 }  // namespace
 
 /*
@@ -3046,12 +3054,12 @@
 */
 const char LuaWareDescription::className[] = "WareDescription";
 const MethodType<LuaWareDescription> LuaWareDescription::Methods[] = {
+   METHOD(LuaWareDescription, consumers),
    METHOD(LuaWareDescription, is_construction_material),
+   METHOD(LuaWareDescription, producers),
    {nullptr, nullptr},
 };
 const PropertyType<LuaWareDescription> LuaWareDescription::Properties[] = {
-   PROP_RO(LuaWareDescription, consumers),
-   PROP_RO(LuaWareDescription, producers),
    {nullptr, nullptr, nullptr},
 };
 
@@ -3075,25 +3083,35 @@
  */
 
 /* RST
-   .. attribute:: consumers
+   .. method:: consumers(tribename)
+
+      :arg tribename: the name of the tribe that this ware gets checked for
+      :type tribename: :class:`string`
 
       (RO) An array with :class:`LuaBuildingDescription` with buildings that
       need this ware for their production.
 */
-int LuaWareDescription::get_consumers(lua_State* L) {
+int LuaWareDescription::consumers(lua_State* L) {
+	if (lua_gettop(L) != 2) {
+		report_error(L, "Takes only one argument.");
+	}
+	const Widelands::TribeDescr& tribe = get_tribe_descr(L, luaL_checkstring(L, 2));
+
 	lua_newtable(L);
 	int index = 1;
 	for (const DescriptionIndex& building_index : get()->consumers()) {
-		lua_pushint32(L, index++);
-		upcasted_map_object_descr_to_lua(
-		   L, get_egbase(L).tribes().get_building_descr(building_index));
-		lua_rawset(L, -3);
+		if (tribe.has_building(building_index)) {
+			lua_pushint32(L, index++);
+			upcasted_map_object_descr_to_lua(
+			   L, get_egbase(L).tribes().get_building_descr(building_index));
+			lua_rawset(L, -3);
+		}
 	}
 	return 1;
 }
 
 /* RST
-   .. method:: is_construction_material
+   .. method:: is_construction_material(tribename)
 
       :arg tribename: the name of the tribe that this ware gets checked for
       :type tribename: :class:`string`
@@ -3114,19 +3132,29 @@
 }
 
 /* RST
-   .. attribute:: producers
+   .. method:: producers(tribename)
+
+      :arg tribename: the name of the tribe that this ware gets checked for
+      :type tribename: :class:`string`
 
       (RO) An array with :class:`LuaBuildingDescription` with buildings that
       can procude this ware.
 */
-int LuaWareDescription::get_producers(lua_State* L) {
+int LuaWareDescription::producers(lua_State* L) {
+	if (lua_gettop(L) != 2) {
+		report_error(L, "Takes only one argument.");
+	}
+	const Widelands::TribeDescr& tribe = get_tribe_descr(L, luaL_checkstring(L, 2));
+
 	lua_newtable(L);
 	int index = 1;
 	for (const DescriptionIndex& building_index : get()->producers()) {
-		lua_pushint32(L, index++);
-		upcasted_map_object_descr_to_lua(
-		   L, get_egbase(L).tribes().get_building_descr(building_index));
-		lua_rawset(L, -3);
+		if (tribe.has_building(building_index)) {
+			lua_pushint32(L, index++);
+			upcasted_map_object_descr_to_lua(
+			   L, get_egbase(L).tribes().get_building_descr(building_index));
+			lua_rawset(L, -3);
+		}
 	}
 	return 1;
 }

=== modified file 'src/scripting/lua_map.h'
--- src/scripting/lua_map.h	2019-03-09 11:20:45 +0000
+++ src/scripting/lua_map.h	2019-04-07 07:50:32 +0000
@@ -564,13 +564,13 @@
 	/*
 	 * Properties
 	 */
-	int get_consumers(lua_State*);
-	int get_producers(lua_State*);
 
 	/*
 	 * Lua methods
 	 */
+	int consumers(lua_State*);
 	int is_construction_material(lua_State*);
+	int producers(lua_State*);
 
 	/*
 	 * C methods

=== modified file 'test/maps/lua_testsuite.wmf/scripting/immovables_descriptions.lua'
--- test/maps/lua_testsuite.wmf/scripting/immovables_descriptions.lua	2018-11-12 08:46:18 +0000
+++ test/maps/lua_testsuite.wmf/scripting/immovables_descriptions.lua	2019-04-07 07:50:32 +0000
@@ -472,13 +472,23 @@
    end
 
    local ware_description = egbase:get_ware_description("coal")
-   assert_equal(true, find_building("barbarians_lime_kiln", ware_description.consumers))
-   assert_equal(true, find_building("empire_smelting_works", ware_description.consumers))
-   assert_equal(true, find_building("atlanteans_smelting_works", ware_description.consumers))
-   assert_equal(true, find_building("barbarians_warmill", ware_description.consumers))
-   assert_equal(true, find_building("barbarians_ax_workshop", ware_description.consumers))
-   assert_equal(true, find_building("barbarians_helmsmithy", ware_description.consumers))
-   assert_equal(false, find_building("atlanteans_crystalmine", ware_description.producers))
+   assert_equal(true, find_building("barbarians_lime_kiln", ware_description:consumers("barbarians")))
+   assert_equal(true, find_building("empire_smelting_works", ware_description:consumers("empire")))
+   assert_equal(true, find_building("atlanteans_smelting_works", ware_description:consumers("atlanteans")))
+   assert_equal(true, find_building("barbarians_warmill", ware_description:consumers("barbarians")))
+   assert_equal(true, find_building("barbarians_ax_workshop", ware_description:consumers("barbarians")))
+   assert_equal(true, find_building("barbarians_helmsmithy", ware_description:consumers("barbarians")))
+   -- Building does not consume this
+   assert_equal(false, find_building("barbarians_helmsmithy", ware_description:consumers("atlanteans")))
+   -- Wrong tribe
+   assert_equal(false, find_building("atlanteans_crystalmine", ware_description:consumers("atlanteans")))
+
+   -- Test when multiple tribes use the same ware
+   ware_description = egbase:get_ware_description("helmet")
+   assert_equal(true, find_building("barbarians_trainingcamp", ware_description:consumers("barbarians")))
+   assert_equal(true, find_building("frisians_training_camp", ware_description:consumers("frisians")))
+   assert_equal(1, #ware_description:consumers("barbarians"))
+   assert_equal(1, #ware_description:consumers("frisians"))
 end
 
 function test_descr:test_icon_name()
@@ -497,11 +507,21 @@
    end
 
    local ware_description = egbase:get_ware_description("coal")
-   assert_equal(true, find_building("empire_charcoal_kiln", ware_description.producers))
-   assert_equal(true, find_building("barbarians_coalmine_deeper", ware_description.producers))
-   assert_equal(true, find_building("barbarians_coalmine_deep", ware_description.producers))
-   assert_equal(true, find_building("atlanteans_coalmine", ware_description.producers))
-   assert_equal(false, find_building("atlanteans_crystalmine", ware_description.producers))
+   assert_equal(true, find_building("empire_charcoal_kiln", ware_description:producers("empire")))
+   assert_equal(true, find_building("barbarians_coalmine_deeper", ware_description:producers("barbarians")))
+   assert_equal(true, find_building("barbarians_coalmine_deep", ware_description:producers("barbarians")))
+   assert_equal(true, find_building("atlanteans_coalmine", ware_description:producers("atlanteans")))
+   -- Building does not produce this
+   assert_equal(false, find_building("atlanteans_crystalmine", ware_description:producers("atlanteans")))
+   -- Wrong tribe
+   assert_equal(false, find_building("atlanteans_coalmine", ware_description:producers("barbarians")))
+
+     -- Test when multiple tribes use the same ware
+   ware_description = egbase:get_ware_description("helmet")
+   assert_equal(true, find_building("barbarians_helmsmithy", ware_description:producers("barbarians")))
+   assert_equal(true, find_building("frisians_armor_smithy_small", ware_description:producers("frisians")))
+   assert_equal(1, #ware_description:producers("barbarians"))
+   assert_equal(1, #ware_description:producers("frisians"))
 end
 
 function test_descr:is_construction_material()