AI builds Barbarian Weaving Mill on non-seafaring maps

No, no such check is done. I can add it eventually, though it does not seem to be critical issue to me.... And I am not able to read the german forum link to see how bad it is..

This was just confusing the player, because the AI build a barbarian weaving mill but he couldn't because of the needs_seafaring check. This causes a feeling of "the AI can more than i can do, thats unfair", despite a barbarian weaving on wouldn't be use full at all on a non seafaring map.

On the other side considering the needs_seafaring setting would help the AI to save some wares, if saving wares is an important thing for the AI.

Psychologically for a new player, the AI has an advantage here, while in fact the AI has a disadvantage by building a useless building.

Do you already check if seafaring is available before building a shipyard? The new code might go in that place.

BTW we have another open bug about the weaving mill, which might now be obsolete due to the recent AI changes: https://bugs.launchpad.net/widelands/+bug/1642196

AI has own variable seafaring_economy. If it is able to build a port, then this is set to True, and it will build also shipyard. But weaving mill is not dependant on it...

Is it possible that there is discrepancy? allows_seafaring() says NO, but still ports can be built?

Also there is another reason for own variable. Before we have a port, it is useless to build a shipyard...

A barbarian weaving mill is useless when the map does not provide seafaring, because cloth is only used for building barbarian ships.

So the conclusion is:

if tribe = barbarian AND NOT 'able to build a port' then
   'do not build weaving mill'

But reading the config file(s) is maybe the better solution:

if needs_seafaring = true
  forbid this building

Are there more of such restrictions?

BTW I had no idea that weaving mill is not shown in GUI for a player if not seafaring map - I dont play that much. I presume the shipyard is treated the same.... And ports the same, f.e. in cause when there is just one port on the map and player happens to see it...

All shipyards and the barbarian weaving mill have "needs_seafaring=true" in the the config file. Human player can build those only if allows_seafaring() is true.
Ports are treated different: They don´t have "needs_seafaring". So you can build a port on any port space you find, even if allows_seafaring() is false.

I think allows_seafaring() is true only if at least two port spaces exist. If there is only one, allows_seafaring() is false even though a port can be built.

So, AI needs the new rule that buildings with "needs_seafaring=true" may be built only if allows_seafaring()==true.
In my opinion, ports shouldn´t be prohibited on non-seafaring maps, since the human player is allowed to build them…

OK, I will do it...

The analysis in #8 is correct.

I don't know why post spaces didn't get the tag - maybe I just assumed that all port spaces will make sense.

OK, I more-less implemented it, but it does not compile because of this:
http://bazaar.launchpad.net/~widelands-dev/widelands/ai_small_requests/view/head:/src/ai/defaultai.cc#L1964

/var/widelands2/BZR/ai_small_requests/src/ai/defaultai.cc: In member function ‘bool
DefaultAI::construct_building(uint32_t)’:
/var/widelands2/BZR/ai_small_requests/src/ai/defaultai.cc:1963:50: error: passing ‘const Widelands::Map’ as ‘this’
argument discards qualifiers [-fpermissive]
  const bool seafaring_map = map.allows_seafaring();

Please advice how to do it....

Probably map.allows_seafaring() isn't marked as a const function, so you will have to get the mutable_map as a pointer.

You should also search the code for allows_seafaring maybe it will trigger "disallowed_buildings", and you could tap into that instead and generally not allow to build any disallowed buildings. I should have thought of that earlier.

This way, we could catch 2 birds with 1 stone: allow scenario scripts to disallow buildings for the AIs, and take care of the useless buildings.

map.allows_seafaring() cannot be changed to const, because it does a lot of stuff. And it is intended for editor only obviously.

But I think this can be good solution:
http://bazaar.launchpad.net/~widelands-dev/widelands/ai_small_requests/view/head:/src/ai/defaultai.cc#L1963

not tested yet...

Well, it does not work. Map tags are empty....

this will also now be flexible i port spaces are added or removed via scripting.

Map* EditorGameBase::mutable_map() will give you a Map pointer that you can use and call allows_seafaring() on.

OK, allows_seafaring now works. BTW I found that this function is almost nowhere used. f.e. here is place where it should be used:

http://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/wui/fieldaction.cc#L421

But try to grep the code for get_port_spaces().size(), there are probably couple of places where this is used as "estimation" if map allows seafaring...

Would you like to fix those in your branch as well?

Yes. It is more-less fixed, but I doubt the allow_seafaring is best approach. It modifies map and I dont see a reason for AI to do it, especially as it has not been needed up to now...

We should revisit that function some time to see if we can do the same check without modifying the map.

I would stick with get_port_spaces().size() >= 2 for now, what do you think?

Sounds OK to me - since we already have it all over the place, it doesn't make things worse.

Fixed in build20-rc1