Review: Approve
A very good change! I reviewed and have 5 inline diff comments which are mostly
nits. I also addressed your NOCOM at the very end.
Diff comments:
>
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc 2016-12-28 21:59:44 +0000
> +++ src/scripting/lua_map.cc 2017-01-14 19:11:16 +0000
> @@ -663,6 +599,120 @@
> }
> NEVER_HERE();
> }
> +
> +// This is used for get_ware/workers functions, when argument can be
> +// 'all', single ware/worker, or array of ware/workers
> +void parse_wares_workers_list(lua_State* L,
I think this interface works. If you cant to make it slightly clearer, you
could return an enum class RequestedWareWorker { kAll, kSingle, kList }; and
still pass 'single_item', 'item_list' and 'all_items'. It might make the
callsites easier to understand (a single switch), but it sorta hides that some
returned values are only valid some times. Your call.
> + const TribeDescr& tribe,
> + Widelands::DescriptionIndex* single_item,
> + std::vector<Widelands::DescriptionIndex>&
> item_list,
pass 'item_list' by pointer to indicate that you mutate it. generally, you
should never use non-const refs in function arguments as of making it clear at
the call site that the item is modified.
> + bool* all_items,
> + bool is_ware) {
> + int32_t nargs = lua_gettop(L);
> + if (nargs != 2) {
> + report_error(L, "One argument is required for
> produced_wares_count()");
> + }
> +
> + /* If we have single string as an argument */
> + if (lua_isstring(L, 2)) {
> +
> + std::string what = luaL_checkstring(L, -1);
> + if (what != "all") {
> + // This is name of ware/worker
> + if (is_ware) {
> + *single_item = tribe.ware_index(what);
> + } else {
> + *single_item = tribe.worker_index(what);
> + }
> + if (*single_item == INVALID_INDEX) {
> + report_error(L, "Unrecognized ware/worker %s",
> what.c_str());
> + }
> + } else {
> + // we collect all wares/workers and push it to item_list
> + *all_items = true;
> + if (is_ware) {
> + for (auto idx : tribe.wares()) {
> + item_list.push_back(idx);
> + }
> + } else {
> + for (auto idx : tribe.workers()) {
> + item_list.push_back(idx);
> + }
> + }
> + }
> + } else {
> + /* we got array of names, and so fill the indexes into
> item_list */
> + luaL_checktype(L, 2, LUA_TTABLE);
> + lua_pushnil(L);
> + while (lua_next(L, 2) != 0) {
> + std::string what = luaL_checkstring(L, -1);
> + lua_pop(L, 1);
> + if (is_ware) {
> + item_list.push_back(tribe.ware_index(what));
> + } else {
> + item_list.push_back(tribe.worker_index(what));
> + }
> + if (item_list.back() == INVALID_INDEX) {
> + report_error(L, "Unrecognized ware %s",
> what.c_str());
> + }
> + }
> + }
> + assert((*single_item == INVALID_INDEX) != item_list.empty());
> +}
> +
> +// Very similar to above function, but expects numbers for every received
> ware/worker
> +void parse_wares_workers_counted(lua_State* L,
> + const TribeDescr& tribe,
> + WaresWorkersMap& ware_workers_list,
> + bool is_ware) {
> + int32_t nargs = lua_gettop(L);
> + if (nargs != 2 && nargs != 3) {
> + report_error(L, "Wrong number of arguments to set ware/worker
> method!");
> + }
> +
> + // We either received, two items string,int:
> + if (nargs == 3) {
> +
> + if (is_ware) {
> + if (tribe.ware_index(luaL_checkstring(L, 2)) ==
> INVALID_INDEX) {
> + report_error(L, "Illegal ware %s",
> luaL_checkstring(L, 2));
> + }
> + ware_workers_list.insert(
> + WareAmount(tribe.ware_index(luaL_checkstring(L, 2)),
> luaL_checkuint32(L, 3)));
> + } else {
> + if (tribe.worker_index(luaL_checkstring(L, 2)) ==
> INVALID_INDEX) {
> + report_error(L, "Illegal worker %s",
> luaL_checkstring(L, 2));
> + }
> + ware_workers_list.insert(
> + WorkerAmount(tribe.worker_index(luaL_checkstring(L,
> 2)), luaL_checkuint32(L, 3)));
> + }
> + } else {
> + // or we got a table with name:quantity
> + luaL_checktype(L, 2, LUA_TTABLE);
> + lua_pushnil(L);
> + while (lua_next(L, 2) != 0) {
> + if (is_ware) {
> + if (tribe.ware_index(luaL_checkstring(L, -2))
> == INVALID_INDEX) {
> + report_error(L, "Illegal ware %s",
> luaL_checkstring(L, -2));
> + }
> + } else {
> + if (tribe.worker_index(luaL_checkstring(L, -2))
> == INVALID_INDEX) {
> + report_error(L, "Illegal worker %s",
> luaL_checkstring(L, -2));
> + }
> + }
> +
> + if (is_ware) {
> + ware_workers_list.insert(
> +
> WareAmount(tribe.ware_index(luaL_checkstring(L, -2)), luaL_checkuint32(L,
> -1)));
> + } else {
> + ware_workers_list.insert(
> +
> WorkerAmount(tribe.worker_index(luaL_checkstring(L, -2)), luaL_checkuint32(L,
> -1)));
> + }
> + lua_pop(L, 1);
> + }
> + }
> +}
> +
> #undef CAST_TO_LUA
>
> /*
> @@ -3550,6 +3601,11 @@
> f->add_ware(egbase, ware);
> }
> }
> + if (sp.second > 0) { // NOCOM temporarily?
I suggest keeping this, but for debug only:
#ifndef NDEBUG
code
#endif
> + c_wares = count_wares_on_flag_(*f, tribes);
> + assert(c_wares.count(sp.first) == 1);
> + assert(c_wares[sp.first] = sp.second);
> + }
> }
> return 0;
> }
> @@ -4143,32 +4248,48 @@
> ProductionSite* ps = get(L, get_egbase(L));
> const TribeDescr& tribe = ps->owner().tribe();
>
> - bool return_number = false;
> - WaresSet wares_set = parse_get_wares_arguments(L, tribe,
> &return_number);
> + // Parsing the arguments, result will be single index or list of indexes
> + DescriptionIndex ware_index = INVALID_INDEX;
> + std::vector<DescriptionIndex> ware_list;
> + bool all_items = false;
> + parse_wares_workers_list(L, tribe, &ware_index, ware_list, &all_items,
> true);
>
> + // Here we identify wares that are allowed as input for the site
> WaresSet valid_wares;
> for (const auto& input_ware : ps->descr().inputs()) {
> valid_wares.insert(input_ware.first);
> }
>
> - if (wares_set.size() == tribe.get_nrwares()) // Want all returned
> - wares_set = valid_wares;
> -
> - if (!return_number)
> - lua_newtable(L);
> -
> - for (const Widelands::DescriptionIndex& ware : wares_set) {
> + // This indicates that all wares are needed, but only some of them are
> allowed
> + if (all_items) {
> + ware_list.clear();
> + for (auto item : valid_wares) {
> + ware_list.push_back(item);
> + }
> + }
> +
> + // We return single integer for single ware
> + if (ware_index != INVALID_INDEX) {
> uint32_t cnt = 0;
> - if (valid_wares.count(ware))
> - cnt = ps->waresqueue(ware).get_filled();
> -
> - if (return_number) { // this is the only thing the customer
> wants to know
> - lua_pushuint32(L, cnt);
> - break;
> - } else {
> + if (valid_wares.count(ware_index)) {
> + cnt = ps->waresqueue(ware_index).get_filled();
> + }
> + lua_pushuint32(L, cnt);
> + // else //{ NOCOM is this needed, or should be ignored quietly?
As it is written right now, for consitentcy with the 'ware_list' case below, I
would say this should probably return 0.
> + // throw LuaError((boost::format("Ware '%s' isn't allowed for
> the productionsite.") %
> + // tribe.get_ware_descr(ware_index)->name()).str());
> + //}
> + } else { // we return table
> + assert(!ware_list.empty());
> + lua_newtable(L);
> + for (const Widelands::DescriptionIndex& ware : ware_list) {
> + uint32_t cnt = 0;
> + if (valid_wares.count(ware)) {
> + cnt = ps->waresqueue(ware).get_filled();
> + }
> lua_pushstring(L, tribe.get_ware_descr(ware)->name());
> lua_pushuint32(L, cnt);
> - lua_rawset(L, -3);
> + lua_settable(L, -3);
> }
> }
> return 1;
>
> === modified file 'test/maps/lua_testsuite.wmf/scripting/gplayer.lua'
> --- test/maps/lua_testsuite.wmf/scripting/gplayer.lua 2015-10-31 12:11:44
> +0000
> +++ test/maps/lua_testsuite.wmf/scripting/gplayer.lua 2017-01-14 19:11:16
> +0000
> @@ -144,3 +144,22 @@
> b1.fields[1].brn.immovable:remove()
> assert_equal(1, #player1:get_buildings("barbarians_lumberjacks_hut"))
> end
> +-- ================
> +-- Players production statistics
> +-- ================
> +--include "scripting/coroutine.lua" -- for sleep NOCOM, it does not work
lua_testsuite.wmf does not run coroutines, I think that is why it is not
working. It expects all tests to be executed immediately when created, so no
sleep is possible. I suggest you add a test in a new file
test/maps/plain.wmf/scripting/test_produced_wares_count.lua. There you can do
what you want here.
> +
> +player_production_statistics = lunit.TestCase("Players production
> statistics")
> +function player_building_access:test_single()
> + self.bs = {
> + player1:place_building("barbarians_lumberjacks_hut",
> map:get_field(10,10)),
> + player1:place_building("barbarians_lumberjacks_hut",
> map:get_field(13,10)),
> + player1:place_building("barbarians_quarry", map:get_field(8,10)),
> + }
> + --old_logs = player1:get_produced_wares_count('log')
> + assert_equal((player1:get_produced_wares_count('all'))['log'],
> player1:get_produced_wares_count('log'))
> + -- while player1:get_produced_wares_count('log') == old_logs do
> + -- NOCOM - I cannot make it work, sleep does not work
> + --sleep(10)
> + --end
> +end
--
https://code.launchpad.net/~widelands-dev/widelands/production_statistics/+merge/314283
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/production_statistics.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
