Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
Review: Approve You missed one rename :). I did it for you and merged. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
Review: Needs Fixing 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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
@SirVer - I changed it. Test works as well. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1281823 into lp:widelands has been updated. Status: Needs review = Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
Review: Approve 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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
@SirVer - I changed the test as you asked -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
great. only the rename from starting_ to initial_ and this is good to go. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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 :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
Review: Needs Fixing
You missed some :). See inline comments.
Diff comments:
=== modified file 'src/editor/map_generator.cc'
--- src/editor/map_generator.cc 2014-09-20 09:37:47 +
+++ src/editor/map_generator.cc 2014-12-01 21:50:51 +
@@ -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.cc2014-09-20
09:37:47 +
+++ src/editor/tools/editor_decrease_resources_tool.cc2014-12-01
21:50:51 +
@@ -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.cc2014-09-20
09:37:47 +
+++ src/editor/tools/editor_increase_resources_tool.cc2014-12-01
21:50:51 +
@@ -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
+
+++ src/editor/tools/editor_set_resources_tool.cc 2014-12-01 21:50:51
+
@@ -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 +
+++ src/logic/field.h 2014-12-01 21:50:51 +
@@ -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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
@SirVer so should I remove :see also: :attr:`resource` line? There are more such/similar lines in the lua_map.cc though. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
It works. It is visible in wiki and link also works -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
Review: Approve
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?).
Diff comments:
=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc 2014-10-27 10:14:10 +
+++ src/scripting/lua_map.cc 2014-11-26 20:51:03 +
@@ -3501,6 +3501,7 @@
PROP_RO(LuaField, viewpoint_y),
PROP_RW(LuaField, resource),
PROP_RW(LuaField, resource_amount),
+ PROP_RO(LuaField, starting_resource_amount),
that is a useful addition for scripters in any way, so I support it being added
:).
PROP_RO(LuaField, claimers),
PROP_RO(LuaField, owner),
{nullptr, nullptr, nullptr},
@@ -3660,10 +3661,21 @@
report_error(L, Illegal amount: %i, must be = 0 and = %i,
amount, max_amount);
field-set_resources(res, amount);
+ field-set_starting_res_amount(amount);
return 0;
}
-
+/* RST
+ .. attribute:: starting_resource_amount
+
+ (RO) Starting value of resource. It is set be resource_amount
+
+ :see also: :attr:`resource`
this tag is not supported I think. This is layouted using http://sphinx-doc.org/
+*/
+int LuaField::get_starting_resource_amount(lua_State * L) {
+ lua_pushuint32(L, fcoords(L).field-get_starting_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 +
+++ src/scripting/lua_map.h 2014-11-26 20:51:03 +
@@ -942,6 +942,7 @@
int set_resource(lua_State *);
int get_resource_amount(lua_State *);
int set_resource_amount(lua_State *);
+ int get_starting_resource_amount(lua_State *);
int get_claimers(lua_State *);
int get_owner(lua_State *);
=== modified file 'test/maps/lua_testsuite.wmf/scripting/efield.lua'
--- test/maps/lua_testsuite.wmf/scripting/efield.lua 2014-01-12 19:06:22
+
+++ test/maps/lua_testsuite.wmf/scripting/efield.lua 2014-11-26 20:51:03
+
@@ -11,5 +11,13 @@
assert_equal(0, self.f.resource_amount)
end
+function field_resources_tests:test_starting_resource_in_editor()
+ -- making sure that (set_) resource_amount sets also starting
resource
+ assert_equal(coal, self.f.resource)
add a check that the starting_resource_amount is different than the
resource_amount here (i.e. before you set it)
+ self.f.resource_amount=10
+ assert_equal(self.f.starting_resource_amount, self.f.resource_amount)
+end
+
+
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
+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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1281823. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
no problem for me to rename
@SirVer - see answer to one of your comments below in diff.
Diff comments:
=== modified file 'src/scripting/lua_map.cc'
--- src/scripting/lua_map.cc 2014-10-27 10:14:10 +
+++ src/scripting/lua_map.cc 2014-11-26 20:51:03 +
@@ -3501,6 +3501,7 @@
PROP_RO(LuaField, viewpoint_y),
PROP_RW(LuaField, resource),
PROP_RW(LuaField, resource_amount),
+ PROP_RO(LuaField, starting_resource_amount),
PROP_RO(LuaField, claimers),
PROP_RO(LuaField, owner),
{nullptr, nullptr, nullptr},
@@ -3660,10 +3661,21 @@
report_error(L, Illegal amount: %i, must be = 0 and = %i,
amount, max_amount);
field-set_resources(res, amount);
+ field-set_starting_res_amount(amount);
return 0;
}
-
+/* RST
+ .. attribute:: starting_resource_amount
+
+ (RO) Starting value of resource. It is set be resource_amount
+
+ :see also: :attr:`resource`
+*/
+int LuaField::get_starting_resource_amount(lua_State * L) {
+ lua_pushuint32(L, fcoords(L).field-get_starting_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 +
+++ src/scripting/lua_map.h 2014-11-26 20:51:03 +
@@ -942,6 +942,7 @@
int set_resource(lua_State *);
int get_resource_amount(lua_State *);
int set_resource_amount(lua_State *);
+ int get_starting_resource_amount(lua_State *);
int get_claimers(lua_State *);
int get_owner(lua_State *);
=== modified file 'test/maps/lua_testsuite.wmf/scripting/efield.lua'
--- test/maps/lua_testsuite.wmf/scripting/efield.lua 2014-01-12 19:06:22
+
+++ test/maps/lua_testsuite.wmf/scripting/efield.lua 2014-11-26 20:51:03
+
@@ -11,5 +11,13 @@
assert_equal(0, self.f.resource_amount)
end
+function field_resources_tests:test_starting_resource_in_editor()
+ -- making sure that (set_) resource_amount sets also starting
resource
+ assert_equal(coal, self.f.resource)
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.
+ self.f.resource_amount=10
+ assert_equal(self.f.starting_resource_amount, self.f.resource_amount)
+end
+
+
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1281823.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
TiborB has proposed merging lp:~widelands-dev/widelands/bug-1281823 into lp:widelands. Requested reviews: Widelands Developers (widelands-dev) Related bugs: Bug #1281823 in widelands: Setting resources via LUA API broken in Editor https://bugs.launchpad.net/widelands/+bug/1281823 For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 The fix is very trivial - one line was added -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1281823 into lp:widelands. === modified file 'src/scripting/lua_map.cc' --- src/scripting/lua_map.cc 2014-10-27 10:14:10 + +++ src/scripting/lua_map.cc 2014-11-25 19:55:23 + @@ -3660,6 +3660,7 @@ report_error(L, Illegal amount: %i, must be = 0 and = %i, amount, max_amount); field-set_resources(res, amount); + field-set_starting_res_amount(amount); return 0; } ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1281823 into lp:widelands
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 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1281823/+merge/242837 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1281823 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
