Better detection of hunters, fishers etc. for the AI

[GunChleoc]

At the moment, the AI uses building inputs and outputs to detect whether a building is a hunter or fisher etc. This is brittle - adding "fur" as an output for the Frisian hunter broke the assumptions.

So, we need to parse the tribes' production and worker programs to provide the AI with accurate data about the buildings - let's call it "collects map resource".

[TiborB (tiborb95)]

But please keep in mind that we still need to distinguish:
well
fisher
hunter
woodcutter
farmers
quarry
and perhaps more....

They all collect map resources, but have different code paths in AI...

[hessenfarmer (stephan-lutz)]

The wrong identification is due to the ANDed condition "bo.outputs.size() == 1" just I don't see any reason for this condition as the identification should work without this condition as well. By the way this is used for the fisher as well

[TiborB (tiborb95)]

Not so simple. Next time somebody will invent farm that produces fur and will will have again to tweak this test...

[hessenfarmer (stephan-lutz)]

Not sure whether I was understood correctly.

I am talking about line 724 and following of defaultai.cc. This is the identification of hunters and other relevant buildings. Is there any other test that deals with this topic?

And yes I agree that if new Tribes or new buildings are to be incorporated we will need to check for compatibility with AI code. If there will be an issue we need to change the AI code or skip the affected feature of the tribe.

So the most simplistic solution for b20 might be to skip the fur production of the hunter. This would be my preferred solution if there is any reason for having the mentioned condition in the code that I don't see yet.

@Tibor: do you remember what was the reason for having this condition. Only reason that I can possibly imagine is to ensure that the condition "tribe_->safe_ware_index("meat") == bo.outputs.at(0))" refers to the correct output slot if there could be multiple slots. I don't know exactly whether the slots for the outputs strictly follow the definition in the init.lua of the building.

[GunChleoc (gunchleoc)]

Simply removing the outputs count does not work, because ASAN barfs on it. I'm busy today but I'll investigate this further in the Frisians merge request tomorrow.

[TiborB (tiborb95)]

The AI test for hunter was written so that it fitted characteristics of hunters, maybe it was too strict but did no harm.

If needed I can change the AI so that if first output is "fur" or "meat" -> it is hunter. (and no check of number of outputs)

Better solution would be new aihint "is_hunter", and that would survive any improvement or creation of new hunter types...

[TiborB (tiborb95)]

update - we need a check to make sure there is at least one output, of course...

[GunChleoc (gunchleoc)]

A combination of "is_hunter" + output == "meat" will work fine for Build 20 :)

[TiborB (tiborb95)]

and what with fur as output? If it gets to position 0?

[GunChleoc (gunchleoc)]

What do you mean? Of course, we will have to use the full list of outputs to check if there is meat among them. We won't care about the fur.

I don't like having a check for fur, too much hard-coding. The more hard-coding we have, the more work we get with changes to tribes.

[TiborB (tiborb95)]

I mean if we have "is_hunter", why would we need second check for meat?

[hessenfarmer (stephan-lutz)]

I would vote for the "is_hunter" AI hint and remove the hardcoded check which building is the hunter. In my opinion any other check is not necessary in this case.

changes to the other tribes would be minimal as well.

if this is not possible we should simply check for produces meat (amongst possibly others) and has no inputs.

[GunChleoc (gunchleoc)]

I am adding a generic AI hint "collects_ware_from_map", and the text against the output is done when the tribes are loaded, just to make sure that there is no nonsense.

I am also replacing logproducer, graniteproducer and mines_water with this and doing the same type of string compare for all of them.

[GunChleoc (gunchleoc)]

Whoever is working on the new branch just now, please stop. I said I would take care of it, so somebody else programming the same thing without telling me will give us 2 solutions to the same problem.

[hessenfarmer (stephan-lutz)]

Well,
What is the difference between this hint and the check for input = 0?
As we need to identify several special buildings in AI (which is asserted and fails for the Frisian hunter) I would prefer to have the hint pointing to the relevant building directly. this would give some flexibility for tribe design and should ensure correct AI behaviour.

[TiborB (tiborb95)]

That branch is mine and it is done&working. But we can scrap it, no big harm is done, it was quite trivial...

I am not sure what exactly you are doing, but we will see, once the code is available

[hessenfarmer (stephan-lutz)]

@GunChleoc
What is the rational behind your proposal? If you implement it this way you should consider the fisher as well?
@all:
We need to ensure that meat is the first output of the hunter. due to line 2737 of defaultai.cc.

I really would recommend just doing a small change to allow the fur production for b20 and to postpone a bigger change to the AI hints for b21. If the thing gets to big for now I would rather skip the feature to produce fur for the Frisian hunter.

[GunChleoc (gunchleoc)]

Well, I did target this bug for Build21 in the first place ;)

What I'd like to have is that we wouldn't need to specify and AI hints for this at all, but that's the future change that this bug was originally about.

We will need a separate bug for line line 2737ff of defaultai.cc. For now, putting meat first will do the trick, but this is very fragile.

[GunChleoc (gunchleoc)]

I have attached a new branch https://code.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints

I am getting crashed/ASAN complaints now and I can't spend any more time on the debugging today. Some vector is out of range, so maybe the AI has problems with the extra output still. If somebody else wants to pick this up, please post here so that we won't work in parallel again.

It's a bit hard to debug, because the emergency save is interfering with the backtrace.

Maybe we should merge the Frisians balancing without the extra fur for now, so that we can get it in?

[TiborB (tiborb95)]

lines like:
 assert(bo.outputs.at(0) == tribe_->safe_ware_index("water"));
can be replaced like:
 assert(bo.outputs.at(0) == tribe_->safe_ware_index(bh.collects_ware_from_map()));
and not repeated every time... or even reworked so that it checks all outputs to make sure at least one matches.

[hessenfarmer (stephan-lutz)]

Ok with the code in your branch I got your concept.

First my general opinion on this. I would be happy to have this in the ai_hints. Reason is that someone could better adopt AI behaviour to the tribes special features. For example the frisians don't have refined log but they use this AI-hint for the brickmaker as the brickmaker provides refined building material as well and should be equally treated by the AI.
Furthermore this solution would avoid ambiguities with other buildings producing the same ware but not collecting it from the map (e.g. Piggeries, aqua farms, etc.)

Some nits about the code:
bo.outputs.at(0)is only used in 9 occurrences where 8 of them are in special functions dealing with the identified buildings (1 time each in well and quarry to separate them from other buildings of the same type, 2 times each in hunter, fisher and lumberjack to separate same type of buildings from each other and search for nearby supporters). As this is already special code for this kind of buildings the relevant ware for this case could be hardcoded in my opinion instead to remove the fragile assertions.

If we don't get this to work we could instead remove the fur from the hunter, but as Tibor already had a working branch with just the small change the deeper reason for the failures should not be in the output definition of the Frisian hunter.

[TiborB (tiborb95)]

Does brickmaker need trees and rangers in vicinity?

[hessenfarmer (stephan-lutz)]

No but the sawmill and the woodhardener don't need it neither.;-)
However the example was not really good as it is no AI-hint where this is defined but in the frisians.lua file the ware "brick" is declared as "refinedlog" for the frisians as are "planks" for example for the empire and the atlanteans.

[hessenfarmer (stephan-lutz)]

By the way I just discovered that AI code identifies kLogRefiner but doesn't do anything other with it than asserting exactly one such building is existent.

[TiborB (tiborb95)]

Oh, I confused logrefiner with ranger.
But if logrefiner is not indeed used, we can get rid of it..

[TiborB (tiborb95)]

BTW I pushed my tweak to frisian_balancing_with_ai_hints branch, please investigate it. Also cleanup is still needed - if you agree this is good direction...

[hessenfarmer (stephan-lutz)]

Just checked it.
I would support the deletion of the assertions.
However I don't know if you got their purpose right. You have to look and deal with the following lines of defaultai.cc in your branch to be able to delete them, cause currently these lines depend on the correct ware being in the correct output position.

2678,2707,2709,2741,2756,2759,2774,2775

Did you investigate the crashes experienced by GunChleoc?

[TiborB (tiborb95)]

There was no crash.

And you are right those lines must be adjusted as well, I will look at it...

[TiborB (tiborb95)]

I added count_producers_nearby() function, please review it.

The similar function can be made for supporters

[hessenfarmer (stephan-lutz)]

As I don't really understand all expressions in C++ doing a proper review is hard for me. Onbly question is: Why should we use a generic function for very specific purposes like in this case. My suggestion would have been to use something like "supporters_nearby.at("ware")" and hardcode the relevant ware for every building as it is already hardcoded with the AI hint. Reason is:
for example we have a hint "collects_ware_from_map = meat" to identify a kHunter so we already Know that a hunter will produce meat. This is asserted by the functions GunChleoc added to productionsites.cc. So if we want to seperate the hunter from other hunter we could punish any other "meat" producer in the vicinity instead of using generic bo.outputs.at(0) which needs to be "meat" anyway. same is for supporting the vicinity of a supporter.
in short I think we could use a hardcoded value in a piece of code which is only reachable by exactly having this hardcoded value.

[TiborB (tiborb95)]

Well, I dont fully understand what you mean, but I now I see more flaws in my approach

F.e. hunter that produces meat + fur: if there is another hunter in vicinity it will be counted twice because of each ware.

Another problem, if there will be farm producing only a meat and a hunter producing only a meat, farm will be counted as well, though it does not make sense (This is present also in current implementation, but just there is no such meat farm)

Another: collected map resource can be only one item, but hunter can collect 2 items, why would we list just one? Perhaps map resource should be just "animal" (even though no such ware exists)

[GunChleoc (gunchleoc)]

Yes, the hunter collects 2 resources now, but in the AI hint we only care about one of them, because we want to find the small building that produces meant and doesn't need any input wares.

The new check whether a building produces an output is no longer needed, because I already check this in ProductionSiteDescr. The asserts were there to make sure that the required ware is in the first position of the outputs. I'll remove both now.

Tibor's changes have fixed the crashes :)

> My suggestion would have been to use something like "supporters_nearby.at("ware")" and hardcode the relevant ware for every building as it is already hardcoded with the AI hint.

I think that's a good idea. I don't know if I can work on this before I leav. Tibor, woulkd sou like to finish this up?

My thinking is to store the ware that we care about as a ware index in the building observer, because we'll be calling it multiple times, and int comparison is more efficient than string comparison.

I think we should also have a count_supporters_nearby function that does the same for supporters - I have marked the relevant liens with NOCOM.

[TiborB (tiborb95)]

But there is also a problem bit downstream in AI. When AI checks produces in vicinity the output looks like
coal producers 1
iron producers 0
meat producerss 1
water producers 1
ax producers 0
fur producer 1

So this information really cannot say whether:

fur and meat producer is the same building...
or
if coal is produced in mine or chalcoal burner...

So this is fragile, but for now it worked. So some new category would be ideal, like suggested map resources, and the stored information would look like:
collect trees 1
collects animals 1
collects stone 1
collects fish
collects fur

But even this is not without flaw, because it does not say whether "collects fur" and "collects meat" is the same building

[TiborB (tiborb95)]

let me update last part of previous comment

Probably best way would be new "collectors nearby" vector, and leave produces nearby" as they are now.

But we would still preserve rule that one building can officially collect only one ware (so for hunter producing fur and meat, we would pretend that it collects only meat)

[hessenfarmer (stephan-lutz)]

@Tibor:
I think in this special case your approach is a bit to widespread. I'll try to explain it in more detail. We need to identify some special buildings cause they need special attention in assigning a build priority this is done for the Hunter in the following code
else if (bo.is(BuildingAttribute::kHunter)) {
if (bf->critters_nearby < 5) {
      continue;
     }

     if (bo.new_building == BuildingNecessity::kForced) {
      prio += 20;
     }

     // Overdue priority here
     prio += bo.primary_priority;

     prio += bf->supporters_nearby.at(bo.outputs.at(0)) * 5;

     prio +=
        (bf->critters_nearby * 3) - 8 - 5 * bf->producers_nearby.at(bo.outputs.at(0));

[TiborB (tiborb95)]

and one step further, if productionsite observer contained new info collected_ware, we would completely ignore actual output(s), but directly check new collectors_nearby vector for particular collected_ware. So collected ware could be completely disconnected from outputs.

[TiborB (tiborb95)]

@hessenfarmer
my latest idea is to change:

prio += bf->supporters_nearby.at(bo.outputs.at(0)) * 5;

to:

prio += bf->collectors_nearby.at(bo.collected_map_ware) * 5;

Where "collected_map_ware" can be existing ware (as proposed now), or just any string saved as integer to ease CPU processing

[hessenfarmer (stephan-lutz)]

Sorry for some reason the comment was posted while not finished so another try

else if (bo.is(BuildingAttribute::kHunter)) {

// kHunter is assigned by defining and checking the AI Hint "collects_ware_from_Map = meat" in the productionsitedescr. it is asserted that a building with that hint really produces the ware//

 if (bf->critters_nearby < 5) { // no sense to build a hunter where there arte no animals
       continue;
      }

     if (bo.new_building == BuildingNecessity::kForced) {
       prio += 20;
      }

     // Overdue priority here
      prio += bo.primary_priority;

     prio += bf->supporters_nearby.at(bo.outputs.at(0)) * 5; // bonus if gamekeepers are near

     prio +=
         (bf->critters_nearby * 3) - 8 - 5 * bf->producers_nearby.at(bo.outputs.at(0));

     //malus if other hunters are near//

the problem with this code is that it counts producers of the first ware in the outputs. This has been valifd for the hunters until now and still is for the Frisian one (meat is the first output in the table fur is the second one. But to have a stable code we need to be sure, to just catch the meat producer any other production of a hunter like fur doesn't matter cause this is just to ensure that there are enough ressources on the map for the building to collect if a new building is built. My suggestion was to change the lines in the following way

     prio += bf->supporters_nearby.at("meat") * 5; // bonus if gamekeepers are near

     prio +=
         (bf->critters_nearby * 3) - 8 - 5 * bf->producers_nearby.at("meat");

     //malus if other hunters are near//

So there is no need to count producers or supporters for all outputs of a Hunter just the meat is of any interest in this case.

The same approach applies to the other special buildings and their primary wares. By the way the empire quarry had 2 produced wares in the pas as well. However as in that code only other quarries of the same tribe are taken into account that never led to a problem.

As you already realized this has currently the flaw that empire piggeries are taken into account as well for the empire hunter. And the mines producing "granite" are taken into account by the quarries code. But so far this has not led to big problems in AI yet and could be targeted for b21 in my opinion.

[hessenfarmer (stephan-lutz)]

Tibor wrote:
@hessenfarmer
 my latest idea is to change:

prio += bf->supporters_nearby.at(bo.outputs.at(0)) * 5;

to:

prio += bf->collectors_nearby.at(bo.collected_map_ware) * 5;

Where "collected_map_ware" can be existing ware (as proposed now), or just any string saved as integer to ease CPU processing

that approach with "bo.collected_map_ware" is ok but we need to stick to the difference between supporters_nearby and producers_nearby cause supporters should deliver a bonus in priority and other producers should deliver a malus.

[TiborB (tiborb95)]

oh, I messed that example, should had been
prio += bf->supporters_nearby.at(bo.collected_map_ware) * 5;

we just need to make sure that production_hint ~ collects_map_ware

but producers_nearby need perhaps to be replaced by
bf->collecting_producers_nearby.at(bo.collected_map_ware)
where collecting_producers_nearby would list producers based on collects_map_ware, and not based on actual output

[hessenfarmer (stephan-lutz)]

prio += bf->supporters_nearby.at(bo.collected_map_ware) * 5;

sounds good to me

what would be the difference between

bf->collecting_producers_nearby.at(bo.collected_map_ware)

and

bf->producers_nearby.at(bo.collected_map_ware)

this new construct would make sense if it would ensure that the piggeries and rockmines would be sorted out for this evaluation and only the real collecting building would be considered. But I don't know how to code this. possible solutions might be to check if a nearby producer has an Collects_ware_from_map AI hint or if inputs are empty

[TiborB (tiborb95)]

collecting_producers_nearby would be populated with buildings that have collect_map_ware (and this will guarantee that such building will be here only once)

producers_nearby is populated based on outputs, and one building will be listed n-times where n is number of outputs

[hessenfarmer (stephan-lutz)]

Perhaps I don't understand your last post properly.

In my understanding this code should deliver the number of let's say hunters in the vicinity of a field where building a new hunter is evaluated.

furthermore I understood that bo.collected_map_ware contains just the single collected ware defined in the hint of the building to be built in the above example this would be "meat".

So currently the producers_nearby will count all buildings already existing on the map that produce "meat". Unfortunately this includes the empire Piggeries.

So if collecting_producers_nearby will count all buildings on the map that have "collects_ware_from_map = "meat"" that will be the correct solution.

[TiborB (tiborb95)]

You are right. We will be using collecting_producers_nearby and it will be 100 % accurate. Producers_nearby might be preserved or removed based on whether it is needed elsewhere.

If you (and Gun) agree with such solution, of course...

[hessenfarmer (stephan-lutz)]

I am pretty sure we still need the producers_nearby for some other parts of the code.

I would be happy to see your code containing this solution.

Thanks for the support and the fruitful discussions.

one little last question do you know where "producers_nearby" is defined?

[TiborB (tiborb95)]

It is member of BuildableField:
https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/ai_help_structs.h#L390

[GunChleoc (gunchleoc)]

#42 sounds good to me - it will be based entirely on the AI hint this way, excluding piggeries etc.

[TiborB (tiborb95)]

OK, going to work on it

[TiborB (tiborb95)]

OK, implemented, it seems to work. I left a printf to print some info if somebody is interested in testing.

[TiborB (tiborb95)]

Also similar situation can be with supporters_nearby.
In case of the hunter only output.at(0) is considered. So if meat gets on position 1 it will not be recognized I think...

See https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L2748

[hessenfarmer (stephan-lutz)]

Yes you already had this in comment #40

bf->supporters_nearby.at(bo.collected_map_ware)

would be correct but only for the special buildings (fisher, hunter, lumberjack)

not every instance of supporters nearby need to be replaced!
from a quick look in the code it seems you replaced to many occurences of the producers_nearby with the new collecting_producers_nearby this should have happen only for the 5 instances in the code for (well, quarry, hunter, fisher and lumberjack)

will try to provide you the lines where it should not have been replaced.

[hessenfarmer (stephan-lutz)]

I wouldn't have used it in line 2801 and 2810 (for the ranger) although currently it doesn't any harm there I think.

we should not use collecting_producers_nearby in lines 2844 and 2845 cause this is not only for the fishbreeders and gamekeepers anymore but also for berry farmers and other frisian buildings.

same for line 2924.

[GunChleoc (gunchleoc)]

The new function looks good to me. I am seeing a new "bo.outputs.at(0)" in the diff though - we should not have any of those left at all when we're done.

[TiborB (tiborb95)]

@ hesenfarmer:
I dont see what is wrong with these lines, can you be more specific?

[TiborB (tiborb95)]

@Gun

do you mean bf->supporters_nearby.at(bo.outputs.at(0)) ?

Yes, these can be converted as well I think....

[TiborB (tiborb95)]

Also I see another room for improvement

producers_nearby can by replaced by productionsites_nearby, which would contain count of buildings by id. So if AI would like to know how many buildings of the type exist in vicinity it would query

bf->productionsites_nearby.at(bo.id)

Partially it tried to do it with

bf->producers_nearby.at(bo.outputs.at(0))

but it would be more straightforward.

But there is only about one occurrence where it would be used now

[hessenfarmer (stephan-lutz)]

@Tibor:
we only defined the attribute "collects_ware_from_map" for 5 special buildings (well, quarry, lumberjack, hunter and fisher) so there are only 5 wares that have a collecting producer (water, granite, log, meat and fish).
line 2801 and 2810 are in a subroutine evaluating the need for a ranger on this field therefore it is ok to use it there.
the lines 2844 and 2845 belong to the programpart that is evaluating any other supporting building (i.e. the building has a "supports production of" AI hint). the problem is that with the frisians this includes at least the berry farm and the clay pit and neither of their producers or collectors has the attribute "collects_ware_from_map". So we need to stick to the old expressions in these lines.
line 2924 is some generic part applicable for all buildings that produce a ware and support the production of another. Again the producers for this wares don't have the collecting attribute.

@Gun this is also the reason why we can't get rid of all instances of "bo.outputs.at(0)" without further changes in the Ai hints.

[hessenfarmer (stephan-lutz)]

the lines with

 bf->supporters_nearby.at(bo.outputs.at(0)

should only be replaced for the code dealing with the following buildings: lumberjack, hunter and fisher.

[TiborB (tiborb95)]

And why can we add "collects_ware_from_map" to those producers that are supported by berry farm and the clay pit and so on.

collects_ware_from_map = "honey" ..... for bee keeper

and so on?

I dont like idea to implement something and implement it only by 50%....

[hessenfarmer (stephan-lutz)]

@ Tibor:

sure we could do that and probably we should do it. Question is when shall we do it? Personally I think changes won't be that big so we could try to do so for b20. However I am currently trying to fix mission empire 04 and Gun is working very hard to fix the remaining bugs for the b20 release. So additional work is not very welcome at the moment I guess.
Anyhow I would let this decision to our chieftain.
If we going for this we should decouple the things from the frisian balancing branch.
So my proposal is getting the currnt state to work properly and after merging we should create a new branch and perhaps a new bug. afterwards we could decide on which build to incorporate this.

Perhaps I should try to learn some C++ though and give it another try to set up a windows development environment. Unfortunately this will not bee any quick help.
 ;-)

[hessenfarmer (stephan-lutz)]

Sorry forgot to express that I fully understand and appreciate your point not to stop halfway down the road with an issue.

Again thanks a lot for your patience and support.

[TiborB (tiborb95)]

There was alternative of "removing fur" - can this be done for b20, and broader change (~this branch) be included afterwards? Len Gun say....
Also this branch should already in current state should be thoroughly tested, I dont trust myself when doing changes here ;)

[hessenfarmer (stephan-lutz)]

ok I did some research in the lua files as well as in the current code.

Current code contains only one instance of bo.outputs.at(0) left -- I have to investigate this further.

Current state of code should work fine if frisian beekeeper and frisian fruit collector get the "collects_ware_from_map" only problem is the relation between claypit and aqua farm. Currently claypit supports production of fish but if we assign the collects_ware_from_map = "fish" attribute to the aqua farm it will be recognized as a fisher now.
if we change the ware to collect and to support to perhaps "pond" we will fail the assertion that the collected map ware will be outputted by the buikding with such AI hint.

Solution might be to check if a kFisher has no inputs in addition to having the AI hint collects = "fish" that would clearly identify only the "real" fishers and solve the issue for the aqua farm.

By this final changes would be minimal,
Any thoughts / opinions?

[hessenfarmer (stephan-lutz)]

Ok just checked the last occurence of the bo.outputs.at(0)

It will be only reached by the frisian beekeeper and fruit collector. So with giving them the collects_ware_from_map attribute it should be save to change this line to collecting_producers_nearby.at(bo.collected_map_ware)

so the additional changes will be minimal. I think

[GunChleoc (gunchleoc)]

Adding a check for empty inputs should be OK for identifying all these types of buildings.

+1 for adding "collects_ware_from_map" to beekeepers before Build20.

I will be on a train today and won't be back at my development machine regularly before August, so there is enough time to do this properly.

[TiborB (tiborb95)]

so I agree that:
frisian beekeeper will support "honey"
frisian fruit collector will support what? (sorry, I am not familiar with relations for frisians)

line https://bazaar.launchpad.net/~widelands-dev/widelands/frisian_balancing_with_ai_hints/view/head:/src/ai/defaultai.cc#L2895 is also for farms and similar sites that should not be grouppped very tightly. Identification by first output is just workaround as we dont have better way right now...

As for fish issue. Is the "pong" actual ware? I see another two approaches:

a)The cleanest solution would be to have two types of fish - wild fish and farmed fish (with affect on consuming productionsites and its production programs)

b) Or just disconnecte "collected ware" from actual wares and create new list of possible collected wares (and have two fishes there).

Both of above proposals means additional work...

[hessenfarmer (stephan-lutz)]

Hi,

beekeeper will collect "honey" and not support anything.
collector will collect "fruit" and not suoport anything.

both wares are supported by the berry farmer.
honey is supported by the farmer as well.

line 2895 is NOT for farms as farms are space consumers the line is in the ELSE branch which means no space consumer with no inputs the only buildings so far in the game with these properties are the two mentioned buildings (beekeeper and collector).

a "pond" is an immovable not a ware these immovables are created by the claypit and used to breed fish in by the aqua farm.

splitting the wares would create a bunch of work which is not in relation to the goal to achieve in my opinion.

disconnect the collected wares from the output of a building would be a very fragile state.

So I am still in favor of checking the fishers for no inputs in addition to the collected ware.

If you want I could pull the branch and make the changes I consider necessary and you could check my changes for correctness as I don't know much about c++?

[TiborB (tiborb95)]

You are right about 2895 line, but I still think it checks for the same buildings, it is quite uncommon that two distinct productionsites has the same output on position 0

And yes, please make your changed directly to the code, it will be better for me...

[hessenfarmer (stephan-lutz)]

Ok I hope I will find time this evening to do this. I need to finish the fix of the empire campaign as well so it might be tomorrow instead.

In my opinion the deeper intention of line 2895 is to ensure, that not to many collecting buildings depleting the same resource are built close to each other. This is to ensure that there are enough wares to collect for the building. Therefore checking for the first output is ok currently as there are no alternative buildings doing similar things (so currently you are right it checks for the same building), but more correctly it would check whether the building collects a resource or not.
If you think of a building producing 2 wares from 1 collected resource (like the Frisian hunter as the starting point of this activity) the check for "collects ware from map" would be more robust than checking for output number 1.

[TiborB (tiborb95)]

re "If you think of a building producing 2 wares from 1 collected resource........ the check for "collects ware from map" would be more robust than checking for output number 1."

No, no matter what order of outputs there will be, as it compares itself to itself it will always match.

But feel free to change it I will not mind...

[hessenfarmer (stephan-lutz)]

Ok made the minimal changes necessary to get this to work. Couldn't check for compilation though.

By this activity I found another issue in the code. I added some NOCOM's to explain it.

Comments? Opinions?

[TiborB (tiborb95)]

As for your comment, the player can be short of minefields and if they are blocked by immovable - it will worse the situation. So this is not only about growth probability on mountains. I believe all productionsites placing immovables are marked as spaceconsumers.

Otherwise looks good to me

[hessenfarmer (stephan-lutz)]

Ok what about leaving the issue like it is for now and analyse it properly in another bug report for b21?

If the code compiles properly I would like you to set a merge request, so I get a working installation file to check and playtest the current changes.

[TiborB (tiborb95)]

Yes, we can..

We can create merge request right now, your changes looks safe to me.

[hessenfarmer (stephan-lutz)]

Ok could you please remove the NOCOM's and create the request.

Thanks a lot. Hopefully I will have some time after b20 to analyse the AI further to find possibilities for improvement.

[TiborB (tiborb95)]

I proposed a merge. If you want, you can modify commit message, because I dont remember exactly what was changes here in this branch...

[hessenfarmer (stephan-lutz)]

Thank you very much will test it as soon as appveyor has finished

[hessenfarmer (stephan-lutz)]

I think this was already solved by the related branch
so I set this to fix released

[GunChleoc (gunchleoc)]

Migrated to https://github.com/widelands/widelands/issues/1707