widelands-dev team mailing list archive

[hessenfarmer <stephan-lutz@xxxxxx>]

In the end I would leave it to your decision. However I still would think it to be worth having it. 
The glacier map bug is an old one but the second bug reported by Toni isn't. As the behaviour detected by Toni (immovables affecting the field count) is affecting a lot of maps where the values of "fields to be conquered" are mostly wrong (e.g crater) it will give some great benefit because the effect of the bug is bigger than only the glacier lake / oasis triangle effect.

The changes aren't that big as they appear in my opinion. 
the c++ change is barely more than a copy of an existing property of a field (that's true because it is the way I have managed to do so - my knowledge/experience of c++ is very limited).
and the lua change is definitely only in one function which delivers its output formatted in the same way as its predecessor. 
so the risk in lua is to have wrong values of "fields to conquer" again for different maps currently not tested. This is no change to the previous situation where the values were wrong for the maps tested.
the risk in c++ is to have a failing method/interface this risk is mitigated by it being a full copy of an existing method/interface. 

So if you value risk against benefit you might rethink about having it in b20. But still your decision, I just wanted to made my point clear.

I have answered your review below.

Diff comments:

> === modified file 'data/scripting/win_conditions/territorial_functions.lua'
> --- data/scripting/win_conditions/territorial_functions.lua	2019-02-12 17:30:21 +0000
> +++ data/scripting/win_conditions/territorial_functions.lua	2019-02-17 19:27:39 +0000
> @@ -14,24 +14,88 @@
>  local wc_had_territory = _"%1$s had %2$3.0f%% of the land (%3$i of %4$i)."
>  
>  -- RST
> --- .. function:: get_buildable_fields()
> ---
> ---    Collects all fields that are buildable
> ---
> ---    :returns: a table with the map's buildable fields
> ---
> -function get_buildable_fields()
> +-- .. function:: get_valuable_fields()
> +--
> +--    Collects all fields that are valuable
> +--
> +--    :returns: a table with the map's valuable fields
> +--
> +function get_valuable_fields()
>     local fields = {}
> +   local check = {}
>     local map = wl.Game().map
> -   for x=0, map.width-1 do
> -      for y=0, map.height-1 do
> -         local f = map:get_field(x,y)
> -         if f.buildable then
> -            table.insert(fields, f)
> -         end
> -      end
> -   end
> -   return fields
> +   local plrs = wl.Game().players
> +   for idx, player in ipairs(plrs) do
> +      local sf = map.player_slots[idx].starting_field
> +   -- initialize the fields table and the check area with the regions around the starting field of each Player
> +      for idx, fg in ipairs(sf:region(9)) do
> +         local index = fg.x * 1000 + fg.y
> +         fields[index] = fg
> +         check[index] = fg
> +      end
> +   end
> +   if map.allows_seafaring == true then
> +      -- TODO: this check is a little bit consuming, if possible we should read the port spaces from the map

I would love if somebody could do that. Because if I would try to do this this might indeed be risky, as I can't copy and paste this.

> +      for x=0, map.width-1 do
> +         for y=0, map.height-1 do
> +            local f = map:get_field(x,y)
> +            if f:has_max_caps("port") then
> +               for idx, fg in ipairs(f:region(5)) do
> +                  local index = fg.x * 1000 + fg.y
> +                  fields[index] = fg
> +                  check[index] = fg
> +               end
> +            end
> +         end
> +      end
> +   end
> +   local loop = true
> +   while loop do
> +      loop = false
> +      local new = {}
> +      -- checking the check region for buildcaps and add fields that can be conquered
> +      for idx, f in pairs(check) do 
> +         if f:has_max_caps("big") then
> +            local radius = f:region(9)
> +            for idx, fg in ipairs(radius) do
> +               local index = fg.x * 1000 + fg.y
> +               if fields[index] == nil and check[index] == nil and fg:has_max_caps("walkable") then 
> +                  -- if new buildable fields are discovered, add them to the fields table and mark them for checking next loop
> +                  new[index] = fg
> +                  fields[index] = fg
> +                  loop = true
> +               end
> +            end
> +         elseif f:has_max_caps("medium") then
> +            local radius = f:region(7)

so the solution would be to determine radius first by evaluating the Caps and just have the for loop once, correct?
I will implement this tonight as it saves a lot of lines

> +            for idx, fg in ipairs(radius) do
> +               local index = fg.x * 1000 + fg.y

see comment no 1. My c++ knowledge is so limited I would feel uncomfortable doing it by myself

> +               if fields[index] == nil and check[index] == nil and fg:has_max_caps("walkable") then 
> +                  new[index] = fg
> +                  fields[index] = fg
> +                  loop = true
> +               end
> +            end
> +         elseif f:has_max_caps("small") then
> +            local radius = f:region(5)
> +            for idx, fg in ipairs(radius) do
> +               local index = fg.x * 1000 + fg.y
> +               if fields[index] == nil and check[index] == nil and fg:has_max_caps("walkable") then 
> +                  new[index] = fg
> +                  fields[index] = fg
> +                  loop = true
> +               end
> +            end
> +         end
> +      end
> +      check = new
> +   end
> +   local result = {}
> +   -- as our fields table is not continuosly indexed we need to build a properly indexed table
> +   for idx,f in pairs(fields) do
> +      table.insert(result, f)
> +   end
> +   return result
>  end
>  
>  -- RST
> 
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc	2018-12-14 15:55:40 +0000
> +++ src/scripting/lua_map.cc	2019-02-17 19:27:39 +0000
> @@ -6497,6 +6497,49 @@
>  	return 1;
>  }
>  
> +/* RST
> +   .. method:: has_max_caps(capname)
> +
> +      Returns :const:`true` if the field has this maximum caps (not taking immovables into account) associated
> +      with it, otherwise returns false.
> +
> +      :arg capname: can be either of
> +
> +      * :const:`small`: Can a small building be build here?

agreed needs to be fixed for the has_caps function as well though, as it is copied from there.
Will fix this tonight

> +      * :const:`medium`: Can a medium building be build here?
> +      * :const:`big`: Can a big building be build here?
> +      * :const:`mine`: Can a mine be build here?
> +      * :const:`port`: Can a port be build here?
> +      * :const:`flag`: Can a flag be build here?
> +      * :const:`walkable`: Is this field passable for walking bobs?
> +      * :const:`swimmable`: Is this field passable for swimming bobs?
> +*/
> +int LuaField::has_max_caps(lua_State* L) {
> +	FCoords f = fcoords(L);
> +	std::string query = luaL_checkstring(L, 2);
> +
> +	if (query == "walkable")
> +		lua_pushboolean(L, f.field->maxcaps() & MOVECAPS_WALK);
> +	else if (query == "swimmable")
> +		lua_pushboolean(L, f.field->maxcaps() & MOVECAPS_SWIM);
> +	else if (query == "small")
> +		lua_pushboolean(L, f.field->maxcaps() & BUILDCAPS_SMALL);
> +	else if (query == "medium")
> +		lua_pushboolean(L, f.field->maxcaps() & BUILDCAPS_MEDIUM);
> +	else if (query == "big")
> +		lua_pushboolean(L, (f.field->maxcaps() & BUILDCAPS_BIG) == BUILDCAPS_BIG);
> +	else if (query == "port") {
> +		lua_pushboolean(
> +		   L, (f.field->maxcaps() & BUILDCAPS_PORT) && get_egbase(L).map().is_port_space(f));
> +	} else if (query == "mine")
> +		lua_pushboolean(L, f.field->maxcaps() & BUILDCAPS_MINE);
> +	else if (query == "flag")
> +		lua_pushboolean(L, f.field->maxcaps() & BUILDCAPS_FLAG);
> +	else
> +		report_error(L, "Unknown caps queried: %s!", query.c_str());
> +
> +	return 1;
> +}
>  /*
>   ==========================================================
>   C METHODS


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1810062-territorial-calculations/+merge/361366
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1810062-territorial-calculations.