widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1823467-trainingsites-dependencies into lp:widelands.

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

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1823467 in widelands: "Barbarian TrainingCamp: Help contains Building from Frisians"
  https://bugs.launchpad.net/widelands/+bug/1823467

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1823467-trainingsites-dependencies/+merge/365633

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.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1823467-trainingsites-dependencies into lp:widelands.

=== 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()