[freeciv-ai] Re: (PR#8992) Patch: Building ferries

["Benedict Adamson" <badamson@xxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8992 >

Per I. Mathisen wrote:
...
> You duplicate overlap_move() from pf_tools.c. Duplicated code is bad. Why
> can you not use pft_fill_unit_overlap_param()?
[One for Gregory]

> Style issue: Always whitespace between operators, so this is bad:
>  +  unsigned int battle_rate = base_rate*BATTLE_RATE/100;

I'll correct that.

> Why use braces around code that does not need it?

I like doing that to emphasise that a code is a single section. If it's 
incompatible with the consensus style for Freeciv, I'll change it.


> I think the use of the word 'straits' is confusing. I am not exactly sure
> what it is you are measuring with this variable.

I have in mind here how I usually use Triremes. Because they are unsafe 
on the high seas, I cross between islands where the gap is narrowest: 
the straits between the islands. Although other ferries don't need to 
travel across straits, I've noticed that their routes tend to anyway. 
The term is inappropriate for islands that are widely separated. I'll 
amend it.

> The ferry_geography() calculations only need be done once per game. So I
> think this should be moved to aidata.c|h and done on game initialization.

Yes. I wanted to keep the code self contained, rather than adding more 
calls elsewhere. However, note that the AI cheats by knowing how many 
islands there are, so a sophisticated version might estimate the number 
of islands each turn. Probably best to have an AI ferries initialisation 
function anyway, for future use.

> Also try putting more of the variables into the aidata struct,

Yes. I wanted to reduce the changes to other parts of the code.

> and pass a
> player struct to some of the functions, so that you don't need to use
> enormous function declarations like new_boats_to_build

I think I'll create a struct for recording information about one sea for 
one player, then pass that struct to new_boats_to build. Then I'll 
extract a function the code that computes those structs. The struct and 
that function could then be altered to handle 'channels' (and, probably, 
not to use PF). The aidata could have a calloc-ed array of those 
structs, one for each sea.

> In ferry_builder_score() perhaps try to build in cities that do not build
> wonders.

Is a wonder a building? At present ferry_builder_score() will build only 
in cities that are building units. But I agree; subsequent changes might 
allow the code to select cities that are building improvements or 
wonders, so that check probably ought to be there.
> 
> +  FIXME: Gross hack, uses pcity->ai.founder_want field for its local
> +  calculations.
> 
> You should fix this. I plan on retaining info in this variable later on to
> avoid recalculating settlers every turn.
[One for Gregory]

> Have you any idea how much time these calculations take?

I've run quite a few auto games with this patch. The program is 
certainly not crawling along, but I do not have measurements.

The code does one PF per ocean per player.

I expected you are worried about the estimate_cost function. A player 
will typically decide to build no ferries, so the function is typically 
called twice per player per ocean. The function is mostly a loop 
executed turns_horizon (typically 10) times per function call, so a game 
will typically have no more than a few hundred executions of the loop 
each turn. Each execution of the loop has a few tens of 'slow' integer 
arithmetic operations (multiplications and divides), so the function 
calls add a few thousand slow integer arithmetic operations each turn. 
My current guess is therefore that the estimate_cost function does not 
slow the program very much. Note that the execution time of the function 
is independent of the number of cities or units, so its execution time 
becomes proportionately less important as the game progresses and the 
number of cities and units increases.

> 
> You call is_ocean_near_tile(acity->tile) a lot of times. Perhaps we should
> add a new variable bool pcity->coastal instead, since the function call
> above is comparatively expensive.

Yes.

> Do not use numbers like 1,2,3 for acity->ai.founder_want (or whatever
> variable you use). Make some defines or enums with readable text, like
> CITY_UNPROCESSED, CITY_COUNTED, CITY_FERRY, CITY_PROCESSED, for example.
[One for Gregory]

> I think the aiferry_choose_build_global() main loop is somewhat odd and
> hard to read.

I'll try restructuring it.

> Lastly, I sometimes with this patch see that the AI no longer builds
> ferries to transport settlers. Although other times it works perfect.

Can you be more specific? Do you have an auto game?

Note that the logic can decide that it is better to let a unit wait a 
few turns for an existing ferry to become free than to build a ferry. 
The logic tends to let unit waits if ferry utilisation is low and ferry 
journeys are quick.

Also, the logic tends not to bother building ferries to transport units 
that will not be completed soon, so you might see cities building 
settlers for transport overseas when there are no ferries and no ferries 
being built. However, as the first such settler approaches completion, 
the logic will switch the city building that settler to ferry building. 
The first ferry built is therefore usually idle for a few turns.

Thanks for the feedback.