widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Notabilis has proposed merging lp:~widelands-dev/widelands/feature-test-supply-crash into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1639444 in widelands: "Workers with wares inside ships can crash the game"
  https://bugs.launchpad.net/widelands/+bug/1639444

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/feature-test-supply-crash/+merge/312422

- Adds Lua-methods to set the ware and worker policies of warehouses (e.g. "remove from here").
- Creates a regression test for the linked bug which crashes the game when only this branch is used. See also the other branch for a fix of the bug:
https://code.launchpad.net/~widelands-dev/widelands/bug-supply
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/feature-test-supply-crash into lp:widelands.

=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2016-10-16 09:31:42 +0000
+++ src/scripting/lua_map.cc	2016-12-03 23:25:00 +0000
@@ -3868,6 +3868,41 @@
 
    Every Headquarter or Warehouse on the Map is of this type.
 */
+
+/* RST
+   .. method:: get_ware_policies(which)
+
+      Gets the policies how the warehouse should handle the given wares.
+      The method to handle is one of the strings SP_Normal, SP_Prefer, SP_DontStock, SP_Remove.
+
+      :arg which: behaves like for :meth:`HasWares.get_wares`.
+
+      :returns: :class:`string` or :class:`table`
+*/
+
+/* RST
+   .. method:: get_worker_policies(which)
+
+      Same as :meth:`Warehouse.get_ware_policies` but for workers.
+*/
+
+/* RST
+   .. method:: set_ware_policies(which, policy)
+
+      Sets the policies how the warehouse should handle the given wares.
+
+      :arg which: behaves like for :meth:`HasWares.get_wares`.
+
+      :arg policy: the policy to apply for all the wares given in `which`.
+      :type policy: a string out of "SP_Normal", "SP_Prefer", "SP_DontStock", "SP_Remove".
+*/
+
+/* RST
+   .. method:: set_worker_policies(which, policy)
+
+      Same as :meth:`Warehouse.set_ware_policies` but for workers.
+*/
+
 const char LuaWarehouse::className[] = "Warehouse";
 const MethodType<LuaWarehouse> LuaWarehouse::Methods[] = {
    METHOD(LuaWarehouse, set_wares),
@@ -3876,6 +3911,10 @@
    METHOD(LuaWarehouse, get_workers),
    METHOD(LuaWarehouse, set_soldiers),
    METHOD(LuaWarehouse, get_soldiers),
+   METHOD(LuaWarehouse, set_ware_policies),
+   METHOD(LuaWarehouse, get_ware_policies),
+   METHOD(LuaWarehouse, set_worker_policies),
+   METHOD(LuaWarehouse, get_worker_policies),
    METHOD(LuaWarehouse, start_expedition),
    METHOD(LuaWarehouse, cancel_expedition),
    {nullptr, nullptr},
@@ -3971,7 +4010,95 @@
 WH_GET(ware, Ware)
 // documented in parent class
 WH_GET(worker, Worker)
-#undef GET
+#undef WH_GET
+
+
+inline void wh_policy_to_string(lua_State* L, Warehouse::StockPolicy p) {
+	switch (p) {
+		case Warehouse::StockPolicy::SP_Normal:
+			lua_pushstring(L, "SP_Normal");
+			break;
+		case Warehouse::StockPolicy::SP_Prefer:
+			lua_pushstring(L, "SP_Prefer");
+			break;
+		case Warehouse::StockPolicy::SP_DontStock:
+			lua_pushstring(L, "SP_DontStock");
+			break;
+		case Warehouse::StockPolicy::SP_Remove:
+			lua_pushstring(L, "SP_Remove");
+			break;
+	}
+}
+
+inline Warehouse::StockPolicy string_to_wh_policy(lua_State* L, uint32_t index) {
+	std::string str = luaL_checkstring(L, index);
+	if (str == "SP_Normal")
+		return Warehouse::StockPolicy::SP_Normal;
+	else if (str == "SP_Prefer")
+		return Warehouse::StockPolicy::SP_Prefer;
+	else if (str == "SP_DontStock")
+		return Warehouse::StockPolicy::SP_DontStock;
+	else if (str == "SP_Remove")
+		return Warehouse::StockPolicy::SP_Remove;
+	else
+		report_error(L, "<%s> is no valid warehouse policy!", str.c_str());
+}
+
+int LuaWarehouse::set_ware_policies(lua_State* L) {
+	Warehouse* wh = get(L, get_egbase(L));
+	Warehouse::StockPolicy p = string_to_wh_policy(L, -1);
+	lua_pop(L, 1);
+	bool return_number = false;
+	WaresSet set = parse_get_wares_arguments(L, wh->owner().tribe(), &return_number);
+	lua_newtable(L);
+	for (WaresSet::iterator i = set.begin(); i != set.end(); ++i) {
+		wh->set_ware_policy(*i, p);
+	}
+	return 0;
+}
+
+int LuaWarehouse::set_worker_policies(lua_State* L) {
+	Warehouse* wh = get(L, get_egbase(L));
+	Warehouse::StockPolicy p = string_to_wh_policy(L, -1);
+	lua_pop(L, 1);
+	bool return_number = false;
+	WorkersSet set = parse_get_workers_arguments(L, wh->owner().tribe(), &return_number);
+	lua_newtable(L);
+	const TribeDescr& tribe = wh->owner().tribe();
+	for (WorkersSet::iterator i = set.begin(); i != set.end(); ++i) {
+		// If the worker does not cost anything, ignore it
+		// Otherwise, an unlimited stream of carriers might leave the warehouse
+		if (tribe.get_worker_descr(*i)->is_buildable()
+				&& tribe.get_worker_descr(*i)->buildcost().empty()) {
+			continue;
+		}
+		wh->set_worker_policy(*i, p);
+	}
+	return 0;
+}
+
+#define WH_GET_POLICY(type, btype)                                                                        \
+	int LuaWarehouse::get_##type##_policies(lua_State* L) {                                           \
+		Warehouse* wh = get(L, get_egbase(L));                                                    \
+		const Tribes& tribes = get_egbase(L).tribes();                                            \
+		bool return_number = false;                                                               \
+		btype##sSet set = parse_get_##type##s_arguments(L, wh->owner().tribe(), &return_number);  \
+		lua_newtable(L);                                                                          \
+		if (return_number)                                                                        \
+			wh_policy_to_string(L, wh->get_##type##_policy(*set.begin()));                    \
+		else {                                                                                    \
+			lua_newtable(L);                                                                  \
+			for (btype##sSet::iterator i = set.begin(); i != set.end(); ++i) {                \
+				lua_pushstring(L, tribes.get_##type##_descr(*i)->name());                 \
+				wh_policy_to_string(L, wh->get_##type##_policy(*i));                      \
+				lua_rawset(L, -3);                                                        \
+			}                                                                                 \
+		}                                                                                         \
+		return 1;                                                                                 \
+	}
+WH_GET_POLICY(ware, Ware)
+WH_GET_POLICY(worker, Worker)
+#undef WH_GET_POLICY
 
 // documented in parent class
 int LuaWarehouse::get_soldiers(lua_State* L) {

=== modified file 'src/scripting/lua_map.h'
--- src/scripting/lua_map.h	2016-08-04 15:49:05 +0000
+++ src/scripting/lua_map.h	2016-12-03 23:25:00 +0000
@@ -1006,6 +1006,10 @@
 	int get_workers(lua_State*);
 	int set_wares(lua_State*);
 	int set_workers(lua_State*);
+	int get_ware_policies(lua_State*);
+	int get_worker_policies(lua_State*);
+	int set_ware_policies(lua_State*);
+	int set_worker_policies(lua_State*);
 	int set_soldiers(lua_State*);
 	int get_soldiers(lua_State*);
 	int start_expedition(lua_State*);

=== modified file 'test/maps/lua_testsuite.wmf/scripting/warehouse.lua'
--- test/maps/lua_testsuite.wmf/scripting/warehouse.lua	2016-02-17 22:13:21 +0000
+++ test/maps/lua_testsuite.wmf/scripting/warehouse.lua	2016-12-03 23:25:00 +0000
@@ -125,6 +125,114 @@
    assert_equal(20, k.barbarians_lumberjack)
 end
 
+-- ========
+-- Policies
+-- ========
+function warehouse_tests:test_get_ware_policy()
+   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
+   k = self.w:get_ware_policies{"log", "granite"}
+   assert_equal("SP_Normal", k.log)
+   assert_equal("SP_Normal", k.granite)
+   assert_equal(nil, k.coal)
+   k = self.w:get_ware_policies("all")
+   assert_equal("SP_Normal", k.log)
+   assert_equal("SP_Normal", k.granite)
+   assert_equal("SP_Normal", k.coal)
+   assert_equal(nil, k.barbarians_lumberjack)
+end
+function warehouse_tests:test_get_worker_policy()
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
+   k = self.w:get_worker_policies{"barbarians_builder", "barbarians_lumberjack"}
+   assert_equal("SP_Normal", k.barbarians_builder)
+   assert_equal("SP_Normal", k.barbarians_lumberjack)
+   assert_equal(nil, k.coal)
+   k = self.w:get_worker_policies("all")
+   assert_equal("SP_Normal", k.barbarians_builder)
+   assert_equal("SP_Normal", k.barbarians_lumberjack)
+   assert_equal(nil, k.coal)
+end
+function warehouse_tests:test_get_non_existing_policy()
+   assert_error("ware policy for non existing ware", function()
+      self.w:get_ware_policy("plastic")
+   end)
+   assert_error("worker policy for non existing worker", function()
+      self.w:get_worker_policy("tree_climber")
+   end)
+end
+
+function warehouse_tests:test_set_ware_policy()
+   -- Make sure it is the normal policy until now
+   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
+   -- Set new policy for wares
+   self.w:set_ware_policies("log", "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
+   self.w:set_ware_policies("log", "SP_DontStock")
+   assert_equal(self.w:get_ware_policies("log"), "SP_DontStock")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
+   self.w:set_ware_policies("log", "SP_Remove")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Remove")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
+   self.w:set_ware_policies("granite", "SP_DontStock")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Remove")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_DontStock")
+   self.w:set_ware_policies({"log", "granite"}, "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Prefer")
+   self.w:set_ware_policies("log", "SP_Normal")
+   self.w:set_ware_policies("granite", "SP_Normal")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Normal")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Normal")
+   self.w:set_ware_policies("all", "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("log"), "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("granite"), "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("meat"), "SP_Prefer")
+   assert_equal(self.w:get_ware_policies("coal"), "SP_Prefer")
+end
+function warehouse_tests:test_set_worker_policy()
+   -- Make sure it is the normal policy until now
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
+   -- Set new policy for workers
+   self.w:set_worker_policies("barbarians_builder", "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
+   self.w:set_worker_policies("barbarians_builder", "SP_DontStock")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_DontStock")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
+   self.w:set_worker_policies("barbarians_builder", "SP_Remove")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Remove")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
+   self.w:set_worker_policies("barbarians_lumberjack", "SP_DontStock")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Remove")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_DontStock")
+   self.w:set_worker_policies({"barbarians_lumberjack", "barbarians_builder"}, "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Prefer")
+   self.w:set_worker_policies("barbarians_builder", "SP_Normal")
+   self.w:set_worker_policies("barbarians_lumberjack", "SP_Normal")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Normal")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Normal")
+   self.w:set_worker_policies("all", "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_builder"), "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_lumberjack"), "SP_Prefer")
+   assert_equal(self.w:get_worker_policies("barbarians_gardener"), "SP_Prefer")
+end
+function warehouse_tests:test_set_non_existing_policy()
+   assert_error("ware policy for non existing ware", function()
+      self.w:set_ware_policies("plastic", "SP_Prefer")
+   end)
+   assert_error("invalid policy for ware", function()
+      self.w:set_ware_policies("log", "SP_Burn")
+   end)
+   assert_error("worker policy for non existing worker", function()
+      self.w:set_worker_policies("tree_climber", "SP_Prefer")
+   end)
+   assert_error("invalid policy for worker", function()
+      self.w:set_worker_policies("barbarians_builder", "SP_Burn")
+   end)
+end
 
 -- =========
 -- Soldiers

=== added file 'test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua'
--- test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua	1970-01-01 00:00:00 +0000
+++ test/maps/ship_transportation.wmf/scripting/test_create_request_while_worker_with_ware_is_in_transit.lua	2016-12-03 23:25:00 +0000
@@ -0,0 +1,70 @@
+run(function()
+   sleep(100)
+
+   game.desired_speed = 50 * 1000
+
+   create_southern_port()
+   create_northern_port()
+
+   -- create a ready-to-work lumberjack connected to the northern port
+   local lumberjack_hut = p1:place_building("barbarians_lumberjacks_hut", map:get_field(18, 4), false, true)
+   if lumberjack_hut.valid_workers then lumberjack_hut:set_workers(lumberjack_hut.valid_workers) end
+   connected_road(p1, map:get_field(18,5).immovable, "l,l|tl,tr|", true)
+
+   -- a long street to capture the woodcutter when his house is burned
+   connected_road(p1, map:get_field(18,5).immovable, "r,r|r,r|r,tr|tr,tl|l,l|l,l|l,l|", true)
+
+   -- plant a tree inside the street to the right of the lumberjack hut
+
+   assert_equal(nil, map:get_field(23,3).immovable)
+   tree = map:place_immovable("oak_summer_old", map:get_field(23,3), "world")
+   assert_not_equal(nil, map:get_field(23,3).immovable)
+
+   -- set logs and woodcutters to "remove from here" in the port
+   np = northern_port()
+   np:set_ware_policies("log", "SP_Remove")
+   np:set_worker_policies("barbarians_lumberjack", "SP_Remove")
+
+   -- aaand action!
+
+   -- sleep until the tree has been felled
+   -- comparision is a hack. How do I find out, whether a MapObject (tree) is valid?
+   while (tree == map:get_field(23,3).immovable) do
+      sleep(100)
+   end
+   print("tree felled")
+   -- some more sleep since "removal of tree" and "worker has log" are not at the same time
+   sleep(2000)
+   -- now the worker should have the tree
+
+
+   lumberjack_hut:destroy()
+   -- worker will find street and enter port, can't stay there, enters ship
+   while not (ship:get_workers() == 1) do
+      sleep(100)
+   end
+   assert_equal(1, ship:get_workers())
+   -- no wares since the worker is still carrying the log
+   assert_equal(0, ship:get_wares())
+
+   -- start to build a lumberjack on the second port while the required log is still in transit
+   local cons = p1:place_building("barbarians_lumberjacks_hut", map:get_field(17, 17), true, true)
+   connected_road(p1, map:get_field(18,18).immovable, "l,tl|", true)
+
+   -- give the log time to reach the building site
+   while not (ship:get_workers() == 0) do
+      sleep(100)
+   end
+   sleep(3000)
+
+   cons:destroy()
+   
+   sleep(6000)
+   -- check if both lumberjack and log come to rest in the port
+   -- if it is possible to check for the log in the building site that would be an alternative
+   assert_equal(1, southern_port():get_wares("log"))
+   assert_equal(1, southern_port():get_workers("barbarians_lumberjack"))
+
+   print("# All Tests passed.")
+   wl.ui.MapView():close()
+end)