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

[TiborB]

The fix is very trivial - one line was added

[SirVer (sirver)]

Could you add a test in test/maps/lua_testsuite.wmf/scripting/efield.lua and make sure that it did not pass before and passes after your fix?

You can run the tests with ./regression_test.py -b ../build/debug/src/widelands -r lua_testsuite

[TiborB (tiborb95)]

Well, I spent more time on that test then fixing the bug itself. I had even add another function to make the test possible...

[SirVer (sirver)]

LGTM, a couple of nits. Could you fix them.

Also starting_res_amount is a bit cumbersome. What do you guys think of renaming it to initial_resource_amount throughout the codebase (i.e. also in cpp?).

review: Approve

[GunChleoc (gunchleoc)]

+1 to renaming. Do we want to do that here or in a separate bug? I think doing it here might be a good idea, so we will have it in before we expose it to the Lua interface.

[TiborB (tiborb95)]

no problem for me to rename

@SirVer - see answer to one of your comments below in diff.

[SirVer (sirver)]

Do the rename here I'd say.

> I dont understand a meaning of this comment. Why should we care that resource_amount!=starting_resource_amount here. In fact they will probably be equal. Depending on a map used for testing.

I would then suggest that you test one more value and also test the absolute value:

self.f.resource_amount=10
assert_equal(self.f.starting_resource_amount, self.f.resource_amount)
assert_equal(10, self.f.starting_resource_amount)

self.f.resource_amount=20
assert_equal(self.f.starting_resource_amount, self.f.resource_amount)
assert_equal(20, self.f.starting_resource_amount)

[TiborB (tiborb95)]

initial_res_amount ? - the difference is so small

I will add the test, the suggestion is reasonable :)

[TiborB (tiborb95)]

@SirVer

so should I remove
 :see also: :attr:`resource`
line? There are more such/similar lines in the lua_map.cc though.

[SirVer (sirver)]

> so should I remove :see also: :attr:`resource`

I thought it would not work. Could you doublecheck that the current occurences are working in https://wl.widelands.org/docs/wl/ ? If so, then leave it in. If not, please take it out.

[TiborB (tiborb95)]

It works. It is visible in wiki and link also works

[TiborB (tiborb95)]

@SirVer - I changed the test as you asked

[SirVer (sirver)]

great. only the rename from starting_ to initial_ and this is good to go.

[TiborB (tiborb95)]

renaming done. I did recursive renaming with sed so quite a lot files has been changed.

BTW: look at the 'Description of the Change' now, funny :) Or terrible overhead I can say :)

[SirVer (sirver)]

You missed some :). See inline comments.

review: Needs Fixing

[TiborB (tiborb95)]

alright, next try

[SirVer (sirver)]

You missed one rename :). I did it for you and merged.

review: Approve

[SirVer (sirver)]

Spoke to early. The fix is flawed - it also changes initial_res_amount in a game. That leads to wrong results if a scenario changes a fields resource amount. I added a failing test.

review: Needs Fixing

[TiborB (tiborb95)]

all right to have common design for both, I the new design will be: change the initial_res_amount to res_amount only if initial_res_amount would be lower then new value of res_amount
Agreed?

[SirVer (sirver)]

No, in the editor it is correct to always change the initial_res_amount too - there is no difference between the too there. And in game you should never touch that. What happens when amount > initial_amount I do not know but there is no reason to change the value if the scripter wants exactly that.

So you will need to check if you are in the editor or in game.

[TiborB (tiborb95)]

"What happens when amount > initial_amount" - I dont think is it good aproach to allow such situation and in addition, there is no set_initial_res_amount available via LUA - but I will change it, no problem...

[TiborB (tiborb95)]

@SirVer - I changed it. Test works as well.

[SirVer (sirver)]

Looks good now. Merged.

> "What happens when amount > initial_amount" - I dont think is it good aproach to allow such situation and in addition, there is no set_initial_res_amount available via LUA - but I will change it, no problem...

I understand what you say. If a scripter sets res > initial that could be bad for the engine. But then again, why should initial be changed if somebody changes res? initial is the value at the beginning of the game, it is not the max value. Why should the current value not be higher than the initial value? For fish it could even make logical sense. So I think your implementation is correct - let's see if it makes any trouble.

review: Approve

=== modified file 'src/editor/map_generator.cc'
--- src/editor/map_generator.cc	2014-09-20 09:37:47 +0000
+++ src/editor/map_generator.cc	2014-12-03 20:14:08 +0000
@@ -138,7 +138,7 @@
 		res_val /= 3;
 		if (editor_change_resource_tool_callback(fc, map_, world, res_idx)) {
 			fc.field->set_resources(res_idx, res_val);
-			fc.field->set_starting_res_amount(res_val);
+			fc.field->set_initial_res_amount(res_val);
 		}
 	};
 

=== modified file 'src/editor/tools/editor_decrease_resources_tool.cc'
--- src/editor/tools/editor_decrease_resources_tool.cc	2014-09-20 09:37:47 +0000
+++ src/editor/tools/editor_decrease_resources_tool.cc	2014-12-03 20:14:08 +0000
@@ -66,10 +66,10 @@
 			map.overlay_manager().remove_overlay(mr.location(), pic);
 			if (!amount) {
 				mr.location().field->set_resources(0, 0);
-				mr.location().field->set_starting_res_amount(0);
+				mr.location().field->set_initial_res_amount(0);
 			} else {
 				mr.location().field->set_resources(args.cur_res, amount);
-				mr.location().field->set_starting_res_amount(amount);
+				mr.location().field->set_initial_res_amount(amount);
 				//  set new overlay
 				str = world.get_resource(args.cur_res)->get_editor_pic(amount);
 				pic = g_gr->images().get(str);

=== modified file 'src/editor/tools/editor_increase_resources_tool.cc'
--- src/editor/tools/editor_increase_resources_tool.cc	2014-09-20 09:37:47 +0000
+++ src/editor/tools/editor_increase_resources_tool.cc	2014-12-03 20:14:08 +0000
@@ -121,10 +121,10 @@
 
 			if (!amount) {
 				mr.location().field->set_resources(0, 0);
-				mr.location().field->set_starting_res_amount(0);
+				mr.location().field->set_initial_res_amount(0);
 			} else {
 				mr.location().field->set_resources(args.cur_res, amount);
-				mr.location().field->set_starting_res_amount(amount);
+				mr.location().field->set_initial_res_amount(amount);
 				//  set new overlay
 				pic = g_gr->images().get
 				        (world.get_resource(args.cur_res)->get_editor_pic(amount));

=== modified file 'src/editor/tools/editor_set_resources_tool.cc'
--- src/editor/tools/editor_set_resources_tool.cc	2014-09-20 09:37:47 +0000
+++ src/editor/tools/editor_set_resources_tool.cc	2014-12-03 20:14:08 +0000
@@ -63,10 +63,10 @@
 
 			if (!amount) {
 				mr.location().field->set_resources(0, 0);
-				mr.location().field->set_starting_res_amount(0);
+				mr.location().field->set_initial_res_amount(0);
 			} else {
 				mr.location().field->set_resources(args.cur_res, amount);
-				mr.location().field->set_starting_res_amount(amount);
+				mr.location().field->set_initial_res_amount(amount);
 				//  set new overlay
 				pic =
 				    g_gr->images().get(world.get_resource(args.cur_res)->get_editor_pic(amount));
@@ -107,10 +107,10 @@
 
 		if (!amount) {
 			mr.location().field->set_resources(0, 0);
-			mr.location().field->set_starting_res_amount(0);
+			mr.location().field->set_initial_res_amount(0);
 		} else {
 			mr.location().field->set_resources(*it, amount);
-			mr.location().field->set_starting_res_amount(amount);
+			mr.location().field->set_initial_res_amount(amount);
 			//  set new overlay
 			pic = g_gr->images().get(world.get_resource(*it)->get_editor_pic(amount));
 			overlay_manager.register_overlay(mr.location(), pic, 4);

=== modified file 'src/logic/field.h'
--- src/logic/field.h	2014-09-14 11:31:58 +0000
+++ src/logic/field.h	2014-12-03 20:14:08 +0000
@@ -124,7 +124,7 @@
 	OwnerInfoAndSelectionsType owner_info_and_selections;
 
 	ResourceIndex m_resources; ///< Resource type on this field, if any
-	uint8_t m_starting_res_amount; ///< Initial amount of m_resources
+	uint8_t m_initial_res_amount; ///< Initial amount of m_resources
 	uint8_t m_res_amount; ///< Current amount of m_resources
 
 	Terrains terrains;
@@ -214,11 +214,11 @@
 	}
 
 	// TODO(unknown): This should take uint8_t
-	void set_starting_res_amount(int32_t const amount) {
-		m_starting_res_amount = amount;
+	void set_initial_res_amount(int32_t const amount) {
+		m_initial_res_amount = amount;
 	}
 	// TODO(unknown): This should return uint8_t
-	int32_t get_starting_res_amount() const {return m_starting_res_amount;}
+	int32_t get_initial_res_amount() const {return m_initial_res_amount;}
 
 	/// \note you must reset this field's + neighbor's brightness when you
 	/// change the height. Map::change_height does this. This function is not

=== modified file 'src/logic/findnode.cc'
--- src/logic/findnode.cc	2014-11-28 16:40:55 +0000
+++ src/logic/findnode.cc	2014-12-03 20:14:08 +0000
@@ -126,7 +126,7 @@
 	if (m_resource != coord.field->get_resources()) {
 		return false;
 	}
-	if (coord.field->get_resources_amount() < coord.field->get_starting_res_amount()) {
+	if (coord.field->get_resources_amount() < coord.field->get_initial_res_amount()) {
 		return true;
 	}
 	for (Direction dir = FIRST_DIRECTION; dir <= LAST_DIRECTION; ++dir) {
@@ -134,7 +134,7 @@
 		if
 			(m_resource == neighb.field->get_resources()
 			 &&
-			 neighb.field->get_resources_amount() < neighb.field->get_starting_res_amount())
+			 neighb.field->get_resources_amount() < neighb.field->get_initial_res_amount())
 		{
 			return true;
 		}

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2014-10-29 06:41:10 +0000
+++ src/logic/map.cc	2014-12-03 20:14:08 +0000
@@ -272,10 +272,10 @@
 
 			if (res == -1 || !amount) {
 				f.field->set_resources(0, 0);
-				f.field->set_starting_res_amount(0);
+				f.field->set_initial_res_amount(0);
 			} else {
 				f.field->set_resources(res, amount);
-				f.field->set_starting_res_amount(amount);
+				f.field->set_initial_res_amount(amount);
 			}
 
 		}

=== modified file 'src/logic/production_program.cc'
--- src/logic/production_program.cc	2014-11-22 11:21:12 +0000
+++ src/logic/production_program.cc	2014-12-03 20:14:08 +0000
@@ -1254,7 +1254,7 @@
 			uint8_t  fres   = mr.location().field->get_resources();
 			uint32_t amount = mr.location().field->get_resources_amount();
 			uint32_t start_amount =
-				mr.location().field->get_starting_res_amount();
+				mr.location().field->get_initial_res_amount();
 
 			if (fres != m_resource) {
 				amount       = 0;

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2014-11-30 18:49:38 +0000
+++ src/logic/worker.cc	2014-12-03 20:14:08 +0000
@@ -235,7 +235,7 @@
 	do {
 		uint8_t fres  = mr.location().field->get_resources();
 		uint32_t amount =
-			mr.location().field->get_starting_res_amount() -
+			mr.location().field->get_initial_res_amount() -
 			mr.location().field->get_resources_amount   ();
 
 		// In the future, we might want to support amount = 0 for
@@ -278,7 +278,7 @@
 			continue;
 
 		uint32_t amount =
-			mr.location().field->get_starting_res_amount() -
+			mr.location().field->get_initial_res_amount() -
 			mr.location().field->get_resources_amount   ();
 
 		pick -= 8 * amount;
@@ -288,7 +288,7 @@
 			--amount;
 
 			mr.location().field->set_resources
-				(res, mr.location().field->get_starting_res_amount() - amount);
+				(res, mr.location().field->get_initial_res_amount() - amount);
 			break;
 		}
 	} while (mr.advance(map));

=== modified file 'src/map_io/map_resources_packet.cc'
--- src/map_io/map_resources_packet.cc	2014-09-20 09:37:47 +0000
+++ src/map_io/map_resources_packet.cc	2014-12-03 20:14:08 +0000
@@ -86,7 +86,7 @@
 				if (0xa < set_id)
 					throw "Unknown resource in map file. It is not in world!\n";
 				map[Coords(x, y)].set_resources(set_id, set_amount);
-				map[Coords(x, y)].set_starting_res_amount(set_start_amount);
+				map[Coords(x, y)].set_initial_res_amount(set_start_amount);
 			}
 		}
 	} else
@@ -133,7 +133,7 @@
 			const Field & f = map[Coords(x, y)];
 			int32_t       res          = f.get_resources          ();
 			int32_t const       amount = f.get_resources_amount   ();
-			int32_t const start_amount = f.get_starting_res_amount();
+			int32_t const start_amount = f.get_initial_res_amount();
 			if (!amount)
 				res = 0;
 			fw.unsigned_8(res);

=== modified file 'src/map_io/s2map.cc'
--- src/map_io/s2map.cc	2014-10-04 09:40:18 +0000
+++ src/map_io/s2map.cc	2014-12-03 20:14:08 +0000
@@ -645,7 +645,7 @@
 			const int32_t real_amount = static_cast<int32_t>
 				(2.86 * static_cast<float>(amount));
 			f->set_resources(nres, real_amount);
-			f->set_starting_res_amount(real_amount);
+			f->set_initial_res_amount(real_amount);
 		}
 
 

=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc	2014-11-30 18:49:38 +0000
+++ src/scripting/lua_map.cc	2014-12-03 20:14:08 +0000
@@ -3502,6 +3502,7 @@
 	PROP_RO(LuaField, viewpoint_y),
 	PROP_RW(LuaField, resource),
 	PROP_RW(LuaField, resource_amount),
+	PROP_RO(LuaField, initial_resource_amount),
 	PROP_RO(LuaField, claimers),
 	PROP_RO(LuaField, owner),
 	{nullptr, nullptr, nullptr},
@@ -3661,10 +3662,26 @@
 		report_error(L, "Illegal amount: %i, must be >= 0 and <= %i", amount, max_amount);
 
 	field->set_resources(res, amount);
+	//in editor, reset also initial amount
+	EditorGameBase & egbase = get_egbase(L);
+	upcast(Game, game, &egbase);
+	if (!game) {
+		field->set_initial_res_amount(amount);
+	}
 
 	return 0;
 }
-
+/* RST
+	.. attribute:: initial_resource_amount
+
+		(RO) Starting value of resource. It is set be resource_amount
+
+		:see also: :attr:`resource`
+*/
+int LuaField::get_initial_resource_amount(lua_State * L) {
+	lua_pushuint32(L, fcoords(L).field->get_initial_res_amount());
+	return 1;
+}
 /* RST
 	.. attribute:: immovable
 

=== modified file 'src/scripting/lua_map.h'
--- src/scripting/lua_map.h	2014-09-14 11:31:58 +0000
+++ src/scripting/lua_map.h	2014-12-03 20:14:08 +0000
@@ -942,6 +942,7 @@
 	int set_resource(lua_State *);
 	int get_resource_amount(lua_State *);
 	int set_resource_amount(lua_State *);
+	int get_initial_resource_amount(lua_State *);
 	int get_claimers(lua_State *);
 	int get_owner(lua_State *);
 

=== modified file 'src/wui/game_debug_ui.cc'
--- src/wui/game_debug_ui.cc	2014-11-30 18:49:38 +0000
+++ src/wui/game_debug_ui.cc	2014-12-03 20:14:08 +0000
@@ -360,12 +360,12 @@
 	{
 		Widelands::ResourceIndex ridx = m_coords.field->get_resources();
 		int ramount = m_coords.field->get_resources_amount();
-		int startingAmount = m_coords.field->get_starting_res_amount();
+		int initial_amount = m_coords.field->get_initial_res_amount();
 
 		str += (boost::format("Resource: %s\n")
 				  % ibase().egbase().world().get_resource(ridx)->name().c_str()).str();
 
-		str += (boost::format("  Amount: %i/%i\n") % ramount % startingAmount).str();
+		str += (boost::format("  Amount: %i/%i\n") % ramount % initial_amount).str();
 	}
 
 	m_ui_field.set_text(str.c_str());

=== modified file 'test/maps/lua_testsuite.wmf/scripting/cfield.lua'
--- test/maps/lua_testsuite.wmf/scripting/cfield.lua	2014-01-12 19:06:22 +0000
+++ test/maps/lua_testsuite.wmf/scripting/cfield.lua	2014-12-03 20:14:08 +0000
@@ -1,5 +1,5 @@
 -- =======================================================================
---                                Field test                               
+--                                Field test
 -- =======================================================================
 field_tests = lunit.TestCase("Field access")
 function field_tests:test_access()
@@ -30,8 +30,8 @@
    assert_error("y is negativ", function() map:get_field(25, -12) end)
 end
 function field_tests:test_direct_change_impossible()
-   assert_error("c.x should be read only", function() c.x = 12 end) 
-   assert_error("c.y should be read only", function() c.y = 12 end) 
+   assert_error("c.x should be read only", function() c.x = 12 end)
+   assert_error("c.y should be read only", function() c.y = 12 end)
 end
 function field_tests:test_hash()
    assert_equal("25_40", map:get_field(25,40).__hash)
@@ -158,7 +158,7 @@
 end
 
 -- ==========
--- Resources 
+-- Resources
 -- ==========
 field_resources_tests = lunit.TestCase("Field resources test")
 function field_resources_tests:setup()
@@ -172,7 +172,7 @@
    self.f.terr = self._terr
    self.f.terd = self._terd
    self.f.resource = self._res
-   self.f.resource_amount = self._amount 
+   self.f.resource_amount = self._amount
 end
 
 function field_resources_tests:test_set_resource_amount()
@@ -198,7 +198,7 @@
 end
 
 -- ==========
--- Fieldcaps 
+-- Fieldcaps
 -- ==========
 field_caps_tests = lunit.TestCase("Field caps tests")
 
@@ -305,7 +305,7 @@
    local s2 = p2:place_building("sentry", f, false, true)
    s2:set_soldiers({0,0,0,0}, 1)
    self.pis[#self.pis + 1] = f.brn.immovable
-   
+
    -- Still owned by first player, second player has same military influence
    local o = map:get_field(10,10).claimers
    assert_equal(2, #o)
@@ -349,4 +349,3 @@
    assert_equal(0, #f.claimers)
    assert_equal(player1, map:get_field(15,37).owner)
 end
-

=== modified file 'test/maps/lua_testsuite.wmf/scripting/efield.lua'
--- test/maps/lua_testsuite.wmf/scripting/efield.lua	2014-01-12 19:06:22 +0000
+++ test/maps/lua_testsuite.wmf/scripting/efield.lua	2014-12-03 20:14:08 +0000
@@ -1,9 +1,9 @@
 -- ==================================================
--- Tests for Fields that are only useful in the Editor 
+-- Tests for Fields that are only useful in the Editor
 -- ==================================================
 
 function field_resources_tests:test_default_resource_in_editor()
-   -- The editor doesn't bother to set the default resources on the map. 
+   -- The editor doesn't bother to set the default resources on the map.
    -- The user might want to overwrite them anyways. It therefore sets
    -- the resource of all fields to the first in the world and the amount
    -- to zero.
@@ -11,5 +11,13 @@
    assert_equal(0, self.f.resource_amount)
 end
 
-
-
+function field_resources_tests:test_initial_resource_in_editor()
+	-- making sure that (set_)	resource_amount sets also starting resource in the editor
+	assert_equal("coal", self.f.resource)
+	self.f.resource_amount=10
+	assert_equal(self.f.initial_resource_amount, self.f.resource_amount)
+	assert_equal(10, self.f.initial_resource_amount)
+	self.f.resource_amount=20
+	assert_equal(self.f.initial_resource_amount, self.f.resource_amount)
+	assert_equal(20, self.f.initial_resource_amount)
+end

=== modified file 'test/maps/lua_testsuite.wmf/scripting/gfield.lua'
--- test/maps/lua_testsuite.wmf/scripting/gfield.lua	2014-01-12 19:06:22 +0000
+++ test/maps/lua_testsuite.wmf/scripting/gfield.lua	2014-12-03 20:14:08 +0000
@@ -1,5 +1,5 @@
 -- ==================================================
--- Tests for Fields that are only useful in the Game 
+-- Tests for Fields that are only useful in the Game
 -- ==================================================
 
 function field_resources_tests:test_default_resource_in_game()
@@ -11,4 +11,11 @@
 
 
 
-
+function field_resources_tests:test_initial_resources_in_game()
+   -- Changing a resource in game should not change the
+   -- initial_resources.
+   assert_equal("water", self.f.resource)
+   assert_equal(5, self.f.initial_resource_amount)
+   self.f.resource_amount = 1
+   assert_equal(5, self.f.initial_resource_amount)
+end