widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

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.