Merge lp:~widelands-dev/widelands/bug-1380286 into lp:widelands

[TiborB]

Adding 3 functions for LUA for flag object to list roads, building (if any) and verify whether the flag is connected to any warehouse

[GunChleoc (gunchleoc)]

Can you please add some regression tests as well?

They should go in test/maps/lua_testsuite.wmf/scripting/flag.lua

You can run these tests directly without having to run the whole test suite:

./widelands --scenario=test/maps/lua_testsuite.wmf

I have also spotted a NOCOM comment in the diff - please grep to make sure you have resolved these, unless they contain a codereview question.

Once this is done, I'll have a closer look :)

[TiborB (tiborb95)]

LUA test done (not pushed up yet) - one issue, though.

It is not trivial to count roads returned as '#field.roads' returns 0, though when accessing a particular road via field.roads.tl works and shows the road is there. So perhaps if convenient, I can add also function field.roads_count returning number of roads. What do you think?

[GunChleoc (gunchleoc)]

My best guess is that #field.roads doesn't work because you have an associative array rather than an indexed one in your table.

Is there a particular reason you need to count roads in your test? You could test for the 6 directions individually if they're there or not.

[TiborB (tiborb95)]

Counting of roads can be used to say whether it is an end or "middle" of road. The bug/request was reported by wl-zocker, if he is fine not having such explicit count, I dont mind

I made some changes to the code, as you wanted

[wl-zocker (wl-zocker)]

I think counting the roads is not necessary. Furthermore, it could be implemented in lua itsself quite easily:
local result = 0
for k,v in pairs(flag.roads) do
   result = result + 1
end
return result
or something along these lines.

The only thing that remains is to use this function in the first tutorial in two or three places (when connecting the quarries). I do not have much time currently, so either one of you does that (there are some comments, I think), or I implement this some day.

[SirVer (sirver)]

I added a bunch of NOCOM(#codereview) comments in the code. I disagree with the has_warehouse method.

Also you added a bunch of method declarations in lua_bases.h, but did not implement these - I removed them, but that was sloppy. Do you look over the diff when you send something for review?

review: Needs Fixing

[TiborB (tiborb95)]

All this is new for me, so it is easily possible I declared something on wrong place or so. As long as it compiles and worked.

Do you really want to reimplement all that is available in C to lua? Lua scripting is used quite a little so I am not sure it is worth the effort.

But all right, I will look at it, and I prefer option 1.

[SirVer (sirver)]

> Do you really want to reimplement all that is available in C to lua?

No, only what makes sense. Asking a flag if it knows about a warehouse somewhere in its road network makes no sense to me. Option 1) is clearer, but it is also more work.

I would suggest removing get_warehouse from this branch and start a new one with this change.

[TiborB (tiborb95)]

As we are not in hurry, I will unpropose merge and add that economy staff

[TiborB (tiborb95)]

I did not unpropose because it would delete all comments and this is what I dont want...

[GunChleoc (gunchleoc)]

I'd say just do the roads stuff in this branch and merge it.

The warehouse thing is separate code, so it's better to have 2 branches and keep the changes smaller.

[TiborB (tiborb95)]

alright, I removed warehouse stuff. There are some NOCOM comments to look at

It seems it would be too difficult for me to add new class like LuaEconomy, but debug_economy atribute is working for buildings and flags so you can test like this:
flag_or_building.debug_economy==warehouse.debug_economy

[TiborB (tiborb95)]

Hey, I am resubmitting this - because it just waits for review...

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

I have pushed a code review. Just split up the tests, and we're good to go.

[SirVer (sirver)]

I looked at this again right now and went ahead and merged.

I thought it was a good idea to split the tests into two separates, but it had some code duplication that I decided in the end it is better to keep the test together. I just made the tests a bit tighter by directly comparing the immovables instead of just != nil.

review: Approve

=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2015-02-09 05:57:08 +0000
+++ src/scripting/lua_map.cc	2015-02-12 21:03:16 +0000
@@ -2103,13 +2103,15 @@
 	MapObject * me = m_get_or_zero(egbase);
 	MapObject * you = other->m_get_or_zero(egbase);
 
-	// Both objects are destroyed: they are equal
-	if (me == you) lua_pushboolean(L, true);
-	else if (!me || !you) // One of the objects is destroyed: they are distinct
+	// Both objects are destroyed (nullptr) or equal: they are equal
+	if (me == you) {
+		lua_pushboolean(L, true);
+	} else if (me == nullptr || you == nullptr) { // One of the objects is destroyed: they are distinct
 		lua_pushboolean(L, false);
-	else // Compare them
+	} else { // Compare their serial number.
 		lua_pushboolean
 			(L, other->get(L, egbase)->serial() == get(L, egbase)->serial());
+	}
 
 	return 1;
 }
@@ -2343,6 +2345,8 @@
 	{nullptr, nullptr},
 };
 const PropertyType<LuaFlag> LuaFlag::Properties[] = {
+	PROP_RO(LuaFlag, roads),
+	PROP_RO(LuaFlag, building),
 	{nullptr, nullptr, nullptr},
 };
 
@@ -2352,7 +2356,52 @@
  PROPERTIES
  ==========================================================
  */
-
+/* RST
+	.. attribute:: roads
+
+		(RO) Array of roads leading to the flag. Directions
+		can be tr,r,br,bl,l and tl
+
+		:returns: The array of 'direction:road', if any
+*/
+int LuaFlag::get_roads(lua_State * L) {
+
+		const std::vector<std::string> directions = {"tr", "r", "br", "bl", "l", "tl"};
+
+		lua_newtable(L);
+
+		EditorGameBase & egbase = get_egbase(L);
+		Flag * f = get(L, egbase);
+
+		for (uint32_t i = 1; i <= 6; i++){
+ 	       if (f->get_road(i) != nullptr)  {
+				lua_pushstring(L, directions.at(i - 1));
+				upcasted_map_object_to_lua(L, f->get_road(i));
+				lua_rawset(L, -3);
+			}
+		}
+		return 1;
+}
+
+/* RST
+	.. attribute:: building
+
+		(RO) building belonging to the flag
+*/
+int LuaFlag::get_building(lua_State * L) {
+
+	EditorGameBase & egbase = get_egbase(L);
+	Flag * f = get(L, egbase);
+
+	PlayerImmovable * building = f->get_building();
+	if (!building)  {
+		return 0;
+	} else {
+		upcasted_map_object_to_lua(L, building);
+	}
+	return 1;
+
+}
 /*
  ==========================================================
  LUA METHODS

=== modified file 'src/scripting/lua_map.h'
--- src/scripting/lua_map.h	2015-01-31 16:03:59 +0000
+++ src/scripting/lua_map.h	2015-02-12 21:03:16 +0000
@@ -596,13 +596,15 @@
 	/*
 	 * Properties
 	 */
-
+	int get_roads(lua_State * L);
+	int get_building(lua_State * L);
 	/*
 	 * Lua Methods
 	 */
 	int set_wares(lua_State *);
 	int get_wares(lua_State *);
 
+
 	/*
 	 * C Methods
 	 */

=== modified file 'test/maps/lua_testsuite.wmf/scripting/flag.lua'
--- test/maps/lua_testsuite.wmf/scripting/flag.lua	2014-07-28 18:30:11 +0000
+++ test/maps/lua_testsuite.wmf/scripting/flag.lua	2015-02-12 21:03:16 +0000
@@ -12,7 +12,10 @@
    self.f = player1:place_flag(self.field, 1)
 end
 function flag_tests:teardown()
-   pcall(self.f.remove, self.f)
+   pcall(function() self.f:remove() end)
+   pcall(function() self.f1:remove() end)
+   pcall(function() self.f2:remove() end)
+   pcall(function() self.w:remove() end)
 end
 
 function flag_tests:test_name()
@@ -139,3 +142,21 @@
       self.f:get_wares{"meat", "balloon"}
    end)
 end
+
+-- ===================
+-- buildings and roads
+-- ===================
+function flag_tests:roads_and_buildings_test()
+   local field = map:get_field(4,14)
+   self.w = player1:place_building("warehouse", field)
+   self.f1 = self.w.flag
+   local road = player1:place_road(self.f1, "br", "br", "br")
+   self.f2 = self.f1.fields[1].brn.brn.brn.immovable
+
+   assert_equal(self.f2.roads.tl, road)
+   assert_equal(self.f2.roads.tl.start_flag, self.f1)
+   assert_equal(self.f2.roads.tl.end_flag, self.f2)
+   assert_equal(self.f1.building, self.w)
+   assert_nil(self.f2.building)
+   assert_equal(self.f2.debug_economy, self.w.debug_economy)
+end

=== modified file 'test/maps/lua_testsuite.wmf/scripting/game.lua'
--- test/maps/lua_testsuite.wmf/scripting/game.lua	2014-01-12 19:06:22 +0000
+++ test/maps/lua_testsuite.wmf/scripting/game.lua	2015-02-12 21:03:16 +0000
@@ -1,5 +1,5 @@
 -- ===================
--- Tests for the game 
+-- Tests for the game
 -- ===================
 
 test_game = lunit.TestCase("Game functions test")
@@ -10,4 +10,3 @@
 function test_game:test_no_editor_class()
    assert_equal(nil, wl.Editor)
 end
-