widelands-dev team mailing list archive

[hessenfarmer <stephan-lutz@xxxxxx>]

Thanks for the quick answer. 
First I like to apologize if I am making silly or uninformed comments. I am still trying to learn and understand. 
Í have answered your comments in the code below.

However I overlooked one major fact yesterday. (probably I was too tired;-))

the real problem is that I made my NOCOMS in the wrong part of the code. 
the mentioned Frisian space consumers are not handled there as they have defined production hints and they are handled earlier in the code.
they will be handled in the else block beginning from 
https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L2817

in this part of the code there is no separation to rangers or trees at all. So from my point of view we need to copy the separation part of space consumers into a new 
elseif (bo.is(BuildingAttribute::kSpaceConsumer)) block beginning in front of line 2817.

can do this tonight if you want.

Unfortunately it seems that there is some problem with appveyor though. 

Diff comments:

> 
> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc	2018-04-29 09:20:29 +0000
> +++ src/ai/defaultai.cc	2018-06-03 20:52:04 +0000
> @@ -2867,16 +2883,21 @@
>  
>  						if (bo.is(BuildingAttribute::kSpaceConsumer)) {
>  							// we dont like trees nearby
> +							// NOCOM this means we must have 30 trees in a radius of 6 to get a negative value
> +							// NOCOM note: the radius to calculate all nearby values is hardcoded to 6 (kProductionArea)
>  							prio += 1 - bf->trees_nearby / 15;

to allow some trees I would propose a value of /3 or /2. If we use generic algorithm here in the future as well. we should ensure a range of (0 to probably 15)

>  							// we attempt to cluster space consumers together
> +							// NOCOM not sure whther rangers are considered here as well cause they have also the
> +							// NOCOM space consumer = true AI hint.
>  							prio += bf->space_consumers_nearby * 2;

Thanks for the hint. I am still struggling to understand and remember everything properly. So we need nothing to do here.

>  							// and be far from rangers
> +							// NOCOM as rangers have a very big working area we might be still to close to them

Ok now I found that for the building influences we already use a radius of 8 (kProductionArea +2).
https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L1570

As rangers have a working radius of 5 this should be ok. 
as we should be safe not placing space consumers in the vicinity of rangers we should ensure Rangers are not placed in the vicinity of space consumers. This is done in.
https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L2813

either we should increase the weighting factor for space consumers nearby to probably 10 or we should use the same weighing factor (using the same military number) as in 
https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L2897
I would vote for the second solution

>  							prio += 1 -
>  							        bf->rangers_nearby *
>  							           std::abs(management_data.get_military_number_at(102)) / 5;
>  						} else {
>  							// leave some free space between them
> -							prio -= bf->producers_nearby.at(bo.outputs.at(0)) *
> +							prio -= bf->collecting_producers_nearby.at(bo.collected_map_resource) *
>  							        std::abs(management_data.get_military_number_at(108)) / 5;
>  						}
>  
> @@ -5714,6 +5736,9 @@
>  			}
>  			assert(bo.stocklevel_count < std::numeric_limits<uint32_t>::max());
>  		} else if (!bo.outputs.empty()) {
> +			// NOCOM from what I understand from the definition of calculate_stocklevel
> +			// it is expecting a ware index or a worker index but I believe we are delivering a building index here
> +			// might lead to nonsense results. 

ok did not recognize this. seems to be ok. Thanks for the explanation. NOCOM can be removed.

>  			bo.stocklevel_count = calculate_stocklevel(bo, what);
>  		} else {
>  			bo.stocklevel_count = 0;


-- 
https://code.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/+merge/347166
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/frisian_balancing_with_ai_hints into lp:widelands.