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

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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

On Sat, 30 Oct 2004, Benedict Adamson wrote:
> Attached is an updated version of my previous patch. This is applicable
> to the CVS development version as of 2004-10-29. It incorporates a bug
> fix which made the estimate_cost function produce incorrect costs for
> cases in which enemy hunters sank all the loaded ferries.

To Gregory and Benedict -

Great work! It is a very interesting way of doing it, simulating future
turns.

I have some comments on the patch itself.

You duplicate overlap_move() from pf_tools.c. Duplicated code is bad. Why
can you not use pft_fill_unit_overlap_param()? If you need a special
version, put this in pf_tools.c like the others, but I don't see that you
do.

*ferries cross between straits, and hunters lurk at straits

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

Why use braces around code that does not need it?

+    {/*rescale, so can use integer arithmetic for continuous variables*/
+      boats *= DENOM;
+      psngrs *= DENOM;
+      avail_boats *= DENOM;
+      hunters *= DENOM;
+      escorts *= DENOM;
+      build *= DENOM;
+      extra_skipped *= DENOM;
+    }

Try to use less braces, please. They make the code harder to read.

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

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.

Also try putting more of the variables into the aidata struct, and pass a
player struct to some of the functions, so that you don't need to use
enormous function declarations like

+static unsigned int new_boats_to_build(unsigned int psngrs, unsigned int 
boats,+                                       unsigned int avail_boats,
+                                       Unit_Type_id boattype,
+                                       unsigned int turns_horizon,
+                                       const unsigned int extra_passengers[],
+                                       const unsigned int extra_escorts[],
+                                       unsigned int journey_length,
+                                       unsigned int n_straits,
+                                       unsigned int area,
+                                       unsigned int hunters,
+                                       unsigned int escorts)

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

+  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.

Have you any idea how much time these calculations take? You can try
timing one turn end of a very large savegame with or without your patch,
for example, or use timers.

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.

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.

I think the aiferry_choose_build_global() main loop is somewhat odd and
hard to read. At least add some more comments describing what it does, and
how the ai_founder_want variable is used to limit loops.

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

  - Per